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

On Mon, Mar 30, 2026 at 7:58 AM Chao Li <li(dot)evan(dot)chao(at)gmail(dot)com> wrote:
>
>
>
> > On Mar 30, 2026, at 08:30, Fujii Masao <masao(dot)fujii(at)gmail(dot)com> wrote:
> >
> > 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().
>
> Actually, after reading this patch, I had the same feeling. Today, in core, those handler functions are only called by procsignal_sigusr1_handler(), but is it possible that they may have other callers in the future?
>
> IMO, removing the current duplicate SetLatch() calls from the individual handler functions is fine. But I do not like the idea of adding the comment "latch will be set by procsignal_sigusr1_handler" to every handler function. My reasons are:
>
> * When a new handler is added, will it become the standard pattern to add the same comment everywhere? That seems like extra burden.
> * Usually, code readers come to procsignal_sigusr1_handler() first, and then read down into the individual handlers.
> * A handler function could, at least in theory, be reused in some other context where calling SetLatch() would still be necessary.
>
> So instead of adding the comment everywhere, I would suggest adding one comment in procsignal_sigusr1_handler(), something like “handlers should not call SetLatch() themselves”. If a handler ever needs to be used in different contexts, then it could take a parameter to decide whether SetLatch() should be called. When the function is called from procsignal_sigusr1_handler(), that parameter could disable SetLatch(), while other callers could enable it as needed. In other words, the control of not calling SetLatch() for handlers stays in procsignal_sigusr1_handler().

Shouldn't we add a comment to the handler function header stating that
SetLatch should be called by the caller? procsignal_sigusr1_handler()
is currently the only caller and handles it, but this would ensure any
future callers are responsible for the same.

--
Regards,
Dilip Kumar
Google