Received: from malur.postgresql.org ([217.196.149.56]) by arkaria.postgresql.org with esmtps (TLS1.3:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1k8K8i-0003Vo-Be for pgsql-hackers@arkaria.postgresql.org; Wed, 19 Aug 2020 09:12:12 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.92) (envelope-from ) id 1k8K8h-0007Vq-9R for pgsql-hackers@arkaria.postgresql.org; Wed, 19 Aug 2020 09:12:11 +0000 Received: from magus.postgresql.org ([2a02:c0:301:0:ffff::29]) by malur.postgresql.org with esmtps (TLS1.3:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1k8K8h-0007Vj-0u for pgsql-hackers@lists.postgresql.org; Wed, 19 Aug 2020 09:12:11 +0000 Received: from oss.nttdata.com ([49.212.34.109]) by magus.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1k8K8e-0006Oh-9p for pgsql-hackers@postgresql.org; Wed, 19 Aug 2020 09:12:10 +0000 Received: from hnk.local (p12054-ipngn2901funabasi.chiba.ocn.ne.jp [153.212.171.54]) by oss.nttdata.com (Postfix) with ESMTPSA id 1A80860171; Wed, 19 Aug 2020 18:12:03 +0900 (JST) X-Virus-Status: Clean X-Virus-Scanned: clamav-milter 0.102.3 at oss.nttdata.com Subject: Re: Creating a function for exposing memory usage of backend process To: torikoshia Cc: PostgreSQL-development , Robert Haas , Kasahara Tatsuhito , Andres Freund , Michael Paquier References: <805313b5f7dc2e6c19b209ff930a7a61@oss.nttdata.com> <9ee242b1-9e70-18f5-0033-6a086e71164d@oss.nttdata.com> <3548926512ddfaedfb5eaf967bf2f196@oss.nttdata.com> <20200708170348.rsqry4b4k5mrsu7y@alap3.anarazel.de> <24ea05d7bbdb652feef50caee6fcfbf8@oss.nttdata.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> <34517933-7e90-f644-f606-86d7132b240b@oss.nttdata.com> <0b339b06646e2c56a3aa081699322346@oss.nttdata.com> <261eebda3e1909475162b968e5a4ad2b@oss.nttdata.com> From: Fujii Masao Message-ID: <66ae5b88-208d-96fa-c20a-fb539b597859@oss.nttdata.com> Date: Wed, 19 Aug 2020 18:12:02 +0900 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.14; rv:68.0) Gecko/20100101 Thunderbird/68.11.0 MIME-Version: 1.0 In-Reply-To: <261eebda3e1909475162b968e5a4ad2b@oss.nttdata.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 8bit List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Precedence: bulk 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! >>>>> >>>>>>> >>>>>>> +     >>>>>>> +      pg_backend_memory_contexts >>>>>>> +      backend memory contexts >>>>>>> +     >>>>>>> >>>>>>> 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. >>>>>>> >>>>>>> >>>>>>> + >>>>>>> + <structname>pg_backend_memory_contexts</structname> >>>>>>> >>>>>>> Same as above. >>>>> >>>>> Modified both. >>>>> >>>>>>> >>>>>>> >>>>>>> +   The view pg_backend_memory_contexts 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 pg_backend_memory_contexts 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