MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII;
 format=flowed
Content-Transfer-Encoding: 7bit
Date: Fri, 04 Dec 2020 19:16:12 +0900
From: torikoshia <torikoshia@oss.nttdata.com>
To: Tom Lane <tgl@sss.pgh.pa.us>, Fujii Masao <masao.fujii@oss.nttdata.com>
Cc: Georgios Kokolatos <gkokolatos@protonmail.com>,
 pgsql-hackers@postgresql.org
Subject: Re: Get memory contexts of an arbitrary backend process
In-Reply-To: <1685539.1606959410@sss.pgh.pa.us>
References: <0271f440ac77f2a4180e0e56ebd944d1@oss.nttdata.com>
 <270ec80dbbebfb5c6f2b9fdd7a7858b5@oss.nttdata.com>
 <160502083340.7362.4386116585349115146.pgcf@coridan.postgresql.org>
 <064d510f2d33edc37dfd6b8f3b6bd30e@oss.nttdata.com>
 <d42d3186-d647-9af4-7c1c-d6ffca8bff40@oss.nttdata.com>
 <1685539.1606959410@sss.pgh.pa.us>
Message-ID: <83ff0aff04fffc95cbd1fc1637a0fccc@oss.nttdata.com>
User-Agent: Roundcube Webmail/1.1.9
Precedence: bulk

On 2020-12-03 10:36, Tom Lane wrote:
> Fujii Masao <masao.fujii@oss.nttdata.com> writes:
>> I'm starting to study how this feature behaves. At first, when I 
>> executed
>> the following query, the function never returned. ISTM that since
>> the autovacuum launcher cannot respond to the request of memory
>> contexts dump, the function keeps waiting infinity. Is this a bug?
>> Probably we should exclude non-backend proceses from the target
>> processes to dump? Sorry if this was already discussed.
> 
>>      SELECT pg_get_backend_memory_contexts(pid) FROM pg_stat_activity;

Thanks for trying it!

It was not discussed explicitly, and I was going to do it later
as commented.

> +		/* TODO: Check also whether backend or not. */

> 
> FWIW, I think this patch is fundamentally unsafe.  It's got a
> lot of the same problems that I complained about w.r.t. the
> nearby proposal to allow cross-backend stack trace dumping.
> It does avoid the trap of thinking that it can do work in
> a signal handler, but instead it supposes that it can do
> work involving very high-level objects such as shared hash tables
> in anyplace that might execute CHECK_FOR_INTERRUPTS.  That's
> never going to be safe: the only real expectation the system
> has is that CHECK_FOR_INTERRUPTS is called at places where our
> state is sane enough that a transaction abort can clean up.
> Trying to do things like taking LWLocks is going to lead to
> deadlocks or worse.  We need not even get into the hard questions,
> such as what happens when one process or the other exits
> unexpectedly.

Thanks for reviewing!

I may misunderstand something, but the dumper works not at
CHECK_FOR_INTERRUPTS but during the client read, i.e.,
ProcessClientReadInterrupt().

Is it also unsafe?


BTW, since there was a comment that the shared hash table
used too much memory, I'm now rewriting this patch not to use
the shared hash table but a simpler static shared memory struct.

> I also find the idea that this should be the same SQL function
> as pg_get_backend_memory_contexts to be a seriously bad decision.
> That means that it's not possible to GRANT the right to examine
> only your own process's memory --- with this proposal, that means
> granting the right to inspect every other process as well.
> 
> Beyond that, the fact that there's no way to restrict the capability
> to just, say, other processes owned by the same user means that
> it's not really safe to GRANT to non-superusers anyway.  Even with
> such a restriction added, things are problematic, since for example
> it would be possible to inquire into the workings of a
> security-definer function executing in another process that
> nominally is owned by your user.

I'm going to change the function name and restrict the executor to
superusers. Is it enough?


Regards,