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>
In-Reply-To: <e4e98e40-40d5-4d53-85de-6c456c2b301a@iki.fi>
From: Alexander Korotkov <aekorotkov@gmail.com>
Date: Tue, 7 Jul 2026 18:25:28 +0300
Message-ID: 
 <CAPpHfdsF9PjvMDoE0Cko=Xahaoep2kSJmWgJkwhfqVpzd8x35A@mail.gmail.com>
Subject: Re: Implement waiting for wal lsn replay: reloaded
To: Heikki Linnakangas <hlinnaka@iki.fi>
Cc: Xuneng Zhou <xunengzhou@gmail.com>, 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/CAPpHfdsF9PjvMDoE0Cko%3DXahaoep2kSJmWgJkwhfqVpzd8x35A%40mail.gmail.com>
Precedence: bulk

Hi, Heikki!

On Mon, Jul 6, 2026 at 2:04=E2=80=AFPM Heikki Linnakangas <hlinnaka@iki.fi>=
 wrote:
> On 01/05/2026 05:44, Xuneng Zhou wrote:
> > On Wed, Apr 29, 2026 at 5:01=E2=80=AFAM Alexander Korotkov <aekorotkov@=
gmail.com
> > <mailto:aekorotkov@gmail.com>> 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, s=
o write and flush
> >                        * waiters can be woken at the replay position to=
o.
> >                        */
> >                       WaitLSNWakeup(WAIT_LSN_TYPE_STANDBY_REPLAY,
> >                                                 XLogRecoveryCtl->lastRe=
playedEndRecPtr);
> >                       WaitLSNWakeup(WAIT_LSN_TYPE_STANDBY_WRITE,
> >                                                 XLogRecoveryCtl->lastRe=
playedEndRecPtr);
> >                       WaitLSNWakeup(WAIT_LSN_TYPE_STANDBY_FLUSH,
> >                                                 XLogRecoveryCtl->lastRe=
playedEndRecPtr);
>
> 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, XLo=
gRecord *record, TimeLineID *repl
> >         /*
> >          * Update lastReplayedEndRecPtr after this record has been succ=
essfully
> >          * replayed.
> >          */
> >         SpinLockAcquire(&XLogRecoveryCtl->info_lck);
> >         XLogRecoveryCtl->lastReplayedReadRecPtr =3D xlogreader->ReadRec=
Ptr;
> >         XLogRecoveryCtl->lastReplayedEndRecPtr =3D xlogreader->EndRecPt=
r;
> >         XLogRecoveryCtl->lastReplayedTLI =3D *replayTLI;
> >         SpinLockRelease(&XLogRecoveryCtl->info_lck);
> >
> > +       /*
> > +        * Wake up processes waiting for standby replay, write, or flus=
h 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->EndRecP=
tr);
> > +       WaitLSNWakeup(WAIT_LSN_TYPE_STANDBY_WRITE, xlogreader->EndRecPt=
r);
> > +       WaitLSNWakeup(WAIT_LSN_TYPE_STANDBY_FLUSH, xlogreader->EndRecPt=
r);
> > +
> >         /* ------
> >          * Wakeup walsenders:
> >          *
> >          * On the standby, the WAL is flushed first (which will only wa=
ke 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?

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