Subject: Re: Implement waiting for wal lsn replay: reloaded

From: Heikki Linnakangas <[email-masked]>
Date: Mon, 6 Jul 2026 14:04:08 +0300

On 01/05/2026 05:44, Xuneng Zhou wrote:
> On Wed, Apr 29, 2026 at 5:01 AM Alexander Korotkov <[email-masked] 
> <mailto:[email-masked]>> 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?

- Heikki

---

From: Xuneng Zhou <[email-masked]>
Date: Mon, 6 Jul 2026 21:49:26 +0800

Hi Heikki,

Thanks for looking into this!

On Mon, Jul 6, 2026 at 7:04 PM Heikki Linnakangas <[email-masked]> wrote:
                      /*
> 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.

> that we'd have all the wakeup actions in one place.

+ 1. This makes the code safer and more readable.

> 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
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.

> clarity (point 3 above). What do you think?

Personally + 1.
--
Regards,
Xuneng Zhou
HighGo Software Co., Ltd.

---

From: Xuneng Zhou <[email-masked]>
Date: Mon, 6 Jul 2026 22:17:41 +0800

> atomic write already.  We might also want to make the contract of

OK, the 'if' here is redundant...

---

From: Alexander Korotkov <[email-masked]>
Date: Tue, 7 Jul 2026 18:25:28 +0300

Hi, Heikki!

> 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

---

From: Xuneng Zhou <[email-masked]>
Date: Wed, 8 Jul 2026 20:08:01 +0800

> 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.

---

From: Alexander Korotkov <[email-masked]>
Date: Wed, 8 Jul 2026 20:38:28 +0300

> 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

---

From: Xuneng Zhou <[email-masked]>
Date: Thu, 9 Jul 2026 12:53:48 +0800

> WaitLSNWakeup() only once and make only 1 memory barrier.

This optimization seems desirable to me and the hierarchy idea is
clever! I pondered it for a while, it may not be necessary for
WAIT_LSN_TYPE_STANDBY_FLUSH, do we need to add an additional wake-up
call for write waiters at flush lsn publication?
Another less clever idea is to add a special helper for relay wake-up,
we can still use a single memory barrier followed by three plan reads.
However, this seems less unified and needs extra clarification and
safe checking. I am curious about your thoughts on this. Maybe I just
don't get the idea right.

---

From: Xuneng Zhou <[email-masked]>
Date: Thu, 9 Jul 2026 20:24:57 +0800

Hi!

As Noah pointed out in [1], auxiliary processes are supported in the
facility, but proper cleanup is absent when they exit. I have attached
a patch trying to address that as suggested before getting sidetracked
for too long with the issue raised in [2].

[1] https://www.postgresql.org/message-id/[email-masked]
[2] https://www.postgresql.org/message-id/flat/20210803023612.iziacxk5syn2r4ut%40alap3.anarazel.de