Re: Creating a function for exposing memory usage of backend process - Mailing list pgsql-hackers

On 2020/08/19 17:40, torikoshia wrote:
> 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.

You're right.

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

Ok, understood. So I'd withdraw my suggestion.

Regards,

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