On 2026-04-07 Tu 2:19 PM, Andres Freund wrote:
Hi,
On 2026-04-07 12:49:19 -0400, Andrew Dunstan wrote:
On 2026-04-07 Tu 10:55 AM, Andres Freund wrote:
This seems completely wrong from a layering POV. The wrapper has no business
whatsoever to know that how SIGTERM is interpreted and thus no business
setting variables like ProcDieSenderPid.
Pretty sure have some sigterm handlers that shouldn't set ProcDieSenderPid.
A more correct answer here would be to forward information about the sender of
a signal to the signal handlers and let them interpret the information if
available.
OK, fair points. Does the attached meet your concerns?
I think the extra data should be forwarded as arguments to the "real" (not
wrapper) handler, not as globals. You can have signal handlers interrupt each
others on some platforms, which means that if you're not careful, you could
end up reading the values from the wrong signal.
OK, maybe this, then? It saves the siginfo before calling the handler,
and restores it after the call, so you should always be looking at the
right one.
cheers
andrew
--
Andrew Dunstan
EDB: https://www.enterprisedb.com
diff --git a/src/backend/replication/syncrep.c b/src/backend/replication/syncrep.c
index 46a778f0917..896ba45412d 100644
--- a/src/backend/replication/syncrep.c
+++ b/src/backend/replication/syncrep.c
@@ -300,22 +300,18 @@ SyncRepWaitForLSN(XLogRecPtr lsn, bool commit)
*/
if (ProcDiePending)
{
- /*
- * ProcDieSenderPid/Uid are read directly from the globals here
- * rather than copied to locals first; a second SIGTERM could
- * change them between reads, but that is harmless because the
- * process is about to die anyway. The signal sender detail is
- * inlined rather than using a separate errdetail() call because
- * it must be appended to the existing detail message.
- */
- ereport(WARNING,
- (errcode(ERRCODE_ADMIN_SHUTDOWN),
- errmsg("canceling the wait for synchronous replication and terminating connection due to administrator command"),
- errdetail("The transaction has already committed locally, but might not have been replicated to the standby.%s",
- ProcDieSenderPid == 0 ? "" :
- psprintf("\nSignal sent by PID %d, UID %d.",
- (int) ProcDieSenderPid,
- (int) ProcDieSenderUid))));
+ if (ProcDieSenderPid != 0)
+ ereport(WARNING,
+ (errcode(ERRCODE_ADMIN_SHUTDOWN),
+ errmsg("canceling the wait for synchronous replication and terminating connection due to administrator command"),
+ errdetail("The transaction has already committed locally, but might not have been replicated to the standby. Signal sent by PID %d, UID %d.",
+ (int) ProcDieSenderPid,
+ (int) ProcDieSenderUid)));
+ else
+ ereport(WARNING,
+ (errcode(ERRCODE_ADMIN_SHUTDOWN),
+ errmsg("canceling the wait for synchronous replication and terminating connection due to administrator command"),
+ errdetail("The transaction has already committed locally, but might not have been replicated to the standby.")));
whereToSendOutput = DestNone;
SyncRepCancelWait();
break;
diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c
index 4fb18741dc5..df336e3f194 100644
--- a/src/backend/tcop/postgres.c
+++ b/src/backend/tcop/postgres.c
@@ -3023,8 +3023,16 @@ die(SIGNAL_ARGS)
/* Don't joggle the elbow of proc_exit */
if (!proc_exit_inprogress)
{
+ int sender_pid = 0;
+ int sender_uid = 0;
+
InterruptPending = true;
ProcDiePending = true;
+#ifdef HAVE_SA_SIGINFO
+ pqsignal_get_sender(&sender_pid, &sender_uid);
+#endif
+ ProcDieSenderPid = sender_pid;
+ ProcDieSenderUid = sender_uid;
}
/* for the cumulative stats system */
diff --git a/src/bin/psql/t/001_basic.pl b/src/bin/psql/t/001_basic.pl
index 7c21204c1f2..9d966c7bece 100644
--- a/src/bin/psql/t/001_basic.pl
+++ b/src/bin/psql/t/001_basic.pl
@@ -142,8 +142,11 @@ my ($ret, $out, $err) = $node->psql('postgres',
is($ret, 2, 'server crash: psql exit code');
like($out, qr/before/, 'server crash: output before crash');
unlike($out, qr/AFTER/, 'server crash: no output after crash');
+my $detail_re = check_pg_config("#define HAVE_SA_SIGINFO 1")
+ ? qr/DETAIL: Signal sent by PID \d+, UID \d+\.\n/
+ : qr//;
like( $err, qr/psql:<stdin>:2: FATAL: terminating connection due to administrator command
-(?:DETAIL: Signal sent by PID \d+, UID \d+\.\n)?psql:<stdin>:2: server closed the connection unexpectedly
+${detail_re}psql:<stdin>:2: server closed the connection unexpectedly
This probably means the server terminated abnormally
before or while processing the request.
psql:<stdin>:2: error: connection to server was lost/,
diff --git a/src/include/libpq/pqsignal.h b/src/include/libpq/pqsignal.h
index 9f494a1fdf9..40ed553cdf6 100644
--- a/src/include/libpq/pqsignal.h
+++ b/src/include/libpq/pqsignal.h
@@ -51,4 +51,8 @@ extern PGDLLIMPORT sigset_t StartupBlockSig;
extern void pqinitmask(void);
+#ifdef HAVE_SA_SIGINFO
+extern void pqsignal_get_sender(int *sender_pid, int *sender_uid);
+#endif
+
#endif /* PQSIGNAL_H */
diff --git a/src/port/pqsignal.c b/src/port/pqsignal.c
index 8841464b5cb..7ee101517aa 100644
--- a/src/port/pqsignal.c
+++ b/src/port/pqsignal.c
@@ -71,6 +71,16 @@ StaticAssertDecl(SIGALRM < PG_NSIG, "SIGALRM >= PG_NSIG");
static volatile pqsigfunc pqsignal_handlers[PG_NSIG];
+#if !defined(FRONTEND) && defined(HAVE_SA_SIGINFO)
+/*
+ * Pointer to the siginfo_t for the signal currently being handled. This
+ * lives on wrapper_handler's stack frame and is saved/restored around the
+ * call to the real handler, so nested signals each see their own siginfo.
+ * Signal handlers can call pqsignal_get_sender() to retrieve it.
+ */
+static volatile siginfo_t *current_siginfo;
+#endif
+
/*
* Except when called with SIG_IGN or SIG_DFL, pqsignal() sets up this function
* as the handler for all signals. This wrapper handler function checks that
@@ -84,7 +94,7 @@ static volatile pqsigfunc pqsignal_handlers[PG_NSIG];
*/
#if !defined(FRONTEND) && defined(HAVE_SA_SIGINFO)
static void
-wrapper_handler(int signo, siginfo_t * info, void *context)
+wrapper_handler(int postgres_signal_arg, siginfo_t *info, void *context)
#else
static void
wrapper_handler(SIGNAL_ARGS)
@@ -92,8 +102,7 @@ wrapper_handler(SIGNAL_ARGS)
{
int save_errno = errno;
#if !defined(FRONTEND) && defined(HAVE_SA_SIGINFO)
- /* SA_SIGINFO signature uses signo, not SIGNAL_ARGS macro */
- int postgres_signal_arg = signo;
+ volatile siginfo_t *saved_siginfo = current_siginfo;
#endif
Assert(postgres_signal_arg > 0);
@@ -116,19 +125,50 @@ wrapper_handler(SIGNAL_ARGS)
}
#ifdef HAVE_SA_SIGINFO
- if (signo == SIGTERM && info)
- {
- ProcDieSenderPid = info->si_pid;
- ProcDieSenderUid = info->si_uid;
- }
-#endif
+ current_siginfo = info;
+ (*pqsignal_handlers[postgres_signal_arg]) (postgres_signal_arg);
+ current_siginfo = saved_siginfo;
+#else
+ (*pqsignal_handlers[postgres_signal_arg]) (postgres_signal_arg);
#endif
+#else /* FRONTEND */
(*pqsignal_handlers[postgres_signal_arg]) (postgres_signal_arg);
+#endif
errno = save_errno;
}
+#if !defined(FRONTEND) && defined(HAVE_SA_SIGINFO)
+/*
+ * pqsignal_get_sender - retrieve the PID and UID of the current signal sender
+ *
+ * This may only be called from within a signal handler. The values come from
+ * the siginfo_t passed by the kernel to wrapper_handler, which is on the
+ * stack and thus safe against nesting: if a different signal interrupts, its
+ * wrapper_handler invocation saves and restores the pointer, so the outer
+ * handler still sees the correct siginfo after the nested handler returns.
+ *
+ * If siginfo is not available (e.g. info was NULL), returns zeros.
+ */
+void
+pqsignal_get_sender(int *sender_pid, int *sender_uid)
+{
+ volatile siginfo_t *info = current_siginfo;
+
+ if (info)
+ {
+ *sender_pid = info->si_pid;
+ *sender_uid = info->si_uid;
+ }
+ else
+ {
+ *sender_pid = 0;
+ *sender_uid = 0;
+ }
+}
+#endif
+
/*
* Set up a signal handler, with SA_RESTART, for signal "signo"
*
