Re: Implement waiting for wal lsn replay: reloaded

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

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