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

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.

--
Regards,
Xuneng Zhou
HighGo Software Co., Ltd.