Re: Avoid multiple SetLatch() calls in procsignal_sigusr1_handler()

On Thu, Mar 9, 2023 at 9:24 PM Drouvot, Bertrand
<bertranddrouvot(dot)pg(at)gmail(dot)com> wrote:
>
> Hi,
>
> On 2/28/23 4:30 PM, Bharath Rupireddy wrote:
> > Hi,
> >
> > Most of the multiplexed SIGUSR1 handlers are setting latch explicitly
> > when the procsignal_sigusr1_handler() can do that for them at the end.
>
> Right.
>
> > These multiplexed handlers are currently being used as SIGUSR1
> > handlers, not as independent handlers, so no problem if SetLatch() is
> > removed from them.
>
> Agree, they are only used in procsignal_sigusr1_handler().
>
> > A few others do it right by saying /* latch will be
> > set by procsignal_sigusr1_handler */.
>
> Yeap, so do HandleProcSignalBarrierInterrupt() and HandleLogMemoryContextInterrupt().
>
> > Although, calling SetLatch() in
> > quick succession does no harm (it just returns if the latch was
> > previously set), it seems unnecessary.
> >
>
> Agree.

While reviewing the patch in [1], I noticed this issue and ended up here.
I agree with the approach and have attached a revised version of the patch.

> > I'm attaching a patch that avoids multiple SetLatch() calls.
> >
> > Thoughts?
> >
>
> I agree with the idea behind the patch. The thing
> that worry me a bit is that the changed functions are defined
> as external and so may produce an impact outside of core pg and I'm
> not sure that's worth it.

I understand the concern. There's no guarantee that PostgreSQL functions
behave identically across major versions, so removing redundant SetLatch()
calls is generally fine. However, as you are concerned, extensions might call
these functions and implicitly rely on the extra SetLatch(). Since the patch
doesn't change the API, such behavioral changes may be hard for extension
authors to notice. Also they will be not in release notes. In practice,
they would probably catch this during testing against a new major version,
though.

I guess similar situations have come up in past major upgrades, so perhaps
I'm overthinking this??

Regards,

[1] https://postgr.es/m/CABdArM6pmn5yFqiU33KTYBXYM=Vny2ULnJY_gqFbsMEdt+1dPA@mail.gmail.com

--
Fujii Masao