Hi Heikki, Thanks for looking into this!
On Mon, Jul 6, 2026 at 7:04 PM Heikki Linnakangas <[email protected]> wrote: /* > > * Apply the record > > */ > > ApplyWalRecord(xlogreader, record, &replayTLI); > > > > /* > > * Wake up processes waiting for standby replay, > > write, or flush > > * LSN to reach current replay position. Replay > > implies that the > > * WAL was already written and flushed to disk, so > > write and flush > > * waiters can be woken at the replay position too. > > */ > > WaitLSNWakeup(WAIT_LSN_TYPE_STANDBY_REPLAY, > > > > XLogRecoveryCtl->lastReplayedEndRecPtr); > > WaitLSNWakeup(WAIT_LSN_TYPE_STANDBY_WRITE, > > > > XLogRecoveryCtl->lastReplayedEndRecPtr); > > WaitLSNWakeup(WAIT_LSN_TYPE_STANDBY_FLUSH, > > > > XLogRecoveryCtl->lastReplayedEndRecPtr); > > That's not wrong, but I've got some comments: > > 1. It's reading XLogRecoveryCtl->lastReplayedEndRecPtr without a lock or > atomics. That's ok, no other process modifies lastReplayedEndRecPtr, but > it feels a little dirty. > > 2. We're now doing three extra function calls on every WAL record. This > is a very hot path, and most of the time, we'll just take the fast path > in WaitLSNWakeup to return without doing anything. Andres and others > assumed up-thread that it's negligible (we used to have pre-checks here > in the caller), but I wonder if you did any performance testing? Agreed, this is a hot path. The performance impact of these extra calls doing real work hasn't been measured yet. I'll do some testing. > 3. There are other "wakeup" calls inside ApplyWalRecord(), to wake up > walsenders and walreceivers. They could perhaps use the same wait-lsn > machinery now, but that's v20 material. However, I think these > WaitLSNWakeup() calls should also be moved inside ApplyWalRecord(), so > that we'd have all the wakeup actions in one place. + 1. This makes the code safer and more readable. > 4. Once you move those calls inside ApplyWalRecord(), like this: > > > @@ -1979,20 +1979,30 @@ ApplyWalRecord(XLogReaderState *xlogreader, > > XLogRecord *record, TimeLineID *repl > > /* > > * Update lastReplayedEndRecPtr after this record has been > > successfully > > * replayed. > > */ > > SpinLockAcquire(&XLogRecoveryCtl->info_lck); > > XLogRecoveryCtl->lastReplayedReadRecPtr = xlogreader->ReadRecPtr; > > XLogRecoveryCtl->lastReplayedEndRecPtr = xlogreader->EndRecPtr; > > XLogRecoveryCtl->lastReplayedTLI = *replayTLI; > > SpinLockRelease(&XLogRecoveryCtl->info_lck); > > > > + /* > > + * Wake up processes waiting for standby replay, write, or flush > > LSN to > > + * reach current replay position. Replay implies that the WAL was > > already > > + * written and flushed to disk, so write and flush waiters can be > > woken at > > + * the replay position too. > > + */ > > + WaitLSNWakeup(WAIT_LSN_TYPE_STANDBY_REPLAY, xlogreader->EndRecPtr); > > + WaitLSNWakeup(WAIT_LSN_TYPE_STANDBY_WRITE, xlogreader->EndRecPtr); > > + WaitLSNWakeup(WAIT_LSN_TYPE_STANDBY_FLUSH, xlogreader->EndRecPtr); > > + > > /* ------ > > * Wakeup walsenders: > > * > > * On the standby, the WAL is flushed first (which will only wake up > > * physical walsenders) and then applied, which will only wake up > > logical > > * walsenders. > > It becomes clear that you don't actually need the memory barrier inside > WaitLSNWakeup(). Not sure if they're needed for other callers, but here > we have just released a spinlock, which acts as a memory barrier. It > might not be worth relaxing, but it does seem a little silly. If we made the move here, I think the memory barrier could be relaxed since other callers are guarded by either the spinlock or full-barrier atomic write already. We might also want to make the contract of WaitLSNWakeup() explicit: callers should not publish the LSN with an unsynchronized plain store and then immediately probe minWaitedLSN. Another motivation for doing this might be slightly better performance though untested. > If nothing else, I'd like to move those calls into ApplyWalRecord() for > clarity (point 3 above). What do you think? Personally + 1. -- Regards, Xuneng Zhou HighGo Software Co., Ltd.
