public inbox for [email protected]  

help / color / mirror / Atom feed

From: Dilip Kumar <[email protected]>
To: Chao Li <[email protected]>
Cc: Fujii Masao <[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: Mon, 30 Mar 2026 09:50:54 +0530
Message-ID: <CAFiTN-vfzMucegigUQhOQgE0FNGhf1ixauQ1fYkbNsvTpBOZRQ@mail.gmail.com> (raw)
In-Reply-To: <[email protected]>
References: <CALj2ACWd5apddj6Cd885WwJ6LquYu_G81C4GoR4xSoDV1x-FEA@mail.gmail.com>
	<[email protected]>
	<CAHGQGwFBnBQZbt4e9qjZoiM0GW2eXCvS=Ny5wt+s1-+iJpK15A@mail.gmail.com>
	<[email protected]>

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.

-- 
Regards,
Dilip Kumar
Google

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: <CAFiTN-vfzMucegigUQhOQgE0FNGhf1ixauQ1fYkbNsvTpBOZRQ@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