Thread

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

Only show signal-sender PID/UID detail in server log
- b772f3fcad18 19 (unreleased) landed
Make psql DETAIL line test unconditionally optional.
- 446c400fd89b 19 (unreleased) landed
Rework signal handler infrastructure to pass sender info as argument.
- 3e2a1496bae6 19 (unreleased) landed
Add errdetail() with PID and UID about source of termination signal.
- 55890a919454 19 (unreleased) landed

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

Jakub Wartak <jakub.wartak@enterprisedb.com> — 2026-02-18T07:32:25Z

Hi all,

From time to time we have scenario where somebody has some backend killed by
some_script_somewhere(TM) in a multi-department company scenario and
to me it was
always unnecessary time intensive to identify the origin of the signal.

eBPF/bpftrace can be used to find the origin, but I think that PG could
simply log which PID/UID (e.g. root or pg-user) raised the signal. So Linux
has SA_SIGINFO, we could use that to provide mentioned things. As expected,
search of course returned that it was discussed earlier here [1] 11 years ago,
but there was no patch back then, so attached is an attempt to do just that.

No GUC, and yes it only displays it on Linux. I think FreeBSD also has
this, but I
haven't tried it there (or I haven't tried other OSes without it -
proper autoconf/meson
sa_sigaction SA_SIGINFO detection  is probably missing with proper
#ifdefs ), but I
would first like to learn if that would be a welcomed feature or not.

-J.

[1] - https://hackorum.dev/topics/32019

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

Jim Jones <jim.jones@uni-muenster.de> — 2026-02-18T16:08:25Z

Hi Jakub

On 18/02/2026 08:32, Jakub Wartak wrote:
> I would first like to learn if that would be a welcomed feature or not.

+1

I think it's a very useful feature (only tested on Linux)

FATAL:  terminating connection due to administrator command
DETAIL:  signal sent by PID 1592705, UID 1000.

I'm wondering if there is a standard style for displaying such values in
DETAIL. For instance, the checkpoint LOG is formatted like this:

LOG:  checkpoint complete: ... write=0.044 s, sync=0.071 s, ...

I'm not sure if it applies for DETAIL, but at least it's what the
example at the error style guide[1] suggests:

Detail:     Failed syscall was shmget(key=%d, size=%u, 0%o).

Thanks!

Best, Jim


1 - https://www.postgresql.org/docs/current/error-style-guide.html

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

Jakub Wartak <jakub.wartak@enterprisedb.com> — 2026-02-23T13:28:40Z

On Wed, Feb 18, 2026 at 5:08 PM Jim Jones <jim.jones@uni-muenster.de> wrote:
>
> Hi Jakub
>
> On 18/02/2026 08:32, Jakub Wartak wrote:
> > I would first like to learn if that would be a welcomed feature or not.
>
> +1
>
> I think it's a very useful feature (only tested on Linux)
>
> FATAL:  terminating connection due to administrator command
> DETAIL:  signal sent by PID 1592705, UID 1000.

Hi Jim, thanks for feedback :)

> I'm wondering if there is a standard style for displaying such values in
> DETAIL. For instance, the checkpoint LOG is formatted like this:
>
> LOG:  checkpoint complete: ... write=0.044 s, sync=0.071 s, ...
>
> I'm not sure if it applies for DETAIL, but at least it's what the
> example at the error style guide[1] suggests:
>
> Detail:     Failed syscall was shmget(key=%d, size=%u, 0%o).

After using `grep -hr errdetail src/ | sed -E 's/^\s+//g' | sort |
uniq` I doubt there is any
real standard, but one can find there:

errdetail("The server process with PID %d is among those with the
   oldest transactions.", minPid)
errdetail("The source process with PID %d is not running anymore.",

One could say that all those DETAIL log messages should start with an
 uppercase letter, yet it didn't look good to me when above
   "terminating connection ..."
started itself with a lowercase letter "t", and then next-line DETAIL
we would start
with an uppercase
  "Signal..".

but, I'm open to any better proposal...

-J.

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

Chao Li <li.evan.chao@gmail.com> — 2026-02-24T08:39:27Z


> On Feb 23, 2026, at 21:28, Jakub Wartak <jakub.wartak@enterprisedb.com> wrote:
> 
> On Wed, Feb 18, 2026 at 5:08 PM Jim Jones <jim.jones@uni-muenster.de> wrote:
>> 
>> Hi Jakub
>> 
>> On 18/02/2026 08:32, Jakub Wartak wrote:
>>> I would first like to learn if that would be a welcomed feature or not.
>> 
>> +1
>> 
>> I think it's a very useful feature (only tested on Linux)
>> 
>> FATAL:  terminating connection due to administrator command
>> DETAIL:  signal sent by PID 1592705, UID 1000.
> 
> Hi Jim, thanks for feedback :)
> 
>> I'm wondering if there is a standard style for displaying such values in
>> DETAIL. For instance, the checkpoint LOG is formatted like this:
>> 
>> LOG:  checkpoint complete: ... write=0.044 s, sync=0.071 s, ...
>> 
>> I'm not sure if it applies for DETAIL, but at least it's what the
>> example at the error style guide[1] suggests:
>> 
>> Detail:     Failed syscall was shmget(key=%d, size=%u, 0%o).
> 
> After using `grep -hr errdetail src/ | sed -E 's/^\s+//g' | sort |
> uniq` I doubt there is any
> real standard, but one can find there:
> 
> errdetail("The server process with PID %d is among those with the
>   oldest transactions.", minPid)
> errdetail("The source process with PID %d is not running anymore.",
> 
> One could say that all those DETAIL log messages should start with an
> uppercase letter, yet it didn't look good to me when above
>   "terminating connection ..."
> started itself with a lowercase letter "t", and then next-line DETAIL
> we would start
> with an uppercase
>  "Signal..".
> 
> but, I'm open to any better proposal...
> 
> -J.

There is guidance in the documentation regarding error message style: https://www.postgresql.org/docs/current/error-style-guide.html
```
Detail and hint messages: Use complete sentences, and end each with a period. Capitalize the first word of sentences. Put two spaces after the period if another sentence follows (for English text; might be inappropriate in other languages).
```

I also noticed that some existing DETAIL and HINT messages do not fully follow this guideline. But I believe new code should adhere to the documented style as much as possible. In particular, DETAIL and HINT messages should begin with a capital letter and follow the complete-sentence convention.

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

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

Jakub Wartak <jakub.wartak@enterprisedb.com> — 2026-02-24T10:05:29Z

On Tue, Feb 24, 2026 at 9:40 AM Chao Li <li.evan.chao@gmail.com> wrote:
[..]

> There is guidance in the documentation regarding error message style: https://www.postgresql.org/docs/current/error-style-guide.html
> ```
> Detail and hint messages: Use complete sentences, and end each with a period. Capitalize the first word of sentences. Put two spaces after the period if another sentence follows (for English text; might be inappropriate in other languages).
> ```
>
> I also noticed that some existing DETAIL and HINT messages do not fully follow this guideline. But I believe new code should adhere to the documented style as much as possible. In particular, DETAIL and HINT messages should begin with a capital letter and follow the complete-sentence convention.

Hi, v2 attached, WIP, the only known remaining issue to me is that
windows might fail to compile as it probably doesn't have concept of
uid_t... I'm wondering what to do there.

1. Rebased due to recent change to remove bgwriter_die()
2. Added auto-detection of SA_SIGINFO to meson and autoconf
3. Changed "Signal" to be in upper case now (I don't like it, but it
follows guideline now)
4. Tested on FreeBSD 14.3 - it works there too now..
5. ...and fixed some clang warnings by changing %d -> %lld in
errdetail due to apparent pid_t differences on platforms.

-J.

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

Andrew Dunstan <andrew@dunslane.net> — 2026-02-24T16:15:01Z

On 2026-02-24 Tu 5:05 AM, Jakub Wartak wrote:
> On Tue, Feb 24, 2026 at 9:40 AM Chao Li <li.evan.chao@gmail.com> wrote:
> [..]
>
>> There is guidance in the documentation regarding error message style: https://www.postgresql.org/docs/current/error-style-guide.html
>> ```
>> Detail and hint messages: Use complete sentences, and end each with a period. Capitalize the first word of sentences. Put two spaces after the period if another sentence follows (for English text; might be inappropriate in other languages).
>> ```
>>
>> I also noticed that some existing DETAIL and HINT messages do not fully follow this guideline. But I believe new code should adhere to the documented style as much as possible. In particular, DETAIL and HINT messages should begin with a capital letter and follow the complete-sentence convention.
> Hi, v2 attached, WIP, the only known remaining issue to me is that
> windows might fail to compile as it probably doesn't have concept of
> uid_t... I'm wondering what to do there.


I don't think it will arise, as Windows doesn't have the siginfo stuff, 
AFAIK.


cheers


andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

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

Jakub Wartak <jakub.wartak@enterprisedb.com> — 2026-02-25T08:26:28Z

On Tue, Feb 24, 2026 at 5:15 PM Andrew Dunstan <andrew@dunslane.net> wrote:
>
>
> On 2026-02-24 Tu 5:05 AM, Jakub Wartak wrote:
> > On Tue, Feb 24, 2026 at 9:40 AM Chao Li <li.evan.chao@gmail.com> wrote:
> > [..]
> >
> >> There is guidance in the documentation regarding error message style: https://www.postgresql.org/docs/current/error-style-guide.html
> >> ```
> >> Detail and hint messages: Use complete sentences, and end each with a period. Capitalize the first word of sentences. Put two spaces after the period if another sentence follows (for English text; might be inappropriate in other languages).
> >> ```
> >>
> >> I also noticed that some existing DETAIL and HINT messages do not fully follow this guideline. But I believe new code should adhere to the documented style as much as possible. In particular, DETAIL and HINT messages should begin with a capital letter and follow the complete-sentence convention.
> > Hi, v2 attached, WIP, the only known remaining issue to me is that
> > windows might fail to compile as it probably doesn't have concept of
> > uid_t... I'm wondering what to do there.
>
>
> I don't think it will arise, as Windows doesn't have the siginfo stuff,
> AFAIK.
>

Hi Andrew. Ok, so V3 is attached with just one change: uid_t/pid_t
changed to uint32 to make win32 happy.

Perhaps one question is whether this should be toggleable with GUC or not.

-J.

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

Chao Li <li.evan.chao@gmail.com> — 2026-02-25T09:15:16Z


> On Feb 25, 2026, at 16:26, Jakub Wartak <jakub.wartak@enterprisedb.com> wrote:
> 
> On Tue, Feb 24, 2026 at 5:15 PM Andrew Dunstan <andrew@dunslane.net> wrote:
>> 
>> 
>> On 2026-02-24 Tu 5:05 AM, Jakub Wartak wrote:
>>> On Tue, Feb 24, 2026 at 9:40 AM Chao Li <li.evan.chao@gmail.com> wrote:
>>> [..]
>>> 
>>>> There is guidance in the documentation regarding error message style: https://www.postgresql.org/docs/current/error-style-guide.html
>>>> ```
>>>> Detail and hint messages: Use complete sentences, and end each with a period. Capitalize the first word of sentences. Put two spaces after the period if another sentence follows (for English text; might be inappropriate in other languages).
>>>> ```
>>>> 
>>>> I also noticed that some existing DETAIL and HINT messages do not fully follow this guideline. But I believe new code should adhere to the documented style as much as possible. In particular, DETAIL and HINT messages should begin with a capital letter and follow the complete-sentence convention.
>>> Hi, v2 attached, WIP, the only known remaining issue to me is that
>>> windows might fail to compile as it probably doesn't have concept of
>>> uid_t... I'm wondering what to do there.
>> 
>> 
>> I don't think it will arise, as Windows doesn't have the siginfo stuff,
>> AFAIK.
>> 
> 
> Hi Andrew. Ok, so V3 is attached with just one change: uid_t/pid_t
> changed to uint32 to make win32 happy.
> 
> Perhaps one question is whether this should be toggleable with GUC or not.
> 
> -J.
> <v3-0001-Add-errdetail-with-PID-and-UID-about-source-of-te.patch>

A few comments for v3:

1 - syncrep.c
```
@@ -302,7 +302,11 @@ SyncRepWaitForLSN(XLogRecPtr lsn, bool commit)
 			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.")));
+					 errdetail("The transaction has already committed locally, but might not have been replicated to the standby."),
+						proc_die_sender_pid == 0 ? 0 :
+							errdetail("Signal sent by PID %lld, UID %lld.",
+								(long long)proc_die_sender_pid, (long long)proc_die_sender_uid)
+						));
```

Here errdetail is used twice. I guess the second conditional one should be errhint.

2 - syncrep.c
```
-					 errdetail("The transaction has already committed locally, but might not have been replicated to the standby.")));
+					 errdetail("The transaction has already committed locally, but might not have been replicated to the standby."),
+						proc_die_sender_pid == 0 ? 0 :
+							errdetail("Signal sent by PID %lld, UID %lld.",
+								(long long)proc_die_sender_pid, (long long)proc_die_sender_uid)
+						));
```

Same as comment 1.

3
```
+volatile uint32 proc_die_sender_pid = 0;
+volatile uint32 proc_die_sender_uid = 0;
```

These two globals are only written in the signal handler, I think they should be sig_atomic_t to ensure atomic writes.

4
```
-					 errmsg("terminating walreceiver process due to administrator command")));
+					 errmsg("terminating walreceiver process due to administrator command"),
+					 proc_die_sender_pid == 0 ? 0 :
+						errdetail("Signal sent by PID %lld, UID %lld.",
+							(long long)proc_die_sender_pid, (long long)proc_die_sender_uid)
+					));
```

Why do we need to format pid and uid in “long long” format? I searched over the source tree, the current postmaster.c just formats pid as int (%d):
```
	/* in parent, successful fork */
	ereport(DEBUG2,
			(errmsg_internal("forked new %s, pid=%d socket=%d",
							 GetBackendTypeDesc(bn->bkend_type),
							 (int) pid, (int) client_sock->sock)));
```

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

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

Jakub Wartak <jakub.wartak@enterprisedb.com> — 2026-02-25T10:45:12Z

On Wed, Feb 25, 2026 at 10:15 AM Chao Li <li.evan.chao@gmail.com> wrote:

Hi Chao, thanks for review.

> A few comments for v3:
>
> 1 - syncrep.c
[..]
> +                                               proc_die_sender_pid == 0 ? 0 :
> +                                                       errdetail("Signal sent by PID %lld, UID %lld.",
> +                                                               (long long)proc_die_sender_pid, (long long)proc_die_sender_uid)
> +                                               ));
> ```
>
> Here errdetail is used twice. I guess the second conditional one should be errhint.
>
> 2 - syncrep.c
[..]
> Same as comment 1.

You are right, apparently I copy/pasted code from src/backend/tcop/postgres.c
way too fast... fixed.

> 3
> ```
> +volatile uint32 proc_die_sender_pid = 0;
> +volatile uint32 proc_die_sender_uid = 0;
> ```
>
> These two globals are only written in the signal handler, I think they should be sig_atomic_t to ensure atomic writes.

Well the problem that sig_atomic_t is int and we need at least uint32 and
I couldn't find better way. I think that 4 bytes writes will be mostly
always atomic (for 64-bits it would depend on
PG_HAVE_8BYTE_SINGLE_COPY_ATOMICITY)

Yet I moved those little below, so it's more aligned to the other uses.

> 4
> ```
> -                                        errmsg("terminating walreceiver process due to administrator command")));
> +                                        errmsg("terminating walreceiver process due to administrator command"),
> +                                        proc_die_sender_pid == 0 ? 0 :
> +                                               errdetail("Signal sent by PID %lld, UID %lld.",
> +                                                       (long long)proc_die_sender_pid, (long long)proc_die_sender_uid)
> +                                       ));
> ```
>
> Why do we need to format pid and uid in “long long” format? I searched over the source tree, the current postmaster.c just formats pid as int (%d):
> ```
>         /* in parent, successful fork */
>         ereport(DEBUG2,
>                         (errmsg_internal("forked new %s, pid=%d socket=%d",
>                                                          GetBackendTypeDesc(bn->bkend_type),
>                                                          (int) pid, (int) client_sock->sock)));
> ```

Yes, I think I was kind of lost when thinking about it (v1 had sig_atomic_t,
later had pid_t, I read somewhere about 64-bit pids, and so on) vs
platform-agnostic hell of putting that into printf). Possible I was
overthinking it
and I have reverted it to just using %d with that uint32. BTW I've also found:
   elog(DEBUG3, "kill(%ld,%d) failed: %m", (long) pid, signal);

v4 attached. Thanks again for the review.

-J.

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

Chao Li <li.evan.chao@gmail.com> — 2026-02-26T03:08:28Z


> On Feb 25, 2026, at 18:45, Jakub Wartak <jakub.wartak@enterprisedb.com> wrote:
> 
> On Wed, Feb 25, 2026 at 10:15 AM Chao Li <li.evan.chao@gmail.com> wrote:
> 
> Hi Chao, thanks for review.
> 
>> A few comments for v3:
>> 
>> 1 - syncrep.c
> [..]
>> +                                               proc_die_sender_pid == 0 ? 0 :
>> +                                                       errdetail("Signal sent by PID %lld, UID %lld.",
>> +                                                               (long long)proc_die_sender_pid, (long long)proc_die_sender_uid)
>> +                                               ));
>> ```
>> 
>> Here errdetail is used twice. I guess the second conditional one should be errhint.
>> 
>> 2 - syncrep.c
> [..]
>> Same as comment 1.
> 
> You are right, apparently I copy/pasted code from src/backend/tcop/postgres.c
> way too fast... fixed.
> 
>> 3
>> ```
>> +volatile uint32 proc_die_sender_pid = 0;
>> +volatile uint32 proc_die_sender_uid = 0;
>> ```
>> 
>> These two globals are only written in the signal handler, I think they should be sig_atomic_t to ensure atomic writes.
> 
> Well the problem that sig_atomic_t is int and we need at least uint32 and
> I couldn't find better way. I think that 4 bytes writes will be mostly
> always atomic (for 64-bits it would depend on
> PG_HAVE_8BYTE_SINGLE_COPY_ATOMICITY)
> 
> Yet I moved those little below, so it's more aligned to the other uses.
> 
>> 4
>> ```
>> -                                        errmsg("terminating walreceiver process due to administrator command")));
>> +                                        errmsg("terminating walreceiver process due to administrator command"),
>> +                                        proc_die_sender_pid == 0 ? 0 :
>> +                                               errdetail("Signal sent by PID %lld, UID %lld.",
>> +                                                       (long long)proc_die_sender_pid, (long long)proc_die_sender_uid)
>> +                                       ));
>> ```
>> 
>> Why do we need to format pid and uid in “long long” format? I searched over the source tree, the current postmaster.c just formats pid as int (%d):
>> ```
>>        /* in parent, successful fork */
>>        ereport(DEBUG2,
>>                        (errmsg_internal("forked new %s, pid=%d socket=%d",
>>                                                         GetBackendTypeDesc(bn->bkend_type),
>>                                                         (int) pid, (int) client_sock->sock)));
>> ```
> 
> Yes, I think I was kind of lost when thinking about it (v1 had sig_atomic_t,
> later had pid_t, I read somewhere about 64-bit pids, and so on) vs
> platform-agnostic hell of putting that into printf). Possible I was
> overthinking it
> and I have reverted it to just using %d with that uint32. BTW I've also found:
>   elog(DEBUG3, "kill(%ld,%d) failed: %m", (long) pid, signal);
> 
> v4 attached. Thanks again for the review.
> 
> -J.
> <v4-0001-Add-errdetail-with-PID-and-UID-about-source-of-te.patch>

I just reviewed v4 again and got a few more comments:

1. This patch only set the global proc_die_sender_pid/uid to 0 at startup, then assign values to them upon receiving SIGTERM, and never reset them, which assumes a process must die upon SIGTERM. Is the assumption true? I guess not. If a process receives SIGTERM and not die immediately, then die for other reason, then it may report a misleading PID and UID. So, I think we may need to reset proc_die_sender_pid/uid somewhere. For example, in ProcessInterrupts(), copy them to local variables and reset them to 0, then use the local variables for ereport().

2. 
```
@@ -319,7 +323,11 @@ SyncRepWaitForLSN(XLogRecPtr lsn, bool commit)
 			ereport(WARNING,
 					(errmsg("canceling wait for synchronous replication due to user request"),
-					 errdetail("The transaction has already committed locally, but might not have been replicated to the standby.")));
+					 errdetail("The transaction has already committed locally, but might not have been replicated to the standby."),
+						proc_die_sender_pid == 0 ? 0 :
+							errhint("Signal sent by PID %d, UID %d.",
+								proc_die_sender_pid, proc_die_sender_uid)
+						));
```

syncrpe.c uses errhint to print PID and UID, and postgres.c uses errdetail. We should keep consistency, maybe all use errhint.

3.
```
@@ -319,7 +323,11 @@ SyncRepWaitForLSN(XLogRecPtr lsn, bool commit)
 			QueryCancelPending = false;
 			ereport(WARNING,
 					(errmsg("canceling wait for synchronous replication due to user request"),
-					 errdetail("The transaction has already committed locally, but might not have been replicated to the standby.")));
+					 errdetail("The transaction has already committed locally, but might not have been replicated to the standby."),
+						proc_die_sender_pid == 0 ? 0 :
+							errhint("Signal sent by PID %d, UID %d.",
+								proc_die_sender_pid, proc_die_sender_uid)
+						));
 			SyncRepCancelWait();
 			break;
 		}
```

I don’t think the query cancel case relates to SIGTERM, so we don’t need to log PID and UID here.

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

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

Jakub Wartak <jakub.wartak@enterprisedb.com> — 2026-02-26T09:25:47Z

On Thu, Feb 26, 2026 at 4:09 AM Chao Li <li.evan.chao@gmail.com> wrote:

Hi Chao,

> I just reviewed v4 again and got a few more comments:
>
> 1. This patch only set the global proc_die_sender_pid/uid to 0 at startup, then assign values to them upon receiving SIGTERM, and never reset them, which assumes a process must die upon SIGTERM. Is the assumption true? I guess not. If a process receives SIGTERM and not die immediately, then die for other reason, then it may report a misleading PID and UID.

Hmm, I'm not sure I follow. If we receive SIGTERM and not die immediately
(for whatever reason), then two scenarios can happen as far as I'm concerned:
* another SIGTERM comes in from the same or different uid/pid and it wll be
reported properly
* different SIGKILL, but in this case we won't report UID/PID at all

am I missing something or do You have any particular scenario in mind?
The flow will be wrapper_handler()->die()->SetLatch()->..->directyl to
err reporting facilities.

> 2.
> syncrpe.c uses errhint to print PID and UID, and postgres.c uses errdetail. We should keep consistency, maybe all use errhint.

Right, let's make it that way.

> 3.
> ```
> @@ -319,7 +323,11 @@ SyncRepWaitForLSN(XLogRecPtr lsn, bool commit)
>                         QueryCancelPending = false;
>                         ereport(WARNING,
>                                         (errmsg("canceling wait for synchronous replication due to user request"),
> -                                        errdetail("The transaction has already committed locally, but might not have been replicated to the standby.")));
> +                                        errdetail("The transaction has already committed locally, but might not have been replicated to the standby."),
> +                                               proc_die_sender_pid == 0 ? 0 :
> +                                                       errhint("Signal sent by PID %d, UID %d.",
> +                                                               proc_die_sender_pid, proc_die_sender_uid)
> +                                               ));
>                         SyncRepCancelWait();
>                         break;
>                 }
> ```
>
> I don’t think the query cancel case relates to SIGTERM, so we don’t need to log PID and UID here.

Right, it was superfluous.

v5 attached.

-J.

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

Andrew Dunstan <andrew@dunslane.net> — 2026-04-06T16:51:40Z

On 2026-02-26 Th 4:25 AM, Jakub Wartak wrote:
> On Thu, Feb 26, 2026 at 4:09 AM Chao Li <li.evan.chao@gmail.com> wrote:
>
> Hi Chao,
>
>> I just reviewed v4 again and got a few more comments:
>>
>> 1. This patch only set the global proc_die_sender_pid/uid to 0 at startup, then assign values to them upon receiving SIGTERM, and never reset them, which assumes a process must die upon SIGTERM. Is the assumption true? I guess not. If a process receives SIGTERM and not die immediately, then die for other reason, then it may report a misleading PID and UID.
> Hmm, I'm not sure I follow. If we receive SIGTERM and not die immediately
> (for whatever reason), then two scenarios can happen as far as I'm concerned:
> * another SIGTERM comes in from the same or different uid/pid and it wll be
> reported properly
> * different SIGKILL, but in this case we won't report UID/PID at all
>
> am I missing something or do You have any particular scenario in mind?
> The flow will be wrapper_handler()->die()->SetLatch()->..->directyl to
> err reporting facilities.
>
>> 2.
>> syncrpe.c uses errhint to print PID and UID, and postgres.c uses errdetail. We should keep consistency, maybe all use errhint.
> Right, let's make it that way.
>
>> 3.
>> ```
>> @@ -319,7 +323,11 @@ SyncRepWaitForLSN(XLogRecPtr lsn, bool commit)
>>                          QueryCancelPending = false;
>>                          ereport(WARNING,
>>                                          (errmsg("canceling wait for synchronous replication due to user request"),
>> -                                        errdetail("The transaction has already committed locally, but might not have been replicated to the standby.")));
>> +                                        errdetail("The transaction has already committed locally, but might not have been replicated to the standby."),
>> +                                               proc_die_sender_pid == 0 ? 0 :
>> +                                                       errhint("Signal sent by PID %d, UID %d.",
>> +                                                               proc_die_sender_pid, proc_die_sender_uid)
>> +                                               ));
>>                          SyncRepCancelWait();
>>                          break;
>>                  }
>> ```
>>
>> I don’t think the query cancel case relates to SIGTERM, so we don’t need to log PID and UID here.
> Right, it was superfluous.
>
> v5 attached.
>


I'd kinda like to sneak this in for pg19, because I think it's useful. 
Here's a v6 that changes one or two things:


- changes the globals to sig_atomic_t

- in ProcessInterrupts, copies to local sender_pid/sender_uid, then 
zeros the globals before any ereport

- uses errdetail() for all the messages


Plus a few more cosmetic changes like consistent casing.


cheers


andrew



--
Andrew Dunstan
EDB: https://www.enterprisedb.com

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

Daniel Gustafsson <daniel@yesql.se> — 2026-04-06T19:36:15Z

> On 6 Apr 2026, at 18:51, Andrew Dunstan <andrew@dunslane.net> wrote:

> I'd kinda like to sneak this in for pg19, because I think it's useful. Here's a v6 that changes one or two things:

+1.  I haven't done an in-depth review but I quite like the feature and have
wanted this very thing in the past.

--
Daniel Gustafsson

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

jie wang <jugierwang@gmail.com> — 2026-04-07T03:55:15Z

Andrew Dunstan <andrew@dunslane.net> 于2026年4月7日周二 00:51写道：

>
> On 2026-02-26 Th 4:25 AM, Jakub Wartak wrote:
> > On Thu, Feb 26, 2026 at 4:09 AM Chao Li <li.evan.chao@gmail.com> wrote:
> >
> > Hi Chao,
> >
> >> I just reviewed v4 again and got a few more comments:
> >>
> >> 1. This patch only set the global proc_die_sender_pid/uid to 0 at
> startup, then assign values to them upon receiving SIGTERM, and never reset
> them, which assumes a process must die upon SIGTERM. Is the assumption
> true? I guess not. If a process receives SIGTERM and not die immediately,
> then die for other reason, then it may report a misleading PID and UID.
> > Hmm, I'm not sure I follow. If we receive SIGTERM and not die immediately
> > (for whatever reason), then two scenarios can happen as far as I'm
> concerned:
> > * another SIGTERM comes in from the same or different uid/pid and it wll
> be
> > reported properly
> > * different SIGKILL, but in this case we won't report UID/PID at all
> >
> > am I missing something or do You have any particular scenario in mind?
> > The flow will be wrapper_handler()->die()->SetLatch()->..->directyl to
> > err reporting facilities.
> >
> >> 2.
> >> syncrpe.c uses errhint to print PID and UID, and postgres.c uses
> errdetail. We should keep consistency, maybe all use errhint.
> > Right, let's make it that way.
> >
> >> 3.
> >> ```
> >> @@ -319,7 +323,11 @@ SyncRepWaitForLSN(XLogRecPtr lsn, bool commit)
> >>                          QueryCancelPending = false;
> >>                          ereport(WARNING,
> >>                                          (errmsg("canceling wait for
> synchronous replication due to user request"),
> >> -                                        errdetail("The transaction has
> already committed locally, but might not have been replicated to the
> standby.")));
> >> +                                        errdetail("The transaction has
> already committed locally, but might not have been replicated to the
> standby."),
> >> +                                               proc_die_sender_pid ==
> 0 ? 0 :
> >> +                                                       errhint("Signal
> sent by PID %d, UID %d.",
> >> +
>  proc_die_sender_pid, proc_die_sender_uid)
> >> +                                               ));
> >>                          SyncRepCancelWait();
> >>                          break;
> >>                  }
> >> ```
> >>
> >> I don’t think the query cancel case relates to SIGTERM, so we don’t
> need to log PID and UID here.
> > Right, it was superfluous.
> >
> > v5 attached.
> >
>
>
> I'd kinda like to sneak this in for pg19, because I think it's useful.
> Here's a v6 that changes one or two things:
>
>
> - changes the globals to sig_atomic_t
>
> - in ProcessInterrupts, copies to local sender_pid/sender_uid, then
> zeros the globals before any ereport
>
> - uses errdetail() for all the messages
>
>
> Plus a few more cosmetic changes like consistent casing.
>
>
> cheers
>
>
> andrew
>
>
>
> --
> Andrew Dunstan
> EDB: https://www.enterprisedb.com



I just reviewed v6 and got 1 comment.

sig_atomic_t is underlying int, but pid_t and uid_t are usually unsigned
int,
so that while saving pid and uid to ProcDieSenderPid and ProcDieSenderUid,
overflow may happen. But that’s fine, as the data is stored in the same way
and we only want to print them. So, for the print statement:

#define ERRDETAIL_SIGNAL_SENDER(pid, uid) \
    ((pid) == 0 ? 0 : \
    errdetail("Signal sent by PID %d, UID %d.", (int) (pid), (int) (uid)))

Does it make sense to use %u and cast to pid_t and uid_t? Like:

#define ERRDETAIL_SIGNAL_SENDER(pid, uid) \
    ((pid) == 0 ? 0 : \
    errdetail("Signal sent by PID %u, UID %u.", (pid_t) (pid), (uid_t)
(uid)))

Thanks!
--
wang jie

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

Jakub Wartak <jakub.wartak@enterprisedb.com> — 2026-04-07T09:10:37Z

On Tue, Apr 7, 2026 at 5:55 AM jie wang <jugierwang@gmail.com> wrote:
>
>
>
> Andrew Dunstan <andrew@dunslane.net> 于2026年4月7日周二 00:51写道：
>>
>>
[..]
>>
>> I'd kinda like to sneak this in for pg19, because I think it's useful.
>> Here's a v6 that changes one or two things:
>>
>>
>> - changes the globals to sig_atomic_t
>>
>> - in ProcessInterrupts, copies to local sender_pid/sender_uid, then
>> zeros the globals before any ereport
>>
>> - uses errdetail() for all the messages
>>
>>
>> Plus a few more cosmetic changes like consistent casing.
>>
>>
>> cheers
>>
>>
>> andrew
>>
>>
>>
>> --
>> Andrew Dunstan
>> EDB: https://www.enterprisedb.com
>
>
>
> I just reviewed v6 and got 1 comment.
>
> sig_atomic_t is underlying int, but pid_t and uid_t are usually unsigned int,
> so that while saving pid and uid to ProcDieSenderPid and ProcDieSenderUid,
> overflow may happen. But that’s fine, as the data is stored in the same way
> and we only want to print them. So, for the print statement:
>
> #define ERRDETAIL_SIGNAL_SENDER(pid, uid) \
>     ((pid) == 0 ? 0 : \
>     errdetail("Signal sent by PID %d, UID %d.", (int) (pid), (int) (uid)))
>
> Does it make sense to use %u and cast to pid_t and uid_t? Like:
>
> #define ERRDETAIL_SIGNAL_SENDER(pid, uid) \
>     ((pid) == 0 ? 0 : \
>     errdetail("Signal sent by PID %u, UID %u.", (pid_t) (pid), (uid_t) (uid)))

I really don't have hard opinion on what we should use here (int vs uint32 vs
pid_t)  and I was scratching my head at this earier, as:

1. Usually win32 doesn't have pid_t, but in win32_port.h we have
   "typedef int pid_t".

2. According to `grep -ri ' pid' src/backend/ we seem to use "int" in most cases
   for this (especially MyProcPid is also int). The only other places
where I found
   %u or %l would be those:

   src/backend/port/win32/signal.c:
        snprintf(pipename, sizeof(pipename),
"\\\\.\\pipe\\pgsignal_%u", (int) pid);

   src/backend/port/win32/signal.c:
        (errmsg("could not create signal listener pipe for PID %d:
error code %lu",
   src/backend/postmaster/datachecksum_state.c:
        "Waiting for worker in database %s (pid %ld)", db->dbname, (long) pid);
   src/backend/postmaster/postmaster.c:
        elog(DEBUG3, "kill(%ld,%d) failed: %m", (long) pid, signal);

   So apparently we are really not consistent, but int looks fine to me.

3. Linux kernel for most allows up to kernel.pid_max (4194304, 22 bits) and
   internally it uses "int" for it's pid_max_max [1] and uses up to
30-bits since always.

So "int" follows old style and seems to be OK at least from my point of view.

-J.

[1] - https://github.com/torvalds/linux/blob/master/kernel/pid.c#L64C17-L64C40
[2] - https://github.com/torvalds/linux/blob/master/include/linux/threads.h#L34

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

Andrew Dunstan <andrew@dunslane.net> — 2026-04-07T14:36:09Z

On 2026-04-07 Tu 5:10 AM, Jakub Wartak wrote:
> On Tue, Apr 7, 2026 at 5:55 AM jie wang<jugierwang@gmail.com> wrote:
>>
>>
>> Andrew Dunstan<andrew@dunslane.net> 于2026年4月7日周二 00:51写道：
>>>
> [..]
>>> I'd kinda like to sneak this in for pg19, because I think it's useful.
>>> Here's a v6 that changes one or two things:
>>>
>>>
>>> - changes the globals to sig_atomic_t
>>>
>>> - in ProcessInterrupts, copies to local sender_pid/sender_uid, then
>>> zeros the globals before any ereport
>>>
>>> - uses errdetail() for all the messages
>>>
>>>
>>> Plus a few more cosmetic changes like consistent casing.
>>>
>>>
>>> cheers
>>>
>>>
>>> andrew
>>>
>>>
>>>
>>> --
>>> Andrew Dunstan
>>> EDB:https://www.enterprisedb.com
>>
>>
>> I just reviewed v6 and got 1 comment.
>>
>> sig_atomic_t is underlying int, but pid_t and uid_t are usually unsigned int,
>> so that while saving pid and uid to ProcDieSenderPid and ProcDieSenderUid,
>> overflow may happen. But that’s fine, as the data is stored in the same way
>> and we only want to print them. So, for the print statement:
>>
>> #define ERRDETAIL_SIGNAL_SENDER(pid, uid) \
>>      ((pid) == 0 ? 0 : \
>>      errdetail("Signal sent by PID %d, UID %d.", (int) (pid), (int) (uid)))
>>
>> Does it make sense to use %u and cast to pid_t and uid_t? Like:
>>
>> #define ERRDETAIL_SIGNAL_SENDER(pid, uid) \
>>      ((pid) == 0 ? 0 : \
>>      errdetail("Signal sent by PID %u, UID %u.", (pid_t) (pid), (uid_t) (uid)))
> I really don't have hard opinion on what we should use here (int vs uint32 vs
> pid_t)  and I was scratching my head at this earier, as:
>
> 1. Usually win32 doesn't have pid_t, but in win32_port.h we have
>     "typedef int pid_t".
>
> 2. According to `grep -ri ' pid' src/backend/ we seem to use "int" in most cases
>     for this (especially MyProcPid is also int). The only other places
> where I found
>     %u or %l would be those:
>
>     src/backend/port/win32/signal.c:
>          snprintf(pipename, sizeof(pipename),
> "\\\\.\\pipe\\pgsignal_%u", (int) pid);
>
>     src/backend/port/win32/signal.c:
>          (errmsg("could not create signal listener pipe for PID %d:
> error code %lu",
>     src/backend/postmaster/datachecksum_state.c:
>          "Waiting for worker in database %s (pid %ld)", db->dbname, (long) pid);
>     src/backend/postmaster/postmaster.c:
>          elog(DEBUG3, "kill(%ld,%d) failed: %m", (long) pid, signal);
>
>     So apparently we are really not consistent, but int looks fine to me.
>
> 3. Linux kernel for most allows up to kernel.pid_max (4194304, 22 bits) and
>     internally it uses "int" for it's pid_max_max [1] and uses up to
> 30-bits since always.
>
> So "int" follows old style and seems to be OK at least from my point of view.
>
> -J.
>
> [1] -https://github.com/torvalds/linux/blob/master/kernel/pid.c#L64C17-L64C40
> [2] -https://github.com/torvalds/linux/blob/master/include/linux/threads.h#L34



OK, went back to using int. Pushed.


cheers


andrew

--
Andrew Dunstan
EDB:https://www.enterprisedb.com

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

Andres Freund <andres@anarazel.de> — 2026-04-07T14:55:53Z

Hi,

On 2026-04-06 12:51:40 -0400, Andrew Dunstan wrote:
> From 450b68d29f02ee1f5bf71db708b380ab389a30c6 Mon Sep 17 00:00:00 2001
> From: Andrew Dunstan <andrew@dunslane.net>
> Date: Mon, 6 Apr 2026 12:39:14 -0400
> Subject: [PATCH v6] Add errdetail() with PID and UID about source of
>  termination signal.
>
> When a backend is terminated via pg_terminate_backend() or an external
> SIGTERM, the error message now includes the sender's PID and UID as
> errdetail, making it easier to identify the source of unexpected
> terminations in multi-user environments.
>
> On platforms that support SA_SIGINFO (Linux, FreeBSD, and most modern
> Unix systems), the signal handler captures si_pid and si_uid from the
> siginfo_t structure.  On platforms without SA_SIGINFO, the detail is
> simply omitted.
>
> Author: Jakub Wartak <jakub.wartak@enterprisedb.com>
> Reviewed-by: Andrew Dunstan <andrew@dunslane.net>
> Reviewed-by: Chao Li <1356863904@qq.com>
> Discussion: https://postgr.es/m/CAKZiRmyrOWovZSdixpLd3PGMQXuQL_zw2Ght5XhHCkQ1uDsxjw@mail.gmail.com


> +++ b/src/backend/replication/syncrep.c
> @@ -303,7 +303,11 @@ SyncRepWaitForLSN(XLogRecPtr lsn, bool commit)
>  			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.")));
> +					 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))));
>  			whereToSendOutput = DestNone;
>  			SyncRepCancelWait();
>  			break;

Pretty sure this is broken from a translateability POV?

It's also somewhat ugly.


> +++ b/src/port/pqsignal.c
> @@ -82,10 +82,19 @@ static volatile pqsigfunc pqsignal_handlers[PG_NSIG];
>   *
>   * This wrapper also handles restoring the value of errno.
>   */
> +#if !defined(FRONTEND) && defined(HAVE_SA_SIGINFO)
> +static void
> +wrapper_handler(int signo, siginfo_t * info, void *context)
> +#else
>  static void
>  wrapper_handler(SIGNAL_ARGS)
> +#endif
>  {
>  	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;
> +#endif

Seems that you then should change what SIGNAL_ARGS means, not randomly hack
around it in one place?


>  	Assert(postgres_signal_arg > 0);
>  	Assert(postgres_signal_arg < PG_NSIG);
> @@ -105,6 +114,14 @@ wrapper_handler(SIGNAL_ARGS)
>  		raise(postgres_signal_arg);
>  		return;
>  	}
> +
> +#ifdef HAVE_SA_SIGINFO
> +	if (signo == SIGTERM && info)
> +	{
> +		ProcDieSenderPid = info->si_pid;
> +		ProcDieSenderUid = info->si_uid;
> +	}
> +#endif
>  #endif
>
>  	(*pqsignal_handlers[postgres_signal_arg]) (postgres_signal_arg);

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.



I think this is nowhere near ready to have been committed.


Greetings,

Andres Freund

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

Andrew Dunstan <andrew@dunslane.net> — 2026-04-07T16:49:19Z

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?


cheers


andrew

--
Andrew Dunstan
EDB:https://www.enterprisedb.com

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

Andres Freund <andres@anarazel.de> — 2026-04-07T18:19:51Z

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.

Greetings,

Andres Freund

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

Andrew Dunstan <andrew@dunslane.net> — 2026-04-07T21:31:18Z

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

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

Andres Freund <andres@anarazel.de> — 2026-04-07T22:56:49Z

Hi,

On 2026-04-07 17:31:18 -0400, Andrew Dunstan wrote:
> On 2026-04-07 Tu 2:19 PM, Andres Freund wrote:
> > 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.

I don't think that addresses my concerns at all unfortunately.  I can give
writing a sketch of how I think it should like a go, but it won't be today and
probably not this week.

I suspect this patch just has missed the boat for 19, but if others think we
can fix it up in a week or two, I'm also ok. It's a feature I wanted for a
long time.

Greetings,

Andres Freund

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

Chao Li <li.evan.chao@gmail.com> — 2026-04-08T01:03:33Z


> On Apr 8, 2026, at 06:56, Andres Freund <andres@anarazel.de> wrote:
> 
> Hi,
> 
> On 2026-04-07 17:31:18 -0400, Andrew Dunstan wrote:
>> On 2026-04-07 Tu 2:19 PM, Andres Freund wrote:
>>> 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.
> 
> I don't think that addresses my concerns at all unfortunately.  I can give
> writing a sketch of how I think it should like a go, but it won't be today and
> probably not this week.
> 
> I suspect this patch just has missed the boat for 19, but if others think we
> can fix it up in a week or two, I'm also ok. It's a feature I wanted for a
> long time.
> 
> Greetings,
> 
> Andres Freund

I tried to understand the layering comment, and I’m proposing a fix where sender information is stored within pqsignal and exposed via a new helper function, pqsignal_get_sender(). Then the signal handler retrieves the signal sender via pqsignal_get_sender() and sets ProcDieSenderPid/ProcDieSenderUid. Please see the attached diff.

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

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

Jim Jones <jim.jones@uni-muenster.de> — 2026-04-08T09:13:40Z

Hi

On 08/04/2026 03:03, Chao Li wrote:
> I tried to understand the layering comment, and I’m proposing a fix where sender information is stored within pqsignal and exposed via a new helper function, pqsignal_get_sender(). Then the signal handler retrieves the signal sender via pqsignal_get_sender() and sets ProcDieSenderPid/ProcDieSenderUid. Please see the attached diff.

I have a few questions (slightly unrelated to Chao's fix)

I'm a bit confused with the data flow and the data types involved:

1) in pgsignal.c it's volatile sig_atomic_t

#if !defined(FRONTEND) && defined(HAVE_SA_SIGINFO)
static volatile sig_atomic_t pqsignal_has_sender[PG_NSIG];
static volatile sig_atomic_t pqsignal_sender_pid[PG_NSIG];
static volatile sig_atomic_t pqsignal_sender_uid[PG_NSIG];
#endif

2) in postgres.c it's int

int  sender_pid;
int  sender_uid;

3) and in globals.c it is volatile int

volatile int ProcDieSenderPid = 0;
volatile int ProcDieSenderUid = 0;

I guess 2) is ok, since it seems to be a one time read, but I'm
wondering if the consumer in 3) should also use volatile sig_atomic_t:

in globals.c
volatile sig_atomic_t ProcDieSenderPid = 0;
volatile sig_atomic_t ProcDieSenderUid = 0;

in miscadmin.h
extern PGDLLIMPORT volatile sig_atomic_t ProcDieSenderPid;
extern PGDLLIMPORT volatile sig_atomic_t ProcDieSenderUid;


If I understood this thread correctly, the feature (v6) introduced a
problematic dual signature for wrapper_handler in pgsignal.c:

+#if !defined(FRONTEND) && defined(HAVE_SA_SIGINFO)
+static void
+wrapper_handler(int signo, siginfo_t * info, void *context)
+#else
 static void
 wrapper_handler(SIGNAL_ARGS)
+#endif

.. and it should be rather done in the source (c.h) where SIGNAL_ARGS is
defined:

#ifndef SIGNAL_ARGS
#define SIGNAL_ARGS  int postgres_signal_arg
#endif

Something like:

#ifndef SIGNAL_ARGS
#ifdef HAVE_SA_SIGINFO
#define SIGNAL_ARGS  int postgres_signal_arg, siginfo_t
*postgres_signal_info, void *postgres_signal_context
#else
#define SIGNAL_ARGS  int postgres_signal_arg
#endif
#endif

But wouldn't it mean that all handlers need to be updated as well, since
they'd get new parameters? If this is the case, the change can be quite
substantial.

Best, Jim

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

Andres Freund <andres@anarazel.de> — 2026-04-08T17:01:15Z

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.

On 2026-04-08 11:13:40 +0200, Jim Jones wrote:
> If I understood this thread correctly, the feature (v6) introduced a
> problematic dual signature for wrapper_handler in pgsignal.c:

> +#if !defined(FRONTEND) && defined(HAVE_SA_SIGINFO)
> +static void
> +wrapper_handler(int signo, siginfo_t * info, void *context)
> +#else
>  static void
>  wrapper_handler(SIGNAL_ARGS)
> +#endif

I think it's not a problem for wrapper_handler to change its signature, that's
a local implementation detail.  The problem is that the way the arguments were
passed was just wrong.  Because signal handlers can be nested and such
nastiness, doing any of that via global variables is a recipe for disaster.
It's also just ugly.

> .. and it should be rather done in the source (c.h) where SIGNAL_ARGS is
> defined:
> 
> #ifndef SIGNAL_ARGS
> #define SIGNAL_ARGS  int postgres_signal_arg
> #endif
> 
> Something like:
> 
> #ifndef SIGNAL_ARGS
> #ifdef HAVE_SA_SIGINFO
> #define SIGNAL_ARGS  int postgres_signal_arg, siginfo_t
> *postgres_signal_info, void *postgres_signal_context
> #else
> #define SIGNAL_ARGS  int postgres_signal_arg
> #endif
> #endif
> 
> But wouldn't it mean that all handlers need to be updated as well, since
> they'd get new parameters?

All the signal handlers actually use SIGNAL_ARGS themselves, so that part is
not a problem.

However, if we did it like you sketch above, they'd all need ifdefs etc to be
able to access any extended information, which seems like a terrible
idea. Especially if we want this information on multiple platforms, where the
struct to be passed would differ.

Hence in my prototype it's hidden behind a platform indepenedent struct of our
own.

> If this is the case, the change can be quite substantial.

It's a bit annoying to do all the s/\b(SIG_(IGN|DFL)/PG_$1/, but it's not that
bad, I think?

I unfortunately don't see a good other way to deal with it.  We could have a
macro wrapper around pqsignal() that checks for SIG_IGN with a cast to the
system type, but that seems exceedingly ugly.

Greetings,

Andres Freund

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

Chao Li <li.evan.chao@gmail.com> — 2026-04-09T03:11:08Z


> On Apr 9, 2026, at 01:01, Andres Freund <andres@anarazel.de> 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.
> 
> 
> On 2026-04-08 11:13:40 +0200, Jim Jones wrote:
>> If I understood this thread correctly, the feature (v6) introduced a
>> problematic dual signature for wrapper_handler in pgsignal.c:
> 
>> +#if !defined(FRONTEND) && defined(HAVE_SA_SIGINFO)
>> +static void
>> +wrapper_handler(int signo, siginfo_t * info, void *context)
>> +#else
>> static void
>> wrapper_handler(SIGNAL_ARGS)
>> +#endif
> 
> I think it's not a problem for wrapper_handler to change its signature, that's
> a local implementation detail.  The problem is that the way the arguments were
> passed was just wrong.  Because signal handlers can be nested and such
> nastiness, doing any of that via global variables is a recipe for disaster.
> It's also just ugly.
> 
> 
>> .. and it should be rather done in the source (c.h) where SIGNAL_ARGS is
>> defined:
>> 
>> #ifndef SIGNAL_ARGS
>> #define SIGNAL_ARGS  int postgres_signal_arg
>> #endif
>> 
>> Something like:
>> 
>> #ifndef SIGNAL_ARGS
>> #ifdef HAVE_SA_SIGINFO
>> #define SIGNAL_ARGS  int postgres_signal_arg, siginfo_t
>> *postgres_signal_info, void *postgres_signal_context
>> #else
>> #define SIGNAL_ARGS  int postgres_signal_arg
>> #endif
>> #endif
>> 
>> But wouldn't it mean that all handlers need to be updated as well, since
>> they'd get new parameters?
> 
> All the signal handlers actually use SIGNAL_ARGS themselves, so that part is
> not a problem.
> 
> However, if we did it like you sketch above, they'd all need ifdefs etc to be
> able to access any extended information, which seems like a terrible
> idea. Especially if we want this information on multiple platforms, where the
> struct to be passed would differ.
> 
> Hence in my prototype it's hidden behind a platform indepenedent struct of our
> own.
> 
> 
>> If this is the case, the change can be quite substantial.
> 
> It's a bit annoying to do all the s/\b(SIG_(IGN|DFL)/PG_$1/, but it's not that
> bad, I think?
> 
> I unfortunately don't see a good other way to deal with it.  We could have a
> macro wrapper around pqsignal() that checks for SIG_IGN with a cast to the
> system type, but that seems exceedingly ugly.
> 
> Greetings,
> 
> Andres Freund
> <v1-0001-WIP-Support-for-extended-information-about-signal.patch>

I think the core idea here is to add a new parameter so signal handlers can receive “pg_signal_info". Compared to my earlier proposal, which stored sender info in file-scope variables, I agree this solution is more flexible. I think my earlier direction was mainly trying to avoid changing the signal handler interface.

So I have no objection to the overall direction. Since the patch is marked as WIP, I didn't review all the details yet. But one thing I want to point out is:
```
+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;
```

For uid, 0 is usually a valid value for root. So using 0 as the “unknown” value seems a bit awkward. Maybe we should instead document something like "uid is only meaningful when pid is not 0".

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

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

Chao Li <li.evan.chao@gmail.com> — 2026-04-09T06:14:23Z


> On Apr 9, 2026, at 11:11, Chao Li <li.evan.chao@gmail.com> wrote:
> 
> 
> 
>> On Apr 9, 2026, at 01:01, Andres Freund <andres@anarazel.de> 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.
>> 
>> 
>> On 2026-04-08 11:13:40 +0200, Jim Jones wrote:
>>> If I understood this thread correctly, the feature (v6) introduced a
>>> problematic dual signature for wrapper_handler in pgsignal.c:
>> 
>>> +#if !defined(FRONTEND) && defined(HAVE_SA_SIGINFO)
>>> +static void
>>> +wrapper_handler(int signo, siginfo_t * info, void *context)
>>> +#else
>>> static void
>>> wrapper_handler(SIGNAL_ARGS)
>>> +#endif
>> 
>> I think it's not a problem for wrapper_handler to change its signature, that's
>> a local implementation detail.  The problem is that the way the arguments were
>> passed was just wrong.  Because signal handlers can be nested and such
>> nastiness, doing any of that via global variables is a recipe for disaster.
>> It's also just ugly.
>> 
>> 
>>> .. and it should be rather done in the source (c.h) where SIGNAL_ARGS is
>>> defined:
>>> 
>>> #ifndef SIGNAL_ARGS
>>> #define SIGNAL_ARGS  int postgres_signal_arg
>>> #endif
>>> 
>>> Something like:
>>> 
>>> #ifndef SIGNAL_ARGS
>>> #ifdef HAVE_SA_SIGINFO
>>> #define SIGNAL_ARGS  int postgres_signal_arg, siginfo_t
>>> *postgres_signal_info, void *postgres_signal_context
>>> #else
>>> #define SIGNAL_ARGS  int postgres_signal_arg
>>> #endif
>>> #endif
>>> 
>>> But wouldn't it mean that all handlers need to be updated as well, since
>>> they'd get new parameters?
>> 
>> All the signal handlers actually use SIGNAL_ARGS themselves, so that part is
>> not a problem.
>> 
>> However, if we did it like you sketch above, they'd all need ifdefs etc to be
>> able to access any extended information, which seems like a terrible
>> idea. Especially if we want this information on multiple platforms, where the
>> struct to be passed would differ.
>> 
>> Hence in my prototype it's hidden behind a platform indepenedent struct of our
>> own.
>> 
>> 
>>> If this is the case, the change can be quite substantial.
>> 
>> It's a bit annoying to do all the s/\b(SIG_(IGN|DFL)/PG_$1/, but it's not that
>> bad, I think?
>> 
>> I unfortunately don't see a good other way to deal with it.  We could have a
>> macro wrapper around pqsignal() that checks for SIG_IGN with a cast to the
>> system type, but that seems exceedingly ugly.
>> 
>> Greetings,
>> 
>> Andres Freund
>> <v1-0001-WIP-Support-for-extended-information-about-signal.patch>
> 
> I think the core idea here is to add a new parameter so signal handlers can receive “pg_signal_info". Compared to my earlier proposal, which stored sender info in file-scope variables, I agree this solution is more flexible. I think my earlier direction was mainly trying to avoid changing the signal handler interface.
> 
> So I have no objection to the overall direction. Since the patch is marked as WIP, I didn't review all the details yet. But one thing I want to point out is:
> ```
> +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;
> ```
> 
> For uid, 0 is usually a valid value for root. So using 0 as the “unknown” value seems a bit awkward. Maybe we should instead document something like "uid is only meaningful when pid is not 0".
> 

Forgot to mention, I got a lot of compile warnings, for example:
```
parallel.c:1052:20: warning: cast from 'void (*)(int)' to 'pqsigfunc' (aka 'void (*)(int, struct pg_signal_info *)') converts to incompatible function type [-Wcast-function-type-mismatch]
 1052 |         pqsignal(SIGPIPE, PG_SIG_IGN);
      |                           ^~~~~~~~~~
../../../src/include/port.h:551:20: note: expanded from macro 'PG_SIG_IGN'
  551 | #define PG_SIG_IGN (pqsigfunc) SIG_IGN
      |                    ^~~~~~~~~~~~~~~~~~~
4 warnings generated.
```

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

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

Andrew Dunstan <andrew@dunslane.net> — 2026-04-09T10:59:39Z

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

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

Andres Freund <andres@anarazel.de> — 2026-04-09T14:00:32Z

Hi,

On 2026-04-09 06:59:39 -0400, Andrew Dunstan 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.

Yea, I think it'd be worse. It's also what I had tried first.


> 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.

I think Chao's point about needing to initialize uid to a better unset value
also needs to be fixed, at least.

Greetings,

Andres Freund

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

Andres Freund <andres@anarazel.de> — 2026-04-09T14:47:03Z

Hi,

On 2026-04-09 14:14:23 +0800, Chao Li wrote:
> > For uid, 0 is usually a valid value for root. So using 0 as the “unknown” value seems a bit awkward. Maybe we should instead document something like "uid is only meaningful when pid is not 0".
> > 
> 
> Forgot to mention, I got a lot of compile warnings, for example:
> ```
> parallel.c:1052:20: warning: cast from 'void (*)(int)' to 'pqsigfunc' (aka 'void (*)(int, struct pg_signal_info *)') converts to incompatible function type [-Wcast-function-type-mismatch]
>  1052 |         pqsignal(SIGPIPE, PG_SIG_IGN);
>       |                           ^~~~~~~~~~
> ../../../src/include/port.h:551:20: note: expanded from macro 'PG_SIG_IGN'
>   551 | #define PG_SIG_IGN (pqsigfunc) SIG_IGN
>       |                    ^~~~~~~~~~~~~~~~~~~
> 4 warnings generated.
> ```

That's an annoying warning, GAH. Can't imagine this is the only thing it
complains about. Yes, compiler, I put a cast there, because I did actually
want to cast, thanks.

I guess we'll have to define PG_SIG_IGN to 1 and PG_SIG_DFL to 0 ourselves.

Greetings,

Andres Freund

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

Chao Li <li.evan.chao@gmail.com> — 2026-04-10T06:18:10Z


> On Apr 9, 2026, at 22:47, Andres Freund <andres@anarazel.de> wrote:
> 
> Hi,
> 
> On 2026-04-09 14:14:23 +0800, Chao Li wrote:
>>> For uid, 0 is usually a valid value for root. So using 0 as the “unknown” value seems a bit awkward. Maybe we should instead document something like "uid is only meaningful when pid is not 0".
>>> 
>> 
>> Forgot to mention, I got a lot of compile warnings, for example:
>> ```
>> parallel.c:1052:20: warning: cast from 'void (*)(int)' to 'pqsigfunc' (aka 'void (*)(int, struct pg_signal_info *)') converts to incompatible function type [-Wcast-function-type-mismatch]
>> 1052 |         pqsignal(SIGPIPE, PG_SIG_IGN);
>>      |                           ^~~~~~~~~~
>> ../../../src/include/port.h:551:20: note: expanded from macro 'PG_SIG_IGN'
>>  551 | #define PG_SIG_IGN (pqsigfunc) SIG_IGN
>>      |                    ^~~~~~~~~~~~~~~~~~~
>> 4 warnings generated.
>> ```
> 
> That's an annoying warning, GAH. Can't imagine this is the only thing it
> complains about. Yes, compiler, I put a cast there, because I did actually
> want to cast, thanks.
> 
> I guess we'll have to define PG_SIG_IGN to 1 and PG_SIG_DFL to 0 ourselves.
> 
> Greetings,
> 
> Andres Freund

I think PG already has pg_funcptr_t to treat this case, this change eliminates the warnings for me:
```
chaol@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);
```

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

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

Chao Li <li.evan.chao@gmail.com> — 2026-04-10T07:40:31Z


> On Apr 9, 2026, at 18:59, Andrew Dunstan <andrew@dunslane.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@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/

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

Jakub Wartak <jakub.wartak@enterprisedb.com> — 2026-04-14T10:40:39Z

Hi Andres, Andrew, Chao

thanks for putting so much effort into enhancing the the previous implementation
of this code. Earlier I was not aware of potential problems involved.

On Fri, Apr 10, 2026 at 9:41 AM Chao Li <li.evan.chao@gmail.com> wrote:
>
>
>
> > On Apr 9, 2026, at 18:59, Andrew Dunstan <andrew@dunslane.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.
> >
>
> 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.
>

TL;DR; win/mingw is really unhappy with the state of rework patch right now.
I've tried to enhance and fix it, and now it's green for default CI run and
also for mingw too. Attached 0002-fixup-win32 patch does not incorporate Chao's
all findings so far.

[..]
> 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.

[..]
> 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.
>


I. I've started from this and I vaguley remembered that I had terrible
experience
when trying to chose the proper types for those field types, but I couldn't
remind myself why, so I've gave a try of the current rework patch and got this
on Windows Server 2022, vs2019, cirrus-ci said:

[08:40:22.848] c:\cirrus\src\include\c.h(1451): error C2061: syntax
error: identifier 'pid_t'
[08:40:22.848] c:\cirrus\src\include\c.h(1452): error C2061: syntax
error: identifier 'uid'
[08:40:22.848] c:\cirrus\src\include\c.h(1452): error C2059: syntax error: ';'
[08:40:22.848] c:\cirrus\src\include\c.h(1453): error C2059: syntax error: '}'

for c.h:
  1449  typedef struct pg_signal_info
  1450  {
  1451          pid_t           pid;                    /* pid of
sending process or 0 if unknown */
  1452          uid_t           uid;                    /* uid of
sending process or 0 if unknown */
  1453  } pg_signal_info;

so maybe we should just move that typedef with SIGNAL_ARGS to after
"include of port.h" (line ~1471) in that c.h, because only then we'll have
access to:
    src/include/port/win32_port.h:typedef int pid_t;
    src/include/port/win32_port.h:typedef int uid_t;
but the problem is that port.h itself requires SIGNAL_ARGS to be defined
and that seems to be like chicken and egg problem.  I thought that just
using native "ints" could be the way to go, but the problem is that now
that uid_t can be bigger than pid_t as Linux kernel headers show this:

x86_64-linux-gnu/bits/types.h:#define   __S32_TYPE              int
x86_64-linux-gnu/bits/types.h:#define __U32_TYPE                unsigned int
x86_64-linux-gnu/bits/types.h:__STD_TYPE __UID_T_TYPE __uid_t;  /*
Type of user identifications.  */
x86_64-linux-gnu/bits/types.h:__STD_TYPE __PID_T_TYPE __pid_t;  /*
Type of process identifications.  */
x86_64-linux-gnu/bits/typesizes.h:#define __UID_T_TYPE          __U32_TYPE
x86_64-linux-gnu/bits/typesizes.h:#define __PID_T_TYPE          __S32_TYPE

so maybe do that typedef struct pg_signal_info with 2x uint32_t and that's
good enough? (it covers the ranges necessary and makes it platform compatible)

II. While we are tthis so with above uid_t of up to being u32, possibly
`volatile int ProcDieSenderUid/Pid` should be also bigger like uint32_t?
My fixup-patch does not incude it, because I don't know really.

III. As Chao said I've used:

+#define PG_SIG_DFL (pqsigfunc) (pg_funcptr_t) SIG_DFL
+#define PG_SIG_IGN (pqsigfunc) (pg_funcptr_t) SIG_IGN

IV.  Then just got lots of warnings for use_wrapper/wrapp_handler and so on

[09:27:33.602] ../src/port/pqsignal.c(206): error C2065:
'use_wrapper': undeclared identifier
[09:27:33.602] ../src/port/pqsignal.c(206): warning C4113: 'pqsigfunc'
differs in parameter lists from 'void (__cdecl *)(int)'

that's for:
   204  #else
   205          /* Forward to Windows native signal system. */
   206          if (signal(signo, use_wrapper ? wrapper_handler :
func) == SIG_ERR)
   207                  Assert(false);                  /* probably
indicates coding error */

In the end, I've ended up using wrapper_handler for the windows path there
as signal() requires function with single param.

IV. Got some further issues, and VC complained that siginfo_t is used for
USE_SIGACTION - isn't it impossible on win32? Anyway, that makes some sense,
USE_SIGINFO is not defined and we use 'siginfo_t', so something like fixes it:

@@ -90,7 +90,7 @@ static volatile pqsigfunc pqsignal_handlers[PG_NSIG];
  *
  * This wrapper also handles restoring the value of errno.
  */
-#ifdef USE_SIGACTION
+#if defined(USE_SIGACTION) && defined(USE_SIGINFO)
 static void
 wrapper_handler(int postgres_signal_arg, siginfo_t *info, void *context)
 #else

V. Later I've stumbled on series of other problems related to
src/backend/port/win32/signal.c (it also uses SIG_DFL but not PG_SIG_DFL and
stil somewhat references pgsigfunc, so those changes seemed to impact it).

Also there was:
[10:37:02.824] ../src/backend/port/win32/signal.c(154): error C2198:
'sig': too few arguments for call

so I've fixed with adding nodata struct there and:
@@ -151,7 +154,7 @@ pgwin32_dispatch_queued_signals(void)
                                                block_mask |= sigmask(i);

                                        sigprocmask(SIG_BLOCK,
&block_mask, &save_mask);
-                                       sig(i);
+                                       sig(i, &nodata);
                                        sigprocmask(SIG_SETMASK,
&save_mask, NULL);

VI. Possibly we could rename USE_SIGACTION define because at least to me it
is confusing to me to reason and communicate about in terms of win32 context
on win32 do we do have it or not? (it can be both ways):
* win32 C API doesnt have it
* PG does have sigaction win32 wrapper with override macro
  #define sigaction.. pqsigaction..
* however src/include/libpq/pqsignal.h says "sa_sigaction not yet implemented"
  for it (so with USE_SIGACTION are we talking about sa_sigaction field memeber
  that it's not used or about sigaction function?)

VII. FWIW, I've also removed superflous "else"
 #ifdef USE_SIGINFO
        if (!(is_ign || is_dfl))
        {
                act.sa_sigaction = wrapper_handler;
                act.sa_flags |= SA_SIGINFO;
        }
-       else
 #else


-J.

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

Andrew Dunstan <andrew@dunslane.net> — 2026-04-14T20:38:37Z

On 2026-04-14 Tu 6:40 AM, Jakub Wartak wrote:
> Hi Andres, Andrew, Chao
>
> thanks for putting so much effort into enhancing the the previous implementation
> of this code. Earlier I was not aware of potential problems involved.
>
> On Fri, Apr 10, 2026 at 9:41 AM Chao Li <li.evan.chao@gmail.com> wrote:
>>
>>
>>> On Apr 9, 2026, at 18:59, Andrew Dunstan <andrew@dunslane.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.
>>>
>> 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.
>>
> TL;DR; win/mingw is really unhappy with the state of rework patch right now.
> I've tried to enhance and fix it, and now it's green for default CI run and
> also for mingw too. Attached 0002-fixup-win32 patch does not incorporate Chao's
> all findings so far.
>
> [..]
>> 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.
> [..]
>> 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.
>>
>
> I. I've started from this and I vaguley remembered that I had terrible
> experience
> when trying to chose the proper types for those field types, but I couldn't
> remind myself why, so I've gave a try of the current rework patch and got this
> on Windows Server 2022, vs2019, cirrus-ci said:
>
> [08:40:22.848] c:\cirrus\src\include\c.h(1451): error C2061: syntax
> error: identifier 'pid_t'
> [08:40:22.848] c:\cirrus\src\include\c.h(1452): error C2061: syntax
> error: identifier 'uid'
> [08:40:22.848] c:\cirrus\src\include\c.h(1452): error C2059: syntax error: ';'
> [08:40:22.848] c:\cirrus\src\include\c.h(1453): error C2059: syntax error: '}'
>
> for c.h:
>    1449  typedef struct pg_signal_info
>    1450  {
>    1451          pid_t           pid;                    /* pid of
> sending process or 0 if unknown */
>    1452          uid_t           uid;                    /* uid of
> sending process or 0 if unknown */
>    1453  } pg_signal_info;
>
> so maybe we should just move that typedef with SIGNAL_ARGS to after
> "include of port.h" (line ~1471) in that c.h, because only then we'll have
> access to:
>      src/include/port/win32_port.h:typedef int pid_t;
>      src/include/port/win32_port.h:typedef int uid_t;
> but the problem is that port.h itself requires SIGNAL_ARGS to be defined
> and that seems to be like chicken and egg problem.  I thought that just
> using native "ints" could be the way to go, but the problem is that now
> that uid_t can be bigger than pid_t as Linux kernel headers show this:
>
> x86_64-linux-gnu/bits/types.h:#define   __S32_TYPE              int
> x86_64-linux-gnu/bits/types.h:#define __U32_TYPE                unsigned int
> x86_64-linux-gnu/bits/types.h:__STD_TYPE __UID_T_TYPE __uid_t;  /*
> Type of user identifications.  */
> x86_64-linux-gnu/bits/types.h:__STD_TYPE __PID_T_TYPE __pid_t;  /*
> Type of process identifications.  */
> x86_64-linux-gnu/bits/typesizes.h:#define __UID_T_TYPE          __U32_TYPE
> x86_64-linux-gnu/bits/typesizes.h:#define __PID_T_TYPE          __S32_TYPE
>
> so maybe do that typedef struct pg_signal_info with 2x uint32_t and that's
> good enough? (it covers the ranges necessary and makes it platform compatible)
>
> II. While we are tthis so with above uid_t of up to being u32, possibly
> `volatile int ProcDieSenderUid/Pid` should be also bigger like uint32_t?
> My fixup-patch does not incude it, because I don't know really.
>
> III. As Chao said I've used:
>
> +#define PG_SIG_DFL (pqsigfunc) (pg_funcptr_t) SIG_DFL
> +#define PG_SIG_IGN (pqsigfunc) (pg_funcptr_t) SIG_IGN
>
> IV.  Then just got lots of warnings for use_wrapper/wrapp_handler and so on
>
> [09:27:33.602] ../src/port/pqsignal.c(206): error C2065:
> 'use_wrapper': undeclared identifier
> [09:27:33.602] ../src/port/pqsignal.c(206): warning C4113: 'pqsigfunc'
> differs in parameter lists from 'void (__cdecl *)(int)'
>
> that's for:
>     204  #else
>     205          /* Forward to Windows native signal system. */
>     206          if (signal(signo, use_wrapper ? wrapper_handler :
> func) == SIG_ERR)
>     207                  Assert(false);                  /* probably
> indicates coding error */
>
> In the end, I've ended up using wrapper_handler for the windows path there
> as signal() requires function with single param.
>
> IV. Got some further issues, and VC complained that siginfo_t is used for
> USE_SIGACTION - isn't it impossible on win32? Anyway, that makes some sense,
> USE_SIGINFO is not defined and we use 'siginfo_t', so something like fixes it:
>
> @@ -90,7 +90,7 @@ static volatile pqsigfunc pqsignal_handlers[PG_NSIG];
>    *
>    * This wrapper also handles restoring the value of errno.
>    */
> -#ifdef USE_SIGACTION
> +#if defined(USE_SIGACTION) && defined(USE_SIGINFO)
>   static void
>   wrapper_handler(int postgres_signal_arg, siginfo_t *info, void *context)
>   #else
>
> V. Later I've stumbled on series of other problems related to
> src/backend/port/win32/signal.c (it also uses SIG_DFL but not PG_SIG_DFL and
> stil somewhat references pgsigfunc, so those changes seemed to impact it).
>
> Also there was:
> [10:37:02.824] ../src/backend/port/win32/signal.c(154): error C2198:
> 'sig': too few arguments for call
>
> so I've fixed with adding nodata struct there and:
> @@ -151,7 +154,7 @@ pgwin32_dispatch_queued_signals(void)
>                                                  block_mask |= sigmask(i);
>
>                                          sigprocmask(SIG_BLOCK,
> &block_mask, &save_mask);
> -                                       sig(i);
> +                                       sig(i, &nodata);
>                                          sigprocmask(SIG_SETMASK,
> &save_mask, NULL);
>
> VI. Possibly we could rename USE_SIGACTION define because at least to me it
> is confusing to me to reason and communicate about in terms of win32 context
> on win32 do we do have it or not? (it can be both ways):
> * win32 C API doesnt have it
> * PG does have sigaction win32 wrapper with override macro
>    #define sigaction.. pqsigaction..
> * however src/include/libpq/pqsignal.h says "sa_sigaction not yet implemented"
>    for it (so with USE_SIGACTION are we talking about sa_sigaction field memeber
>    that it's not used or about sigaction function?)
>
> VII. FWIW, I've also removed superflous "else"
>   #ifdef USE_SIGINFO
>          if (!(is_ign || is_dfl))
>          {
>                  act.sa_sigaction = wrapper_handler;
>                  act.sa_flags |= SA_SIGINFO;
>          }
> -       else
>   #else
>
>


I'm not 100% sure that else shouldn't be there. Maybe have another look?

Attached is a consolidation that includes your other fixes, plus:

. uid comment -- "only meaningful when pid is not 0"
. const pg_signal_info *pg_siginfo in SIGNAL_ARGS
. 2 spaces after period in syncrep.c errdetail


cheers


andrew



--
Andrew Dunstan
EDB: https://www.enterprisedb.com

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

Chao Li <li.evan.chao@gmail.com> — 2026-04-15T06:11:46Z


> On Apr 15, 2026, at 04:38, Andrew Dunstan <andrew@dunslane.net> wrote:
> 
> 
> On 2026-04-14 Tu 6:40 AM, Jakub Wartak wrote:
>> Hi Andres, Andrew, Chao
>> 
>> thanks for putting so much effort into enhancing the the previous implementation
>> of this code. Earlier I was not aware of potential problems involved.
>> 
>> On Fri, Apr 10, 2026 at 9:41 AM Chao Li <li.evan.chao@gmail.com> wrote:
>>> 
>>> 
>>>> On Apr 9, 2026, at 18:59, Andrew Dunstan <andrew@dunslane.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.
>>>> 
>>> 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.
>>> 
>> TL;DR; win/mingw is really unhappy with the state of rework patch right now.
>> I've tried to enhance and fix it, and now it's green for default CI run and
>> also for mingw too. Attached 0002-fixup-win32 patch does not incorporate Chao's
>> all findings so far.
>> 
>> [..]
>>> 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.
>> [..]
>>> 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.
>>> 
>> 
>> I. I've started from this and I vaguley remembered that I had terrible
>> experience
>> when trying to chose the proper types for those field types, but I couldn't
>> remind myself why, so I've gave a try of the current rework patch and got this
>> on Windows Server 2022, vs2019, cirrus-ci said:
>> 
>> [08:40:22.848] c:\cirrus\src\include\c.h(1451): error C2061: syntax
>> error: identifier 'pid_t'
>> [08:40:22.848] c:\cirrus\src\include\c.h(1452): error C2061: syntax
>> error: identifier 'uid'
>> [08:40:22.848] c:\cirrus\src\include\c.h(1452): error C2059: syntax error: ';'
>> [08:40:22.848] c:\cirrus\src\include\c.h(1453): error C2059: syntax error: '}'
>> 
>> for c.h:
>>   1449  typedef struct pg_signal_info
>>   1450  {
>>   1451          pid_t           pid;                    /* pid of
>> sending process or 0 if unknown */
>>   1452          uid_t           uid;                    /* uid of
>> sending process or 0 if unknown */
>>   1453  } pg_signal_info;
>> 
>> so maybe we should just move that typedef with SIGNAL_ARGS to after
>> "include of port.h" (line ~1471) in that c.h, because only then we'll have
>> access to:
>>     src/include/port/win32_port.h:typedef int pid_t;
>>     src/include/port/win32_port.h:typedef int uid_t;
>> but the problem is that port.h itself requires SIGNAL_ARGS to be defined
>> and that seems to be like chicken and egg problem.  I thought that just
>> using native "ints" could be the way to go, but the problem is that now
>> that uid_t can be bigger than pid_t as Linux kernel headers show this:
>> 
>> x86_64-linux-gnu/bits/types.h:#define   __S32_TYPE              int
>> x86_64-linux-gnu/bits/types.h:#define __U32_TYPE                unsigned int
>> x86_64-linux-gnu/bits/types.h:__STD_TYPE __UID_T_TYPE __uid_t;  /*
>> Type of user identifications.  */
>> x86_64-linux-gnu/bits/types.h:__STD_TYPE __PID_T_TYPE __pid_t;  /*
>> Type of process identifications.  */
>> x86_64-linux-gnu/bits/typesizes.h:#define __UID_T_TYPE          __U32_TYPE
>> x86_64-linux-gnu/bits/typesizes.h:#define __PID_T_TYPE          __S32_TYPE
>> 
>> so maybe do that typedef struct pg_signal_info with 2x uint32_t and that's
>> good enough? (it covers the ranges necessary and makes it platform compatible)
>> 
>> II. While we are tthis so with above uid_t of up to being u32, possibly
>> `volatile int ProcDieSenderUid/Pid` should be also bigger like uint32_t?
>> My fixup-patch does not incude it, because I don't know really.
>> 
>> III. As Chao said I've used:
>> 
>> +#define PG_SIG_DFL (pqsigfunc) (pg_funcptr_t) SIG_DFL
>> +#define PG_SIG_IGN (pqsigfunc) (pg_funcptr_t) SIG_IGN
>> 
>> IV.  Then just got lots of warnings for use_wrapper/wrapp_handler and so on
>> 
>> [09:27:33.602] ../src/port/pqsignal.c(206): error C2065:
>> 'use_wrapper': undeclared identifier
>> [09:27:33.602] ../src/port/pqsignal.c(206): warning C4113: 'pqsigfunc'
>> differs in parameter lists from 'void (__cdecl *)(int)'
>> 
>> that's for:
>>    204  #else
>>    205          /* Forward to Windows native signal system. */
>>    206          if (signal(signo, use_wrapper ? wrapper_handler :
>> func) == SIG_ERR)
>>    207                  Assert(false);                  /* probably
>> indicates coding error */
>> 
>> In the end, I've ended up using wrapper_handler for the windows path there
>> as signal() requires function with single param.
>> 
>> IV. Got some further issues, and VC complained that siginfo_t is used for
>> USE_SIGACTION - isn't it impossible on win32? Anyway, that makes some sense,
>> USE_SIGINFO is not defined and we use 'siginfo_t', so something like fixes it:
>> 
>> @@ -90,7 +90,7 @@ static volatile pqsigfunc pqsignal_handlers[PG_NSIG];
>>   *
>>   * This wrapper also handles restoring the value of errno.
>>   */
>> -#ifdef USE_SIGACTION
>> +#if defined(USE_SIGACTION) && defined(USE_SIGINFO)
>>  static void
>>  wrapper_handler(int postgres_signal_arg, siginfo_t *info, void *context)
>>  #else
>> 
>> V. Later I've stumbled on series of other problems related to
>> src/backend/port/win32/signal.c (it also uses SIG_DFL but not PG_SIG_DFL and
>> stil somewhat references pgsigfunc, so those changes seemed to impact it).
>> 
>> Also there was:
>> [10:37:02.824] ../src/backend/port/win32/signal.c(154): error C2198:
>> 'sig': too few arguments for call
>> 
>> so I've fixed with adding nodata struct there and:
>> @@ -151,7 +154,7 @@ pgwin32_dispatch_queued_signals(void)
>>                                                 block_mask |= sigmask(i);
>> 
>>                                         sigprocmask(SIG_BLOCK,
>> &block_mask, &save_mask);
>> -                                       sig(i);
>> +                                       sig(i, &nodata);
>>                                         sigprocmask(SIG_SETMASK,
>> &save_mask, NULL);
>> 
>> VI. Possibly we could rename USE_SIGACTION define because at least to me it
>> is confusing to me to reason and communicate about in terms of win32 context
>> on win32 do we do have it or not? (it can be both ways):
>> * win32 C API doesnt have it
>> * PG does have sigaction win32 wrapper with override macro
>>   #define sigaction.. pqsigaction..
>> * however src/include/libpq/pqsignal.h says "sa_sigaction not yet implemented"
>>   for it (so with USE_SIGACTION are we talking about sa_sigaction field memeber
>>   that it's not used or about sigaction function?)
>> 
>> VII. FWIW, I've also removed superflous "else"
>>  #ifdef USE_SIGINFO
>>         if (!(is_ign || is_dfl))
>>         {
>>                 act.sa_sigaction = wrapper_handler;
>>                 act.sa_flags |= SA_SIGINFO;
>>         }
>> -       else
>>  #else
>> 
>> 
> 
> 
> I'm not 100% sure that else shouldn't be there. Maybe have another look?
> 
> Attached is a consolidation that includes your other fixes, plus:
> 
> . uid comment -- "only meaningful when pid is not 0"
> . const pg_signal_info *pg_siginfo in SIGNAL_ARGS
> . 2 spaces after period in syncrep.c errdetail
> 
> 
> cheers
> 
> 
> andrew
> 
> 
> 
> --
> Andrew Dunstan
> EDB: https://www.enterprisedb.com
> <v2-0001-Rework-signal-handler-infrastructure-to-pass-send.patch>

V2 LGTM.

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

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

Jakub Wartak <jakub.wartak@enterprisedb.com> — 2026-04-15T09:27:29Z

On Tue, Apr 14, 2026 at 10:38 PM Andrew Dunstan <andrew@dunslane.net> wrote:
>
>
> On 2026-04-14 Tu 6:40 AM, Jakub Wartak wrote:
> > Hi Andres, Andrew, Chao
> >
> > thanks for putting so much effort into enhancing the the previous implementation
> > of this code. Earlier I was not aware of potential problems involved.
> >
> > On Fri, Apr 10, 2026 at 9:41 AM Chao Li <li.evan.chao@gmail.com> wrote:
> >>
> >>
> >>> On Apr 9, 2026, at 18:59, Andrew Dunstan <andrew@dunslane.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.
> >>>
> >> 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.
> >>
> > TL;DR; win/mingw is really unhappy with the state of rework patch right now.
> > I've tried to enhance and fix it, and now it's green for default CI run and
> > also for mingw too. Attached 0002-fixup-win32 patch does not incorporate Chao's
> > all findings so far.
> >
> > [..]
> >> 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.
> > [..]
> >> 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.
> >>
> >
> > I. I've started from this and I vaguley remembered that I had terrible
> > experience
> > when trying to chose the proper types for those field types, but I couldn't
> > remind myself why, so I've gave a try of the current rework patch and got this
> > on Windows Server 2022, vs2019, cirrus-ci said:
> >
> > [08:40:22.848] c:\cirrus\src\include\c.h(1451): error C2061: syntax
> > error: identifier 'pid_t'
> > [08:40:22.848] c:\cirrus\src\include\c.h(1452): error C2061: syntax
> > error: identifier 'uid'
> > [08:40:22.848] c:\cirrus\src\include\c.h(1452): error C2059: syntax error: ';'
> > [08:40:22.848] c:\cirrus\src\include\c.h(1453): error C2059: syntax error: '}'
> >
> > for c.h:
> >    1449  typedef struct pg_signal_info
> >    1450  {
> >    1451          pid_t           pid;                    /* pid of
> > sending process or 0 if unknown */
> >    1452          uid_t           uid;                    /* uid of
> > sending process or 0 if unknown */
> >    1453  } pg_signal_info;
> >
> > so maybe we should just move that typedef with SIGNAL_ARGS to after
> > "include of port.h" (line ~1471) in that c.h, because only then we'll have
> > access to:
> >      src/include/port/win32_port.h:typedef int pid_t;
> >      src/include/port/win32_port.h:typedef int uid_t;
> > but the problem is that port.h itself requires SIGNAL_ARGS to be defined
> > and that seems to be like chicken and egg problem.  I thought that just
> > using native "ints" could be the way to go, but the problem is that now
> > that uid_t can be bigger than pid_t as Linux kernel headers show this:
> >
> > x86_64-linux-gnu/bits/types.h:#define   __S32_TYPE              int
> > x86_64-linux-gnu/bits/types.h:#define __U32_TYPE                unsigned int
> > x86_64-linux-gnu/bits/types.h:__STD_TYPE __UID_T_TYPE __uid_t;  /*
> > Type of user identifications.  */
> > x86_64-linux-gnu/bits/types.h:__STD_TYPE __PID_T_TYPE __pid_t;  /*
> > Type of process identifications.  */
> > x86_64-linux-gnu/bits/typesizes.h:#define __UID_T_TYPE          __U32_TYPE
> > x86_64-linux-gnu/bits/typesizes.h:#define __PID_T_TYPE          __S32_TYPE
> >
> > so maybe do that typedef struct pg_signal_info with 2x uint32_t and that's
> > good enough? (it covers the ranges necessary and makes it platform compatible)
> >
> > II. While we are tthis so with above uid_t of up to being u32, possibly
> > `volatile int ProcDieSenderUid/Pid` should be also bigger like uint32_t?
> > My fixup-patch does not incude it, because I don't know really.
> >
> > III. As Chao said I've used:
> >
> > +#define PG_SIG_DFL (pqsigfunc) (pg_funcptr_t) SIG_DFL
> > +#define PG_SIG_IGN (pqsigfunc) (pg_funcptr_t) SIG_IGN
> >
> > IV.  Then just got lots of warnings for use_wrapper/wrapp_handler and so on
> >
> > [09:27:33.602] ../src/port/pqsignal.c(206): error C2065:
> > 'use_wrapper': undeclared identifier
> > [09:27:33.602] ../src/port/pqsignal.c(206): warning C4113: 'pqsigfunc'
> > differs in parameter lists from 'void (__cdecl *)(int)'
> >
> > that's for:
> >     204  #else
> >     205          /* Forward to Windows native signal system. */
> >     206          if (signal(signo, use_wrapper ? wrapper_handler :
> > func) == SIG_ERR)
> >     207                  Assert(false);                  /* probably
> > indicates coding error */
> >
> > In the end, I've ended up using wrapper_handler for the windows path there
> > as signal() requires function with single param.
> >
> > IV. Got some further issues, and VC complained that siginfo_t is used for
> > USE_SIGACTION - isn't it impossible on win32? Anyway, that makes some sense,
> > USE_SIGINFO is not defined and we use 'siginfo_t', so something like fixes it:
> >
> > @@ -90,7 +90,7 @@ static volatile pqsigfunc pqsignal_handlers[PG_NSIG];
> >    *
> >    * This wrapper also handles restoring the value of errno.
> >    */
> > -#ifdef USE_SIGACTION
> > +#if defined(USE_SIGACTION) && defined(USE_SIGINFO)
> >   static void
> >   wrapper_handler(int postgres_signal_arg, siginfo_t *info, void *context)
> >   #else
> >
> > V. Later I've stumbled on series of other problems related to
> > src/backend/port/win32/signal.c (it also uses SIG_DFL but not PG_SIG_DFL and
> > stil somewhat references pgsigfunc, so those changes seemed to impact it).
> >
> > Also there was:
> > [10:37:02.824] ../src/backend/port/win32/signal.c(154): error C2198:
> > 'sig': too few arguments for call
> >
> > so I've fixed with adding nodata struct there and:
> > @@ -151,7 +154,7 @@ pgwin32_dispatch_queued_signals(void)
> >                                                  block_mask |= sigmask(i);
> >
> >                                          sigprocmask(SIG_BLOCK,
> > &block_mask, &save_mask);
> > -                                       sig(i);
> > +                                       sig(i, &nodata);
> >                                          sigprocmask(SIG_SETMASK,
> > &save_mask, NULL);
> >
> > VI. Possibly we could rename USE_SIGACTION define because at least to me it
> > is confusing to me to reason and communicate about in terms of win32 context
> > on win32 do we do have it or not? (it can be both ways):
> > * win32 C API doesnt have it
> > * PG does have sigaction win32 wrapper with override macro
> >    #define sigaction.. pqsigaction..
> > * however src/include/libpq/pqsignal.h says "sa_sigaction not yet implemented"
> >    for it (so with USE_SIGACTION are we talking about sa_sigaction field memeber
> >    that it's not used or about sigaction function?)
> >
> > VII. FWIW, I've also removed superflous "else"
> >   #ifdef USE_SIGINFO
> >          if (!(is_ign || is_dfl))
> >          {
> >                  act.sa_sigaction = wrapper_handler;
> >                  act.sa_flags |= SA_SIGINFO;
> >          }
> > -       else
> >   #else
> >
> >
>
>
> I'm not 100% sure that else shouldn't be there. Maybe have another look?


So in 0001 we had pqsignal() like below:

```
#ifdef USE_SIGINFO
        if (!(is_ign || is_dfl))
        {
                act.sa_sigaction = wrapper_handler;
                act.sa_flags |= SA_SIGINFO;
        }
        else // <--- this is the problematic line that I wanted to remove
#else
        else
                act.sa_handler = wrapper_handler;
#endif

#ifdef SA_NOCLDSTOP
        if (signo == SIGCHLD)
                act.sa_flags |= SA_NOCLDSTOP;
#endif
```
(in latest v2-0001 it seems to be refactored and looks more readable to me)

IMHO it was superflous for the following reasons:
a) previous releases did not have such condition (flow went unconditially to
   `if (signo == SIGCHLD)` and not based on function spec. in a lot of places we
   call `pqsignal(SIGCHLD, PG_SIG_DFL)` but in PostmasterMain() we call
   `pqsignal(SIGCHLD, handle_pm_child_exit_signal);` so we should set
   SA_NOCLDSTOP in both cases not just one.

b) if the platform wouldn't have SA_NOCLDSTOP, so stil with USE_SIGACTION (it
   would have to be basically non POSIX.1-1990, but not WIN32, so impossible?),
   that would shift the code flow to later lines (sigaction() itself) so
   that would be a bug

I would say I would even propose removal of #ifdef SA_NOCLDSTOP and assume it's
always there, as personally I hate such macro complexity.

BTW tiny nitpick to myself:
--- a/src/port/pqsignal.c
+++ b/src/port/pqsignal.c
@@ -206,7 +206,7 @@ pqsignal(int signo, pqsigfunc func)
         * Forward to Windows native signal system, we need to send this though
         * wrapper handler as it it needs to take single argument only.
         */
-       if(is_ign)
+       if (is_ign)


> Attached is a consolidation that includes your other fixes, plus:
>
> . uid comment -- "only meaningful when pid is not 0"
> . const pg_signal_info *pg_siginfo in SIGNAL_ARGS
> . 2 spaces after period in syncrep.c errdetail

v2-0001 LGTM.

-J.

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

Andrew Dunstan <andrew@dunslane.net> — 2026-04-15T11:37:50Z

On 2026-04-15 We 5:27 AM, Jakub Wartak wrote:
> v2-0001 LGTM.
>

OK, pushed. Thanks.


cheers


andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

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

Kirill Reshke <reshkekirill@gmail.com> — 2026-04-15T11:45:28Z

I am seeing 3e2a1496bae628c379ca0a11ef5f5ba666f24ae8 merged into
master today, and I am a bit surprised.

This is a feature for my taste and the feature freeze was on 8th April
12 UTC. Am I wrong?

-- 
Best regards,
Kirill Reshke

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

Andrew Dunstan <andrew@dunslane.net> — 2026-04-15T12:14:52Z

On 2026-04-15 We 7:45 AM, Kirill Reshke wrote:
> I am seeing 3e2a1496bae628c379ca0a11ef5f5ba666f24ae8 merged into
> master today, and I am a bit surprised.
>
> This is a feature for my taste and the feature freeze was on 8th April
> 12 UTC. Am I wrong?
>


It's a fix for a feature that was committed before feature freeze.


cheers


andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

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

Kirill Reshke <reshkekirill@gmail.com> — 2026-04-15T12:45:01Z

On Wed, 15 Apr 2026 at 17:14, Andrew Dunstan <andrew@dunslane.net> wrote:
>
>
> On 2026-04-15 We 7:45 AM, Kirill Reshke wrote:
> > I am seeing 3e2a1496bae628c379ca0a11ef5f5ba666f24ae8 merged into
> > master today, and I am a bit surprised.
> >
> > This is a feature for my taste and the feature freeze was on 8th April
> > 12 UTC. Am I wrong?
> >
>
>
> It's a fix for a feature that was committed before feature freeze.
>
>
> cheers
>
>
> andrew
>
> --
> Andrew Dunstan
> EDB: https://www.enterprisedb.com


Ok, sorry


-- 
Best regards,
Kirill Reshke

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

Tom Lane <tgl@sss.pgh.pa.us> — 2026-04-15T14:37:32Z

Andrew Dunstan <andrew@dunslane.net> writes:
> OK, pushed. Thanks.

The OpenBSD members of the buildfarm don't seem to like this.

			regards, tom lane

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

Andrew Dunstan <andrew@dunslane.net> — 2026-04-15T15:26:18Z

On 2026-04-15 We 10:37 AM, Tom Lane wrote:
> Andrew Dunstan <andrew@dunslane.net> writes:
>> OK, pushed. Thanks.
> The OpenBSD members of the buildfarm don't seem to like this.
>
> 			


Ugh.


I'm will take a look later today.


cheers


andrew


--
Andrew Dunstan
EDB: https://www.enterprisedb.com

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

Tom Lane <tgl@sss.pgh.pa.us> — 2026-04-15T16:04:12Z

Andrew Dunstan <andrew@dunslane.net> writes:
> On 2026-04-15 We 10:37 AM, Tom Lane wrote:
>> The OpenBSD members of the buildfarm don't seem to like this.

> Ugh.
> I'm will take a look later today.

I reproduced it locally on OpenBSD 7.7.  HAVE_SA_SIGINFO is defined,
and the code to grab the pid/uid out of siginfo_t is definitely
getting compiled.  As best I can tell, the kernel is simply passing
zero for info->si_pid and si_uid.  This does not match up with the
info available on the net, so I'm not sure what the issue is.

Some googling suggested that on some platforms si_pid will be zero
if the process signaled itself, but I can eliminate that theory:
it's still zero if I do the pg_terminate_backend() from another
session.

As a short-term fix, we could just go back to allowing the regex to
consider the match optional.

			regards, tom lane

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

Jacob Champion <jacob.champion@enterprisedb.com> — 2026-04-15T16:23:14Z

On Wed, Apr 15, 2026 at 7:17 AM Andrew Dunstan <andrew@dunslane.net> wrote:
> OK, pushed. Thanks.

I hit the following in the pg_basebackup tests just now, running on Linux:

    [08:41:21.621](0.377s) ok 196 - Walsender killed
    [09:09:11.134](1669.513s) # pump_until: timeout expired when
searching for "(?^:background process terminated unexpectedly)" with
stream: "pg_basebackup: error: unexpected termination of replication
stream: FATAL:  terminating connection due to administrator command
    # DETAIL:  Signal sent by PID 155573, UID 1000.
    # "
    [09:09:11.134](0.000s) not ok 197 - background process exit message
    [09:09:11.134](0.000s) #   Failed test 'background process exit message'
    #   at src/postgres/src/bin/pg_basebackup/t/010_pg_basebackup.pl line 1049.

But I haven't been able to reproduce since, so I don't know if this is
a new race, or the commit just exposed one that was there before?

Thanks,
--Jacob

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

Andrew Dunstan <andrew@dunslane.net> — 2026-04-15T17:20:03Z

On 2026-04-15 We 12:04 PM, Tom Lane wrote:
> Andrew Dunstan <andrew@dunslane.net> writes:
>> On 2026-04-15 We 10:37 AM, Tom Lane wrote:
>>> The OpenBSD members of the buildfarm don't seem to like this.
>> Ugh.
>> I'm will take a look later today.
> I reproduced it locally on OpenBSD 7.7.  HAVE_SA_SIGINFO is defined,
> and the code to grab the pid/uid out of siginfo_t is definitely
> getting compiled.  As best I can tell, the kernel is simply passing
> zero for info->si_pid and si_uid.  This does not match up with the
> info available on the net, so I'm not sure what the issue is.
>
> Some googling suggested that on some platforms si_pid will be zero
> if the process signaled itself, but I can eliminate that theory:
> it's still zero if I do the pg_terminate_backend() from another
> session.
>
> As a short-term fix, we could just go back to allowing the regex to
> consider the match optional.
>
> 			


Ok, so we can get the buildfarm green I'll go and do that. But I think 
we should have an open item to tighten the test.


cheers


andrew


--
Andrew Dunstan
EDB: https://www.enterprisedb.com

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

Tom Lane <tgl@sss.pgh.pa.us> — 2026-04-15T18:49:06Z

Andrew Dunstan <andrew@dunslane.net> writes:
> On 2026-04-15 We 12:04 PM, Tom Lane wrote:
>> As a short-term fix, we could just go back to allowing the regex to
>> consider the match optional.

> Ok, so we can get the buildfarm green I'll go and do that. But I think 
> we should have an open item to tighten the test.

I did some more digging, and got this from Google's AI Mode:

-----
openbsd does not fill siginfo_t si_pid for SIGTERM

On OpenBSD, si_pid is indeed not guaranteed to be filled for SIGTERM
(and many other signals), even when using SA_SIGINFO. This is a known
architectural behavior of the OpenBSD kernel rather than a bug. 

Why si_pid is zero or empty

Minimalist Kernel Design: Unlike Linux, which often populates si_pid
and si_uid for most user-sent signals, the OpenBSD kernel only
guarantees these fields for specific signals where they are
functionally required by POSIX, such as SIGCHLD.

Security & Information Leakage: OpenBSD has a history of limiting
information available across process boundaries to prevent
side-channel attacks or unnecessary information leaks about other
processes on the system [0.31].

Signal Queueing: Standard signals like SIGTERM are not "queued" with
data in the same way real-time signals (which OpenBSD does not fully
support in the same manner as Linux) would be.
-----

Now, none of the links it provided in support of these claims say
any such thing AFAICS, so maybe this is all an AI hallucination.
We could probably look into the OpenBSD kernel to check it, if we
were sufficiently motivated.  But I'm inclined to believe it and
just say "this info is not available on all platforms, even some
that HAVE_SA_SIGINFO".

			regards, tom lane

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

Andrew Dunstan <andrew@dunslane.net> — 2026-04-15T19:39:21Z

On 2026-04-15 We 2:49 PM, Tom Lane wrote:
> Andrew Dunstan<andrew@dunslane.net> writes:
>> On 2026-04-15 We 12:04 PM, Tom Lane wrote:
>>> As a short-term fix, we could just go back to allowing the regex to
>>> consider the match optional.
>> Ok, so we can get the buildfarm green I'll go and do that. But I think
>> we should have an open item to tighten the test.
> I did some more digging, and got this from Google's AI Mode:
>
> -----
> openbsd does not fill siginfo_t si_pid for SIGTERM
>
> On OpenBSD, si_pid is indeed not guaranteed to be filled for SIGTERM
> (and many other signals), even when using SA_SIGINFO. This is a known
> architectural behavior of the OpenBSD kernel rather than a bug.
>
> Why si_pid is zero or empty
>
> Minimalist Kernel Design: Unlike Linux, which often populates si_pid
> and si_uid for most user-sent signals, the OpenBSD kernel only
> guarantees these fields for specific signals where they are
> functionally required by POSIX, such as SIGCHLD.
>
> Security & Information Leakage: OpenBSD has a history of limiting
> information available across process boundaries to prevent
> side-channel attacks or unnecessary information leaks about other
> processes on the system [0.31].
>
> Signal Queueing: Standard signals like SIGTERM are not "queued" with
> data in the same way real-time signals (which OpenBSD does not fully
> support in the same manner as Linux) would be.
> -----
>
> Now, none of the links it provided in support of these claims say
> any such thing AFAICS, so maybe this is all an AI hallucination.
> We could probably look into the OpenBSD kernel to check it, if we
> were sufficiently motivated.  But I'm inclined to believe it and
> just say "this info is not available on all platforms, even some
> that HAVE_SA_SIGINFO".
>
> 			



Thanks for looking into this. I guess we could make a test to see what 
the platform will support, but it seems like overkill. So now I'm just 
inclined to go back to making the line completely optional in the test 
and leave it at that.


cheers


andrew

--
Andrew Dunstan
EDB:https://www.enterprisedb.com

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

Jakub Wartak <jakub.wartak@enterprisedb.com> — 2026-04-16T05:50:25Z

On Wed, Apr 15, 2026 at 6:23 PM Jacob Champion
<jacob.champion@enterprisedb.com> wrote:
>
> On Wed, Apr 15, 2026 at 7:17 AM Andrew Dunstan <andrew@dunslane.net> wrote:
> > OK, pushed. Thanks.
>
> I hit the following in the pg_basebackup tests just now, running on Linux:
>
>     [08:41:21.621](0.377s) ok 196 - Walsender killed
>     [09:09:11.134](1669.513s) # pump_until: timeout expired when
> searching for "(?^:background process terminated unexpectedly)" with
> stream: "pg_basebackup: error: unexpected termination of replication
> stream: FATAL:  terminating connection due to administrator command
>     # DETAIL:  Signal sent by PID 155573, UID 1000.
>     # "
>     [09:09:11.134](0.000s) not ok 197 - background process exit message
>     [09:09:11.134](0.000s) #   Failed test 'background process exit message'
>     #   at src/postgres/src/bin/pg_basebackup/t/010_pg_basebackup.pl line 1049.
>
> But I haven't been able to reproduce since, so I don't know if this is
> a new race, or the commit just exposed one that was there before?

Hi Jacob, the time baseback took seems strange to me (27mins?!). It was
properly killed by a timeout, and the new code added the exact PID
that caused the issue.

If you happen to spot it again long running it might make some sense
to find where the time is spent there during that basebackup (in this
test we shouldn't be taking large backups).

Alternative would be to check pg server logs of that specific failed run
to see exactly where it was stuck after 08:41 (but before 09:09).

-J.

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

Jakub Wartak <jakub.wartak@enterprisedb.com> — 2026-04-16T06:08:06Z

On Wed, Apr 15, 2026 at 8:49 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Andrew Dunstan <andrew@dunslane.net> writes:
> > On 2026-04-15 We 12:04 PM, Tom Lane wrote:
> >> As a short-term fix, we could just go back to allowing the regex to
> >> consider the match optional.
>
> > Ok, so we can get the buildfarm green I'll go and do that. But I think
> > we should have an open item to tighten the test.
>
> I did some more digging, and got this from Google's AI Mode:
>
> -----
> openbsd does not fill siginfo_t si_pid for SIGTERM
>
> On OpenBSD, si_pid is indeed not guaranteed to be filled for SIGTERM
> (and many other signals), even when using SA_SIGINFO. This is a known
> architectural behavior of the OpenBSD kernel rather than a bug.
>
> Why si_pid is zero or empty
>
> Minimalist Kernel Design: Unlike Linux, which often populates si_pid
> and si_uid for most user-sent signals, the OpenBSD kernel only
> guarantees these fields for specific signals where they are
> functionally required by POSIX, such as SIGCHLD.
>
> Security & Information Leakage: OpenBSD has a history of limiting
> information available across process boundaries to prevent
> side-channel attacks or unnecessary information leaks about other
> processes on the system [0.31].
>
> Signal Queueing: Standard signals like SIGTERM are not "queued" with
> data in the same way real-time signals (which OpenBSD does not fully
> support in the same manner as Linux) would be.
> -----
>
> Now, none of the links it provided in support of these claims say
> any such thing AFAICS, so maybe this is all an AI hallucination.
> We could probably look into the OpenBSD kernel to check it, if we
> were sufficiently motivated.  But I'm inclined to believe it and
> just say "this info is not available on all platforms, even some
> that HAVE_SA_SIGINFO".

Hi Tom,

It seems to be not a hallucination: it appears that they do initsiginfo() [1]
which zeros out struct siginfo_t without setting si_pid there. The
only reference about si_pid is that their waitid(2) fills it properly
and that's visibile in their dowait*() kernel-side implementation
too.

-J.

[1] - https://github.com/openbsd/src/blob/master/sys/kern/kern_sig.c#L2166

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

Jakub Wartak <jakub.wartak@enterprisedb.com> — 2026-04-16T06:09:43Z

On Wed, Apr 15, 2026 at 9:39 PM Andrew Dunstan <andrew@dunslane.net> wrote:
>
>
> On 2026-04-15 We 2:49 PM, Tom Lane wrote:
>
> Andrew Dunstan <andrew@dunslane.net> writes:
>
> On 2026-04-15 We 12:04 PM, Tom Lane wrote:
>
> As a short-term fix, we could just go back to allowing the regex to
> consider the match optional.
>
> Ok, so we can get the buildfarm green I'll go and do that. But I think
> we should have an open item to tighten the test.
>
> I did some more digging, and got this from Google's AI Mode:
>
> -----
> openbsd does not fill siginfo_t si_pid for SIGTERM
>
> On OpenBSD, si_pid is indeed not guaranteed to be filled for SIGTERM
> (and many other signals), even when using SA_SIGINFO. This is a known
> architectural behavior of the OpenBSD kernel rather than a bug.
>
> Why si_pid is zero or empty
>
> Minimalist Kernel Design: Unlike Linux, which often populates si_pid
> and si_uid for most user-sent signals, the OpenBSD kernel only
> guarantees these fields for specific signals where they are
> functionally required by POSIX, such as SIGCHLD.
>
> Security & Information Leakage: OpenBSD has a history of limiting
> information available across process boundaries to prevent
> side-channel attacks or unnecessary information leaks about other
> processes on the system [0.31].
>
> Signal Queueing: Standard signals like SIGTERM are not "queued" with
> data in the same way real-time signals (which OpenBSD does not fully
> support in the same manner as Linux) would be.
> -----
>
> Now, none of the links it provided in support of these claims say
> any such thing AFAICS, so maybe this is all an AI hallucination.
> We could probably look into the OpenBSD kernel to check it, if we
> were sufficiently motivated.  But I'm inclined to believe it and
> just say "this info is not available on all platforms, even some
> that HAVE_SA_SIGINFO".
>
>
>
>
>
> Thanks for looking into this. I guess we could make a test to see what the platform will support, but it seems like overkill. So now I'm just inclined to go back to making the line completely optional in the test and leave it at that.
>

+1

-J.

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

Jakub Wartak <jakub.wartak@enterprisedb.com> — 2026-04-16T08:34:58Z

On Thu, Apr 16, 2026 at 8:09 AM Jakub Wartak
<jakub.wartak@enterprisedb.com> wrote:
>
> On Wed, Apr 15, 2026 at 9:39 PM Andrew Dunstan <andrew@dunslane.net> wrote:
> >
> >
> > On 2026-04-15 We 2:49 PM, Tom Lane wrote:
> >
> > Andrew Dunstan <andrew@dunslane.net> writes:
> >
> > On 2026-04-15 We 12:04 PM, Tom Lane wrote:
> >
> > As a short-term fix, we could just go back to allowing the regex to
> > consider the match optional.
> >
> > Ok, so we can get the buildfarm green I'll go and do that. But I think
> > we should have an open item to tighten the test.
> >
> > I did some more digging, and got this from Google's AI Mode:
> >
> > -----
> > openbsd does not fill siginfo_t si_pid for SIGTERM
> >
> > On OpenBSD, si_pid is indeed not guaranteed to be filled for SIGTERM
> > (and many other signals), even when using SA_SIGINFO. This is a known
> > architectural behavior of the OpenBSD kernel rather than a bug.
> >
> > Why si_pid is zero or empty
> >
> > Minimalist Kernel Design: Unlike Linux, which often populates si_pid
> > and si_uid for most user-sent signals, the OpenBSD kernel only
> > guarantees these fields for specific signals where they are
> > functionally required by POSIX, such as SIGCHLD.
> >
> > Security & Information Leakage: OpenBSD has a history of limiting
> > information available across process boundaries to prevent
> > side-channel attacks or unnecessary information leaks about other
> > processes on the system [0.31].
> >
> > Signal Queueing: Standard signals like SIGTERM are not "queued" with
> > data in the same way real-time signals (which OpenBSD does not fully
> > support in the same manner as Linux) would be.
> > -----
> >
> > Now, none of the links it provided in support of these claims say
> > any such thing AFAICS, so maybe this is all an AI hallucination.
> > We could probably look into the OpenBSD kernel to check it, if we
> > were sufficiently motivated.  But I'm inclined to believe it and
> > just say "this info is not available on all platforms, even some
> > that HAVE_SA_SIGINFO".
> >
> >
> >
> >
> >
> > Thanks for looking into this. I guess we could make a test to see what the platform will support, but it seems like overkill. So now I'm just inclined to go back to making the line completely optional in the test and leave it at that.
> >
>
> +1

And here is the patch for that.


-J.

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

Andrew Dunstan <andrew@dunslane.net> — 2026-04-16T20:58:53Z

On 2026-04-16 Th 4:34 AM, Jakub Wartak wrote:
> On Thu, Apr 16, 2026 at 8:09 AM Jakub Wartak
> <jakub.wartak@enterprisedb.com> wrote:
>> On Wed, Apr 15, 2026 at 9:39 PM Andrew Dunstan<andrew@dunslane.net> wrote:
>>>
>>> On 2026-04-15 We 2:49 PM, Tom Lane wrote:
>>>
>>> Andrew Dunstan<andrew@dunslane.net> writes:
>>>
>>> On 2026-04-15 We 12:04 PM, Tom Lane wrote:
>>>
>>> As a short-term fix, we could just go back to allowing the regex to
>>> consider the match optional.
>>>
>>> Ok, so we can get the buildfarm green I'll go and do that. But I think
>>> we should have an open item to tighten the test.
>>>
>>> I did some more digging, and got this from Google's AI Mode:
>>>
>>> -----
>>> openbsd does not fill siginfo_t si_pid for SIGTERM
>>>
>>> On OpenBSD, si_pid is indeed not guaranteed to be filled for SIGTERM
>>> (and many other signals), even when using SA_SIGINFO. This is a known
>>> architectural behavior of the OpenBSD kernel rather than a bug.
>>>
>>> Why si_pid is zero or empty
>>>
>>> Minimalist Kernel Design: Unlike Linux, which often populates si_pid
>>> and si_uid for most user-sent signals, the OpenBSD kernel only
>>> guarantees these fields for specific signals where they are
>>> functionally required by POSIX, such as SIGCHLD.
>>>
>>> Security & Information Leakage: OpenBSD has a history of limiting
>>> information available across process boundaries to prevent
>>> side-channel attacks or unnecessary information leaks about other
>>> processes on the system [0.31].
>>>
>>> Signal Queueing: Standard signals like SIGTERM are not "queued" with
>>> data in the same way real-time signals (which OpenBSD does not fully
>>> support in the same manner as Linux) would be.
>>> -----
>>>
>>> Now, none of the links it provided in support of these claims say
>>> any such thing AFAICS, so maybe this is all an AI hallucination.
>>> We could probably look into the OpenBSD kernel to check it, if we
>>> were sufficiently motivated.  But I'm inclined to believe it and
>>> just say "this info is not available on all platforms, even some
>>> that HAVE_SA_SIGINFO".
>>>
>>>
>>>
>>>
>>>
>>> Thanks for looking into this. I guess we could make a test to see what the platform will support, but it seems like overkill. So now I'm just inclined to go back to making the line completely optional in the test and leave it at that.
>>>
>> +1
> And here is the patch for that.
>
>


Thanks, I have pushed the fix.


cheers


andrew

--
Andrew Dunstan
EDB:https://www.enterprisedb.com

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

Chao Li <li.evan.chao@gmail.com> — 2026-04-23T09:27:23Z


> On Apr 17, 2026, at 04:58, Andrew Dunstan <andrew@dunslane.net> wrote:
> 
> 
> On 2026-04-16 Th 4:34 AM, Jakub Wartak wrote:
>> On Thu, Apr 16, 2026 at 8:09 AM Jakub Wartak
>> <jakub.wartak@enterprisedb.com> wrote:
>> 
>>> On Wed, Apr 15, 2026 at 9:39 PM Andrew Dunstan <andrew@dunslane.net> wrote:
>>> 
>>>> 
>>>> On 2026-04-15 We 2:49 PM, Tom Lane wrote:
>>>> 
>>>> Andrew Dunstan <andrew@dunslane.net> writes:
>>>> 
>>>> On 2026-04-15 We 12:04 PM, Tom Lane wrote:
>>>> 
>>>> As a short-term fix, we could just go back to allowing the regex to
>>>> consider the match optional.
>>>> 
>>>> Ok, so we can get the buildfarm green I'll go and do that. But I think
>>>> we should have an open item to tighten the test.
>>>> 
>>>> I did some more digging, and got this from Google's AI Mode:
>>>> 
>>>> -----
>>>> openbsd does not fill siginfo_t si_pid for SIGTERM
>>>> 
>>>> On OpenBSD, si_pid is indeed not guaranteed to be filled for SIGTERM
>>>> (and many other signals), even when using SA_SIGINFO. This is a known
>>>> architectural behavior of the OpenBSD kernel rather than a bug.
>>>> 
>>>> Why si_pid is zero or empty
>>>> 
>>>> Minimalist Kernel Design: Unlike Linux, which often populates si_pid
>>>> and si_uid for most user-sent signals, the OpenBSD kernel only
>>>> guarantees these fields for specific signals where they are
>>>> functionally required by POSIX, such as SIGCHLD.
>>>> 
>>>> Security & Information Leakage: OpenBSD has a history of limiting
>>>> information available across process boundaries to prevent
>>>> side-channel attacks or unnecessary information leaks about other
>>>> processes on the system [0.31].
>>>> 
>>>> Signal Queueing: Standard signals like SIGTERM are not "queued" with
>>>> data in the same way real-time signals (which OpenBSD does not fully
>>>> support in the same manner as Linux) would be.
>>>> -----
>>>> 
>>>> Now, none of the links it provided in support of these claims say
>>>> any such thing AFAICS, so maybe this is all an AI hallucination.
>>>> We could probably look into the OpenBSD kernel to check it, if we
>>>> were sufficiently motivated. But I'm inclined to believe it and
>>>> just say "this info is not available on all platforms, even some
>>>> that HAVE_SA_SIGINFO".
>>>> 
>>>> 
>>>> 
>>>> 
>>>> 
>>>> Thanks for looking into this. I guess we could make a test to see what the platform will support, but it seems like overkill. So now I'm just inclined to go back to making the line completely optional in the test and leave it at that.
>>>> 
>>>> 
>>> +1
>>> 
>> And here is the patch for that.
>> 
>> 
>> 
> 
> 
> Thanks, I have pushed the fix.
> 
> cheers
> 
> andrew --
> Andrew Dunstan
> EDB: https://www.enterprisedb.com
> 

I just got a suspicion about this feature. The repro is very simple: let a normal user connect to the server, then run pg_ctl stop, and from psql you get:
```
evantest=> select 1;
FATAL:  terminating connection due to administrator command
DETAIL:  Signal sent by PID 17523, UID 501.
server closed the connection unexpectedly
	This probably means the server terminated abnormally
	before or while processing the request.
The connection to the server was lost. Attempting reset: Failed.
The connection to the server was lost. Attempting reset: Failed.
!?>
```

Do we really need to show the DETAIL message with the PID and UID to an ordinary client? Is there any concern about leaking the UID in a shared production deployment?

If this is confirmed an issue, I made a simple fix by using errdetail_log() to only emit the detail message to server log. Please the attached diff file.

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

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

Jakub Wartak <jakub.wartak@enterprisedb.com> — 2026-04-23T10:20:18Z

On Thu, Apr 23, 2026 at 11:28 AM Chao Li <li.evan.chao@gmail.com> wrote:

Hi Chao

>
> I just got a suspicion about this feature. The repro is very simple: let a normal user connect to the server, then run pg_ctl stop, and from psql you get:
> ```
> evantest=> select 1;
> FATAL:  terminating connection due to administrator command
> DETAIL:  Signal sent by PID 17523, UID 501.
> server closed the connection unexpectedly
>         This probably means the server terminated abnormally
>         before or while processing the request.
> The connection to the server was lost. Attempting reset: Failed.
> The connection to the server was lost. Attempting reset: Failed.
> !?>
> ```
>
> Do we really need to show the DETAIL message with the PID and UID to an ordinary client? Is there any concern about leaking the UID in a shared production deployment?
>
> If this is confirmed an issue, I made a simple fix by using errdetail_log() to only emit the detail message to server log. Please the attached diff file.

+1, I think logging just to file is even better than sending it to the
client(s) and it also solves the potential security risk (if any).

-J.

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

Andrew Dunstan <andrew@dunslane.net> — 2026-05-01T17:26:51Z

On Thu, Apr 23, 2026 at 6:20 AM Jakub Wartak <jakub.wartak@enterprisedb.com>
wrote:

> On Thu, Apr 23, 2026 at 11:28 AM Chao Li <li.evan.chao@gmail.com> wrote:
>
> Hi Chao
>
> >
> > I just got a suspicion about this feature. The repro is very simple: let
> a normal user connect to the server, then run pg_ctl stop, and from psql
> you get:
> > ```
> > evantest=> select 1;
> > FATAL:  terminating connection due to administrator command
> > DETAIL:  Signal sent by PID 17523, UID 501.
> > server closed the connection unexpectedly
> >         This probably means the server terminated abnormally
> >         before or while processing the request.
> > The connection to the server was lost. Attempting reset: Failed.
> > The connection to the server was lost. Attempting reset: Failed.
> > !?>
> > ```
> >
> > Do we really need to show the DETAIL message with the PID and UID to an
> ordinary client? Is there any concern about leaking the UID in a shared
> production deployment?
> >
> > If this is confirmed an issue, I made a simple fix by using
> errdetail_log() to only emit the detail message to server log. Please the
> attached diff file.
>
> +1, I think logging just to file is even better than sending it to the
> client(s) and it also solves the potential security risk (if any).
>
>
> I agree, I have pushed the patch.

cheers

andrew