Hi Tomas,
On 2021/01/09 9:01, Tomas Vondra wrote:
> On 1/8/21 1:14 AM, Tomas Vondra wrote:
>> On 1/8/21 12:52 AM, Tatsuro Yamada wrote:
>>> On 2021/01/08 0:56, Tomas Vondra wrote:
>>>> On 1/7/21 3:47 PM, Alvaro Herrera wrote:
>>>>> On 2021-Jan-07, Tomas Vondra wrote:
>>>>>> On 1/7/21 1:46 AM, Tatsuro Yamada wrote:
>>>>>
>>>>>>> I overlooked the check for MCV in the logic building query
>>>>>>> because I created the patch as a new feature on PG14.
>>>>>>> I'm not sure whether we should do back patch or not. However, I'll
>>>>>>> add the check on the next patch because it is useful if you decide to
>>>>>>> do the back patch on PG10, 11, 12, and 13.
>>>>>>
>>>>>> BTW perhaps a quick look at the other \d commands would show if
>>>>>> there are
>>>>>> precedents. I didn't have time for that.
>>>>>
>>>>> Yes, we do promise that new psql works with older servers.
>>>>>
>>>>
>>>> Yeah, makes sense. That means we need add the check for 12 / MCV.
>>>
>>>
>>> Ah, I got it.
>>> I fixed the patch to work with older servers to add the checking
>>> versions. And I tested \dX command on older servers (PG10 - 13).
>>> These results look fine.
>>>
>>> 0001:
>>> Added the check code to handle pre-PG12. It has not MCV and
>>> pg_statistic_ext_data.
>>> 0002:
>>> This patch is the same as the previous patch (not changed).
>>>
>>> Please find the attached files.
>>>
>>
>> OK, thanks. I'll take a look and probably push tomorrow. FWIW I plan to
>> squash the patches into a single commit.
>>
>
> Attached is a patch I plan to commit - 0001 is the last submitted
> version with a couple minor tweaks, mostly in docs/comments, and small
> rework of branching to be more like the other functions in describe.c.
Thanks for revising the patch.
I reviewed the 0001, and the branching and comments look good to me.
However, I added an alias name in processSQLNamePattern() on the patch:
s/"stxname"/"es.stxname"/
> While working on that, I realized that 'defined' might be a bit
> ambiguous, I initially thought it means 'NOT NULL' (which it does not).
> I propose to change it to 'requested' instead. Tatsuro, do you agree, or
> do you think 'defined' is better?
Regarding the status of extended stats, I think the followings:
- "defined": it shows the extended stats defined only. We can't know
whether it needs to analyze or not. I agree this name was
ambiguous. Therefore we should replace it with a more suitable
name.
- "requested": it shows the extended stats needs something. Of course,
we know it needs to ANALYZE because we can create the patch.
However, I feel there is a little ambiguity for DBA.
To solve this, it would be better to write an explanation of
the status in the document. For example,
======
The column of the kind of extended stats (e. g. Ndistinct) shows some statuses.
"requested" means that it needs to gather data by ANALYZE. "built" means ANALYZE
was finished, and the planner can use it. NULL means that it doesn't exists.
======
What do you think? :-D
Thanks,
Tatsuro Yamada
