> 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().
Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/
