public inbox for [email protected]help / color / mirror / Atom feed
Avoid multiple SetLatch() calls in procsignal_sigusr1_handler() 9+ messages / 6 participants [nested] [flat]
* Avoid multiple SetLatch() calls in procsignal_sigusr1_handler() @ 2023-02-28 15:30 Bharath Rupireddy <[email protected]> 0 siblings, 2 replies; 9+ messages in thread From: Bharath Rupireddy @ 2023-02-28 15:30 UTC (permalink / raw) To: PostgreSQL Hackers <[email protected]> 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 Attachments: [application/octet-stream] v1-0001-Avoid-multiple-SetLatch-calls-in-procsignal_sigus.patch (3.7K, 2-v1-0001-Avoid-multiple-SetLatch-calls-in-procsignal_sigus.patch) download | inline diff: From 128d45f53606aaeac17cdfd43df801ec6d7a3e50 Mon Sep 17 00:00:00 2001 From: Bharath Rupireddy <[email protected]> Date: Tue, 28 Feb 2023 12:11:39 +0000 Subject: [PATCH v1] Avoid multiple SetLatch() calls in procsignal_sigusr1_handler() --- src/backend/access/transam/parallel.c | 2 +- src/backend/commands/async.c | 4 +--- .../replication/logical/applyparallelworker.c | 2 +- src/backend/replication/walsender.c | 2 ++ src/backend/storage/ipc/sinval.c | 4 +--- src/backend/tcop/postgres.c | 12 ++++-------- 6 files changed, 10 insertions(+), 16 deletions(-) diff --git a/src/backend/access/transam/parallel.c b/src/backend/access/transam/parallel.c index b26f2a64fb..e1b48295ed 100644 --- a/src/backend/access/transam/parallel.c +++ b/src/backend/access/transam/parallel.c @@ -1015,7 +1015,7 @@ HandleParallelMessageInterrupt(void) { InterruptPending = true; ParallelMessagePending = true; - SetLatch(MyLatch); + /* latch will be set by procsignal_sigusr1_handler */ } /* diff --git a/src/backend/commands/async.c b/src/backend/commands/async.c index ef909cf4e0..ad533db655 100644 --- a/src/backend/commands/async.c +++ b/src/backend/commands/async.c @@ -1859,9 +1859,7 @@ HandleNotifyInterrupt(void) /* signal that work needs to be done */ notifyInterruptPending = true; - - /* make sure the event is processed in due course */ - SetLatch(MyLatch); + /* latch will be set by procsignal_sigusr1_handler */ } /* diff --git a/src/backend/replication/logical/applyparallelworker.c b/src/backend/replication/logical/applyparallelworker.c index 4518683779..6db40cc121 100644 --- a/src/backend/replication/logical/applyparallelworker.c +++ b/src/backend/replication/logical/applyparallelworker.c @@ -987,7 +987,7 @@ HandleParallelApplyMessageInterrupt(void) { InterruptPending = true; ParallelApplyMessagePending = true; - SetLatch(MyLatch); + /* latch will be set by procsignal_sigusr1_handler */ } /* diff --git a/src/backend/replication/walsender.c b/src/backend/replication/walsender.c index 75e8363e24..f063a712d4 100644 --- a/src/backend/replication/walsender.c +++ b/src/backend/replication/walsender.c @@ -3210,6 +3210,8 @@ HandleWalSndInitStopping(void) kill(MyProcPid, SIGTERM); else got_STOPPING = true; + + /* latch will be set by procsignal_sigusr1_handler */ } /* diff --git a/src/backend/storage/ipc/sinval.c b/src/backend/storage/ipc/sinval.c index b405f08313..bd6adcef53 100644 --- a/src/backend/storage/ipc/sinval.c +++ b/src/backend/storage/ipc/sinval.c @@ -161,9 +161,7 @@ HandleCatchupInterrupt(void) */ catchupInterruptPending = true; - - /* make sure the event is processed in due course */ - SetLatch(MyLatch); + /* latch will be set by procsignal_sigusr1_handler */ } /* diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c index cab709b07b..2a3477dc80 100644 --- a/src/backend/tcop/postgres.c +++ b/src/backend/tcop/postgres.c @@ -3007,8 +3007,6 @@ FloatExceptionHandler(SIGNAL_ARGS) void RecoveryConflictInterrupt(ProcSignalReason reason) { - int save_errno = errno; - /* * Don't joggle the elbow of proc_exit */ @@ -3123,13 +3121,11 @@ RecoveryConflictInterrupt(ProcSignalReason reason) } /* - * Set the process latch. This function essentially emulates signal - * handlers like die() and StatementCancelHandler() and it seems prudent - * to behave similarly as they do. + * Latch will be set by procsignal_sigusr1_handler. This function + * essentially emulates signal handlers like die() and + * StatementCancelHandler() and it seems prudent to behave similarly as + * they do. */ - SetLatch(MyLatch); - - errno = save_errno; } /* -- 2.34.1 ^ permalink raw reply [nested|flat] 9+ messages in thread
* Re: Avoid multiple SetLatch() calls in procsignal_sigusr1_handler() @ 2023-03-01 09:44 Kuntal Ghosh <[email protected]> parent: Bharath Rupireddy <[email protected]> 1 sibling, 0 replies; 9+ messages in thread From: Kuntal Ghosh @ 2023-03-01 09:44 UTC (permalink / raw) To: Bharath Rupireddy <[email protected]>; +Cc: PostgreSQL Hackers <[email protected]> On Tue, Feb 28, 2023 at 9:01 PM Bharath Rupireddy <[email protected]> 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 ^ permalink raw reply [nested|flat] 9+ messages in thread
* Re: Avoid multiple SetLatch() calls in procsignal_sigusr1_handler() @ 2023-03-09 12:24 Drouvot, Bertrand <[email protected]> parent: Bharath Rupireddy <[email protected]> 1 sibling, 1 reply; 9+ messages in thread From: Drouvot, Bertrand @ 2023-03-09 12:24 UTC (permalink / raw) To: Bharath Rupireddy <[email protected]>; PostgreSQL Hackers <[email protected]> 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 ^ permalink raw reply [nested|flat] 9+ messages in thread
* Re: Avoid multiple SetLatch() calls in procsignal_sigusr1_handler() @ 2026-03-30 00:30 Fujii Masao <[email protected]> parent: Drouvot, Bertrand <[email protected]> 0 siblings, 2 replies; 9+ messages in thread From: Fujii Masao @ 2026-03-30 00:30 UTC (permalink / raw) To: Drouvot, Bertrand <[email protected]>; +Cc: Bharath Rupireddy <[email protected]>; PostgreSQL Hackers <[email protected]> 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(). 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 Attachments: [application/octet-stream] v2-0001-Remove-redundant-SetLatch-calls-in-interrupt-hand.patch (3.5K, 2-v2-0001-Remove-redundant-SetLatch-calls-in-interrupt-hand.patch) download | inline diff: From 21d8e36f51f1c95596845979c44740d39df74eee Mon Sep 17 00:00:00 2001 From: Fujii Masao <[email protected]> Date: Mon, 30 Mar 2026 08:48:05 +0900 Subject: [PATCH v2] Remove redundant SetLatch() calls in interrupt handling functions Interrupt handling functions (e.g., HandleCatchupInterrupt(), HandleParallelApplyMessageInterrupt()) are called only by procsignal_sigusr1_handler(), which already calls SetLatch() for the current process at the end of its processing. Therefore, these interrupt handling functions do not need to call SetLatch() themselves. However, previously, some of these functions redundantly called SetLatch(). This commit removes those unnecessary calls. While duplicate SetLatch() calls are redundant, they are harmless, so this change is not backpatched. Author: Bharath Rupireddy <[email protected]> Reviewed-by: Bertrand Drouvot <[email protected]> Reviewed-by: Fujii Masao <[email protected]> Discussion: https://postgr.es/m/CALj2ACWd5apddj6Cd885WwJ6LquYu_G81C4GoR4xSoDV1x-FEA@mail.gmail.com --- src/backend/access/transam/parallel.c | 2 +- src/backend/commands/async.c | 3 +-- src/backend/replication/logical/applyparallelworker.c | 2 +- src/backend/replication/walsender.c | 2 ++ src/backend/storage/ipc/sinval.c | 3 +-- 5 files changed, 6 insertions(+), 6 deletions(-) diff --git a/src/backend/access/transam/parallel.c b/src/backend/access/transam/parallel.c index ab1dfb30e73..89e9d224eec 100644 --- a/src/backend/access/transam/parallel.c +++ b/src/backend/access/transam/parallel.c @@ -1047,7 +1047,7 @@ HandleParallelMessageInterrupt(void) { InterruptPending = true; ParallelMessagePending = true; - SetLatch(MyLatch); + /* latch will be set by procsignal_sigusr1_handler */ } /* diff --git a/src/backend/commands/async.c b/src/backend/commands/async.c index 5c9a56c3d40..e91a62ff42a 100644 --- a/src/backend/commands/async.c +++ b/src/backend/commands/async.c @@ -2560,8 +2560,7 @@ HandleNotifyInterrupt(void) /* signal that work needs to be done */ notifyInterruptPending = true; - /* make sure the event is processed in due course */ - SetLatch(MyLatch); + /* latch will be set by procsignal_sigusr1_handler */ } /* diff --git a/src/backend/replication/logical/applyparallelworker.c b/src/backend/replication/logical/applyparallelworker.c index d78693ffa8e..798e8d85b3e 100644 --- a/src/backend/replication/logical/applyparallelworker.c +++ b/src/backend/replication/logical/applyparallelworker.c @@ -1000,7 +1000,7 @@ HandleParallelApplyMessageInterrupt(void) { InterruptPending = true; ParallelApplyMessagePending = true; - SetLatch(MyLatch); + /* latch will be set by procsignal_sigusr1_handler */ } /* diff --git a/src/backend/replication/walsender.c b/src/backend/replication/walsender.c index 66507e9c2dd..2bb3f34dc6d 100644 --- a/src/backend/replication/walsender.c +++ b/src/backend/replication/walsender.c @@ -3730,6 +3730,8 @@ HandleWalSndInitStopping(void) kill(MyProcPid, SIGTERM); else got_STOPPING = true; + + /* latch will be set by procsignal_sigusr1_handler */ } /* diff --git a/src/backend/storage/ipc/sinval.c b/src/backend/storage/ipc/sinval.c index 5559f7c1cfa..1540c7e0696 100644 --- a/src/backend/storage/ipc/sinval.c +++ b/src/backend/storage/ipc/sinval.c @@ -160,8 +160,7 @@ HandleCatchupInterrupt(void) catchupInterruptPending = true; - /* make sure the event is processed in due course */ - SetLatch(MyLatch); + /* latch will be set by procsignal_sigusr1_handler */ } /* -- 2.51.2 ^ permalink raw reply [nested|flat] 9+ messages in thread
* Re: Avoid multiple SetLatch() calls in procsignal_sigusr1_handler() @ 2026-03-30 01:30 Bharath Rupireddy <[email protected]> parent: Fujii Masao <[email protected]> 1 sibling, 0 replies; 9+ messages in thread From: Bharath Rupireddy @ 2026-03-30 01:30 UTC (permalink / raw) To: Fujii Masao <[email protected]>; +Cc: Drouvot, Bertrand <[email protected]>; PostgreSQL Hackers <[email protected]> Hi, On Sun, Mar 29, 2026 at 5:30 PM Fujii Masao <[email protected]> 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 ^ permalink raw reply [nested|flat] 9+ messages in thread
* Re: Avoid multiple SetLatch() calls in procsignal_sigusr1_handler() @ 2026-03-30 04:20 Dilip Kumar <[email protected]> parent: Fujii Masao <[email protected]> 1 sibling, 1 reply; 9+ messages in thread From: Dilip Kumar @ 2026-03-30 04:20 UTC (permalink / raw) To: Chao Li <[email protected]>; +Cc: Fujii Masao <[email protected]>; Drouvot, Bertrand <[email protected]>; Bharath Rupireddy <[email protected]>; PostgreSQL Hackers <[email protected]> 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 ^ permalink raw reply [nested|flat] 9+ messages in thread
* Re: Avoid multiple SetLatch() calls in procsignal_sigusr1_handler() @ 2026-04-01 03:17 Fujii Masao <[email protected]> parent: Dilip Kumar <[email protected]> 0 siblings, 1 reply; 9+ messages in thread From: Fujii Masao @ 2026-04-01 03:17 UTC (permalink / raw) To: Dilip Kumar <[email protected]>; +Cc: Chao Li <[email protected]>; Drouvot, Bertrand <[email protected]>; Bharath Rupireddy <[email protected]>; PostgreSQL Hackers <[email protected]> On Mon, Mar 30, 2026 at 1:21 PM Dilip Kumar <[email protected]> wrote: > > 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. 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 ^ permalink raw reply [nested|flat] 9+ messages in thread
* Re: Avoid multiple SetLatch() calls in procsignal_sigusr1_handler() @ 2026-04-01 03:44 Bertrand Drouvot <[email protected]> parent: Fujii Masao <[email protected]> 0 siblings, 1 reply; 9+ messages in thread From: Bertrand Drouvot @ 2026-04-01 03:44 UTC (permalink / raw) To: Fujii Masao <[email protected]>; +Cc: Dilip Kumar <[email protected]>; Chao Li <[email protected]>; Bharath Rupireddy <[email protected]>; PostgreSQL Hackers <[email protected]> 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 <[email protected]> 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 ^ permalink raw reply [nested|flat] 9+ messages in thread
* Re: Avoid multiple SetLatch() calls in procsignal_sigusr1_handler() @ 2026-04-02 14:57 Fujii Masao <[email protected]> parent: Bertrand Drouvot <[email protected]> 0 siblings, 0 replies; 9+ messages in thread From: Fujii Masao @ 2026-04-02 14:57 UTC (permalink / raw) To: Bertrand Drouvot <[email protected]>; +Cc: Dilip Kumar <[email protected]>; Chao Li <[email protected]>; Bharath Rupireddy <[email protected]>; PostgreSQL Hackers <[email protected]> On Wed, Apr 1, 2026 at 12:44 PM Bertrand Drouvot <[email protected]> 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 <[email protected]> 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 ^ permalink raw reply [nested|flat] 9+ messages in thread
end of thread, other threads:[~2026-04-02 14:57 UTC | newest] Thread overview: 9+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2023-02-28 15:30 Avoid multiple SetLatch() calls in procsignal_sigusr1_handler() Bharath Rupireddy <[email protected]> 2023-03-01 09:44 ` Kuntal Ghosh <[email protected]> 2023-03-09 12:24 ` Drouvot, Bertrand <[email protected]> 2026-03-30 00:30 ` Fujii Masao <[email protected]> 2026-03-30 01:30 ` Bharath Rupireddy <[email protected]> 2026-03-30 04:20 ` Dilip Kumar <[email protected]> 2026-04-01 03:17 ` Fujii Masao <[email protected]> 2026-04-01 03:44 ` Bertrand Drouvot <[email protected]> 2026-04-02 14:57 ` Fujii Masao <[email protected]>
This inbox is served by agora; see mirroring instructions for how to clone and mirror all data and code used for this inbox