Hi Amit-san,
On 2019/11/28 10:59, Amit Langote wrote:
> Yamada-san,
>
> Thank you for updating the patch.
>
> On Wed, Nov 27, 2019 at 12:46 PM Tatsuro Yamada
> <tatsuro.yamada.tf@nttcom.co.jp> wrote:
>> But I just remembered I replaced column name "*_table" with "*_relid"
>> based on Robert's comment three months ago, see below:
>>
>>> /me reviews.
>>>
>>> + <entry><structfield>scanning_table</structfield></entry>
>>>
>>> I think this should be retitled to something that ends in 'relid',
>>> like all of the corresponding cases in existing progress views.
>>> Perhaps 'active_relid' or 'current_relid'.
>>
>> So, it would be better to use same rule against child_tables_total and
>> child_tables_done. Thus I changed these column names to others and added
>> to the view.
>
> Robert's comment seems OK for a column that actually reports an OID,
> but for columns that report counts, names containing "relids" sound a
> bit strange to me. So, I prefer child_tables_total /
> child_tables_done over child_relids_total / child_relids_done. Would
> like to hear more opinions.
Hmmm... I understand your opinion but I'd like to get more opinions too. :)
Do you prefer these column names? See below:
<Columns of the view>
pid
datid
datname
relid
phase
sample_blks_total
heap_blks_scanned
ext_stats_total
ext_stats_computed
child_tables_total <= Renamed: s/relid/table/
child_tables_done <= Renamed: s/relid/table/
current_child_table <= Renamed: s/relid/table/
>>>> Also, inheritance tree stats are created *after* creating single table
>>>> stats, so I think that it would be better to have a distinct phase
>>>> name for that, say "acquiring inherited sample rows". In
>>>> do_analyze_rel(), you can select which of two phases to set based on
>>>> whether inh is true or not.
>>>
>>> Okay! I'll also add the new phase "acquiring inherited sample rows" on
>>> the next patch.
>>
>>
>> Fixed.
>>
>> I tried to abbreviate it to "acquiring inh sample rows" because I thought
>> "acquiring inherited sample rows" is a little long for the phase name.
>
> I think phase names should contain full English words, because they
> are supposed to be descriptive. Users are very likely to not
> understand what "inh" means without looking up the docs.
Okay, I will fix it.
s/acquiring inh sample rows/acquiring inherited sample rows/
Thanks,
Tatsuro Yamada
