Re: [PATCH] Optimize ProcSignal to avoid redundant SIGUSR1 signals - Mailing list pgsql-hackers

On Tue, Jul 22, 2025, at 23:54, Thomas Munro wrote:
> On Wed, Jul 23, 2025 at 8:08 AM Joel Jacobson <joel@compiler.org> wrote:
>> Previously, ProcSignal used an array of volatile sig_atomic_t flags, one
>> per signal reason. A sender would set a flag and then unconditionally
>> send a SIGUSR1 to the target process. This could result in a storm of
>> redundant signals if multiple processes signaled the same target before
>> it had a chance to run its signal handler.
>
> It's a good idea with great results!
>
> FWIW the "new interrupts systems" patch set[1] also contains this
> idea.  But it goes further, because it also removes all the
> corresponding "pending" local variables: notice how
> HandleNotifyInterrupt() really just sets notifyInterruptPending = true
> and does SetLatch(MyLatch).  So instead of having
> CHECK_FOR_INTERRUPTS() check all those local variables, we can merge
> "procsignals" with "interrupts", and have CHECK_FOR_INTERRUPTS()
> itself read that atomic word directly, skipping the middleman and
> deleting the whole ProcSignal system.  Originally I proposed that the
> sending backend would now do SendInterrupt() directly, essentially
> moving the contents of the signal handler into the sender and setting
> the receiver's latch directly.  But then Heikki went further and
> proposed deleting latches too, and having "interrupts" as our only
> remaining inter-backend wakeup abstraction, with one of the interrupt
> flags used as a "general" one that replaces all the places that use
> SetLatch() directly today.
>
> The end state could include a WaitEventSetWait() implementation that
> waits for that single remaining atomic word with a (multiplexed) futex
> wait, also removing the "maybe sleeping" dance because futexes already
> have ideal protection against races.  Or you can send a byte to a pipe
> or post a custom event to an io_uring or kqueue or whatever.
>
> That's part of a larger project to get rid of all inter-backend
> signals, on the pathway to multi-threaded backends.  Some things along
> the way have included making it true that all real
> IPC-request-handling work is done in CHECK_FOR_INTERRUPTS(), not
> signal handlers (ie making it all "cooperative"), and making it safe
> to use uint32_t atomics at all in signal handlers as you're doing here
> (previously they were allowed to be emulated with spinlocks), as an
> intermediate step because in the short term we still have signals for
> timers.
>
> I'm planning to post a rebase of that work with some improvements
> soon.  Hmm, I wonder if there is any sense in restructuring it so that
> your patch becomes an incremental stepping stone.  I think one of our
> problems might have been trying to change too many things at once.
> What you're doing is certainly independently good, but I haven't
> studied your patch too closely yet.
>
> [1]
> https://www.postgresql.org/message-id/flat/CA%2BhUKG%2B3MkS21yK4jL4cgZywdnnGKiBg0jatoV6kzaniBmcqbQ%40mail.gmail.com

Great to hear you're also working in this area!

I like the idea of incremental stepping stones,
I can certainly help both reviewing and participating in that work.

To give you an idea of the size of my patch:

 src/backend/storage/ipc/procsignal.c | 103
++++++++++++++++++++++++++++++++++++++++++++++++++++++-------------------------------------------------
 src/include/storage/procsignal.h     |   3 +++
 2 files changed, 57 insertions(+), 49 deletions(-)

When you rebase your patchset, I can try to see if there is a natural fit.

/Joel