Re: Implement waiting for wal lsn replay: reloaded

Hi, Heikki!

On Mon, Jul 6, 2026 at 2:04 PM Heikki Linnakangas <hlinnaka(at)iki(dot)fi> wrote:
> On 01/05/2026 05:44, Xuneng Zhou wrote:
> > On Wed, Apr 29, 2026 at 5:01 AM Alexander Korotkov <aekorotkov(at)gmail(dot)com
> > <mailto:aekorotkov(at)gmail(dot)com>> wrote:
> >
> >> LGTM, I've added some comments for new functions in 0006. I propose
> >> to push this patchset. Probably something is still missing and we
> >> will have to go back to this. But it seems to make a lot of aspects
> >> much better.
> >
> > I reviewed the patchset and found a potential issue in the test for
> > patch 5, similar to the log-checking problem in the cascading timeline-
> > switch test. I've applied a minor fix to address it. Other parts LGTM.
> I happened to look around this code now. To recap, the code in the main
> WAL redo loop now looks like this:
>
> >
> > /*
> > * 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?
>
> 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.
>
> 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 nothing else, I'd like to move those calls into ApplyWalRecord() for
> clarity (point 3 above). What do you think?

Thank you for bringing this up.

+1 for moving calls into ApplyWalRecord()

Regarding the overhead. I've tried to apply 158.6 MB of pgbench
generated WAL on my local Macbook M3 Pro.

* With calls, sec: 1.95/1.86/1.91/1.85/1.90
* Without calls, sec: 1.83/1.87/1.96/1.90/1.85

So, the difference is less than the measurement error.

Do you prefer to commit the movement of the calls yourself, or do you
prefer me to do it?

------
Regards,
Alexander Korotkov
Supabase