Re: Interrupts vs signals - Mailing list pgsql-hackers

Hi,

A rebased version would be good, there are some not-entirely-trivial
conflicts.

> One notable change is that I merged storage/interrupt.[ch] with
> postmaster/interrupt.[ch], so the awkwardness of having two files with same
> name is gone.

I don't particularly like that. postmaster/interrupt.[ch] now is some very
general infrastructure, making postmaster/ inappropriate. Kinda wondering
about going the other way round.




On 2025-03-06 18:43:47 +0200, Heikki Linnakangas wrote:
> Subject: [PATCH v7 1/4] Replace Latches with Interrupts

> diff --git a/src/backend/postmaster/interrupt.c b/src/backend/postmaster/interrupt.c
> index 0ae9bf906ec..4cde7d52bd3 100644
> --- a/src/backend/postmaster/interrupt.c
> +++ b/src/backend/postmaster/interrupt.c
> @@ -1,13 +1,13 @@
>  /*-------------------------------------------------------------------------
>   *
>   * interrupt.c
> - *      Interrupt handling routines.
> + *      Inter-process interrupts
>   *
> - * Portions Copyright (c) 1996-2025, PostgreSQL Global Development Group
> + * Portions Copyright (c) 1996-2024, PostgreSQL Global Development Group
>   * Portions Copyright (c) 1994, Regents of the University of California
>   *
>   * IDENTIFICATION
> - *      src/backend/postmaster/interrupt.c
> + *      src/backend/storage/ipc/interrupt.c


I don't think this renaming was actually done :)



> +/*
> + * Switch to local interrupts.  Other backends can't send interrupts to this
> + * one.  Only RaiseInterrupt() can set them, from inside this process.
> + */
> +void
> +SwitchToLocalInterrupts(void)
> +{
> +    if (MyPendingInterrupts == &LocalPendingInterrupts)
> +        return;
> +
> +    MyPendingInterrupts = &LocalPendingInterrupts;
> +
> +    /*
> +     * Make sure that SIGALRM handlers that call RaiseInterrupt() are now
> +     * seeing the new MyPendingInterrupts destination.
> +     */
> +    pg_memory_barrier();
> +
> +    /*
> +     * Mix in the interrupts that we have received already in our shared
> +     * interrupt vector, while atomically clearing it.  Other backends may
> +     * continue to set bits in it after this point, but we've atomically
> +     * transferred the existing bits to our local vector so we won't get
> +     * duplicated interrupts later if we switch backx.
> +     */
> +    pg_atomic_fetch_or_u32(MyPendingInterrupts,
> +                           pg_atomic_exchange_u32(&MyProc->pendingInterrupts, 0));
> +}

pg_atomic_fetch_or_u32 is a barrier, so I'm not following why we need the
pg_memory_barrier().



> +/*
> + * Set an interrupt flag in this backend.
> + */
> +void
> +RaiseInterrupt(uint32 interruptMask)

I think it'd be good to mention that this needs to be async-signal-safe...


> +{
> +    uint32        old_pending;
> +
> +    old_pending = pg_atomic_fetch_or_u32(MyPendingInterrupts, interruptMask);

This makes interrupts a tad more expensive than before, previously we only had
a memory barrier (i.e. an atomic op on a backend-local var), now it's on
shared state.  Most likely fine.


> +    /*
> +     * If the process is currently blocked waiting for an interrupt to arrive,
> +     * and the interrupt wasn't already pending, wake it up.
> +     */
> +    if ((old_pending & (interruptMask | SLEEPING_ON_INTERRUPTS)) == SLEEPING_ON_INTERRUPTS)
> +        WakeupMyProc();
> +}



> +/*
> + * Set an interrupt flag in another backend.
> + */
> +void
> +SendInterrupt(uint32 interruptMask, ProcNumber pgprocno)
> +{
> +    PGPROC       *proc;
> +    uint32        old_pending;
> +
> +    Assert(pgprocno != INVALID_PROC_NUMBER);
> +    Assert(pgprocno >= 0);
> +    Assert(pgprocno < ProcGlobal->allProcCount);
> +
> +    proc = &ProcGlobal->allProcs[pgprocno];
> +    old_pending = pg_atomic_fetch_or_u32(&proc->pendingInterrupts, interruptMask);
> +
> +    /*
> +     * If the process is currently blocked waiting for an interrupt to arrive,
> +     * and the interrupt wasn't already pending, wake it up.
> +     */
> +    if ((old_pending & (interruptMask | SLEEPING_ON_INTERRUPTS)) == SLEEPING_ON_INTERRUPTS)
> +        WakeupOtherProc(proc);
> +}

I wonder if we shouldn't share the implementation of these two functions...



> @@ -205,6 +206,8 @@ StartupProcExit(int code, Datum arg)
>      /* Shutdown the recovery environment */
>      if (standbyState != STANDBY_DISABLED)
>          ShutdownRecoveryTransactionEnvironment();
> +
> +    ProcGlobal->startupProc = INVALID_PROC_NUMBER;
>  }

What if we instead had a ProcGlobal->auxProc[auxproxtype]? We have different
versions of this for different types auf aux processes, which doesn't really
make sense.


> +         * Perform a plain atomic read first as a fast path for the case that
> +         * an interrupt is already pending.
>           */
> -        if (set->latch && !set->latch->is_set)
> +        old_mask = pg_atomic_read_u32(MyPendingInterrupts);
> +        already_pending = ((old_mask & set->interrupt_mask) != 0);
> +
> +        if (!already_pending)
>          {

Hm, I think this may be incorrect - what if there is *some* overlap between
MyPendingInterrupts and set->interrupt_mask, but they're not equal?


> +            /* If we're not done, update cur_timeout for next iteration */
> +            if (timeout >= 0)
> +            {
> +                INSTR_TIME_SET_CURRENT(cur_time);
> +                INSTR_TIME_SUBTRACT(cur_time, start_time);
> +                cur_timeout = timeout - (long) INSTR_TIME_GET_MILLISEC(cur_time);
> +                if (cur_timeout <= 0)
> +                    break;
> +            }

This is completely orthogonal, but: It bothers me that we use millisecons
rather than microseconds...



> diff --git a/src/backend/storage/lmgr/proc.c b/src/backend/storage/lmgr/proc.c
> index 749a79d48ef..4ebff9a4b37 100644
> --- a/src/backend/storage/lmgr/proc.c
> +++ b/src/backend/storage/lmgr/proc.c
> @@ -39,6 +39,7 @@
>  #include "miscadmin.h"
>  #include "pgstat.h"
>  #include "postmaster/autovacuum.h"
> +#include "postmaster/interrupt.h"
>  #include "replication/slotsync.h"
>  #include "replication/syncrep.h"
>  #include "storage/condition_variable.h"
> @@ -258,14 +259,28 @@ InitProcGlobal(void)
>          Assert(fpPtr <= fpEndPtr);
>  
>          /*
> -         * Set up per-PGPROC semaphore, latch, and fpInfoLock.  Prepared xact
> -         * dummy PGPROCs don't need these though - they're never associated
> -         * with a real process
> +         * Set up per-PGPROC semaphore, interrupt wakeup event (on Windows),
> +         * and fpInfoLock.  Prepared xact dummy PGPROCs don't need these
> +         * though - they're never associated with a real process
>           */
>          if (i < MaxBackends + NUM_AUXILIARY_PROCS)
>          {
> +#ifdef WIN32
> +            SECURITY_ATTRIBUTES sa;
> +
> +            /*
> +             * Set up security attributes to specify that the events are
> +             * inherited.
> +             */
> +            ZeroMemory(&sa, sizeof(sa));
> +            sa.nLength = sizeof(sa);
> +            sa.bInheritHandle = TRUE;
> +
> +            proc->interruptWakeupEvent = CreateEvent(&sa, TRUE, FALSE, NULL);
> +            if (proc->interruptWakeupEvent == NULL)
> +                elog(ERROR, "CreateEvent failed: error code %lu", GetLastError());
> +#endif
>              proc->sem = PGSemaphoreCreate();
> -            InitSharedLatch(&(proc->procLatch));
>              LWLockInitialize(&(proc->fpInfoLock), LWTRANCHE_LOCK_FASTPATH);
>          }
>  

What's the story behind this aspect of the change? Does it have to be done as
part of a rather huge and largely mechanical commit?


> +/*
> + * Test an interrupt flag (of flags).
> + */
> +static inline bool
> +IsInterruptPending(uint32 interruptMask)
> +{
> +    return (pg_atomic_read_u32(MyPendingInterrupts) & interruptMask) != 0;
> +}

Do we need to care about memory ordering?

>
> +extern PGDLLIMPORT volatile sig_atomic_t ConfigReloadPending;
> +extern PGDLLIMPORT volatile sig_atomic_t ShutdownRequestPending;

I'm not a huge fan of these being declared in the same file as the low-level
implementation of interrupts...



> From e623492590052f16841ffa171383766eb157074b Mon Sep 17 00:00:00 2001
> From: Heikki Linnakangas <heikki.linnakangas@iki.fi>
> Date: Thu, 6 Mar 2025 18:16:16 +0200
> Subject: [PATCH v7 4/4] Replace ProcSignals and other ad hoc signals with
>  interrupts
> 
> This replaces the INTERRUPT_GENERAL, or setting the process's latch
> before that, with more fine-grained interrupts. All the ProcSignals
> are converted into interrupts, as well as things like timers and
> config-reload requests.
> 
> Most callers of WaitInterrupt now use INTERRUPT_CFI_MASK(), which is
> a bitmask that includes all the interrupts that are handled by
> CHECK_FOR_INTERRUPTS() in the current state. CHECK_FOR_INTERRUPTS()
> now clears the interrupts bits that it processes, the caller is no
> longer required to do ClearInterrupt (or ResetLatch) for those
> bits. If you wake up for other interrupts that are not handled by
> CHECK_FOR_INTERRUPTS(), you still need to clear those.
> 
> Now that there are separate bits for different things, it's possible
> to do e.g. WaitInterrupt(INTERRUPT_CONFIG_RELOAD |
> INTERRUPT_QUERY_CANCEL) to wait just for a config reload request or
> query cancellation, and not wake up on other events like a shutdown
> request. That's not very useful in practice, as CHECK_FOR_INTERRUPTS()
> still processes those interrupts if you call it, and you would want to
> process all the standard interrupts promptly anyway. It might be
> useful to check for specific interrupts with InterruptPending(),
> without immediately processing them, however. spgdoinsert() does
> something like that. In this commit I didn't change its overall logic,
> but it probably could be made more fine-grained now.
> 
> More importantly, you can now have additional interrupts that are
> processed less frequently and do not wake up the backend when it's not
> prepared to process them. For example, the SINVAL_CATCHUP interrupt is
> only processed when the backend is idle. Previously, a catchup request
> would wake up the process whether it was ready to handle it or not;
> now it only wakes up when the backend is idle. That's not too
> significant for catchup interrupts because they don't occur that often
> anyway, but it's still nice and could be more significant with other
> interrupts.
> 
> There's still a general-purpose INTERRUPT_GENERAL interrupt that is
> multiplexed for many different purposes in different processes, for
> example to wake up the walwriter when it has work to do, and to wake
> up processes waiting on a condition variable. The common property of
> those uses is that there's some other flag or condition that is
> checked on each wakeup, the wakeup interrupt itself means merely that
> something interesting might've happened.

I guess extensions that just need to use INTERRUPT_GENERAL, given that new
interrupt reasons can't be dynamically registered?


> diff --git a/src/backend/access/transam/xlogrecovery.c b/src/backend/access/transam/xlogrecovery.c
> index c244f6c5ec0..4837739a868 100644
> --- a/src/backend/access/transam/xlogrecovery.c
> +++ b/src/backend/access/transam/xlogrecovery.c
> @@ -3004,18 +3004,12 @@ recoveryApplyDelay(XLogReaderState *record)
>  
>      while (true)
>      {
> -        /*
> -         * INTERRUPT_GENERAL is used for all the usual interrupts, like config
> -         * reloads.  The wakeups when more WAL arrive use a different
> -         * interrupt number (INTERRUPT_RECOVERY_CONTINUE) so that more WAL
> -         * arriving don't wake up the startup process excessively when we're
> -         * waiting in other places, like for recovery conflicts.
> -         */
> -        ClearInterrupt(INTERRUPT_GENERAL);
> -
>          /* This might change recovery_min_apply_delay. */
>          ProcessStartupProcInterrupts();
>  
> +        /*
> +         * INTERRUPT_RECOVERY_CONTINUE indicates that more WAL has arrived.
> +         */
>          ClearInterrupt(INTERRUPT_RECOVERY_CONTINUE);
>          if (CheckForStandbyTrigger())
>              break;

Why is it named RECOVERY_CONTINUE rather than WAL_ARRIVED or such?



> @@ -163,23 +165,15 @@ throttle(bbsink_throttle *sink, size_t increment)
>          if (sleep <= 0)
>              break;
>  
> -        ClearInterrupt(INTERRUPT_GENERAL);
> -
> -        /* We're eating a wakeup, so check for interrupts */
> -        CHECK_FOR_INTERRUPTS();
> -
>          /*
>           * (TAR_SEND_SIZE / throttling_sample * elapsed_min_unit) should be
>           * the maximum time to sleep. Thus the cast to long is safe.
>           */
> -        wait_result = WaitInterrupt(INTERRUPT_GENERAL,
> +        wait_result = WaitInterrupt(INTERRUPT_CFI_MASK(),
>                                      WL_INTERRUPT | WL_TIMEOUT | WL_EXIT_ON_PM_DEATH,
>                                      (long) (sleep / 1000),
>                                      WAIT_EVENT_BASE_BACKUP_THROTTLE);
>  
> -        if (wait_result & WL_INTERRUPT)
> -            CHECK_FOR_INTERRUPTS();
> -

Not specific to this code: Why is INTERRUPT_CFI_MASK() a function like macro?



> @@ -1834,11 +1779,10 @@ HandleNotifyInterrupt(void)
>  void
>  ProcessNotifyInterrupt(bool flush)
>  {
> -    if (IsTransactionOrTransactionBlock())
> -        return;                    /* not really idle */
> +    Assert(!IsTransactionOrTransactionBlock());
>  
>      /* Loop in case another signal arrives while sending messages */
> -    while (notifyInterruptPending)
> +    while (ConsumeInterrupt(INTERRUPT_ASYNC_NOTIFY))
>          ProcessIncomingNotify(flush);
>  }

It's not new as of this change, but doesn't this have the danger that we
process notifies endlessly while not accepting e.g. a termination interrupt?


> +    if (ConsumeInterrupt(INTERRUPT_CONFIG_RELOAD))
>      {
>          int            autovacuum_max_workers_prev = autovacuum_max_workers;
>  
> -        ConfigReloadPending = false;
>          ProcessConfigFile(PGC_SIGHUP);
>  
>          /* shutdown requested in config file? */
> @@ -771,11 +774,11 @@ ProcessAutoVacLauncherInterrupts(void)
>      }
>  
>      /* Process barrier events */
> -    if (ProcSignalBarrierPending)
> +    if (IsInterruptPending(INTERRUPT_BARRIER))
>          ProcessProcSignalBarrier();

Why do we have some code immediately consuming and others just checking if an
interrupt is pending?


> @@ -572,10 +574,18 @@ CheckpointerMain(const void *startup_data, size_t startup_data_len)
>              cur_timeout = Min(cur_timeout, XLogArchiveTimeout - elapsed_secs);
>          }
>  
> -        (void) WaitInterrupt(INTERRUPT_GENERAL,
> -                             WL_INTERRUPT | WL_TIMEOUT | WL_EXIT_ON_PM_DEATH,
> -                             cur_timeout * 1000L /* convert to ms */ ,
> -                             WAIT_EVENT_CHECKPOINTER_MAIN);
> +        (void) WaitInterrupt(
> +            /* these are handled in the main loop */
> +            INTERRUPT_GENERAL |     /* checkpoint requested */

Why is a checkpoint request done via GENERAL? Seems that's specifically one
that we do *not* want to absorb via CFI?


> +            INTERRUPT_SHUTDOWN_XLOG |
> +            INTERRUPT_SHUTDOWN_AUX |
> +            /* ProcessCheckpointerInterrupts() */
> +            INTERRUPT_BARRIER |
> +            INTERRUPT_LOG_MEMORY_CONTEXT |
> +            INTERRUPT_CONFIG_RELOAD,

It seems like this will somewhat painful to maintain over time. Whenever some
relatively generic type of interrupt is added, all the aux processes need to
be updated...


> @@ -584,7 +594,7 @@ CheckpointerMain(const void *startup_data, size_t startup_data_len)
>       */
>      ExitOnAnyError = true;
>  
> -    if (ShutdownXLOGPending)
> +    if (IsInterruptPending(INTERRUPT_SHUTDOWN_XLOG))
>      {
>          /*
>           * Close down the database.
> @@ -602,7 +612,7 @@ CheckpointerMain(const void *startup_data, size_t startup_data_len)
>           * Tell postmaster that we're done.
>           */
>          SendPostmasterSignal(PMSIGNAL_XLOG_IS_SHUTDOWN);
> -        ShutdownXLOGPending = false;
> +        ClearInterrupt(INTERRUPT_SHUTDOWN_XLOG);
>      }

Why not use ConsumeInterrupt()?


> @@ -283,20 +287,17 @@ WaitInterruptOrSocket(uint32 interruptMask, int wakeEvents, pgsocket sock,
>  void
>  ProcessMainLoopInterrupts(void)
>  {
> -    if (ProcSignalBarrierPending)
> +    if (IsInterruptPending(INTERRUPT_BARRIER))
>          ProcessProcSignalBarrier();
>  
> -    if (ConfigReloadPending)
> -    {
> -        ConfigReloadPending = false;
> +    if (ConsumeInterrupt(INTERRUPT_CONFIG_RELOAD))
>          ProcessConfigFile(PGC_SIGHUP);
> -    }
>  
> -    if (ShutdownRequestPending)
> +    if (IsInterruptPending(INTERRUPT_SHUTDOWN_AUX))
>          proc_exit(0);
>  
>      /* Perform logging of memory contexts of this process */
> -    if (LogMemoryContextPending)
> +    if (IsInterruptPending(INTERRUPT_LOG_MEMORY_CONTEXT))
>          ProcessLogMemoryContextInterrupt();
>  }

This got more expensive with this change. Presumably matters more for
ProcessInterrupts, but I haven't gotten there yet :)

The problem is that each of the IsInterruptPending/ConsumeInterrupt calls will
do its own read of the interrupt mask, which now includes a read memory
barrier. On Arm or such that means we'll inject multiple subsequent memory
barriers into code that was just checking a few local vars beforehand.



> @@ -304,14 +305,13 @@ ProcessMainLoopInterrupts(void)
>   * Simple signal handler for triggering a configuration reload.
>   *
>   * Normally, this handler would be used for SIGHUP. The idea is that code
> - * which uses it would arrange to check the ConfigReloadPending flag at
> - * convenient places inside main loops, or else call ProcessMainLoopInterrupts.
> + * which uses it would arrange to check the INTERRUPT_CONFIG_RELOAD interrupt at
> + * convenient places inside main loops, or else call HandleMainLoopInterrupts.
>   */
>  void
>  SignalHandlerForConfigReload(SIGNAL_ARGS)
>  {
> -    ConfigReloadPending = true;
> -    RaiseInterrupt(INTERRUPT_GENERAL);
> +    RaiseInterrupt(INTERRUPT_CONFIG_RELOAD);
>  }

Somewhat funny that we continue to use SIGHUP from another process, just to
then raise an interrupt...


>          if (rc & WL_INTERRUPT)
>          {
> -            ClearInterrupt(INTERRUPT_GENERAL);
>              CHECK_FOR_INTERRUPTS();
> +            ClearInterrupt(INTERRUPT_GENERAL);
>          }

Huh, what is this type of change about?


> @@ -3488,13 +3489,13 @@ GetConflictingVirtualXIDs(TransactionId limitXmin, Oid dbOid)
>   * Returns pid of the process signaled, or 0 if not found.
>   */
>  pid_t
> -CancelVirtualTransaction(VirtualTransactionId vxid, ProcSignalReason sigmode)
> +CancelVirtualTransaction(VirtualTransactionId vxid, InterruptType reason)
>  {
> -    return SignalVirtualTransaction(vxid, sigmode, true);
> +    return SignalVirtualTransaction(vxid, reason, true);
>  }
>  
>  pid_t
> -SignalVirtualTransaction(VirtualTransactionId vxid, ProcSignalReason sigmode,
> +SignalVirtualTransaction(VirtualTransactionId vxid, InterruptType reason,
>                           bool conflictPending)
>  {
>      ProcArrayStruct *arrayP = procArray;
> @@ -3522,7 +3523,7 @@ SignalVirtualTransaction(VirtualTransactionId vxid, ProcSignalReason sigmode,
>                   * Kill the pid if it's still here. If not, that's what we
>                   * wanted so ignore any errors.
>                   */
> -                (void) SendProcSignal(pid, sigmode, vxid.procNumber);
> +                SendInterrupt(reason, vxid.procNumber);
>              }
>              break;
>          }

This made me look at SignalVirtualTransaction() and I'm somewhat shocked. What
on earth are we doing?

pid_t
SignalVirtualTransaction(VirtualTransactionId vxid, InterruptType reason,
                         bool conflictPending)
{
    ProcArrayStruct *arrayP = procArray;
    int            index;
    pid_t        pid = 0;

    LWLockAcquire(ProcArrayLock, LW_SHARED);

    for (index = 0; index < arrayP->numProcs; index++)
    {
        int            pgprocno = arrayP->pgprocnos[index];
        PGPROC       *proc = &allProcs[pgprocno];
        VirtualTransactionId procvxid;

        GET_VXID_FROM_PGPROC(procvxid, *proc);

        if (procvxid.procNumber == vxid.procNumber &&
            procvxid.localTransactionId == vxid.localTransactionId)
        {
            proc->recoveryConflictPending = conflictPending;
            pid = proc->pid;
            if (pid != 0)
            {
                /*
                 * Kill the pid if it's still here. If not, that's what we
                 * wanted so ignore any errors.
                 */
                SendInterrupt(reason, vxid.procNumber);
            }
            break;
        }
    }

    LWLockRelease(ProcArrayLock);

    return pid;
}

We're iterating over the whole procarray, for each proc entry, we extract the
procNumber from the vxid and the backend and check if they're the same.  Even
though the procNumber is the friggin index into the procarray!  What the hell?



> @@ -947,6 +953,7 @@ void
>  StandbyDeadLockHandler(void)
>  {
>      got_standby_deadlock_timeout = true;
> +    RaiseInterrupt(INTERRUPT_GENERAL);
>  }
>  
>  /*
> @@ -956,6 +963,7 @@ void
>  StandbyTimeoutHandler(void)
>  {
>      got_standby_delay_timeout = true;
> +    RaiseInterrupt(INTERRUPT_GENERAL);
>  }
>  
>  /*
> @@ -965,6 +973,7 @@ void
>  StandbyLockTimeoutHandler(void)
>  {
>      got_standby_lock_timeout = true;
> +    RaiseInterrupt(INTERRUPT_GENERAL);
>  }

Why are these using INTERRUPT_GENERAL?


> +
> +    if (ConsumeInterrupt(INTERRUPT_WALSND_INIT_STOPPING))
> +        ProcessWalSndInitStopping();

Huh. So ProcessWalSndInitStopping() raises different inerrupts to actually
exit and thus relies on being called first? That seems rather clunky.


> diff --git a/src/include/miscadmin.h b/src/include/miscadmin.h
> index f5b773b79e5..08de6146800 100644
> --- a/src/include/miscadmin.h
> +++ b/src/include/miscadmin.h
> @@ -28,124 +28,28 @@
>  #include "datatype/timestamp.h" /* for TimestampTz */
>  #include "pgtime.h"                /* for pg_time_t */
>  
> +/*
> + * for backwards-compatibility: CHECK_FOR_INTERRUPTS() used to be here, and it's
> + * used all over the place
> + */
> +#ifndef FRONTEND
> +#include "postmaster/interrupt.h"
> +#endif

Seems rather bonkers for frontend code to include this. I see that it's
required right now, but man, that feels wrong.

I don't think it's great to include atomics.h, waiteventset.h in quite that
many backend files either.


> + * Waiting on an interrupt
> + * -----------------------
> + *
> + * The correct pattern to wait for event(s) using INTERRUPT_GENERAL (or any
> + * bespoken interrupt flag) is:
>   *
>   * for (;;)
>   * {
> + *       CHECK_FOR_INTERRUPTS();
> + *
>   *       ClearInterrupt(INTERRUPT_GENERAL);
>   *       if (work to do)
>   *           Do Stuff();
> - *       WaitInterrupt(INTERRUPT_GENERAL, ...);
> + *       WaitInterrupt(INTERRUPT_CFI_MASAK() | INTERRUPT_GENERAL, ...);
>   * }
>   *
>   * It's important to clear the interrupt *before* checking if there's work to
> - * do. Otherwise, if someone sets the interrupt between the check and the
> + * do.  Otherwise, if someone sets the interrupt between the check and the
>   * ClearInterrupt() call, you will miss it and Wait will incorrectly block.

Isn't the change to move CHECK_FOR_INTERRUPTS() before ClearInterrupt()
violating what the paragraph explains?


>  /*
>   * Flags in the pending interrupts bitmask. Each value represents one bit in
>   * the bitmask.
>   */
> -typedef enum
> +typedef enum InterruptType
>  {

I'm rather concerned about the number of interrupt bits we've already
consume. I'll respond about that in a separate, higher-level, email.


>  /*
> - * Clear an interrupt flag.
> + * Clear an interrupt flag (or flags).
>   */
>  static inline void
>  ClearInterrupt(uint32 interruptMask)
>  {
>      pg_atomic_fetch_and_u32(MyPendingInterrupts, ~interruptMask);
> +    pg_write_barrier();
>  }

pg_atomic_fetch_and_u32 is a full barrier, no separate barrier needed.



>  #endif
> diff --git a/src/include/postmaster/startup.h b/src/include/postmaster/startup.h
> index 158f52255a6..a0316202b95 100644
> --- a/src/include/postmaster/startup.h
> +++ b/src/include/postmaster/startup.h
> @@ -25,6 +25,14 @@
>  
>  extern PGDLLIMPORT int log_startup_progress_interval;
>  
> +/* The set of interrupts that are processed by ProcessStartupProcInterrupts */
> +#define INTERRUPT_STARTUP_PROC_MASK    (            \
> +        INTERRUPT_BARRIER |                        \
> +        INTERRUPT_DIE |                            \
> +        INTERRUPT_LOG_MEMORY_CONTEXT |            \
> +        INTERRUPT_CONFIG_RELOAD                    \
> +        )

Somehow I find this name a bit confusing, the first parse attempt ends up with
PROC_MASK as one of the components of the name.  How about
INTERRUPT_MASK_STARTUP[_PROC]?



Greetings,

Andres Freund