MIME-Version: 1.0
References: 
 <CALj2ACWd5apddj6Cd885WwJ6LquYu_G81C4GoR4xSoDV1x-FEA@mail.gmail.com>
 <b5eab65a-e11b-6435-b6aa-dac206e17d38@gmail.com>
 <CAHGQGwFBnBQZbt4e9qjZoiM0GW2eXCvS=Ny5wt+s1-+iJpK15A@mail.gmail.com>
 <29CA73B2-872A-4D7D-B8E9-D89F76DC18AF@gmail.com>
In-Reply-To: <29CA73B2-872A-4D7D-B8E9-D89F76DC18AF@gmail.com>
From: Dilip Kumar <dilipbalaut@gmail.com>
Date: Mon, 30 Mar 2026 09:50:54 +0530
Message-ID: 
 <CAFiTN-vfzMucegigUQhOQgE0FNGhf1ixauQ1fYkbNsvTpBOZRQ@mail.gmail.com>
Subject: Re: Avoid multiple SetLatch() calls in procsignal_sigusr1_handler()
To: Chao Li <li.evan.chao@gmail.com>
Cc: Fujii Masao <masao.fujii@gmail.com>,
	"Drouvot, Bertrand" <bertranddrouvot.pg@gmail.com>,
	Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>,
	PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Archived-At: 
 <https://www.postgresql.org/message-id/CAFiTN-vfzMucegigUQhOQgE0FNGhf1ixauQ1fYkbNsvTpBOZRQ%40mail.gmail.com>
Precedence: bulk

On Mon, Mar 30, 2026 at 7:58=E2=80=AFAM Chao Li <li.evan.chao@gmail.com> wr=
ote:
>
>
>
> > On Mar 30, 2026, at 08:30, Fujii Masao <masao.fujii@gmail.com> wrote:
> >
> > On Thu, Mar 9, 2023 at 9:24=E2=80=AFPM Drouvot, Bertrand
> > <bertranddrouvot.pg@gmail.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 HandleLogMemoryCont=
extInterrupt().
> >>
> >>> 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 her=
e.
> > I agree with the approach and have attached a revised version of the pa=
tch.
> >
> >
> >>> 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 function=
s
> > behave identically across major versions, so removing redundant SetLatc=
h()
> > calls is generally fine. However, as you are concerned, extensions migh=
t call
> > these functions and implicitly rely on the extra SetLatch().
>
> Actually, after reading this patch, I had the same feeling. Today, in cor=
e, 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 functio=
n. 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 t=
hen read down into the individual handlers.
> * A handler function could, at least in theory, be reused in some other c=
ontext where calling SetLatch() would still be necessary.
>
> So instead of adding the comment everywhere, I would suggest adding one c=
omment in procsignal_sigusr1_handler(), something like =E2=80=9Chandlers sh=
ould not call SetLatch() themselves=E2=80=9D. If a handler ever needs to be=
 used in different contexts, then it could take a parameter to decide wheth=
er SetLatch() should be called. When the function is called from procsignal=
_sigusr1_handler(), that parameter could disable SetLatch(), while other ca=
llers 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.

--=20
Regards,
Dilip Kumar
Google