public inbox for [email protected]
help / color / mirror / Atom feedFrom: Fujii Masao <[email protected]>
To: Drouvot, Bertrand <[email protected]>
Cc: Bharath Rupireddy <[email protected]>
Cc: PostgreSQL Hackers <[email protected]>
Subject: Re: Avoid multiple SetLatch() calls in procsignal_sigusr1_handler()
Date: Mon, 30 Mar 2026 09:30:08 +0900
Message-ID: <CAHGQGwFBnBQZbt4e9qjZoiM0GW2eXCvS=Ny5wt+s1-+iJpK15A@mail.gmail.com> (raw)
In-Reply-To: <[email protected]>
References: <CALj2ACWd5apddj6Cd885WwJ6LquYu_G81C4GoR4xSoDV1x-FEA@mail.gmail.com>
<[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
view thread (9+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected]
Subject: Re: Avoid multiple SetLatch() calls in procsignal_sigusr1_handler()
In-Reply-To: <CAHGQGwFBnBQZbt4e9qjZoiM0GW2eXCvS=Ny5wt+s1-+iJpK15A@mail.gmail.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox