Subject: Re: Creating a function for exposing memory usage of backend process
To: torikoshia <torikoshia@oss.nttdata.com>
Cc: pgsql-hackers@postgresql.org
References: <72a656e0f71d0860161e0b3f67e4d771@oss.nttdata.com>
 <398eb7cd-5f75-be1c-3897-36865b7bfabf@oss.nttdata.com>
 <CA+TgmobkpFV0UB67kzXuD36--OFHwz1bs=L_6PZbD4nxKqUQMw@mail.gmail.com>
 <3193c7d590f3212e543b63df977da758@oss.nttdata.com>
 <78268b5f-53ed-4645-1abb-23c1030cce0a@oss.nttdata.com>
 <6c828f3a1d44dba81999fc7990b8f1c5@oss.nttdata.com>
 <e068e42d-8eda-fbe5-e84c-7188e9386778@oss.nttdata.com>
 <062bab2db5645812fae12e3a965de463@oss.nttdata.com>
From: Fujii Masao <masao.fujii@oss.nttdata.com>
Message-ID: <5cbed0cc-79fc-3b98-120a-20e07848d10a@oss.nttdata.com>
Date: Wed, 1 Jul 2020 22:58:23 +0900
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.14; rv:68.0)
 Gecko/20100101 Thunderbird/68.10.0
MIME-Version: 1.0
In-Reply-To: <062bab2db5645812fae12e3a965de463@oss.nttdata.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 8bit
Precedence: bulk


On 2020/07/01 22:15, torikoshia wrote:
> On Wed, Jul 1, 2020 at 4:43 PM Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
> 
> Thanks for reviewing!
> 
>> You treat pg_stat_local_memory_contexts view as a dynamic statistics view.
>> But isn't it better to treat it as just system view like pg_shmem_allocations
>> or pg_prepared_statements  because it's not statistics information? If yes,
>> we would need to rename the view, move the documentation from
>> monitoring.sgml to catalogs.sgml, and move the code from pgstat.c to
>> the more appropriate source file.
> 
> Agreed.
> At first, I thought not only statistical but dynamic information about exactly
> what is going on was OK when reading the sentence on the manual below.
> 
>> PostgreSQL also supports reporting dynamic information about exactly what is going on in the system right now, such as the exact command currently being executed by other server processes, and which other connections exist in the system. This facility is independent of the collector process.
> 
> However, now I feel something strange because existing pg_stats_* views seem
> to be per cluster information but the view I'm adding is about per backend
> information.
> 
> I'm going to do some renaming and transportations.
> 
> - view name: pg_memory_contexts
> - function name: pg_get_memory_contexts()
> - source file: mainly src/backend/utils/mmgr/mcxt.c
> 
> 
>> +       tupdesc = rsinfo->setDesc;
>> +       tupstore = rsinfo->setResult;
>>
>> These seem not to be necessary.
> 
> Thanks!
> 
>> +       /*
>> +        * It seems preferable to label dynahash contexts with just the hash table
>> +        * name.  Those are already unique enough, so the "dynahash" part isn't
>> +        * very helpful, and this way is more consistent with pre-v11 practice.
>> +        */
>> +       if (ident && strcmp(name, "dynahash") == 0)
>> +       {
>> +               name = ident;
>> +               ident = NULL;
>> +       }
>>
>> IMO it seems better to report both name and ident even in the case of
>> dynahash than report only ident (as name). We can easily understand
>> the context is used for dynahash when it's reported. But you think it's
>> better to report NULL rather than "dynahash"?
> 
> These codes come from MemoryContextStatsPrint() and my intension is to
> keep consistent with it.

Ok, understood! I agree that it's strange to display different names
for the same memory context between this view and logging.

It's helpful if the comment there refers to MemoryContextStatsPrint()
and mentions the reason that you told.


> 
>> +/* ----------
>> + * The max bytes for showing identifiers of MemoryContext.
>> + * ----------
>> + */
>> +#define MEMORY_CONTEXT_IDENT_SIZE      1024
>>
>> Do we really need this upper size limit? Could you tell me why
>> this is necessary?
> 
> It also derived from MemoryContextStatsPrint().
> I suppose it is for keeping readability of the log..

Understood. I may want to change the upper limit of query size to display.
But at the first step, I'm fine with limitting 1024 bytes.


> 
> I'm going to follow the discussion on the mailing list and find why
> these codes were introduced.

https://www.postgresql.org/message-id/12319.1521999065%40sss.pgh.pa.us

Regards,

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