Re: Creating a function for exposing memory usage of backend process - Mailing list pgsql-hackers
From | Fujii Masao |
---|---|
Subject | Re: Creating a function for exposing memory usage of backend process |
Date | |
Msg-id | 34517933-7e90-f644-f606-86d7132b240b@oss.nttdata.com Whole thread Raw |
In response to | Re: Creating a function for exposing memory usage of backend process (torikoshia <torikoshia@oss.nttdata.com>) |
Responses |
Re: Creating a function for exposing memory usage of backend process
|
List | pgsql-hackers |
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? > > | 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); Regards, -- Fujii Masao Advanced Computing Technology Center Research and Development Headquarters NTT DATA CORPORATION
pgsql-hackers by date: