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

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.

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?

Regards,

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