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>
 <CAFiTN-vfzMucegigUQhOQgE0FNGhf1ixauQ1fYkbNsvTpBOZRQ@mail.gmail.com>
In-Reply-To: 
 <CAFiTN-vfzMucegigUQhOQgE0FNGhf1ixauQ1fYkbNsvTpBOZRQ@mail.gmail.com>
From: Fujii Masao <masao.fujii@gmail.com>
Date: Wed, 1 Apr 2026 12:17:28 +0900
Message-ID: 
 <CAHGQGwGCuccg81-PCMFbKDuwU-R7aNTD21WpkB9aL_5qopTURg@mail.gmail.com>
Subject: Re: Avoid multiple SetLatch() calls in procsignal_sigusr1_handler()
To: Dilip Kumar <dilipbalaut@gmail.com>
Cc: Chao Li <li.evan.chao@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/CAHGQGwGCuccg81-PCMFbKDuwU-R7aNTD21WpkB9aL_5qopTURg%40mail.gmail.com>
Precedence: bulk

On Mon, Mar 30, 2026 at 1:21=E2=80=AFPM Dilip Kumar <dilipbalaut@gmail.com>=
 wrote:
>
> On Mon, Mar 30, 2026 at 7:58=E2=80=AFAM Chao Li <li.evan.chao@gmail.com> =
wrote:
> >
> >
> >
> > > 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 explicit=
ly
> > >>> when the procsignal_sigusr1_handler() can do that for them at the e=
nd.
> > >>
> > >> 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 HandleLogMemoryCo=
ntextInterrupt().
> > >>
> > >>> 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 h=
ere.
> > > 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 functi=
ons
> > > behave identically across major versions, so removing redundant SetLa=
tch()
> > > calls is generally fine. However, as you are concerned, extensions mi=
ght call
> > > these functions and implicitly rely on the extra SetLatch().
> >
> > Actually, after reading this patch, I had the same feeling. Today, in c=
ore, 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 individua=
l handler functions is fine. But I do not like the idea of adding the comme=
nt "latch will be set by procsignal_sigusr1_handler" to every handler funct=
ion. My reasons are:
> >
> > * When a new handler is added, will it become the standard pattern to a=
dd 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 =E2=80=9Chandlers =
should 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 whe=
ther SetLatch() should be called. When the function is called from procsign=
al_sigusr1_handler(), that parameter could disable SetLatch(), while other =
callers could enable it as needed. In other words, the control of not calli=
ng 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 make=
s
sense to keep that explanation in the handler functions themselves.

The existing comment seems sufficient to me. The code isn't complicated eno=
ugh
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 thinki=
ng
to commit v2 patch.

Regards,

--=20
Fujii Masao