Some feedback on v4-0001-Replace-Latches-with-Interrupts.patch:
+ latch.c -> waiteventset.c
+1
+ /*
+ * INTERRUPT_RECOVERY_WAKEUP is used to wake up startup process, to tell
+ * it that it should continue WAL replay. It's sent by WAL receiver when
+ * more WAL arrives, or when promotion is requested.
+ */
+ INTERRUPT_RECOVERY_CONTINUE,
Names don't match here. I prefer _CONTINUE. As for the general one,
I'm on the fence about INTERRUPT_GENERAL_WAKEUP, since wakeups aren't
necessarily involved, but I don't have a specific better idea so I'm
not objecting... Perhaps it's more like INTERRUPT_GENERAL_NOTIFY,
except that _NOTIFY is already a well known thing, and the procsignal
patch introduces INTERRUPT_NOTIFY...
It looks like maybeSleepingOnInterrupts replaces maybe_sleeping, and
SendInterrupt() would need to read it to suppress needless kill()
calls, but doesn't yet, or am I missing something? Hmm, I think there
are two kinds of kill() suppression that are missing compared to
master:
1. No wakeup is needed if the bit is already set:
SendInterrupt(InterruptType reason, ProcNumber pgprocno)
{
PGPROC *proc;
+ uint32 old_interrupts;
Assert(pgprocno != INVALID_PROC_NUMBER);
Assert(pgprocno >= 0);
Assert(pgprocno < ProcGlobal->allProcCount);
proc = &ProcGlobal->allProcs[pgprocno];
- pg_atomic_fetch_or_u32(&proc->pendingInterrupts, 1 << reason);
- WakeupOtherProc(proc);
+ old_interrupts =
pg_atomic_fetch_or_u32(&proc->pendingInterrupts, 1 << reason);
+ if ((old_interrupts & (1 << reason)) == 0)
+ WakeupOtherProc(proc);
}
That's equivalent to this removed latch code:
- /* Quick exit if already set */
- if (latch->is_set)
- return;
... except it's atomic, which I find a lot easier to think about.
2. Would it make sense to use a bit in pendingInterrupts as a
replacement for the old maybe_sleeping flag? (Similar to typical
implementation of mutexes and other such things, where you adjust the
lock and atomically know whether you have to wake anyone.) Then we we
could easily extend the check above to test that at the same time,
with the same simple race-free atomic goodness:
+ if ((old_interrupts & (WAKEME | (1 << reason))) == WAKEME)
+ WakeupOtherProc(proc);
I think the waiting side would also be tidier and simpler than what
you have: you could use atomic_fetch_or to announce you're about to
sleep, while atomically reading the interrupt bits already set up to
that moment.
More soon...
