On Sat, Oct 10, 2026 at 5:27 PM Xuneng Zhou <[email protected]> wrote: > > On Sat, Oct 10, 2026 at 5:24 PM Xuneng Zhou <[email protected]> wrote: > > > > Hi Vitaly, > > > > On Fri, Sep 4, 2026 at 10:16 PM Vitaly Davydov <[email protected]> wrote: > > > > > > Hi Xuneng Zhou, > > > > > > > * The caller must already be registered as the shared buffer's > > > > + * BM_PIN_COUNT_WAITER. > > > > This line of comment for PinCountWaiterCheckReadyForCleanup in v8 > > > > seems not accurate to me. RegisterPinCountWaiter() explicitly permits > > > > the shared bit BM_PIN_COUNT_WAITER to be absent. > > > > > > Agreed, thank you. Your edits to the comment for > > > PinCountWaiterCheckReadyForCleanup() look great. > > > > > > > The invariante seems to be: Assert(PinCountWaitBuf == bufHdr); > > > > This process owns the logical cleanup wait for this buffer. > > > > > > Yes, this invariant is checked in PinCountWaiterCheckReadyForCleanup(). > > > > > > > Updated this, and extended the commit message like we discussed > > > > earlier though it might not get used at the end. > > > > > > Thank you very much for such an excellent commit message. Prior to it > > > we discussed to unite the explanation in one single paragraph. It seems, > > > I did poorly and I missed the description of changes. Your commit looks > > > very descriptive and complete. > > > > Thanks for proof-reading it! > > > > > One note about the commit message is the combining of words with a hyphen. > > > I see, that hot-standby appears in commit messages, but the hyphen in > > > buffer-pin, recovery-conflict or startup-progress seems excessive. > > > > > > I'm not sure, but the commit title ("Fix premature wakeups...") might > > > suggest > > > that the wakeups are being prevented entirely, rather than fixing how they > > > are handled. > > > > Ok, that's right. Agreed that the header line is somewhat misleading > > and needs to be softened. We did not *prevent* the general unrelated > > premature wakeups in ProcWaitForSignal. And those hyphens are > > unnecessary, they are removed. I also made some minor adjustments to > > the patch. Add timeouts_armed flag to tell the already expired timeout > > to avoid the extra round of cancellation request without > > re-introducing the original problem. If it is set, break the for loop, > > let the resolver send another cancellation request if the conflict is > > still there. > > > > There's another counterpart case for ResolveRecoveryConflictWithLock > > as the attached reproducer 913 shows. The startup process waits for a > > lock held by a standby session, which in turn waits for another lock > > held by the startup process. An unrelated wakeup before the set > > deadlock timeout causes the same kernel timeout rearming issue, > > preventing the startup process from detecting and breaking the cycle. > > > > > > The timeout para in that message is basically a summary of the first > > > email > > > > in this thread. I am not that familiar with it and too tired to > > > proof-read it. > > > > Can you do me a favor? > > > > > > The paragraph looks technically sound and explains the core issue in > > > detail. > > > Previously, a hang could occur after just a single premature wake-up. > > > This is > > > now stated clearly. While interference from multiple active timeouts can > > > lead > > > to more complex behavior, the fundamental problem remains unchanged. > > > > > > > > > > Just wondering whether we need a test for it. > > > > > > I have a test for issue reproduction that is attached to the first email > > > and > > > the patch in [1] where test/recovery/t/031_recovery_conflict.pl was > > > updated to reproduce the issue. But it uses log_startup_progress_interval > > > to activate a premature wakeup. There is an opinion in [2] that > > > log_startup_progress_interval behaviour may be changed, that invalidates > > > the test. I have no ideas about other simple alternative ways to trigger > > > premature wakeups. May be, injection points may help. > > > > > > There is a concern in [2] that changes to the behavior of > > > log_startup_progress_interval will invalidate the test. I cannot think of > > > other simple ways to trigger premature wakeups. Perhaps injection points > > > could help here. > > > > > > [1] > > > https://www.postgresql.org/message-id/CAHGQGwFED52rRVb_a%3DbVSdbOA%3DoARVOUyw381GC_6uKoNWJAtQ%40mail.gmail.com > > > [2] > > > https://www.postgresql.org/message-id/CAHGQGwHh5j%3DsG3AZh%3DHyuXtPM6cmymp6MJkRAMJcJUyN2qpHrQ%40mail.gmail.com > > > > pg_log_backend_memory_contexts seems able to trigger the premature > > wakeups for the startup. V10 adds two tests(lock/buffer pin) based on > > that. As usual, the hard part is to make it deterministic end-to-end. > > > > To make the stall occur, we need this sequence: > > > > 1) Startup sets a deadlock deadline and a kernel alarm for it. > > 2) An unrelated wakeup makes broken code move the deadline later, > > leaving the old alarm pending. > > 3) If startup handles that alarm before the new deadline, the handler > > wakes startup and schedules another alarm without firing the deadlock > > timeout. > > 4) Broken code moves the deadline later again, leaving that alarm pending. > > 5) Steps 3–4 can repeat indefinitely. > > > > The tricky part is in step 3. If there's some delay for the startup > > handling the alarm due to scheduling or other factors, the woken > > process would possibly find that the logical deadline has expired, > > subsequently, the handler would fire the deadlock timeout to send the > > cancellation request to break the cycle. There's no facility in the > > tree to simulate the time to let it 'pause' until the startup handles > > the alarm. Therefore, the added test exercises the mechanism rather > > than the behavior -- the timeout would not be rearmed after unrelated > > wakeups. > > Sorry for the missing reproducer.
Add them to the commifest [1]. Attach again to let the cfbots do their work. [1] https://commitfest.postgresql.org/patch/7372/ -- Regards, Xuneng Zhou HighGo Software Co., Ltd.
v10-0001-Handle-unrelated-wakeups-during-recovery-buffer-.patch
Description: Binary data
v10-0003-Use-RegisterPinCountWaiter-in-LockBufferForClean.patch
Description: Binary data
v10-0002-Test-recovery-conflict-waits-after-unrelated-wak.patch
Description: Binary data
