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>
From: Fujii Masao <masao.fujii@oss.nttdata.com>
Message-ID: <e068e42d-8eda-fbe5-e84c-7188e9386778@oss.nttdata.com>
Date: Wed, 1 Jul 2020 16:43:42 +0900
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.14; rv:68.0)
 Gecko/20100101 Thunderbird/68.9.0
MIME-Version: 1.0
In-Reply-To: <6c828f3a1d44dba81999fc7990b8f1c5@oss.nttdata.com>
Content-Type: text/plain; charset=windows-1252; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 8bit
Precedence: bulk


On 2020/07/01 14:48, torikoshia wrote:
> On Mon, Jun 29, 2020 at 3:13 PM Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
> 
>> Could you add this patch to Commitfest 2020-07?
> 
> Thanks for notifying, I've added it.
> BTW, I registered you as an author because this patch used
> lots of pg_cheat_funcs' codes.
> 
>    https://commitfest.postgresql.org/28/2622/

Thanks!

> 
>> This patch provides only the function, but isn't it convenient to
>> provide the view like pg_shmem_allocations?
> 
>> Sounds good. But isn't it better to document each column?
>> Otherwise, users cannot undersntad what "ident" column indicates.
> 
> Agreed.
> Attached a patch for creating a view for local memory context
> and its explanation on the document.

Thanks for updating the patch!

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.

+	tupdesc = rsinfo->setDesc;
+	tupstore = rsinfo->setResult;

These seem not to be necessary.

+	/*
+	 * 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"?

+/* ----------
+ * 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?

Regards,

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