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

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?


> 
> | 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);

Regards,

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