Обсуждение: Avoid multiple SetLatch() calls in procsignal_sigusr1_handler()
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
Вложения
On Tue, Feb 28, 2023 at 9:01 PM Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> wrote: > > 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. > +1 -- Thanks & Regards, Kuntal Ghosh
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. > 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. Otherwise the patch LGTM. Regards, -- Bertrand Drouvot PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com
On Thu, Mar 9, 2023 at 9:24 PM 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 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(). 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=Vny2ULnJY_gqFbsMEdt+1dPA@mail.gmail.com -- Fujii Masao
Вложения
Hi, On Sun, Mar 29, 2026 at 5:30 PM Fujii Masao <masao.fujii@gmail.com> wrote: > > 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. Thank you for reviving this patch. > 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'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. -- Bharath Rupireddy Amazon Web Services: https://aws.amazon.com
> On Mar 30, 2026, at 08:30, Fujii Masao <masao.fujii@gmail.com> wrote: > > On Thu, Mar 9, 2023 at 9:24 PM 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 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 theidea 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 extraburden. * 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(), somethinglike “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 otherwords, 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/
On Mon, Mar 30, 2026 at 7:58 AM 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 PM 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 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 byprocsignal_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 theidea 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 likeextra 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 benecessary. > > So instead of adding the comment everywhere, I would suggest adding one comment in procsignal_sigusr1_handler(), somethinglike “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 otherwords, 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
On Mon, Mar 30, 2026 at 1:21 PM Dilip Kumar <dilipbalaut@gmail.com> wrote: > > On Mon, Mar 30, 2026 at 7:58 AM 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 PM 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 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 byprocsignal_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 likethe idea of adding the comment "latch will be set by procsignal_sigusr1_handler" to every handler function. My reasonsare: > > > > * When a new handler is added, will it become the standard pattern to add the same comment everywhere? That seems likeextra 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 stillbe necessary. > > > > So instead of adding the comment everywhere, I would suggest adding one comment in procsignal_sigusr1_handler(), somethinglike “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 otherwords, 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. 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
Hi, On Wed, Apr 01, 2026 at 12:17:28PM +0900, Fujii Masao wrote: > On Mon, Mar 30, 2026 at 1:21 PM Dilip Kumar <dilipbalaut@gmail.com> wrote: > > 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 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. That sounds reasonable to me to proceed as v2 is doing. Regards, -- Bertrand Drouvot PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com
On Wed, Apr 1, 2026 at 12:44 PM Bertrand Drouvot <bertranddrouvot.pg@gmail.com> wrote: > > Hi, > > On Wed, Apr 01, 2026 at 12:17:28PM +0900, Fujii Masao wrote: > > On Mon, Mar 30, 2026 at 1:21 PM Dilip Kumar <dilipbalaut@gmail.com> wrote: > > > 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 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. > > That sounds reasonable to me to proceed as v2 is doing. Thanks! I've pushed the patch. Regards, -- Fujii Masao