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

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.

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.

> 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

With best regards,
Vitaly Davydov
vitprof(at)gmail(dot)com (aka v(dot)davydov(at)postgrespro(dot)ru)
vitcpp(at)mail(dot)ru