On Mon, Jul 6, 2026 at 9:49 PM Xuneng Zhou <[email protected]> wrote: > > 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
OK, the 'if' here is redundant... > 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.
