From: | torikoshia <torikoshia(at)oss(dot)nttdata(dot)com> |
---|---|
To: | Fujii Masao <masao(dot)fujii(at)oss(dot)nttdata(dot)com> |
Cc: | Michael Paquier <michael(at)paquier(dot)xyz>, Kasahara Tatsuhito <kasahara(dot)tatsuhito(at)gmail(dot)com>, Andres Freund <andres(at)anarazel(dot)de>, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, PostgreSQL-development <pgsql-hackers(at)postgresql(dot)org>, Robert Haas <robertmhaas(at)gmail(dot)com> |
Subject: | Re: Creating a function for exposing memory usage of backend process |
Date: | 2020-08-24 12:56:06 |
Message-ID: | ae118b75ede9327c6dc8e9e457639dc9@oss.nttdata.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
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
Attachment | Content-Type | Size |
---|---|---|
0002-Added-a-regression-test-for-pg_backend_memory_contex.patch | text/x-diff | 1.9 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | gkokolatos | 2020-08-24 13:08:53 | Commitfest manager 2020-11 |
Previous Message | Magnus Hagander | 2020-08-24 12:23:30 | Re: deb repo doesn't have latest. or possible to update web page? |