Re: Implement waiting for wal lsn replay: reloaded

On Mon, Jul 6, 2026 at 10:17 PM Xuneng Zhou <xunengzhou(at)gmail(dot)com> wrote:
>
> On Mon, Jul 6, 2026 at 9:49 PM Xuneng Zhou <xunengzhou(at)gmail(dot)com> wrote:
> >
> > Hi Heikki,
> >
> > Thanks for looking into this!
> >
> > On Mon, Jul 6, 2026 at 7:04 PM Heikki Linnakangas <hlinnaka(at)iki(dot)fi> 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.

--
Regards,
Xuneng Zhou
HighGo Software Co., Ltd.