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

From: Bharath Rupireddy <[email-masked]>
Date: Tue, 28 Feb 2023 21:00:00 +0530

Hi,

Most of the multiplexed SIGUSR1 handlers are setting latch explicitly
when the procsignal_sigusr1_handler() can do that for them at the end.
These multiplexed handlers are currently being used as SIGUSR1
handlers, not as independent handlers, so no problem if SetLatch() is
removed from them. A few others do it right by saying /* latch will be
set by procsignal_sigusr1_handler */. Although, calling SetLatch() in
quick succession does no harm (it just returns if the latch was
previously set), it seems unnecessary.

I'm attaching a patch that avoids multiple SetLatch() calls.

Thoughts?

--
Bharath Rupireddy
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

---

From: Kuntal Ghosh <[email-masked]>
Date: Wed, 1 Mar 2023 15:14:36 +0530

> previously set), it seems unnecessary.
+1

-- 
Thanks & Regards,
Kuntal Ghosh

---

From: "Drouvot, Bertrand" <[email-masked]>
Date: Thu, 9 Mar 2023 13:24:32 +0100

Hi,

> when the procsignal_sigusr1_handler() can do that for them at the end.

Right.

> 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

Agree.

> I'm attaching a patch that avoids multiple SetLatch() calls.

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.

Otherwise the patch LGTM.

Regards,

-- 
Bertrand Drouvot

---

From: Fujii Masao <[email-masked]>
Date: Mon, 30 Mar 2026 09:30:08 +0900

> > previously set), it seems unnecessary.

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.

> 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(). Since the patch
doesn't change the API, such behavioral changes may be hard for extension
authors to notice. Also they will be not in release notes. In practice,
they would probably catch this during testing against a new major version,
though.

I guess similar situations have come up in past major upgrades, so perhaps
I'm overthinking this??

Regards,

[1] https://postgr.es/m/CABdArM6pmn5yFqiU33KTYBXYM=[email-masked]

-- 
Fujii Masao

---

From: Bharath Rupireddy <[email-masked]>
Date: Sun, 29 Mar 2026 18:30:00 -0700

Hi,

> I agree with the approach and have attached a revised version of the patch.

Thank you for reviving this patch.

> they would probably catch this during testing against a new major version,

I'm still +1 for removing these redundant SetLatch calls from the
multiplexed SIGUSR1 handlers. It not only keeps the signal handlers
consistent but also avoids an additional function call and memory
barrier from within the signal handler (a micro optimization).

I reviewed the v2 patch and it looks good to me.

---

From: Dilip Kumar <[email-masked]>
Date: Mon, 30 Mar 2026 09:50:54 +0530

On Mon, Mar 30, 2026 at 7:58 AM Chao Li <[email-masked]> wrote:
>
>
>
> > On Mar 30, 2026, at 08:30, Fujii Masao <[email-masked]> wrote:
> >
> > 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

---

From: Fujii Masao <[email-masked]>
Date: Wed, 1 Apr 2026 12:17:28 +0900

> 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

---

From: Bertrand Drouvot <[email-masked]>
Date: Wed, 1 Apr 2026 03:44:44 +0000

Hi,

> revisit it if the functions change in the future. Based on this, I'm thinking

That sounds reasonable to me to proceed as v2 is doing.

Regards,

---

From: Fujii Masao <[email-masked]>
Date: Thu, 2 Apr 2026 23:57:50 +0900

> That sounds reasonable to me to proceed as v2 is doing.

Thanks! I've pushed the patch.

Regards,

-- 
Fujii Masao