public inbox for [email protected]
help / color / mirror / Atom feedFrom: Heikki Linnakangas <[email protected]>
To: Xuneng Zhou <[email protected]>
To: 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 14:04:08 +0300
Message-ID: <[email protected]> (raw)
In-Reply-To: <CABPTF7W-gaO=FAkhda=_pDQJjLne68ioNHHU8vuB4iEnswR1=w@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>
On 01/05/2026 05:44, Xuneng Zhou wrote:
> On Wed, Apr 29, 2026 at 5:01 AM Alexander Korotkov <[email protected]
> <mailto:[email protected]>> 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, 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?
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, 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 nothing else, I'd like to move those calls into ApplyWalRecord() for
clarity (point 3 above). What do you think?
- Heikki
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: <[email protected]>
* 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