MIME-Version: 1.0
References: <1959506.1775527693@sss.pgh.pa.us>
 <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>
 <e4e98e40-40d5-4d53-85de-6c456c2b301a@iki.fi>
 <CABPTF7XV=rPe9iVTyR5O=CT0oMUYTPJpr+4+T35n0oKeaaNZHQ@mail.gmail.com>
 <CABPTF7V1GqXRe9Kyiw=oyDoUU4Oss49nMBhco12V7xb72P6WvQ@mail.gmail.com>
 <CABPTF7XdpM8LCvrkQdXrj3qt88auofiVw_STbUnXg0GEuzbDNw@mail.gmail.com>
 <CAPpHfdvu7bPzP1p3eZW3t6QgF39stTce=OyObyn6N2J+ZC5Khw@mail.gmail.com>
In-Reply-To: 
 <CAPpHfdvu7bPzP1p3eZW3t6QgF39stTce=OyObyn6N2J+ZC5Khw@mail.gmail.com>
From: Xuneng Zhou <xunengzhou@gmail.com>
Date: Thu, 9 Jul 2026 12:53:48 +0800
Message-ID: 
 <CABPTF7UkddAnG=6X0JLHEOVWO-xoG3XYXgc1uWhbjj5eHRjipw@mail.gmail.com>
Subject: Re: Implement waiting for wal lsn replay: reloaded
To: Alexander Korotkov <aekorotkov@gmail.com>
Cc: Heikki Linnakangas <hlinnaka@iki.fi>, Andres Freund <andres@anarazel.de>,
 Tom Lane <tgl@sss.pgh.pa.us>,
	Peter Eisentraut <peter@eisentraut.org>,
 Thomas Munro <thomas.munro@gmail.com>,
	=?UTF-8?Q?=C3=81lvaro_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>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Archived-At: 
 <https://www.postgresql.org/message-id/CABPTF7UkddAnG%3D6X0JLHEOVWO-xoG3XYXgc1uWhbjj5eHRjipw%40mail.gmail.com>
Precedence: bulk

On Thu, Jul 9, 2026 at 1:38=E2=80=AFAM Alexander Korotkov <aekorotkov@gmail=
.com> wrote:
>
> On Wed, Jul 8, 2026 at 3:08=E2=80=AFPM Xuneng Zhou <xunengzhou@gmail.com>=
 wrote:
> > On Mon, Jul 6, 2026 at 10:17=E2=80=AFPM Xuneng Zhou <xunengzhou@gmail.c=
om> wrote:
> > >
> > > On Mon, Jul 6, 2026 at 9:49=E2=80=AFPM Xuneng Zhou <xunengzhou@gmail.=
com> wrote:
> > > >
> > > > Hi Heikki,
> > > >
> > > > Thanks for looking into this!
> > > >
> > > > On Mon, Jul 6, 2026 at 7:04=E2=80=AFPM Heikki Linnakangas <hlinnaka=
@iki.fi> wrote:
> > > >                       /*
> > > > > >                        * Apply the record
> > > > > >                        */
> > > > > >                       ApplyWalRecord(xlogreader, record, &repla=
yTLI);
> > > > > >
> > > > > >                       /*
> > > > > >                        * 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 pos=
ition too.
> > > > > >                        */
> > > > > >                       WaitLSNWakeup(WAIT_LSN_TYPE_STANDBY_REPLA=
Y,
> > > > > >                                                 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 lastReplayedEndRecP=
tr, 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 fas=
t path
> > > > > in WaitLSNWakeup to return without doing anything. Andres and oth=
ers
> > > > > assumed up-thread that it's negligible (we used to have pre-check=
s 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 testin=
g.
> > > >
> > > > > 3. There are other "wakeup" calls inside ApplyWalRecord(), to wak=
e 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 *xlogrea=
der, XLogRecord *record, TimeLineID *repl
> > > > > >         /*
> > > > > >          * Update lastReplayedEndRecPtr after this record has b=
een successfully
> > > > > >          * replayed.
> > > > > >          */
> > > > > >         SpinLockAcquire(&XLogRecoveryCtl->info_lck);
> > > > > >         XLogRecoveryCtl->lastReplayedReadRecPtr =3D xlogreader-=
>ReadRecPtr;
> > > > > >         XLogRecoveryCtl->lastReplayedEndRecPtr =3D xlogreader->=
EndRecPtr;
> > > > > >         XLogRecoveryCtl->lastReplayedTLI =3D *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 wait=
ers 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 on=
ly 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, bu=
t 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 relax=
ed
> > > > since other callers are guarded by either the spinlock or full-barr=
ier
> > > > atomic write already.  We might also want to make the contract of
> > >
> > > 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 =3D PG_UINT64_MAX
> >   replayLSN =3D 90
> >
> > Waiter:
> >   stores minWaitedLSN =3D 100
> >   reads replayLSN before the waker publishes the new replay position
> >   sees replayLSN =3D 90
> >   decides it should sleep
> >
> > Waker:
> >   publishes replayLSN =3D 100
> >   reads old minWaitedLSN =3D 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.
>
> 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.

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.

--=20
Regards,
Xuneng Zhou
HighGo Software Co., Ltd.