MIME-Version: 1.0
References: <44c24dcf-5710-410f-b1b6-d10b315f3d51@postgrespro.ru>
 <b178ea8d-9ed9-48b3-b4f7-5cfc3ff6ee44@postgrespro.ru>
 <CAHGQGwExVv1-mtPEX-JFGBXNY2r8DfP+sQVPD8GpDTefW0KdsQ@mail.gmail.com>
 <25cba756-9459-4811-aeaf-4a8715735c82@postgrespro.ru>
In-Reply-To: <25cba756-9459-4811-aeaf-4a8715735c82@postgrespro.ru>
From: Fujii Masao <masao.fujii@gmail.com>
Date: Wed, 27 May 2026 00:01:38 +0900
Message-ID: 
 <CAHGQGwFCqEkeaoUKHPJ5kpGjrKrJ5nzqE6NqjT-Rz3S6Zg7fug@mail.gmail.com>
Subject: Re: Deadlock detector fails to activate on a hot standby replica
To: Vitaly Davydov <v.davydov@postgrespro.ru>
Cc: pgsql-hackers@lists.postgresql.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Archived-At: 
 <https://www.postgresql.org/message-id/CAHGQGwFCqEkeaoUKHPJ5kpGjrKrJ5nzqE6NqjT-Rz3S6Zg7fug%40mail.gmail.com>
Precedence: bulk

On Mon, May 25, 2026 at 11:20=E2=80=AFPM 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 wi=
th
those flags still set, which could cause SendRecoveryConflictWithBufferPin(=
) to
be called unnecessarily in the subsequent cycles.


+ if (BufferGetRefCount(buffer) <=3D 1)

Should this be "BufferGetRefCount(buffer) =3D=3D 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 =3D BufferGetRefCount(buffer);

    Assert(refcount > 0);

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

    if (refcount =3D=3D 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 sep=
arate
> 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,

--=20
Fujii Masao