public inbox for [email protected]
help / color / mirror / Atom feedFrom: Xuneng Zhou <[email protected]>
To: Heikki Linnakangas <[email protected]>
Cc: Alexander Korotkov <[email protected]>
Cc: Andres Freund <[email protected]>
Cc: Tom Lane <[email protected]>
Cc: Peter Eisentraut <[email protected]>
Cc: Thomas Munro <[email protected]>
Cc: Álvaro Herrera <[email protected]>
Cc: Chao Li <[email protected]>
Cc: pgsql-hackers <[email protected]>
Cc: Michael Paquier <[email protected]>
Cc: jian he <[email protected]>
Cc: Tomas Vondra <[email protected]>
Cc: Yura Sokolov <[email protected]>
Subject: Re: Implement waiting for wal lsn replay: reloaded
Date: Mon, 6 Jul 2026 22:17:41 +0800
Message-ID: <CABPTF7V1GqXRe9Kyiw=oyDoUU4Oss49nMBhco12V7xb72P6WvQ@mail.gmail.com> (raw)
In-Reply-To: <CABPTF7XV=rPe9iVTyR5O=CT0oMUYTPJpr+4+T35n0oKeaaNZHQ@mail.gmail.com>
References: <[email protected]>
<CABPTF7WPRVJGdDeuWo-=3csnDs5FGfUsg97xppPyzoj9fRAMeQ@mail.gmail.com>
<CABPTF7WAcV_uEaJa3jmYPc26rApa64hV5HmjRBN=OS17MAybUg@mail.gmail.com>
<CAPpHfdvM9Tp1_g1JY9H+cx1H0Q4jH-BZFZGQwgviLNs1HSg+CA@mail.gmail.com>
<CABPTF7X0iV=kGC4gjsTj4NvK_NNEJGM3YTc7Obxs5GOiYoMhEw@mail.gmail.com>
<CABPTF7UBdEfyxATWntmCfoJrwB6iPrnhkXO7y_Avmqc2bOn27A@mail.gmail.com>
<CAPpHfduhQsm44j_ziZ6ykFTDZ2SFvZp04iCrc5_dD9PGZUrhrw@mail.gmail.com>
<d2ogsp54if47w35kx4vu7o3nkfghpuzctwbaudzey5brxavomw@bgfs3p7twoib>
<CABPTF7W7=uK0ypteDLRTiR4PnRP-BEUEJJtit+Q17aiPPhpLrw@mail.gmail.com>
<CABPTF7Ukk8iJF7TpnK2mFOaboNJgWL1csfXu4e3J4GT0o7x0GQ@mail.gmail.com>
<CAPpHfdupZ3ooxmnJvhjx7rnZ7VMcz_uyxGC4drMSEM_P3PtGzw@mail.gmail.com>
<CABPTF7WJ35p7uidJJZs7fzxBtbVL_0xSFUdZ2Fe8pXh00e=Mxw@mail.gmail.com>
<CAPpHfdufWG032J=fyv1eWoveeyPwqJ57PGU2edA5OsOmexGDTw@mail.gmail.com>
<CABPTF7W-gaO=FAkhda=_pDQJjLne68ioNHHU8vuB4iEnswR1=w@mail.gmail.com>
<[email protected]>
<CABPTF7XV=rPe9iVTyR5O=CT0oMUYTPJpr+4+T35n0oKeaaNZHQ@mail.gmail.com>
On Mon, Jul 6, 2026 at 9:49 PM Xuneng Zhou <[email protected]> wrote:
>
> Hi Heikki,
>
> Thanks for looking into this!
>
> On Mon, Jul 6, 2026 at 7:04 PM Heikki Linnakangas <[email protected]> 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...
> 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.
>
> > If nothing else, I'd like to move those calls into ApplyWalRecord() for
> > clarity (point 3 above). What do you think?
>
> Personally + 1.
--
Regards,
Xuneng Zhou
HighGo Software Co., Ltd.
view thread (30+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected]
Subject: Re: Implement waiting for wal lsn replay: reloaded
In-Reply-To: <CABPTF7V1GqXRe9Kyiw=oyDoUU4Oss49nMBhco12V7xb72P6WvQ@mail.gmail.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox