Hi, Heikki! On Mon, Jul 6, 2026 at 2:04 PM Heikki Linnakangas <[email protected]> wrote: > 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?
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
