Hi,
On 2024-07-09 13:12:59 -0500, Nathan Bossart wrote:
> I've committed 0001.
I justed ended up looking at this code for some boring reason. One thing that
has me worried a bit is that pg_signal_backend() now does
pgstat_get_beentry_by_proc_number(), triggering a pgstat_read_current_status()
further down.
pgstat_read_current_status() can be fairly expensive, both in CPU and in
memory. It copies each proc's activity strings, which each can be 1kB by
default!
The "saving grace" is that most of the time the pid will be sourced from
pg_stat_activity, which already will will have done
pgstat_read_current_status(). But I don't think that's always the case.
Another concern is that pgstat_read_current_status() won't actually refresh
the information if already cached, which might cause us to operate on outdated
information. A malicious user could start a transaction, cause
pgstat_read_current_status() to be called, and wait for the PID to be reused
for some interesting process, and then signal, which the outdated information
from the prior pgstat_read_current_status() would allow.
The threat of this isn't huge, there's some fundamental raciness around
pg_signal_backend() but this does open the window a lot wider.
And all of this just because we need the target proc's BackendType, a single 4
byte value.
I think it'd be better to introduce something that fetches a live
BackendType. We have such functionality already, see
pgstat_get_backend_current_activity().
Or we could have a copy of the backend type in PGPROC.
Greetings,
Andres Freund
