On Wed, Jul 8, 2026 at 3:08 PM Xuneng Zhou <[email protected]> wrote: > On Mon, Jul 6, 2026 at 10:17 PM Xuneng Zhou <[email protected]> wrote: > > > > 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... > > After revisiting the memory barrier in WaitLSNWakeup and why it is > introduced there in a80a593ab63 rather than recalling it from memory, > I think relaxing it here could be unsafe. > > In WaitLSNWakeup(), use pg_atomic_read_membarrier_u64() in the > fast-path check so the waker's preceding position store is globally > visible before minWaitedLSN is read. > > Without the barrier in WaitLSNWakeup(), this interleaving is possible: > > Initial: > minWaitedLSN = PG_UINT64_MAX > replayLSN = 90 > > Waiter: > stores minWaitedLSN = 100 > reads replayLSN before the waker publishes the new replay position > sees replayLSN = 90 > decides it should sleep > > Waker: > publishes replayLSN = 100 > reads old minWaitedLSN = PG_UINT64_MAX > skips the wakeup > > Then the waiter goes to sleep even though replay has reached its > target LSN. To avoid this, we still need to make sure that the > publication of replayLSN precedes the read of minWaitedLSN, so that > the waker cannot decide "nobody is waiting" before its own progress is > still not visible to the waiter.
Yes, I also think the memory barrier for waker between publishing replayLSN and reading minWaitedLSN is required. However, sequence of three WaitLSNWakeup() calls makes 3 memory barriers while only one is required. We could introduce a hierarchy for WAIT_LSN_TYPE_STANDBY_*: WAIT_LSN_TYPE_STANDBY_FLUSH implies WAIT_LSN_TYPE_STANDBY_WRITE, WAIT_LSN_TYPE_STANDBY_REPLAY implies WAIT_LSN_TYPE_STANDBY_WRITE and WAIT_LSN_TYPE_STANDBY_FLUSH. Then ApplyWalRecord() can call WaitLSNWakeup() only once and make only 1 memory barrier. ------ Regards, Alexander Korotkov Supabase
