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

On 2020-08-19 15:48, Fujii Masao wrote:
> On 2020/08/19 9:43, torikoshia wrote:
>> On 2020-08-18 22:54, Fujii Masao wrote:
>>> On 2020/08/18 18:41, torikoshia wrote:
>>>> On 2020-08-17 21:19, Fujii Masao wrote:
>>>>> On 2020/08/17 21:14, Fujii Masao wrote:
>>>>>>> On 2020-08-07 16:38, Kasahara Tatsuhito wrote:
>>>>>>>> The following review has been posted through the commitfest 
>>>>>>>> application:
>>>>>>>> make installcheck-world:  tested, passed
>>>>>>>> Implements feature:       tested, passed
>>>>>>>> Spec compliant:           not tested
>>>>>>>> Documentation:            tested, passed
>>>>>>>> 
>>>>>>>> I tested the latest
>>>>>>>> patch(0007-Adding-a-function-exposing-memory-usage-of-local-backend.patch)
>>>>>>>> with the latest PG-version 
>>>>>>>> (199cec9779504c08aaa8159c6308283156547409)
>>>>>>>> and test was passed.
>>>>>>>> It looks good to me.
>>>>>>>> 
>>>>>>>> The new status of this patch is: Ready for Committer
>>>>>>> 
>>>>>>> Thanks for your testing!
>>>>>> 
>>>>>> Thanks for updating the patch! Here are the review comments.
>>>> 
>>>> Thanks for reviewing!
>>>> 
>>>>>> 
>>>>>> +     <row>
>>>>>> +      <entry><link 
>>>>>> linkend="view-pg-backend-memory-contexts"><structname>pg_backend_memory_contexts</structname></link></entry>
>>>>>> +      <entry>backend memory contexts</entry>
>>>>>> +     </row>
>>>>>> 
>>>>>> The above is located just after pg_matviews entry. But it should 
>>>>>> be located
>>>>>> just after pg_available_extension_versions entry. Because the rows 
>>>>>> in the table
>>>>>> "System Views" should be located in alphabetical order.
>>>>>> 
>>>>>> 
>>>>>> + <sect1 id="view-pg-backend-memory-contexts">
>>>>>> + 
>>>>>> <title><structname>pg_backend_memory_contexts</structname></title>
>>>>>> 
>>>>>> Same as above.
>>>> 
>>>> Modified both.
>>>> 
>>>>>> 
>>>>>> 
>>>>>> +   The view <structname>pg_backend_memory_contexts</structname> 
>>>>>> displays all
>>>>>> +   the local backend memory contexts.
>>>>>> 
>>>>>> This description seems a bit confusing because maybe we can 
>>>>>> interpret this
>>>>>> as "... displays the memory contexts of all the local backends" 
>>>>>> wrongly. Thought?
>>>>>> What about the following description, instead?
>>>> 
>>>>>>      The view <structname>pg_backend_memory_contexts</structname> 
>>>>>> displays all
>>>>>>      the memory contexts of the server process attached to the 
>>>>>> current session.
>>>> 
>>>> Thanks! it seems better.
>>>> 
>>>>>> +    const char *name = context->name;
>>>>>> +    const char *ident = context->ident;
>>>>>> +
>>>>>> +    if (context == NULL)
>>>>>> +        return;
>>>>>> 
>>>>>> The above check "context == NULL" is useless? If "context" is 
>>>>>> actually NULL,
>>>>>> "context->name" would cause segmentation fault, so ISTM that the 
>>>>>> check
>>>>>> will never be performed.
>>>>>> 
>>>>>> If "context" can be NULL, the check should be performed before 
>>>>>> accessing
>>>>>> to "contect". OTOH, if "context" must not be NULL per the 
>>>>>> specification of
>>>>>> PutMemoryContextStatsTupleStore(), assertion test checking
>>>>>> "context != NULL" should be used here, instead?
>>>> 
>>>> Yeah, "context" cannot be NULL because "context" must be 
>>>> TopMemoryContext
>>>> or it is already checked as not NULL as follows(child != NULL).
>>>> 
>>>> I added the assertion check.
>>> 
>>> Isn't it better to add AssertArg(MemoryContextIsValid(context)), 
>>> instead?
>> 
>> Thanks, that's better.
>> 
>>>> 
>>>> | for (child = context->firstchild; child != NULL; child = 
>>>> child->nextchild)
>>>> | {
>>>> |  ...
>>>> |         PutMemoryContextsStatsTupleStore(tupstore, tupdesc,
>>>> |                                                           child, 
>>>> parentname, level + 1);
>>>> | }
>>>> 
>>>>> Here is another comment.
>>>>> 
>>>>> +     if (parent == NULL)
>>>>> +             nulls[2] = true;
>>>>> +     else
>>>>> +             /*
>>>>> +              * We labeled dynahash contexts with just the hash 
>>>>> table name.
>>>>> +              * To make it possible to identify its parent, we 
>>>>> also display
>>>>> +              * parent's ident here.
>>>>> +              */
>>>>> +             if (parent->ident && strcmp(parent->name, "dynahash") 
>>>>> == 0)
>>>>> +                     values[2] = 
>>>>> CStringGetTextDatum(parent->ident);
>>>>> +             else
>>>>> +                     values[2] = 
>>>>> CStringGetTextDatum(parent->name);
>>>>> 
>>>>> PutMemoryContextsStatsTupleStore() doesn't need "parent" memory 
>>>>> context,
>>>>> but uses only the name of "parent" memory context. So isn't it 
>>>>> better to use
>>>>> "const char *parent" instead of "MemoryContext parent", as the 
>>>>> argument of
>>>>> the function? If we do that, we can simplify the above code.
>>>> 
>>>> Thanks, the attached patch adopted the advice.
>>>> 
>>>> However, since PutMemoryContextsStatsTupleStore() used not only the 
>>>> name
>>>> but also the ident of the "parent", I could not help but adding 
>>>> similar
>>>> codes before calling the function.
>>>> The total amount of codes and complexity seem not to change so much.
>>>> 
>>>> Any thoughts? Am I misunderstanding something?
>>> 
>>> I was thinking that we can simplify the code as follows.
>>> That is, we can just pass "name" as the argument of
>>> PutMemoryContextsStatsTupleStore()
>>> since "name" indicates context->name or ident (if name is 
>>> "dynahash").
>>> 
>>>      for (child = context->firstchild; child != NULL; child = 
>>> child->nextchild)
>>>      {
>>> -        const char *parentname;
>>> -
>>> -        /*
>>> -         * We labeled dynahash contexts with just the hash table 
>>> name.
>>> -         * To make it possible to identify its parent, we also use
>>> -         * the hash table as its context name.
>>> -         */
>>> -        if (context->ident && strcmp(context->name, "dynahash") == 
>>> 0)
>>> -            parentname = context->ident;
>>> -        else
>>> -            parentname = context->name;
>>> -
>>>          PutMemoryContextsStatsTupleStore(tupstore, tupdesc,
>>> -                                  child, parentname, level + 1);
>>> +                                  child, name, level + 1);
>> 
>> I got it, thanks for the clarification!
>> 
>> Attached a revised patch.
> 
> Thanks for updating the patch! I pushed it.

Thanks a lot!

> 
> BTW, I guess that you didn't add the regression test for this view 
> because
> the contents of the view are not stable. Right? But isn't it better to 
> just
> add the "stable" test like
> 
>     SELECT name, ident, parent, level, total_bytes >= free_bytes FROM
> pg_backend_memory_contexts WHERE level = 0;
> 
> rather than adding nothing?

Yes, I didn't add regression tests because of the unstability of the 
output.
I thought it would be OK since other views like pg_stat_slru and 
pg_shmem_allocations
didn't have tests for their outputs.

I don't have strong objections for adding tests like you proposed, but 
I'm not sure
whether there are special reasons to add tests compared with these 
existing views.


Regards,

--
Atsushi Torikoshi
NTT DATA CORPORATION