On Thu, Jul 9, 2026 at 1:38 AM Alexander Korotkov <[email protected]> wrote: > > On Wed, Jul 8, 2026 at 3:08 PM Xuneng Zhou <[email protected]> wrote: > > On Mon, Jul 6, 2026 at 10:17 PM Xuneng Zhou <[email protected]> wrote: > > > > > > 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... > > > > 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 = PG_UINT64_MAX > > replayLSN = 90 > > > > Waiter: > > stores minWaitedLSN = 100 > > reads replayLSN before the waker publishes the new replay position > > sees replayLSN = 90 > > decides it should sleep > > > > Waker: > > publishes replayLSN = 100 > > reads old minWaitedLSN = 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. -- Regards, Xuneng Zhou HighGo Software Co., Ltd.
