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

On Mon, Mar 30, 2026 at 1:21 PM Dilip Kumar <dilipbalaut(at)gmail(dot)com> wrote:
>
> 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.

I *guess* the original comment was added because readers of the interrupt
handling code might just wonder why SetLatch() isn't called. If so, it makes
sense to keep that explanation in the handler functions themselves.

The existing comment seems sufficient to me. The code isn't complicated enough
to require more comment for future use of functions in advance, and we can
revisit it if the functions change in the future. Based on this, I'm thinking
to commit v2 patch.

Regards,

--
Fujii Masao