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

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.

Regards,
Xuneng Zhou
HighGo Software Co., Ltd.