Re: Creating a function for exposing memory usage of backend process

On 2020/07/07 22:02, torikoshia wrote:
> On 2020-07-06 15:16, Fujii Masao wrote:
>> On 2020/07/06 12:12, torikoshia wrote:
>>> On Fri, Jul 3, 2020 at 7:33 PM Fujii Masao <masao(dot)fujii(at)oss(dot)nttdata(dot)com> wrote:
>>>
>>> Thanks for your review!
>>>
>>>> I like more specific name like pg_backend_memory_contexts.
>>>
>>> Agreed.
>>>
>>> When I was trying to add this function as statistics function,
>>> I thought that naming pg_stat_getbackend_memory_context()
>>> might make people regarded it as a "per-backend statistics
>>> function", whose parameter is backend ID number.
>>> So I removed "backend", but now there is no necessity to do
>>> so.
>>>
>>>> But I'd like to hear more opinions about the name from others.
>>>
>>> I changed the name to pg_backend_memory_contexts for the time
>>> being.
>>
>> +1
>>
>>
>>>>> - function name: pg_get_memory_contexts()
>>>>> - source file: mainly src/backend/utils/mmgr/mcxt.c
>>>
>>>
>>>>> +       Identification information of the memory context. This field is truncated if the identification field is longer than 1024 characters
>>>>
>>>> "characters" should be "bytes"?
>>>
>>> Fixed, but I used "characters" while referring to the
>>> descriptions on the manual of pg_stat_activity.query
>>> below.
>>>
>>> | By default the query text is truncated at 1024 characters;
>>>
>>> It has nothing to do with this thread, but considering
>>> multibyte characters, it also may be better to change it
>>> to "bytes".
>>
>> Yeah, I agree we should write the separate patch fixing that. You will?
>> If not, I will do that later.
>
> Thanks, I will try it!

Thanks!

>
>>> Regarding the other comments, I revised the patch as you pointed.
>>
>> Thanks for updating the patch! The patch basically looks good to me/
>> Here are some minor comments:
>>
>> +#define MEMORY_CONTEXT_IDENT_SIZE    1024
>>
>> This macro varible name sounds like the maximum allowed length of ident that
>> each menory context has. But actually this limits the maximum bytes of ident
>> to display. So I think that it's better to rename this macro to something like
>> MEMORY_CONTEXT_IDENT_DISPLAY_SIZE. Thought?
>
> Agreed.
> MEMORY_CONTEXT_IDENT_DISPLAY_SIZE seems more accurate.
>
>> +#define PG_GET_MEMORY_CONTEXTS_COLS    9
>> +    Datum        values[PG_GET_MEMORY_CONTEXTS_COLS];
>> +    bool        nulls[PG_GET_MEMORY_CONTEXTS_COLS];
>>
>> This macro variable name should be PG_GET_BACKEND_MEMORY_CONTEXTS_COLS
>> for the consistency with the function name?
>
> Thanks! Fixed it.

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?

>
>>
>> +{ 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?

Regards,

--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION