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

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.

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


Regards,

--
Atsushi Torikoshi
NTT DATA CORPORATION