Horton
Menu

Re: Implement waiting for wal lsn replay: reloaded

Xuneng Zhou <xunengzhou@gmail.com>

View thread
From: Xuneng Zhou <xunengzhou@gmail.com>
To: Alexander Korotkov <aekorotkov@gmail.com>
Cc: Andres Freund <andres@anarazel.de>, Tom Lane <tgl@sss.pgh.pa.us>, Heikki Linnakangas <hlinnaka@iki.fi>, Peter Eisentraut <peter@eisentraut.org>, Thomas Munro <thomas.munro@gmail.com>, Álvaro Herrera <alvherre@kurilemu.de>, Chao Li <li.evan.chao@gmail.com>, pgsql-hackers <pgsql-hackers@lists.postgresql.org>, Michael Paquier <michael@paquier.xyz>, jian he <jian.universality@gmail.com>, Tomas Vondra <tomas@vondra.me>, Yura Sokolov <y.sokolov@postgrespro.ru>
Date: 2026-04-07T13:05:40Z
Lists: pgsql-hackers 
Hi Alexander,

On Tue, Apr 7, 2026 at 8:52 PM Alexander Korotkov <aekorotkov@gmail.com> wrote:
>
> On Tue, Apr 7, 2026 at 7:03 AM Xuneng Zhou <xunengzhou@gmail.com> wrote:
> > On Tue, Apr 7, 2026 at 11:31 AM Andres Freund <andres@anarazel.de> wrote:
> > >
> > > Hi,
> > >
> > > On 2026-04-06 23:07:45 -0400, Andres Freund wrote:
> > > > But, leaving that aside, looking at this code I'm somewhat concerned - it
> > > > seems to not worry at all about memory ordering?
> > > >
> > > >
> > > > static void
> > > > XLogWalRcvWrite(char *buf, Size nbytes, XLogRecPtr recptr, TimeLineID tli)
> > > > ...
> > > >       /* Update shared-memory status */
> > > >       pg_atomic_write_u64(&WalRcv->writtenUpto, LogstreamResult.Write);
> > > >
> > > >       /*
> > > >        * If we wrote an LSN that someone was waiting for, notify the waiters.
> > > >        */
> > > >       if (waitLSNState &&
> > > >               (LogstreamResult.Write >=
> > > >                pg_atomic_read_u64(&waitLSNState->minWaitedLSN[WAIT_LSN_TYPE_STANDBY_WRITE])))
> > > >               WaitLSNWakeup(WAIT_LSN_TYPE_STANDBY_WRITE, LogstreamResult.Write);
> > > >
> > > > There are no memory barriers here, so the CPU would be entirely free to not
> > > > make the writtenUpto write visible to a waiter that's in the process of
> > > > registering and is checking whether it needs to wait in WaitForLSN().
> > > >
> > > > And WaitForLSN()->GetCurrentLSNForWaitType()->GetWalRcvWriteRecPtr() also has
> > > > no barriers.  That MAYBE is ok, due addLSNWaiter() providing the barrier at
> > > > loop entry and maybe kinda you can think that WaitLatch() will somehow also
> > > > have barrier semantic.  But if so, that would need to be very carefully
> > > > documented.  And it seems completely unnecessary here, it's hard to believe
> > > > using a barrier (via pg_atomic_read_membarrier_u64() or such) would be a
> > > > performance issue
> >
> > Thanks for pointing this out.  This is indeed a store-load ordering issue.
> >
> > > And separately from the memory ordering, how can it make sense that there's
> > > at least 5 copies of this
> > >
> > >                 if (waitLSNState &&
> > >                         (LogstreamResult.Flush >=
> > >                          pg_atomic_read_u64(&waitLSNState->minWaitedLSN[WAIT_LSN_TYPE_STANDBY_FLUSH])))
> > >                         WaitLSNWakeup(WAIT_LSN_TYPE_STANDBY_FLUSH, LogstreamResult.Flush);
> > >
> > > around?  That needs to be encapsulated so that if you have a bug, like the
> > > memory ordering problem I describe above, it can be fixed once, not in
> > > multiple places.
> >
> > Yeah, this duplication is not ok.
> >
> > > And why do these callers even have that pre-check?  Seems WaitLSNWakeup()
> > > does so itself?
> > >
> > >         /*
> > >          * Fast path check.  Skip if currentLSN is InvalidXLogRecPtr, which means
> > >          * "wake all waiters" (e.g., during promotion when recovery ends).
> > >          */
> > >         if (XLogRecPtrIsValid(currentLSN) &&
> > >                 pg_atomic_read_u64(&waitLSNState->minWaitedLSN[i]) > currentLSN)
> > >                 return;
> > >
> > > And why is the code checking if waitLSNState is non-NULL?
> > >
> >
> > These fast checks are unnecessary copy-pastos and waitLSNState checks
> > also do not make sense except for the one in WaitLSNCleanup.
> >
> > I'll prepare a patch set addressing them.
>
> Thanks to Tom and Andres for catching these issues!
> I'm planning to work on this during this evening.  I'll review
> Xuneng's patches (or write my own if they wouldn't arrive yet).
>

I’ve posted two patches. The first fixes the duplication issue
reported by Andres and is fairly straightforward. The second turned
out to be more complex than expected, and I’m still working through
possible solutions. Feedback or alternative approaches would be very
helpful.
I also spent some time drafting a patch to address the memory ordering
issue and will post it later.


-- 
Best,
Xuneng

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. Clean up 019_replslot_limit.pl comments

  2. Stabilize 019_replslot_limit.pl: wait on slot restart_lsn

  3. Fix memory ordering in WAIT FOR LSN wakeup mechanism

  4. Improve WAIT FOR LSN test coverage

  5. Remove redundant WAIT FOR LSN caller-side pre-checks

  6. Use barrier semantics when reading/writing writtenUpto

  7. Use replay position as floor for WAIT FOR LSN standby_(write|flush)

  8. Wake standby_write/standby_flush waiters from the WAL replay loop

  9. Minimal fix for WAIT FOR ... MODE 'standby_flush'

  10. Avoid syscache lookup while building a WAIT FOR tuple descriptor

  11. Document that WAIT FOR may be interrupted by recovery conflicts

  12. Use WAIT FOR LSN in PostgreSQL::Test::Cluster::wait_for_catchup()

  13. Wake LSN waiters before recovery target stop

  14. Remove redundant pg_unreachable() after elog(ERROR) from ExecWaitStmt()

  15. Revert "Use WAIT FOR LSN in PostgreSQL::Test::Cluster::wait_for_catchup()"

  16. Fix variable usage in wakeupWaiters()

  17. Add tab completion for the WAIT FOR LSN MODE option

  18. Add the MODE option to the WAIT FOR LSN command

  19. Extend xlogwait infrastructure with write and flush wait types

  20. Unify error messages

  21. Optimize shared memory usage for WaitLSNProcInfo

  22. Fix WaitLSNWakeup() fast-path check for InvalidXLogRecPtr

  23. Fix incorrect function name in comments

  24. Add infrastructure for efficient LSN waiting

  25. Add pairingheap_initialize() for shared memory usage

  26. Implement WAIT FOR command