MIME-Version: 1.0
Content-Type: multipart/mixed;
 boundary="=_4a266b3c4573ff31d36757f2c63082d7"
Date: Mon, 24 Aug 2020 21:56:06 +0900
From: torikoshia <torikoshia@oss.nttdata.com>
To: Fujii Masao <masao.fujii@oss.nttdata.com>
Cc: Michael Paquier <michael@paquier.xyz>, Kasahara Tatsuhito
 <kasahara.tatsuhito@gmail.com>, Andres Freund <andres@anarazel.de>, Tom Lane
 <tgl@sss.pgh.pa.us>, PostgreSQL-development <pgsql-hackers@postgresql.org>,
 Robert Haas <robertmhaas@gmail.com>
Subject: Re: Creating a function for exposing memory usage of backend process
In-Reply-To: <07da4267-74bf-4a6b-3312-4ab7c1e72f66@oss.nttdata.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>
 <07da4267-74bf-4a6b-3312-4ab7c1e72f66@oss.nttdata.com>
Message-ID: <ae118b75ede9327c6dc8e9e457639dc9@oss.nttdata.com>
User-Agent: Roundcube Webmail/1.1.9
Precedence: bulk

--=_4a266b3c4573ff31d36757f2c63082d7
Content-Transfer-Encoding: 8bit
Content-Type: text/plain; charset=UTF-8;
 format=flowed

On 2020-08-24 13:13, Fujii Masao wrote:
> 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;

OK!
Attached an updated patch.

> 
> 
>>> 
>>>> 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?

Oops, I meant 
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,

Regards,

--
Atsushi Torikoshi
NTT DATA CORPORATION


--=_4a266b3c4573ff31d36757f2c63082d7
Content-Transfer-Encoding: base64
Content-Type: text/x-diff;
 name=0002-Added-a-regression-test-for-pg_backend_memory_contex.patch
Content-Disposition: attachment;
 filename=0002-Added-a-regression-test-for-pg_backend_memory_contex.patch;
 size=1940

RnJvbSAzMzViOWViMGM2MGE3ZjEyZGViZDRjNDVkNDM1ODg4MTA5YjJiZmNmIE1vbiBTZXAgMTcg
MDA6MDA6MDAgMjAwMQpGcm9tOiBBdHN1c2hpIFRvcmlrb3NoaSA8dG9yaWtvc2hpYUBvc3MubnR0
ZGF0YS5jb20+CkRhdGU6IE1vbiwgMjQgQXVnIDIwMjAgMjE6Mjg6MjAgKzA5MDAKU3ViamVjdDog
W1BBVENIXSBBZGRlZCBhIHJlZ3Jlc3Npb24gdGVzdCBmb3IgcGdfYmFja2VuZF9tZW1vcnlfY29u
dGV4dHMuCgotLS0KIHNyYy90ZXN0L3JlZ3Jlc3MvZXhwZWN0ZWQvc3lzdmlld3Mub3V0IHwgOSAr
KysrKysrKysKIHNyYy90ZXN0L3JlZ3Jlc3Mvc3FsL3N5c3ZpZXdzLnNxbCAgICAgIHwgNSArKysr
KwogMiBmaWxlcyBjaGFuZ2VkLCAxNCBpbnNlcnRpb25zKCspCgpkaWZmIC0tZ2l0IGEvc3JjL3Rl
c3QvcmVncmVzcy9leHBlY3RlZC9zeXN2aWV3cy5vdXQgYi9zcmMvdGVzdC9yZWdyZXNzL2V4cGVj
dGVkL3N5c3ZpZXdzLm91dAppbmRleCAwNmM0YzNlNDc2Li4xY2ZmYzMzNDlkIDEwMDY0NAotLS0g
YS9zcmMvdGVzdC9yZWdyZXNzL2V4cGVjdGVkL3N5c3ZpZXdzLm91dAorKysgYi9zcmMvdGVzdC9y
ZWdyZXNzL2V4cGVjdGVkL3N5c3ZpZXdzLm91dApAQCAtMTksNiArMTksMTUgQEAgc2VsZWN0IGNv
dW50KCopID49IDAgYXMgb2sgZnJvbSBwZ19hdmFpbGFibGVfZXh0ZW5zaW9uczsKICB0CiAoMSBy
b3cpCiAKKy0tIFRoZSBlbnRpcmUgb3V0cHV0IG9mIHBnX2JhY2tlbmRfbWVtb3J5X2NvbnRleHRz
IGlzIG5vdCBzdGFibGUsCistLSB3ZSB0ZXN0IG9ubHkgdGhlIGV4aXN0YW5jZSBhbmQgYmFzaWMg
Y29uZGl0aW9uIG9mIFRvcE1lbW9yeUNvbnRleHQuCitzZWxlY3QgbmFtZSwgaWRlbnQsIHBhcmVu
dCwgbGV2ZWwsIHRvdGFsX2J5dGVzID49IGZyZWVfYnl0ZXMKKyAgZnJvbSBwZ19iYWNrZW5kX21l
bW9yeV9jb250ZXh0cyB3aGVyZSBsZXZlbCA9IDA7CisgICAgICAgbmFtZSAgICAgICB8IGlkZW50
IHwgcGFyZW50IHwgbGV2ZWwgfCA/Y29sdW1uPyAKKy0tLS0tLS0tLS0tLS0tLS0tLSstLS0tLS0t
Ky0tLS0tLS0tKy0tLS0tLS0rLS0tLS0tLS0tLQorIFRvcE1lbW9yeUNvbnRleHQgfCAgICAgICB8
ICAgICAgICB8ICAgICAwIHwgdAorKDEgcm93KQorCiAtLSBBdCBpbnRyb2R1Y3Rpb24sIHBnX2Nv
bmZpZyBoYWQgMjMgZW50cmllczsgaXQgbWF5IGdyb3cKIHNlbGVjdCBjb3VudCgqKSA+IDIwIGFz
IG9rIGZyb20gcGdfY29uZmlnOwogIG9rIApkaWZmIC0tZ2l0IGEvc3JjL3Rlc3QvcmVncmVzcy9z
cWwvc3lzdmlld3Muc3FsIGIvc3JjL3Rlc3QvcmVncmVzcy9zcWwvc3lzdmlld3Muc3FsCmluZGV4
IDI4ZTQxMmI3MzUuLmFjNGEwZTFjYmIgMTAwNjQ0Ci0tLSBhL3NyYy90ZXN0L3JlZ3Jlc3Mvc3Fs
L3N5c3ZpZXdzLnNxbAorKysgYi9zcmMvdGVzdC9yZWdyZXNzL3NxbC9zeXN2aWV3cy5zcWwKQEAg
LTEyLDYgKzEyLDExIEBAIHNlbGVjdCBjb3VudCgqKSA+PSAwIGFzIG9rIGZyb20gcGdfYXZhaWxh
YmxlX2V4dGVuc2lvbl92ZXJzaW9uczsKIAogc2VsZWN0IGNvdW50KCopID49IDAgYXMgb2sgZnJv
bSBwZ19hdmFpbGFibGVfZXh0ZW5zaW9uczsKIAorLS0gVGhlIGVudGlyZSBvdXRwdXQgb2YgcGdf
YmFja2VuZF9tZW1vcnlfY29udGV4dHMgaXMgbm90IHN0YWJsZSwKKy0tIHdlIHRlc3Qgb25seSB0
aGUgZXhpc3RhbmNlIGFuZCBiYXNpYyBjb25kaXRpb24gb2YgVG9wTWVtb3J5Q29udGV4dC4KK3Nl
bGVjdCBuYW1lLCBpZGVudCwgcGFyZW50LCBsZXZlbCwgdG90YWxfYnl0ZXMgPj0gZnJlZV9ieXRl
cworICBmcm9tIHBnX2JhY2tlbmRfbWVtb3J5X2NvbnRleHRzIHdoZXJlIGxldmVsID0gMDsKKwog
LS0gQXQgaW50cm9kdWN0aW9uLCBwZ19jb25maWcgaGFkIDIzIGVudHJpZXM7IGl0IG1heSBncm93
CiBzZWxlY3QgY291bnQoKikgPiAyMCBhcyBvayBmcm9tIHBnX2NvbmZpZzsKIAotLSAKMi4xOC4x
Cgo=
--=_4a266b3c4573ff31d36757f2c63082d7--