On Mon, Mar 30, 2026 at 7:58 AM Chao Li <[email protected]> wrote: > > > > > 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().
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
