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

public inbox for [email protected]  
help / color / mirror / Atom feed

From: Fujii Masao <[email protected]>
To: Dilip Kumar <[email protected]>
Cc: Chao Li <[email protected]>
Cc: Drouvot, Bertrand <[email protected]>
Cc: Bharath Rupireddy <[email protected]>
Cc: PostgreSQL Hackers <[email protected]>
Subject: Re: Avoid multiple SetLatch() calls in procsignal_sigusr1_handler()
Date: Wed, 1 Apr 2026 12:17:28 +0900
Message-ID: <CAHGQGwGCuccg81-PCMFbKDuwU-R7aNTD21WpkB9aL_5qopTURg@mail.gmail.com> (raw)
In-Reply-To: <CAFiTN-vfzMucegigUQhOQgE0FNGhf1ixauQ1fYkbNsvTpBOZRQ@mail.gmail.com>
References: <CALj2ACWd5apddj6Cd885WwJ6LquYu_G81C4GoR4xSoDV1x-FEA@mail.gmail.com>
	<[email protected]>
	<CAHGQGwFBnBQZbt4e9qjZoiM0GW2eXCvS=Ny5wt+s1-+iJpK15A@mail.gmail.com>
	<[email protected]>
	<CAFiTN-vfzMucegigUQhOQgE0FNGhf1ixauQ1fYkbNsvTpBOZRQ@mail.gmail.com>

On Mon, Mar 30, 2026 at 1:21 PM Dilip Kumar <[email protected]> wrote:
>
> On Mon, Mar 30, 2026 at 7:58 AM Chao Li <[email protected]> wrote:
> >
> >
> >
> > > On Mar 30, 2026, at 08:30, Fujii Masao <[email protected]> wrote:
> > >
> > > On Thu, Mar 9, 2023 at 9:24 PM Drouvot, Bertrand
> > > <[email protected]> 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

view thread (9+ messages)  latest in thread

reply

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Reply to all the recipients using the --to and --cc options:
  reply via email

  To: [email protected]
  Cc: [email protected], [email protected], [email protected], [email protected], [email protected], [email protected]
  Subject: Re: Avoid multiple SetLatch() calls in procsignal_sigusr1_handler()
  In-Reply-To: <CAHGQGwGCuccg81-PCMFbKDuwU-R7aNTD21WpkB9aL_5qopTURg@mail.gmail.com>

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox