Re: Deadlock between backend and recovery may not be detected - Mailing list pgsql-hackers

At Tue, 5 Jan 2021 15:26:50 +0900, Fujii Masao <masao.fujii@oss.nttdata.com> wrote in 
> 
> 
> On 2020/12/25 13:16, Kyotaro Horiguchi wrote:
> > At Wed, 23 Dec 2020 21:42:47 +0900, Fujii Masao
> > <masao.fujii@oss.nttdata.com> wrote in
> >> you. Attached
> >> is the updated of the patch. What about this version?
> > The patch contains a hunk in the following structure.
> > +    if (got_standby_lock_timeout)
> > +        goto cleanup;
> > +
> > +    if (got_standby_deadlock_timeout)
> > +    {
> > ...
> > +    }
> > +
> > +cleanup:
> > It is eqivalent to
> > +    if (!got_standby_lock_timeout && got_standby_deadlock_timeout)
> > +    {
> > ...
> > +    }
> > Is there any reason for the goto?
> 
> Yes. That's because we have the following code using goto.
> 
> +               /* Quick exit if there's no work to be done */
> +               if (!VirtualTransactionIdIsValid(*backends))
> +                       goto cleanup;

It doesn't seem to be the *cause*.  Such straight-forward logic with
three-depth indentation is not a thing that should be avoided using
goto even if PROCSIG_RECOVERY_CONFLICT_STARTUP_DEADLOCK is too lengty
and sticks out of 80 coloumns.

> Regarding the back-patch, I was thinking to back-patch this to all the
> supported branches. But I found that it's not easy to do that to v9.5
> because v9.5 doesn't have some infrastructure code that this bug fix
> patch depends on. So at least the commit 37c54863cf as the
> infrastructure
> also needs to be back-patched to v9.5. And ISTM that some related
> commits
> f868a8143a and 8f0de712c3 need to be back-patched. Probably there
> might
> be some other changes to be back-patched. Unfortunately they cannot be
> applied to v9.5 cleanly and additional changes are necessary.
> 
> This situation makes me feel that I'm inclined to skip the back-patch
> to v9.5.
> Because the next minor version release is the final one for v9.5. So
> if we
> unexpectedly introduce the bug to v9.5 by the back-patch, there is no
> chance to fix that. OTOH, of course, if we don't do the back-patch,
> there is
> no chance to fix the deadlock detection bug since the final minor
> version
> for v9.5 doesn't include that bug fix. But I'm afraid that the
> back-patch
> to v9.5 may give more risk than gain.
> 
> Thought?

It seems to me that the final minor release should get fixes only for
issues that we have actually gotten complaints on, or critical-ish
known issues such as ones lead to server crash in normal paths. This
particular issue is neither of them.

So +1 for not back-patching to 9.5.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center