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

> On Mar 30, 2026, at 08:30, Fujii Masao <masao(dot)fujii(at)gmail(dot)com> wrote:
>
> On Thu, Mar 9, 2023 at 9:24 PM Drouvot, Bertrand
> <bertranddrouvot(dot)pg(at)gmail(dot)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 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().

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/