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