Re: Add errdetail() with PID and UID about source of termination signal

From: Chao Li <li(dot)evan(dot)chao(at)gmail(dot)com>
To: Andrew Dunstan <andrew(at)dunslane(dot)net>
Cc: Andres Freund <andres(at)anarazel(dot)de>, Jim Jones <jim(dot)jones(at)uni-muenster(dot)de>, Jakub Wartak <jakub(dot)wartak(at)enterprisedb(dot)com>, PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org>
Subject: Re: Add errdetail() with PID and UID about source of termination signal
Date: 2026-04-10 07:40:31
Message-ID: 8B3A69A4-8412-472D-BB39-47617E430420@gmail.com
Views: Whole Thread | Raw Message | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

> On Apr 9, 2026, at 18:59, Andrew Dunstan <andrew(at)dunslane(dot)net> wrote:
>
>
> On 2026-04-08 We 1:01 PM, Andres Freund wrote:
>> Hi,
>>
>> Attached is a very rough first draft for how I think this needs to look like.
>>
>> Basically, SIGNAL_INFO always will pass both the signal number and extended
>> information along to the signal handler. The extended information is a
>> postgres specific struct. If the platform can't provide the extended
>> information, the values are instead set to some default value indicating that
>> the information is not known.
>>
>> With that die() (and also StatementCancelHandler, ...) can just set whatever
>> globals it wants, without pqsignal.c needing to know about it.
>>
>> It also allows us to extend the amount of information in the future. E.g. I'd
>> like to log the reason for a segfault (could e.g. be an OOM kill or an umapped
>> region) to stderr.
>>
>> The annoying thing about it is needing to change nearly all the existing
>> references to SIG_IGN/SIG_DFL, to avoid warnings due to mismatched types.
>
> I agree that's annoying. The only way around it I found was via some casting to/from void* that I suspect you would find a cure worse than the disease.
> I reworked your patch slightly. This version fixes the translatability issue you raised earlier, makes the TAP test from the original commit more robust, and tries to resolve your XXX issue by moving the assignment of ProcDieSenderPid/Uid inside the "if (!proc_exit_inprogress)" block.
>
> cheers
>
> andrew
>
>
>
> --
> Andrew Dunstan
> EDB: https://www.enterprisedb.com
>
> <0001-Rework-signal-sender-errdetail-to-pass-info-via-hand.patch>

I reviewed this version. Besides the compile warning and uid 0 issues, I got a few more comments, so I try to put them all together as below.

1 - compile warnings
As talked in an earlier email, this eliminates the compile warnings for me:
```
chaol(at)ChaodeMacBook-Air postgresql % git diff
diff --git a/src/include/port.h b/src/include/port.h
index 7db476d7b01..c029878c6be 100644
--- a/src/include/port.h
+++ b/src/include/port.h
@@ -547,8 +547,8 @@ extern int pg_mkdir_p(char *path, int omode);
#define pqsignal pqsignal_be
#endif

-#define PG_SIG_DFL (pqsigfunc) SIG_DFL
-#define PG_SIG_IGN (pqsigfunc) SIG_IGN
+#define PG_SIG_DFL (pqsigfunc) (pg_funcptr_t) SIG_DFL
+#define PG_SIG_IGN (pqsigfunc) (pg_funcptr_t) SIG_IGN
typedef void (*pqsigfunc) (SIGNAL_ARGS);
extern void pqsignal(int signo, pqsigfunc func);
```

2 - uid 0 problem
```
+typedef struct pg_signal_info
+{
+ pid_t pid; /* pid of sending process or 0 if unknown */
+ uid_t uid; /* uid of sending process or 0 if unknown */
+} pg_signal_info;
```

I think we can mention that “uid” is only meaningful when pid is set.

3 Also for the struct pg_signal_info. As the struct name is generic in order to hold some more fields in the future, does it make sense to rename “pid” to “sender_pid” and “uid” to “sender_pid” to reflect their actual meanings? I am thinking that, some other pid/uid might be added to this struct in the future.

4
```
-#ifndef SIGNAL_ARGS
-#define SIGNAL_ARGS int postgres_signal_arg
-#endif
+/*
+ * The following is used as the arg list for signal handlers. These days we
+ * use the same argument to all signal handlers and hide the difference
+ * between platforms in wrapper functions.
+ *
+ * SIGNAL_ARGS just exists separately from the pqsignal() definition for
+ * historical reasons.
+ */
+#define SIGNAL_ARGS int postgres_signal_arg, pg_signal_info *pg_siginfo
```

Given we now define new PG_SIG_IGN and PG_SIG_DFL, does it make sense to rename SIGNAL_ARGS to PG_SIGNAL_ARGS?

5
```
+ pg_info.uid = 0;
```

If you take comment 2, then when unavailable, we can just don’t assign anything to pg_info.uid. Or "(uid_t)-1", maybe.

6
```
+#define SIGNAL_ARGS int postgres_signal_arg, pg_signal_info *pg_siginfo
```

Maybe pg_siginfo can be const.

7
```
+ errdetail("The transaction has already committed locally, but might not have been replicated to the standby. Signal sent by PID %d, UID %d.”,
```

Per https://www.postgresql.org/docs/current/error-style-guide.html, for hint and detail messages, “Put two spaces after the period if another sentence follows”.

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/

In response to

Browse pgsql-hackers by date

  From Date Subject
Next Message Rafia Sabih 2026-04-10 07:41:48 Re: Bypassing cursors in postgres_fdw to enable parallel plans
Previous Message Amit Langote 2026-04-10 07:39:16 Re: Eliminating SPI / SQL from some RI triggers - take 3