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

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!

>> 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.

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

We show each context using a recursive function and this is
a kind of depth-first search. So, as far as I understand,
we can identify its parent using both the "parent" column
and the order of the rows.

Documenting these things may worth for who want to identify
the relation between parents and children.

Of course, in the relational model, the order of relation
does not have meaning so it's also unusual in this sense..

Regards,

--
Atsushi Torikoshi
NTT DATA CORPORATION