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

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.

Regards,

--
Atsushi Torikoshi
NTT DATA CORPORATION