Deadlock detector fails to activate on a hot standby replica

Dear Hackers,

I would like to propose a patch that fixes the problem, which has the roots in
the possibility of spontaneous SIGALRM signals when waiting for some timeouts.
The idea of the patch - ignore spontaneous SIGALRM signals and continue waiting
for expected timeouts or buffer unpinning by the conflicting backend. This
patch is not a final version. I plan to add a tap-test for this case.

I'm in doubt to put the calls of some page buffer specific functions into
ResolveRecoveryConflictWithBufferPin (standby.c), but otherwise we have to
do more changes in LockBufferForCleanup and ResolveRecoveryConflictWithBufferPin.

I also think, we have to add some description of the found problem in timeout.c,
because the implemented optimization of setitimer calls leads to some not
evident consequences. The optimization seems to be implemented in the commit:
09cf1d52267644cdbdb734294012cf1228745aaa

With best regards,
Vitaly

On Fri, Jan 23, 2026 at 8:52 PM Vitaly Davydov <v.davydov@postgrespro.ru> wrote:

Dear Hackers,

I would like to propose a patch that fixes the problem, which has the roots in
the possibility of spontaneous SIGALRM signals when waiting for some timeouts.
The idea of the patch - ignore spontaneous SIGALRM signals and continue waiting
for expected timeouts or buffer unpinning by the conflicting backend. This
patch is not a final version. I plan to add a tap-test for this case.

Thanks for the patch!

#include "storage/sinvaladt.h"
#include "storage/standby.h"
+#include "storage/buf_internals.h"

This include should be placed in alphabetical order.

+ * We assume that only UnpinBuffer() and the timeout requests established
+ * above can wake us up here. WakeupRecovery() called by walreceiver or
+ * SIGHUP signal handler, etc cannot do that because it uses the different
+ * latch from that ProcWaitForSignal() waits on.

As your investigation showed, that assumption does not seem to hold. If so,
I think something like the following would be more accurate:

---------------------------------
ProcWaitForSignal() can also wake up for unrelated reasons, so recheck
whether we're still the waiter after each wakeup. If we are and no timeout
fired, continue waiting without resetting the active timeouts.
---------------------------------

+ uint32 buf_refcount = BUF_STATE_GET_REFCOUNT(buf_state);
<snip>
+ if (buf_refcount > 1)
+ continue;

Wouldn't it be better to check explicitly whether we're still the waiter,
instead of using BUF_STATE_GET_REFCOUNT()?

(buf_state & BM_PIN_COUNT_WAITER) != 0 &&
bufHdr->wait_backend_pgprocno == MyProcNumber

The current control flow in the loop feels a bit hard to follow.
Would something like the following be simpler?

for (;;)
{
....
ProcWaitForSignal(...);

if (!StillWaitingForBufferPin(...))
break;

if (got_standby_delay_timeout)
{
SendRecoveryConflictWithBufferPin(...);
break;
}
else if (got_standby_deadlock_timeout)
{
SendRecoveryConflictWithBufferPin(...);
break;
}
}

Regards,

--
Fujii Masao

Dear Fujii Masao,

Thank you for the review of the patch. Based on your comments I propose
a new version of the patch.

+#include "storage/buf_internals.h"
This include should be placed in alphabetical order.

I removed the include of the header file. I'm not sure, that it is a good
way to expose buffer internals here. Instead, I implemented a new function
BufferGetRefCount to be used in standby.c.

+ uint32 buf_refcount = BUF_STATE_GET_REFCOUNT(buf_state);
<snip>
+ if (buf_refcount > 1)
+ continue;

Wouldn't it be better to check explicitly whether we're still the waiter,
instead of using BUF_STATE_GET_REFCOUNT()?

(buf_state & BM_PIN_COUNT_WAITER) != 0 &&
bufHdr->wait_backend_pgprocno == MyProcNumber

This is a precondition for the ResolveRecoveryConflictWithBufferPin function
that the current process should be a waiter process. See LockBufferForCleanup
where this state is set before the call of ResolveRecoveryConflictWithBufferPin.
I believe, there is no sense to check the waiter state because it doesn't
change in this function. Instead, I think, it is enough to just check for
refcount. One of the suggestions is to add an assert to check that the current
process is a waiter process, but I'm not sure about it. Let me know please if
I missed anything.

The current control flow in the loop feels a bit hard to follow.
Would something like the following be simpler?

I reorganized the control flow as well.

I also added a new tap-test as a part of the patch. I did some changes in the
tap test to make it stable. Let me know, please, if it should be in a separate
commit.

With best regards,
Vitaly

On Mon, May 25, 2026 at 11:20 PM Vitaly Davydov
<v.davydov@postgrespro.ru> wrote:

Dear Fujii Masao,

Thank you for the review of the patch. Based on your comments I propose
a new version of the patch.

Thanks for updating the patch!

+ if (got_standby_delay_timeout)
+ SendRecoveryConflictWithBufferPin(RECOVERY_CONFLICT_BUFFERPIN);
+ else if (got_standby_deadlock_timeout)
+ {

Shouldn't we break out of the loop when either got_standby_delay_timeout or
got_standby_deadlock_timeout becomes true? Otherwise, the loop continues with
those flags still set, which could cause SendRecoveryConflictWithBufferPin() to
be called unnecessarily in the subsequent cycles.

+ if (BufferGetRefCount(buffer) <= 1)

Should this be "BufferGetRefCount(buffer) == 1" instead? I don't think
BufferGetRefCount(buffer) should ever return 0 here. If that's correct,
would it make sense to explicitly detect that case, for example:

-----------------
uint32 refcount = BufferGetRefCount(buffer);

Assert(refcount > 0);

if (refcount == 0)
elog(ERROR, "buffer refcount dropped to zero while waiting for
cleanup lock");

if (refcount == 1)
break;
-----------------

I also added a new tap-test as a part of the patch. I did some changes in the
tap test to make it stable. Let me know, please, if it should be in a separate
commit.

Do we really need a new TAP test file for this? We already have
a startup/backend deadlock test in t/031_recovery_conflict.pl. Extending
the deadlock section there to cover this test case seems simpler and easier to
follow than introducing a new t/053_* test.

Regards,

--
Fujii Masao

On Wed, May 27, 2026 at 12:01 AM Fujii Masao <masao.fujii@gmail.com> wrote:

+ if (got_standby_delay_timeout)
+ SendRecoveryConflictWithBufferPin(RECOVERY_CONFLICT_BUFFERPIN);
+ else if (got_standby_deadlock_timeout)
+ {

Shouldn't we break out of the loop when either got_standby_delay_timeout or
got_standby_deadlock_timeout becomes true? Otherwise, the loop continues with
those flags still set, which could cause SendRecoveryConflictWithBufferPin() to
be called unnecessarily in the subsequent cycles.

+ if (BufferGetRefCount(buffer) <= 1)

Should this be "BufferGetRefCount(buffer) == 1" instead? I don't think
BufferGetRefCount(buffer) should ever return 0 here. If that's correct,
would it make sense to explicitly detect that case, for example:

-----------------
uint32 refcount = BufferGetRefCount(buffer);

Assert(refcount > 0);

if (refcount == 0)
elog(ERROR, "buffer refcount dropped to zero while waiting for
cleanup lock");

if (refcount == 1)
break;
-----------------

I've updated the patch based on these comments.
Attached is the latest version.

I removed the TAP test from this patch for now. I'll consider adding
a test for this separately later.

BTW, I'm just wondering whether ResolveRecoveryConflictWithLock()
might have the same issue. I need to investigate that further.

Regards,

--
Fujii Masao

On 6/5/26 14:58, Fujii Masao wrote:

I've updated the patch based on these comments.
Attached is the latest version.

I removed the TAP test from this patch for now. I'll consider adding
a test for this separately later.

Thank you again for your assistance. I agree with the code changes you've
proposed. I've updated 031_recovery_conflict.pl to include the deadlock
test, instead of creating a separate TAP test. The attached patch contains
these changes in its second commit. I've kept the patch version as v3 since
the original fix remains unchanged; this update only adds new tests.

With best regards,
Vitaly

On Wed, Jun 10, 2026 at 1:18 AM Vitaly Davydov <v.davydov@postgrespro.ru> wrote:

On 6/5/26 14:58, Fujii Masao wrote:

I've updated the patch based on these comments.
Attached is the latest version.

I removed the TAP test from this patch for now. I'll consider adding
a test for this separately later.

Thank you again for your assistance. I agree with the code changes you've
proposed. I've updated 031_recovery_conflict.pl to include the deadlock
test, instead of creating a separate TAP test. The attached patch contains
these changes in its second commit. I've kept the patch version as v3 since
the original fix remains unchanged; this update only adds new tests.

Thanks for updating the regression test patch!

I reviewed both patches and found a few issues, so I fixed them and
attached updated v4 versions.

For v3-0001, when the startup process wakes up from
ProcWaitForSignal() and finds the pin count is still not 1, it waits
again. However, before waiting again, it must set
BM_PIN_COUNT_WAITER again. Otherwise, a backend releasing the pin
cannot signal the startup process because the flag was already cleared.

For example:
1. Backend A releases the pin, signals the startup process, and clears
BM_PIN_COUNT_WAITER.
2. The startup process wakes up from ProcWaitForSignal().
3. Before it rechecks the pin count, backend B acquires a pin.
4. The startup process sees the pin count is still 2.
5. The startup process waits again.

At step 5, BM_PIN_COUNT_WAITER is already cleared, so backend B will
not signal the startup process when releasing the pin.

This race is unlikely in practice, but it is still possible. So in
v4-0001, I fixed this by re-registering the startup process as the
waiter before waiting again.

For v3-0002, the two deadlock tests contained very similar scenarios.
I think it's simpler to move the common logic into a shared subroutine.
So in v4-0002, I added a helper routine for testing the deadlock
between the startup process and a backend, and both tests now use it.

Could you review the v4 patches?

Regards,

--
Fujii Masao

Dear Fujii Masao,

On 6/10/26 11:42, Fujii Masao wrote:

Could you review the v4 patches?

I've got the issue with BM_PIN_COUNT_WAITER. The origin of the issue is that
the other backend resets the flag before sending the notification (signal)
to the waiter. I agree with your changes where this flag is set again.

I would like to propose BufferIsReadyForCleanup function name instead of
BufferShouldWaitForCleanupLockForStandby (and change the returning value
semantics). There are other possible alternatives: BufferIsReadyForWriting,
BufferIsReadyForExclusiveAccess or something else. The code will look like
below:

if (BufferIsReadyForCleanup(bufid + 1))
break;

I've also found strange email in the commit messages like below:
Author: masao.fujii <masao.fujii@masao.fujii’s-MacBook-Pro>

I tried the modified 031 tap test but I do not see that it reproduces the issue
on the version without the fix. My version of 031 tap test hangs without the
fix. I haven't yet reviewed the tap test code at the moment, but I'm going to do
it as well.

Below one more note that is not directly related to the fix but may probably
improve the code readability.

1. BufferIsReadyForCleanup can be re-used with some small modifications in
LockBufferForCleanup like shown below:

LockBuffer(buffer, BUFFER_LOCK_EXCLUSIVE);
if (BufferIsReadyForCleanup())
{
// log recovery conflict
return;
}

In this case, we have to reset PM_PIN_COUNT_WAITER state in BufferIsReadyForCleanup:

if (buf_refcount == 1)
{
bufHdr->wait_backend_procno = -1;
PinCountWaitBuf = NULL;
UnlockBufHdrExt(bufHdr, 0, PM_PIN_COUNT_WAITER);
return false;
}

I may to prepare one more commit with this change.

With best regards,
Vitaly

On Fri, Jun 12, 2026 at 3:00 PM Vitaly Davydov <v.davydov@postgrespro.ru> wrote:

Dear Fujii Masao,

On 6/10/26 11:42, Fujii Masao wrote:

Could you review the v4 patches?

I've got the issue with BM_PIN_COUNT_WAITER. The origin of the issue is that
the other backend resets the flag before sending the notification (signal)
to the waiter. I agree with your changes where this flag is set again.

I would like to propose BufferIsReadyForCleanup function name instead of
BufferShouldWaitForCleanupLockForStandby (and change the returning value
semantics). There are other possible alternatives: BufferIsReadyForWriting,
BufferIsReadyForExclusiveAccess or something else. The code will look like
below:

if (BufferIsReadyForCleanup(bufid + 1))
break;

Okay.

I've also found strange email in the commit messages like below:
Author: masao.fujii <masao.fujii@masao.fujii’s-MacBook-Pro>

That's my mistake. Sorry. I'll fix it in the next version of the patch.

I tried the modified 031 tap test but I do not see that it reproduces the issue
on the version without the fix. My version of 031 tap test hangs without the
fix. I haven't yet reviewed the tap test code at the moment, but I'm going to do
it as well.

I retried the modified 031 TAP test on my side without the fix, and
the test actually passed, so the test did not reliably reproduce the issue.
However, after increasing deadlock_timeout to 10s, the test hung
without the fix as expected, while it passed with the fix.

Based on this, I suspect the behavior depends on timing interactions
between deadlock_timeout and SIGALRM delivery caused by
log_startup_progress_interval. My test changes may simply have altered
the timing of those SIGALRM deliveries.

Also, the current test assumes that
log_startup_progress_interval is effective during standby WAL replay.
But as discussed in [1]/messages/by-id/CAHGQGwFJixYTymPZ6dwOO-G0VdjO1AgZTma6CtJgc0Ptxfz3BA@mail.gmail.com, that assumption may actually be wrong. It
seems to work today, but that could just be accidental and might
change in the future. So I think we should consider a testing approach
that does not depend on that assumption.

At the moment, the only alternative I can think of is to send SIGALRM
periodically to the startup process directly from the test, though I'm
not sure that's a good approach either...

Below one more note that is not directly related to the fix but may probably
improve the code readability.

I agree that some refactoring along those lines should be possible.
However, for backpatching to older branches, I'd prefer to avoid more
code churn than necessary for the bug fix itself. So I think it would
be better to commit the bug fix first, and then consider a refactoring
version separately for v20dev.

Regards,

[1]: /messages/by-id/CAHGQGwFJixYTymPZ6dwOO-G0VdjO1AgZTma6CtJgc0Ptxfz3BA@mail.gmail.com

--
Fujii Masao

On Fri, Jun 12, 2026 at 6:23 PM Fujii Masao <masao.fujii@gmail.com> wrote:

On Fri, Jun 12, 2026 at 3:00 PM Vitaly Davydov <v.davydov@postgrespro.ru> wrote:

Dear Fujii Masao,

On 6/10/26 11:42, Fujii Masao wrote:

Could you review the v4 patches?

I've got the issue with BM_PIN_COUNT_WAITER. The origin of the issue is that
the other backend resets the flag before sending the notification (signal)
to the waiter. I agree with your changes where this flag is set again.

I would like to propose BufferIsReadyForCleanup function name instead of
BufferShouldWaitForCleanupLockForStandby (and change the returning value
semantics). There are other possible alternatives: BufferIsReadyForWriting,
BufferIsReadyForExclusiveAccess or something else. The code will look like
below:

if (BufferIsReadyForCleanup(bufid + 1))
break;

Okay.

I've updated the patch accordingly.

Also, the current test assumes that
log_startup_progress_interval is effective during standby WAL replay.
But as discussed in [1], that assumption may actually be wrong. It
seems to work today, but that could just be accidental and might
change in the future. So I think we should consider a testing approach
that does not depend on that assumption.

At the moment, the only alternative I can think of is to send SIGALRM
periodically to the startup process directly from the test, though I'm
not sure that's a good approach either...

I still haven't come up with a good way to test this without depending on
log_startup_progress_interval being active during standby WAL replay.
Even so, I think the fix is still worth committing because it addresses
a real issue. So how about committing 0001 patch first, even without
a regression test?

Regards,

--
Fujii Masao

Dear Fujii Masao,

Thank you for the v5 patch. It looks good for me.

I still haven't come up with a good way to test this without depending on
log_startup_progress_interval being active during standby WAL replay.
Even so, I think the fix is still worth committing because it addresses
a real issue. So how about committing 0001 patch first, even without
a regression test?

I have no objections to commit the v5 patch without the tap-test. Once, it is
the bug fix, should we prepare the fix for other major branches? I'm ready to
prepare the patches based on the v5 version.

I looked at the tap test and found the reason why the test doesn't reproduce
the issue. The function adjust_conf in Cluster.pm changes the existing
parameter but doesn't add new new parameter to the configuration file if it is
missed. It doesn't report any errors as well. If to use the append_conf method
the test starts to reproduce the issue pretty stable, because the parameter is
actually added to the configuration file.

One more note - in my original test I set autovacuum = off for all nodes, but
in modified 031 test autovacuum seems to be enabled. Disabling autovacuum may
make the test more stable.

With best regards,
Vitaly

On Thu, Jun 18, 2026 at 10:11 PM Vitaly Davydov
<v.davydov@postgrespro.ru> wrote:

Dear Fujii Masao,

Thank you for the v5 patch. It looks good for me.

Thanks for the review!

I have no objections to commit the v5 patch without the tap-test. Once, it is
the bug fix, should we prepare the fix for other major branches? I'm ready to
prepare the patches based on the v5 version.

Yes, it's really helpful if you prepare the patches for stable versions.

I looked at the tap test and found the reason why the test doesn't reproduce
the issue. The function adjust_conf in Cluster.pm changes the existing
parameter but doesn't add new new parameter to the configuration file if it is
missed. It doesn't report any errors as well. If to use the append_conf method
the test starts to reproduce the issue pretty stable, because the parameter is
actually added to the configuration file.

One more note - in my original test I set autovacuum = off for all nodes, but
in modified 031 test autovacuum seems to be enabled. Disabling autovacuum may
make the test more stable.

Thanks for the analysis!

Regards,

--
Fujii Masao

Dear Fujii Masao, All

While working on porting the patch to other majors, I've found that, the code
in RegisterPinCountWaiter() (see below) should be improved because it doesn't
unlock the bufHdr when the error is raised, as in LockBufferForCleanup().
I guess it works, but it keeps the locks a bit longer.

RegisterPinCountWaiter v5 patch code:
...
if ((buf_state & BM_PIN_COUNT_WAITER) != 0 &&
bufHdr->wait_backend_pgprocno != MyProcNumber)
elog(ERROR, "multiple processes attempting to wait for pincount 1");

RegisterPinCountWaiter proposed changes:
...
if ((buf_state & BM_PIN_COUNT_WAITER) != 0 &&
bufHdr->wait_backend_pgprocno != MyProcNumber)
{
UnlockBufHdr(bufHdr);
elog(ERROR, "multiple processes attempting to wait for pincount 1");
}

What do you think?

With best regards,
Vitaly

On Tue, Jun 23, 2026 at 3:32 AM Vitaly Davydov <v.davydov@postgrespro.ru> wrote:

Dear Fujii Masao, All

While working on porting the patch to other majors, I've found that, the code
in RegisterPinCountWaiter() (see below) should be improved because it doesn't
unlock the bufHdr when the error is raised, as in LockBufferForCleanup().
I guess it works, but it keeps the locks a bit longer.

RegisterPinCountWaiter v5 patch code:
...
if ((buf_state & BM_PIN_COUNT_WAITER) != 0 &&
bufHdr->wait_backend_pgprocno != MyProcNumber)
elog(ERROR, "multiple processes attempting to wait for pincount 1");

RegisterPinCountWaiter proposed changes:
...
if ((buf_state & BM_PIN_COUNT_WAITER) != 0 &&
bufHdr->wait_backend_pgprocno != MyProcNumber)
{
UnlockBufHdr(bufHdr);
elog(ERROR, "multiple processes attempting to wait for pincount 1");
}

What do you think?

You're right. Thanks for pointing out that!

I've updated the patch accordingly.

Regards,

--
Fujii Masao