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

On 2020-08-19 15:48, Fujii Masao wrote:
> 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.

Thanks a lot!

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

Yes, I didn't add regression tests because of the unstability of the
output.
I thought it would be OK since other views like pg_stat_slru and
pg_shmem_allocations
didn't have tests for their outputs.

I don't have strong objections for adding tests like you proposed, but
I'm not sure
whether there are special reasons to add tests compared with these
existing views.

Regards,

--
Atsushi Torikoshi
NTT DATA CORPORATION