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,
