Re: Interrupts vs signals

On 10/07/2024 09:48, Thomas Munro wrote:
> The next problems to remove are, I think, the various SIGUSR2, SIGINT,
> SIGTERM signals sent by the postmaster. These should clearly become
> SendInterrupt() or ProcSetLatch(). The problem here is that the
> postmaster doesn't have the proc numbers yet. One idea is to teach
> the postmaster to assign them! Not explored yet.

With my latest patches on the "Refactoring postmaster's code to cleanup
after child exit" thread [1], every postmaster child process is assigned
a slot in the pmsignal.c array, including all the aux processes. If we
moved 'pending_interrupts' and the process Latch to the pmsignal.c
array, then you could send an interrupt also to a process that doesn't
have a PGPROC entry. That includes dead-end backends, backends that are
still in authentication, and the syslogger.

That would also make it so that the postmaster would never need to poke
into the procarray. pmsignal.c is already designated as the limited
piece of shared memory that is accessed by the postmaster
(BackgroundWorkerSlots is the other exception), so it would be kind of
nice if all the information that the postmaster needs to send an
interrupt was there. That would mean that where you currently use a
ProcNumber to identify a process, you'd use an index into the
PMSignalState array instead.

I don't insist on changing that right now, I think this patch is OK as
it is, but that might be a good next step later.

[1]
https://www.postgresql.org/message-id/8f2118b9-79e3-4af7-b2c9-bd5818193ca4%40iki.fi

I'm also wondering about the relationship between interrupts and
latches. Currently, SendInterrupt sets a latch to wake up the target
process. I wonder if it should be the other way 'round? Move all the
wakeup code, with the signalfd, the self-pipe etc to interrupt.c, and in
SetLatch, call SendInterrupt to wake up the target process? Somehow that
feels more natural to me, I think.

> This version is passing on Windows. I'll create a CF entry. Still
> work in progress!

Some comments on the patch details:

> ereport(WARNING,
> (errmsg("NOTIFY queue is %.0f%% full", fillDegree * 100),
> - (minPid != InvalidPid ?
> - errdetail("The server process with PID %d is among those with
the oldest transactions.", minPid)
> + (minPgprocno != INVALID_PROC_NUMBER ?
> + errdetail("The server process with pgprocno %d is among those
with the oldest transactions.", minPgprocno)
> : 0),
> - (minPid != InvalidPid ?
> + (minPgprocno != INVALID_PROC_NUMBER ?
> errhint("The NOTIFY queue cannot be emptied until that process
ends its current transaction.")
> : 0)));

This makes the message less useful to the user, as the ProcNumber isn't
exposed to users. With the PID, you can do "pg_terminate_backend(pid)"

> diff --git a/src/backend/optimizer/util/pathnode.c
b/src/backend/optimizer/util/pathnode.c
> index c42742d2c7b..bfb89049020 100644
> --- a/src/backend/optimizer/util/pathnode.c
> +++ b/src/backend/optimizer/util/pathnode.c
> @@ -18,6 +18,7 @@
>
> #include "foreign/fdwapi.h"
> #include "miscadmin.h"
> +#include "postmaster/interrupt.h"
> #include "nodes/extensible.h"
> #include "optimizer/appendinfo.h"
> #include "optimizer/clauses.h"

misordered

> + * duplicated interrupts later if we switch backx.

typo: backx -> back

> - if (IdleInTransactionSessionTimeoutPending)
> + if (ConsumeInterrupt(INTERRUPT_IDLE_TRANSACTION_TIMEOUT))
> {
> /*
> * If the GUC has been reset to zero, ignore the signal. This is
> @@ -3412,7 +3361,6 @@ ProcessInterrupts(void)
> * interrupt. We need to unset the flag before the injection point,
> * otherwise we could loop in interrupts checking.
> */
> - IdleInTransactionSessionTimeoutPending = false;
> if (IdleInTransactionSessionTimeout > 0)
> {
> INJECTION_POINT("idle-in-transaction-session-timeout");

The "We need to unset the flag.." comment is a bit out of place now,
since the flag was already cleared by ConsumeInterrupt(). Same in the
INTERRUPT_TRANSACTION_TIMEOUT and INTERRUPT_IDLE_SESSION_TIMEOUT
handling after this.

--
Heikki Linnakangas
Neon (https://neon.tech)