Re: Implement waiting for wal lsn replay: reloaded

On Fri, Jul 31, 2026 at 2:23 PM Xuneng Zhou <xunengzhou(at)gmail(dot)com> wrote:
>
> On Fri, Jul 31, 2026 at 1:02 PM Xuneng Zhou <xunengzhou(at)gmail(dot)com> wrote:
> >
> > On Fri, Jul 31, 2026 at 11:39 AM Xuneng Zhou <xunengzhou(at)gmail(dot)com> wrote:
> > >
> > > On Fri, Jul 31, 2026 at 9:46 AM Xuneng Zhou <xunengzhou(at)gmail(dot)com> wrote:
> > > >
> > > > On Fri, Jul 31, 2026 at 8:58 AM Xuneng Zhou <xunengzhou(at)gmail(dot)com> wrote:
> > > > >
> > > > > On Thu, Jul 30, 2026 at 10:51 AM Xuneng Zhou <xunengzhou(at)gmail(dot)com> wrote:
> > > > > >
> > > > > > On Tue, Jul 28, 2026 at 9:25 PM Xuneng Zhou <xunengzhou(at)gmail(dot)com> wrote:
> > > > > > >
> > > > > > > On Mon, Jul 27, 2026 at 7:03 PM Xuneng Zhou <xunengzhou(at)gmail(dot)com> wrote:
> > > > > > > >
> > > > > > > > Hi Alexander,
> > > > > > > >
> > > > > > > > On Thu, Jul 9, 2026 at 8:24 PM Xuneng Zhou <xunengzhou(at)gmail(dot)com> wrote:
> > > > > > > > >
> > > > > > > > > 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].
> > > > > > > >
> > > > > > > > I'd like to propose a series of patches to do some further clean-ups &
> > > > > > > > enhancements. The first few are mostly mechanical, straightforward,
> > > > > > > > and the last two are more subtle. I appreciate your inputs/thoughts on
> > > > > > > > them.
> > > > > > > >
> > > > > > > > 1) Patch 1 to address the issue raised by Noah.
> > > > > > > >
> > > > > > > > 2) Patch 2 to fix two small docs mismatches.
> > > > > > > >
> > > > > > > > 3) Patch 3 to optimize the WaitLSNLock acquisition in deleteLSNWaiter.
> > > > > > > >
> > > > > > > > 4) Patch 4 to clarify lsn waiter cleanup after wakeup.
> > > > > > > >
> > > > > > > > 5) Patch 5 to address a rare issue of deregistration of waiters whose
> > > > > > > > target lsn has reached but then flashed back.
> > > > > > > >
> > > > > > > > Currently, the waiters are removed from the heap by the wakers if
> > > > > > > > their target lsn has reached. We offer no re-registration service in
> > > > > > > > the reception desk for them assuming they won't need to extend their
> > > > > > > > stay. This assumption holds if the lsn is monotonically increasing,
> > > > > > > > which is true for most cases. However, there seems to be a catch for
> > > > > > > > the wal write/flush lsn from the wal receiver side. In the cases of
> > > > > > > > restarting wal receiver, the write/flush lsn could be reset to
> > > > > > > > positions lagging behind(the attached restarting cases file summarized
> > > > > > > > by Sol with inputs from me shows a matrix of this).
> > > > > > > >
> > > > > > > > One problematic interleaving, for reset cases 2–4 in the attached
> > > > > > > > file, is the following:
> > > > > > > >
> > > > > > > > Let W be the old published write position, X the waiter’s target, S
> > > > > > > > the new streaming segment start, and R the replay position, with:
> > > > > > > > max(N, R) < X ≤ W
> > > > > > > >
> > > > > > > > [T0] A backend registers a standby_write waiter for target X and sleeps.
> > > > > > > >
> > > > > > > > [T1] The walreceiver publishes writtenUpto = W and calls
> > > > > > > > WaitLSNWakeup(W). Because X ≤ W, the waker removes the waiter. from
> > > > > > > > the heap, sets inHeap = false, and sets its latch.
> > > > > > > >
> > > > > > > > [T2] Before the waiter runs, streaming is restarted.
> > > > > > > > RequestXLogStreaming() resets shared writtenUpto from W to S. The
> > > > > > > > effective standby_write position is now: max(writtenUpto, replay) =
> > > > > > > > max(S, R) = R. Therefore the effective position has regressed below X.
> > > > > > > >
> > > > > > > > [T3] The waiter consumes the old latch notification and rechecks its
> > > > > > > > condition. It observes R < X. In the pre-fix code, it goes back to
> > > > > > > > sleep without re-registering, even though it is no longer in the
> > > > > > > > waiters heap.
> > > > > > >
> > > > > > > I took a second look at the wording. I had asked Sol to polish this
> > > > > > > part for clarity, but the revised version is inaccurate. It should be:
> > > > > > >
> > > > > > > [T2] Before the waiter runs, streaming is restarted.
> > > > > > > RequestXLogStreaming() resets writtenUpto from W to S. The effective
> > > > > > > standby_write position becomes max(S, R), which is below X.
> > > > > > >
> > > > > > > [T3] The waiter consumes the earlier latch notification and rechecks
> > > > > > > its condition. It observes that max(S, R) < X. In the pre-fix code, it
> > > > > > > waits again without re-registering, even though the waker has already
> > > > > > > removed it from the heap.
> > > > > > >
> > > > > > > Sorry for not double-checking it.
> > > > > > >
> > > > > > > > [T4] The walreceiver later receives and publishes WAL through X and
> > > > > > > > calls WaitLSNWakeup() again. The waiter is absent from the heap, so it
> > > > > > > > receives no notification and can remain asleep even though its target
> > > > > > > > has now been reached.
> > > > > > > >
> > > > > > > > One possible fix for the issue that I have considered is to make
> > > > > > > > write/flush lsn monotonic so it won't flash back once advanced. This
> > > > > > > > seems to work well at first glance. However, it has two shortcomings:
> > > > > > > > one is that it can blur the definition of write lsn and another is
> > > > > > > > that it won't fix the cross-timeline lsn regression. Currently,
> > > > > > > > writtenUpto is the current walreceiver stream’s write frontier. Giving
> > > > > > > > that value monotonicity would turn it into the greatest numeric lsn
> > > > > > > > ever written by any receiver incarnation. Cross-timeline lsn flashback
> > > > > > > > is expected given the fork point is smaller than the pre-restart lsns.
> > > > > > > > Extending it across timelines would incorrectly associate progress on
> > > > > > > > an old timeline with the new timeline. The patch places the fix in the
> > > > > > > > waiter side -- let the backend do a recheck and re-registration if the
> > > > > > > > published lsn is regressed.
> > > > > > > >
> > > > > > > > Another aspect is the test, it's relatively simple to construct the
> > > > > > > > failure case, but seems hard to do so deterministically. I spent a lot
> > > > > > > > of time(maybe too much for an unconfirmed rare issue) trying to tame
> > > > > > > > it in a simple way; it seems hard to orchestrate the startup process,
> > > > > > > > wal receiver, waiting backend into above-like interleavings end-to-end
> > > > > > > > without using complex synchronization like three injection points. The
> > > > > > > > tricky part is to ensure max(N, R) < X ≤ W. We cannot stop the startup
> > > > > > > > then the wal receiver to make sure that because we need the former to
> > > > > > > > manipulate the later one. The current workaround is to add a synthetic
> > > > > > > > helper for removing the heap node even if its target lsn has not
> > > > > > > > actually reached, then trigger the real wake-up call by advancing the
> > > > > > > > published lsn to check whether the waiter is notified and the wait
> > > > > > > > completes. The rationale behind this is that the premature removal
> > > > > > > > from the heap without re-registration is the underlying issue
> > > > > > > > regardless of the specific lsn types and failure scenarios. That said,
> > > > > > > > I am unsure whether this is a proper way or better alternatives exist.
> > > > > > > >
> > > > > > > > 6) Patch 6 to cover a missing wake-up point for primary-flush waiters [WIP]
> > > > > >
> > > > > > After learning more of the test/modules, I became less convinced to
> > > > > > use an injection point for the proof of re-registration in the test
> > > > > > which seems somewhat hacky to me. It works in a non-obvious manner --
> > > > > > we infer from the position of the code that the registration occurs,
> > > > > > but it is prone to future changes unless we add more comments to
> > > > > > explain its usage. Just wondering whether it makes sense to add a
> > > > > > helper to probe & prove the waiter is registered in the heap.
> > > > > >
> > > > >
> > > > > Updated the patch 5 according to the methodology. Added a new test
> > > > > helper, test_wait_lsn_waiter_is_registered(), that checks whether a
> > > > > process is registered in the waiters heap for a specified target LSN,
> > > > > wait type, and PID.
> > > >
> > > > Generalized the test_wait_lsn_wakeup helper to specify the wait type
> > > > to pair with test_wait_lsn_waiter_is_registered.
> > >
> > > V4 improved some commit messages and comments of patch 1-4.
> >
> > There are two inaccurate expressions in the comments of patch 1 and 4.
> > They are now fixed in v5. Sorry for the noise here.
>
> Here's v6 to update the comments just like the two immediate
> precedents did. It seems annoying to do/say so. Should've shipped a
> self-settled version. Sorry for haven't done otherwise...

The patchset looks good to me. I made some minor corrections.

0003 Removed extra check in WaitLSNCleanup() as deleteLSNWaiter()
already does it.
0005 Improved comment about the situation on when LSN can go backwards.

I'm going to push this if no objections.

------
Regards,
Alexander Korotkov
Supabase