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 1k8Jdg-0002SH-J0 for pgsql-hackers@arkaria.postgresql.org; Wed, 19 Aug 2020 08:40:08 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.92) (envelope-from ) id 1k8Jdf-00089m-FB for pgsql-hackers@arkaria.postgresql.org; Wed, 19 Aug 2020 08:40:07 +0000 Received: from makus.postgresql.org ([2001:4800:3e1:1::229]) by malur.postgresql.org with esmtps (TLS1.3:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1k8Jdf-00089e-3U for pgsql-hackers@lists.postgresql.org; Wed, 19 Aug 2020 08:40:07 +0000 Received: from oss.nttdata.com ([49.212.34.109]) by makus.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1k8Jdc-0003F8-7k for pgsql-hackers@postgresql.org; Wed, 19 Aug 2020 08:40:06 +0000 Received: from oss.nttdata.com (localhost [127.0.0.1]) by oss.nttdata.com (Postfix) with ESMTP id 610A76015F; Wed, 19 Aug 2020 17:40:00 +0900 (JST) X-Virus-Status: Clean X-Virus-Scanned: clamav-milter 0.102.3 at oss.nttdata.com MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit Date: Wed, 19 Aug 2020 17:40:00 +0900 From: torikoshia To: Fujii Masao Cc: PostgreSQL-development , Robert Haas , Kasahara Tatsuhito , Andres Freund , Michael Paquier Subject: Re: Creating a function for exposing memory usage of backend process In-Reply-To: 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> Message-ID: <261eebda3e1909475162b968e5a4ad2b@oss.nttdata.com> X-Sender: torikoshia@oss.nttdata.com User-Agent: Roundcube Webmail/1.1.9 List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Precedence: bulk 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! >>>> >>>>>> >>>>>> +     >>>>>> +      >>>>> linkend="view-pg-backend-memory-contexts">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. 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