From: Fujii Masao <masao.fujii@oss.nttdata.com>
To: torikoshia <torikoshia@oss.nttdata.com>
Cc: PostgreSQL-development <pgsql-hackers@postgresql.org>
Cc: Robert Haas <robertmhaas@gmail.com>
Cc: Kasahara Tatsuhito <kasahara.tatsuhito@gmail.com>
Cc: Andres Freund <andres@anarazel.de>
Cc: Michael Paquier <michael@paquier.xyz>
Subject: Re: Creating a function for exposing memory usage of backend process
Date: Wed, 19 Aug 2020 18:12:02 +0900
Message-ID: <66ae5b88-208d-96fa-c20a-fb539b597859@oss.nttdata.com> (raw)
In-Reply-To: <261eebda3e1909475162b968e5a4ad2b@oss.nttdata.com>
References: <805313b5f7dc2e6c19b209ff930a7a61@oss.nttdata.com>
<cd0e961fd42e5708fdea70f7420bf214@oss.nttdata.com>
<9ee242b1-9e70-18f5-0033-6a086e71164d@oss.nttdata.com>
<3548926512ddfaedfb5eaf967bf2f196@oss.nttdata.com>
<20200708170348.rsqry4b4k5mrsu7y@alap3.anarazel.de>
<24ea05d7bbdb652feef50caee6fcfbf8@oss.nttdata.com>
<CAP0=ZVJ2JPS41CffcG-E2CnBkfOjFRa4YoKB=A8vbWGkjEtA1w@mail.gmail.com>
<e1b81c88dc750ecd2ef03924af751567@oss.nttdata.com>
<CA+TgmoZU6Tr2mEY2Cjf5-Cce5NF6B0eknCge1v7Uk3D5H2njOA@mail.gmail.com>
<20200808014409.GA1600@paquier.xyz>
<66c9d45297ad820cd092b125aa91ce2c@oss.nttdata.com>
<98e53274-253f-6bd1-b542-489ecbda8753@oss.nttdata.com>
<083781e2-339e-9844-a02c-53a0ee3beb82@oss.nttdata.com>
<e687d9ec8ca6e7f387131a2455bc9e1f@oss.nttdata.com>
<34517933-7e90-f644-f606-86d7132b240b@oss.nttdata.com>
<0b339b06646e2c56a3aa081699322346@oss.nttdata.com>
<f1ae4071-31b8-d7aa-db5d-2781e5612e6f@oss.nttdata.com>
<261eebda3e1909475162b968e5a4ad2b@oss.nttdata.com>
On 2020/08/19 17:40, torikoshia wrote:
> 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.
You're right.
> 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.
Ok, understood. So I'd withdraw my suggestion.
Regards,
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: masao.fujii@oss.nttdata.com, torikoshia@oss.nttdata.com, robertmhaas@gmail.com, kasahara.tatsuhito@gmail.com, andres@anarazel.de, michael@paquier.xyz
Subject: Re: Creating a function for exposing memory usage of backend process
In-Reply-To: <66ae5b88-208d-96fa-c20a-fb539b597859@oss.nttdata.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox