Horton

Thread

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

    Fujii Masao <masao.fujii@gmail.com> — 2026-05-20T16:26:53Z

    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

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

    Vitaly Davydov <v.davydov@postgrespro.ru> — 2026-05-25T14:20:02Z

    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

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

    Fujii Masao <masao.fujii@gmail.com> — 2026-05-26T15:01:38Z

    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