Re: Creating a function for exposing memory usage of backend process

On 2020/07/06 12:12, torikoshia wrote:
> On Fri, Jul 3, 2020 at 7:33 PM Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
> 
> Thanks for your review!
> 
>> I like more specific name like pg_backend_memory_contexts.
> 
> Agreed.
> 
> When I was trying to add this function as statistics function,
> I thought that naming pg_stat_getbackend_memory_context()
> might make people regarded it as a "per-backend statistics
> function", whose parameter is backend ID number.
> So I removed "backend", but now there is no necessity to do
> so.
> 
>> But I'd like to hear more opinions about the name from others.
> 
> I changed the name to pg_backend_memory_contexts for the time
> being.

+1


>>> - function name: pg_get_memory_contexts()
>>> - source file: mainly src/backend/utils/mmgr/mcxt.c
> 
> 
>>> +       Identification information of the memory context. This field is truncated if the identification field is
longerthan 1024 characters
 
>>
>> "characters" should be "bytes"?
> 
> Fixed, but I used "characters" while referring to the
> descriptions on the manual of pg_stat_activity.query
> below.
> 
> | By default the query text is truncated at 1024 characters;
> 
> It has nothing to do with this thread, but considering
> multibyte characters, it also may be better to change it
> to "bytes".

Yeah, I agree we should write the separate patch fixing that. You will?
If not, I will do that later.


> Regarding the other comments, I revised the patch as you pointed.

Thanks for updating the patch! The patch basically looks good to me/
Here are some minor comments:

+#define MEMORY_CONTEXT_IDENT_SIZE    1024

This macro varible name sounds like the maximum allowed length of ident that
each menory context has. But actually this limits the maximum bytes of ident
to display. So I think that it's better to rename this macro to something like
MEMORY_CONTEXT_IDENT_DISPLAY_SIZE. Thought?

+#define PG_GET_MEMORY_CONTEXTS_COLS    9
+    Datum        values[PG_GET_MEMORY_CONTEXTS_COLS];
+    bool        nulls[PG_GET_MEMORY_CONTEXTS_COLS];

This macro variable name should be PG_GET_BACKEND_MEMORY_CONTEXTS_COLS
for the consistency with the function name?

+{ oid => '2282', descr => 'statistics: information about all memory contexts of local backend',

Isn't it better to remove "statistics: " from the above description?

+     <row>
+      <entry role="catalog_table_entry"><para role="column_definition">
+       <structfield>parent</structfield> <type>text</type>

There can be multiple memory contexts with the same name. So I'm afraid
that it's difficult to identify the actual parent memory context from this
"parent" column. This is ok when logging memory contexts by calling
MemoryContextStats() via gdb. Because child memory contexts are printed
just under their parent, with indents. But this doesn't work in the view.
To identify the actual parent memory or calculate the memory contexts tree
from the view, we might need to assign unique ID to each memory context
and display it. But IMO this is overkill. So I'm fine with current "parent"
column. Thought? Do you have any better idea?


Regards,

-- 
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION