Subject: Re: Creating a function for exposing memory usage of backend process
To: torikoshia <torikoshia@oss.nttdata.com>,
 Michael Paquier <michael@paquier.xyz>,
 Kasahara Tatsuhito <kasahara.tatsuhito@gmail.com>
Cc: Andres Freund <andres@anarazel.de>, Tom Lane <tgl@sss.pgh.pa.us>,
 PostgreSQL-development <pgsql-hackers@postgresql.org>,
 Robert Haas <robertmhaas@gmail.com>
References: <261eebda3e1909475162b968e5a4ad2b@oss.nttdata.com>
 <66ae5b88-208d-96fa-c20a-fb539b597859@oss.nttdata.com>
 <20200819135545.GC19121@paquier.xyz> <1414992.1597849297@sss.pgh.pa.us>
 <20200819225343.c7rxteydi35nhrdp@alap3.anarazel.de>
 <1554676.1597886946@sss.pgh.pa.us>
 <20200820014301.5pr7qjugnvpovu62@alap3.anarazel.de>
 <d97ad430-444d-9ec8-21dd-a1584569309b@oss.nttdata.com>
 <CAP0=ZVJ5VZsUYLFkc-E5_JHTc1ZdemX28VgQqUXJLTywC=9hfA@mail.gmail.com>
 <a8f3c385e9edeba4833c712270d6e6d9@oss.nttdata.com>
 <20200822121641.GE2108@paquier.xyz>
 <c9515ae497b4a077360a31547782acb8@oss.nttdata.com>
From: Fujii Masao <masao.fujii@oss.nttdata.com>
Message-ID: <07da4267-74bf-4a6b-3312-4ab7c1e72f66@oss.nttdata.com>
Date: Mon, 24 Aug 2020 13:13:39 +0900
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.14; rv:68.0)
 Gecko/20100101 Thunderbird/68.11.0
MIME-Version: 1.0
In-Reply-To: <c9515ae497b4a077360a31547782acb8@oss.nttdata.com>
Content-Type: text/plain; charset=windows-1252; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 8bit
Precedence: bulk


On 2020/08/24 13:01, torikoshia wrote:
> On 2020-08-22 21:18, Michael Paquier wrote:
> 
> Thanks for reviewing!
> 
>> On Fri, Aug 21, 2020 at 11:27:06PM +0900, torikoshia wrote:
>>> OK. Added a regression test on sysviews.sql.
>>> (0001-Added-a-regression-test-for-pg_backend_memory_contex.patch)
>>>
>>> Fujii-san gave us an example, but I added more simple one considering
>>> the simplicity of other tests on that.
>>
>> What you have sent in 0001 looks fine to me.  A small test is much
>> better than nothing.

+1

But as I proposed upthread, what about a bit complicated test as follows,
e.g., to confirm that the internal logic for level works expectedly?

      SELECT name, ident, parent, level, total_bytes >= free_bytes FROM pg_backend_memory_contexts WHERE level = 0;


>>
>>> Added a patch for relocating the codes to mcxtfuncs.c.
>>> (patches/0001-Rellocated-the-codes-for-pg_backend_memory_contexts-.patch)

Thanks for the patch! Looks good to me.
Barring any objection, I will commit this patch at first.


>>
>> The same code is moved around line-by-line.
>>
>>> Of course, this restriction makes pg_backend_memory_contexts hard to use
>>> when the user of the target session is not granted pg_monitor because the
>>> scope of this view is session local.
>>>
>>> In this case, I imagine additional operations something like temporarily
>>> granting pg_monitor to that user.
>>
>> Hmm.  I am not completely sure either that pg_monitor is the best fit
>> here, because this view provides information about a bunch of internal
>> structures.  Something that could easily be done though is to revoke
>> the access from public, and then users could just set up GRANT
>> permissions post-initdb, with pg_monitor as one possible choice.  This
>> is the safest path by default, and this stuff is of a caliber similar
>> to pg_shmem_allocations in terms of internal contents.
> 
> I think this is a better way than what I did in
> 0001-Rellocated-the-codes-for-pg_backend_memory_contexts-.patch.

You mean 0001-Restrict-the-access-to-pg_backend_memory_contexts-to.patch?


> Attached a patch.

Thanks for updating the patch! This also looks good to me.


>> It seems to me that you are missing one "REVOKE ALL on
>> pg_backend_memory_contexts FROM PUBLIC" in patch 0003.
>>
>> By the way, if that was just for me, I would remove used_bytes, which
>> is just a computation from the total and free numbers.  I'll defer
>> that point to Fujii-san.

Yeah, I was just thinking that displaying also used_bytes was useful,
but this might be inconsistent with the other views' ways.

Regards,

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