Re: Deadlock detector fails to activate on a hot standby replica

On Sat, Oct 10, 2026 at 5:27 PM Xuneng Zhou <xunengzhou(at)gmail(dot)com> wrote:
>
> On Sat, Oct 10, 2026 at 5:24 PM Xuneng Zhou <xunengzhou(at)gmail(dot)com> wrote:
> >
> > Hi Vitaly,
> >
> > On Fri, Sep 4, 2026 at 10:16 PM Vitaly Davydov <vitprof(at)gmail(dot)com> 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.