Re: Creating a function for exposing memory usage of backend process
От | torikoshia |
---|---|
Тема | Re: Creating a function for exposing memory usage of backend process |
Дата | |
Msg-id | e687d9ec8ca6e7f387131a2455bc9e1f@oss.nttdata.com обсуждение исходный текст |
Ответ на | Re: Creating a function for exposing memory usage of backend process (Fujii Masao <masao.fujii@oss.nttdata.com>) |
Ответы |
Re: Creating a function for exposing memory usage of backend process
(Fujii Masao <masao.fujii@oss.nttdata.com>)
|
Список | pgsql-hackers |
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. | 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? Regards, -- Atsushi Torikoshi NTT DATA CORPORATION
Вложения
В списке pgsql-hackers по дате отправления:
Предыдущее
От: Magnus HaganderДата:
Сообщение: Re: EDB builds Postgres 13 with an obsolete ICU version
Следующее
От: David RowleyДата:
Сообщение: Re: Hybrid Hash/Nested Loop joins and caching results from subplans