Re: Interrupts vs signals

Hi,

On 2026-02-20 16:22:48 +0200, Heikki Linnakangas wrote:
> Patch attached. The relevant changes for this "attention mechanism" are in
> the last patch, but there are some other small changes too so this split
> into patches is a little messy. I moved the list of standard interrupts to a
> separate header file, for example. So for reviewing, I recommend reading the
> resulting interrupt.h and interrupt.c files after applying all the patches,
> instead of trying to read the diff for those.

I was just looking at these patches. They needed a rebase. Attached without
other changes.


A few comments:

- I don't think I really understand the interrupts.h / standard_interrupts.h
  split.  Also, the latter doesn't have the normal function header etc.


- It probably doesn't matter, but it seems that for SendOrRaiseInterrupt()
  could we skip the setting of PI_FLAG_ATTENTION if already set?

- I see a bunch of core files like this:

#4  0x0000000000d6a4d8 in ExceptionalCondition (conditionName=0x1089ff8 "(pg_atomic_read_u32(&MyPendingInterrupts->flags) & PI_FLAG_SLEEPING) == 0",
    fileName=0x1089f70 "../../../../../home/andres/src/postgresql/src/backend/ipc/interrupt.c", lineNumber=262)
    at ../../../../../home/andres/src/postgresql/src/backend/utils/error/assert.c:65
#5  0x000000000091aa12 in SwitchMyPendingInterruptsPtr (new_ptr=0x161dc30 <LocalPendingInterrupts>) at ../../../../../home/andres/src/postgresql/src/backend/ipc/interrupt.c:262
#6  0x000000000091aaaa in SwitchToLocalInterrupts () at ../../../../../home/andres/src/postgresql/src/backend/ipc/interrupt.c:313
#7  0x0000000000b71dca in AuxiliaryProcKill (code=1, arg=4) at ../../../../../home/andres/src/postgresql/src/backend/storage/lmgr/proc.c:1075
#8  0x0000000000b411c4 in shmem_exit (code=1) at ../../../../../home/andres/src/postgresql/src/backend/storage/ipc/ipc.c:282
#9  0x0000000000b40faa in proc_exit_prepare (code=1) at ../../../../../home/andres/src/postgresql/src/backend/storage/ipc/ipc.c:198
#10 0x0000000000b40ef6 in proc_exit (code=1) at ../../../../../home/andres/src/postgresql/src/backend/storage/ipc/ipc.c:113
#11 0x0000000000b53ba6 in WaitEventSetWaitBlock (set=0x257a1b40, cur_timeout=200, occurred_events=0x7fff8d4db130, nevents=1)
    at ../../../../../home/andres/src/postgresql/src/backend/storage/ipc/waiteventset.c:1306
#12 0x0000000000b53922 in WaitEventSetWait (set=0x257a1b40, timeout=200, occurred_events=0x7fff8d4db130, nevents=1, wait_event_info=83886083)
    at ../../../../../home/andres/src/postgresql/src/backend/storage/ipc/waiteventset.c:1170
#13 0x000000000091ae79 in WaitInterrupt (interruptMask=122, wakeEvents=41, timeout=200, wait_event_info=83886083)
    at ../../../../../home/andres/src/postgresql/src/backend/ipc/interrupt.c:482
#14 0x0000000000a7629c in BackgroundWriterMain (startup_data=0x0, startup_data_len=0) at ../../../../../home/andres/src/postgresql/src/backend/postmaster/bgwriter.c:298
#15 0x0000000000a7869a in postmaster_child_launch (child_type=B_BG_WRITER, child_slot=35, startup_data=0x0, startup_data_len=0, client_sock=0x0)
    at ../../../../../home/andres/src/postgresql/src/backend/postmaster/launch_backend.c:268

  interestingly that doesn't trigger any test failures.


- Are we generally ok with having no memory barriers around
  CHECK_FOR_INTERRUPTS()/InterruptPending()/ConsumeInterrupt()? It might be,
  because we shouldn't rely on immediate processing of the interrupts, but at
  the very least we need some documentation for why that is true.

  Practically it's probably ok, even on architectures with loose memory
  ordering, we don't typically have loops that do something like

  while (true)
  {
      CFI();
      very_cpu_intensive_but_nothing_else();
  }

  And once you actually wait for an interrupt, lock a page, or ... we'll be
  guaranteed to see the interrupt.


- CFI now is:

  ; determine address of MyPendingInterruptsFlags
  mov    0x0(%rip),%rax        # 3c9 <ExecScan+0x3c9>
                        3c5: R_X86_64_REX_GOTPCRELX     MyPendingInterruptsFlags-0x4

  ; load *MyPendingInterruptsFlags
  mov    (%rax),%rax

  ; load the contents of flags
  mov    (%rax),%eax

  ; test if flags is 0
  test   %eax,%eax

  ; jump if flags are set
  jne    51b <ExecScan+0x51b>


  That's one indirection worse than what you had in
  https://www.postgresql.org/message-id/78c102b3-f1a6-4c70-a46f-2f04221b6193%40iki.fi

  I don't know if it matters.


- This continues to be a very very large commit.  As I IIRC suggested before,
  I'd at least introduce ipc/interrupt.h (including adding the #includes for
  it)

  I'd probably go further and introduce the new infra separately from
  switching over to it, but...


- Wonder if it's worth putting the WIN32 stuff from InitProcGlobal() into its
  own helper?  Probably not, at least until there's another platform specific
  thing?


- Does it matter that we have a bunch of code that does
  if (InterruptPending(a))
      stuff();
  if (InterruptPending(b))
      stuff();
  if (ConsumeInterrupt(c))
      stuff();
  if (ConsumeInterrupt(d))
      stuff();

  that does end up rereading the same variable from shared memory
  repeatedly...


- Personally I rather dislike anonymous struct types (like PendingInterrupts),
  as some debuggers and similar tooling won't be able to show names for them.


- I wonder if we should abstract the PG_HAVE_ATOMIC_U64_SIMULATION crap
  somehow (or just remove platforms without it, but I don't think I want to be
  the one doing that fight)


Greetings,

Andres Freund