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