On 2020-07-08 22:12, Fujii Masao wrote:
> Thanks for updating the patch! It basically looks good to me.
>
> + <indexterm zone="view-pg-backend-memory-contexts">
> + <primary>backend memory contexts</primary>
> + </indexterm>
>
> Do we need this indexterm?
Thanks! it's not necessary. I remove this indexterm.
>>>
>>> +{ oid => '2282', descr => 'statistics: information about all memory
>>> contexts of local backend',
>>>
>>> Isn't it better to remove "statistics: " from the above description?
>>
>> Yeah, it's my oversight.
>>
>>>
>>> + <row>
>>> + <entry role="catalog_table_entry"><para
>>> role="column_definition">
>>> + <structfield>parent</structfield> <type>text</type>
>>>
>>> There can be multiple memory contexts with the same name. So I'm
>>> afraid
>>> that it's difficult to identify the actual parent memory context from
>>> this
>>> "parent" column. This is ok when logging memory contexts by calling
>>> MemoryContextStats() via gdb. Because child memory contexts are
>>> printed
>>> just under their parent, with indents. But this doesn't work in the
>>> view.
>>> To identify the actual parent memory or calculate the memory contexts
>>> tree
>>> from the view, we might need to assign unique ID to each memory
>>> context
>>> and display it. But IMO this is overkill. So I'm fine with current
>>> "parent"
>>> column. Thought? Do you have any better idea?
>>
>> Indeed.
>> I also feel it's not usual to assign a unique ID, which
>> can vary every time the view displayed.
>
> Agreed. Displaying such ID would be more confusing to users.
> Ok, let's leave the code as it is.
>
> Another comment about parent column is: dynahash can be parent?
> If yes, its indent instead of name should be displayed in parent
> column?
I'm not sure yet, but considering the changes in the future, it seems
better to do so.
But if we add information for identifying parent-child relation like the
memory address suggested from Andres, it seems not necessary.
So I'd like to go back to this point.
Regards,
--
Atsushi Torikoshi
NTT DATA CORPORATION