Hi Rui, Thanks for your detailed feedback! Sorry for the late response.
On Mon, Jun 8, 2026 at 10:49 PM Rui Zhao <[email protected]> wrote: > > Hi Xuneng, > > I took a look at v11. Overall it looks good, and I think the direction > is right: now that the LSN-wait facility was committed independently > (3b4e53a, 447aae1), the patch no longer depends on the full WAIT FOR > machinery -- which I believe addresses Michael's main earlier concern -- > and the change in read_local_xlog_page_guts() is small and focused. > > I traced the wakeup paths against current master to convince myself the > busy-poll can be safely dropped: > > - primary flush waiters are woken in xlog.c (XLogFlush / > XLogBackgroundFlush), > - standby replay waiters in xlogrecovery.c, > - promotion wakes standby replay waiters in xlog.c, > so the cascading A => B => C case (B promoted while decoding on C) > is covered via the replay advance. > > WaitForLSN() also adds the waiter to the heap before re-reading the > current LSN, so there is no lost-wakeup window, and it checks for > interrupts before each WaitLatch(). So far so good. Thanks for your confirmation. d61f9c90f2ef96b38eceb592b63c688d74cf7611 added a new wake-up call for implicit WAL flushes. > A few comments, all minor: > > 1. CHECK_FOR_INTERRUPTS() right before the wait is now redundant. It > was needed for the old pg_usleep() busy-loop (pg_usleep does not > service interrupts), but WaitForLSN() already calls > CHECK_FOR_INTERRUPTS() before each WaitLatch(). It can be removed. Agreed. Will remove it in the next version. > 2. The switch on the standby WaitForLSN() result (the > SUCCESS/NOT_IN_RECOVERY/default block) doesn't seem to earn its keep. > Both non-default arms just break and fall through to the same "loop > back" as the primary branch, and WAIT_LSN_RESULT_TIMEOUT is > unreachable here because we pass timeout = -1. The existing callers > in commands/wait.c and commands/repack_worker.c switch on the result > precisely because they pass a finite timeout and act differently per > outcome; here the outcomes all lead to the same action, so the > primary branch's "ignore the result" style fits better and makes the > two branches symmetric. > > The one part worth keeping is the comment about promotion, which > documents real behavior. So I'd drop the switch and keep the > explanation inline, e.g.: > > else > { > /* > * On a standby, wait for replay. We ignore the result: if > * we get promoted while waiting, the next iteration of the > * outer loop takes the primary/flush path; if a cascading > * upstream switches timeline, replay advances and wakes us. > * Either way we loop back and re-evaluate, so no per-result > * handling is needed here. > */ > WaitForLSN(WAIT_LSN_TYPE_STANDBY_REPLAY, loc, -1); > } +1, we don't need the switch statement. Something like this seems suffice: WaitForLSN(RecoveryInProgress() ? WAIT_LSN_TYPE_STANDBY_REPLAY : WAIT_LSN_TYPE_PRIMARY_FLUSH, loc, -1); We may also want to mention that the patch preserves the old behavior in the commit message. > 3. The patch has no test. I realize a pure performance change is hard > to test, but the trickiest path is "promoted while waiting" > (WAIT_LSN_RESULT_NOT_IN_RECOVERY -> loop back -> flush path). A small > TAP test exercising that, plus a line in the commit message about how > you validated this (the performance comparison), would make reviewers > more comfortable since this touches sensitive WAL-read code. Even if > such a test doesn't end up being committed (recovery TAP tests are > expensive, and the committer may prefer not to add one), writing it > would at least let us confirm the promotion path behaves as intended. Make sense. Will add one to cover that. > One question on the design: the wakeup/queue pattern here is close to > what synchronous replication does, and Michael raised possible > consolidation with syncrep.c earlier. v11 leaves that out, which seems > reasonable to me for keeping the change small -- but it might be worth > saying so explicitly in the commit message so it's a recorded decision > rather than an omission. Not sure about this. That decision seems less relevant to the $subject to me. Those two modules do share some patterns but may also differ in expected workloads. It seems more about a research direction. -- Regards, Xuneng Zhou HighGo Software Co., Ltd.
