Content-Type: text/plain;
	charset=utf-8
Mime-Version: 1.0 (Mac OS X Mail 16.0 \(3864.400.21\))
Subject: Re: Add errdetail() with PID and UID about source of termination
 signal
From: Chao Li <li.evan.chao@gmail.com>
In-Reply-To: 
 <CAKZiRmzmV6H4Cg=Yn=-i0myrTp2qhK_vuNAtFJppiQXmPmgy3A@mail.gmail.com>
Date: Thu, 26 Feb 2026 11:08:28 +0800
Cc: Andrew Dunstan <andrew@dunslane.net>,
 Jim Jones <jim.jones@uni-muenster.de>,
 PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Content-Transfer-Encoding: quoted-printable
Message-Id: <CFD6C011-7D95-418B-B7BA-5F864F682C83@gmail.com>
References: 
 <CAKZiRmyrOWovZSdixpLd3PGMQXuQL_zw2Ght5XhHCkQ1uDsxjw@mail.gmail.com>
 <94268299-f61e-4588-89b7-15a74a705cca@uni-muenster.de>
 <CAKZiRmzCaB0Fo7sury6dktbkAPGDi3A4ODu4RM+00kzhneGd3A@mail.gmail.com>
 <E4EA8B50-258A-4FB7-8D8F-142B4F308606@gmail.com>
 <CAKZiRmxrS=--uSO_zGoaVhu2Wq8heh-3sYQKriSA_MiZZn_DfQ@mail.gmail.com>
 <fb82d152-bcfc-4b46-901d-b9735c9f2fb4@dunslane.net>
 <CAKZiRmz6RxKoANQEJcC+K4AtBREGt5-5JcXjEotd6AsE2c2=Bw@mail.gmail.com>
 <C1589B0B-9CBC-4957-8F65-44FBB7C5BBB1@gmail.com>
 <CAKZiRmzmV6H4Cg=Yn=-i0myrTp2qhK_vuNAtFJppiQXmPmgy3A@mail.gmail.com>
To: Jakub Wartak <jakub.wartak@enterprisedb.com>
Archived-At: 
 <https://www.postgresql.org/message-id/CFD6C011-7D95-418B-B7BA-5F864F682C83%40gmail.com>
Precedence: bulk


> On Feb 25, 2026, at 18:45, Jakub Wartak =
<jakub.wartak@enterprisedb.com> wrote:
>=20
> On Wed, Feb 25, 2026 at 10:15=E2=80=AFAM Chao Li =
<li.evan.chao@gmail.com> wrote:
>=20
> Hi Chao, thanks for review.
>=20
>> A few comments for v3:
>>=20
>> 1 - syncrep.c
> [..]
>> +                                               proc_die_sender_pid =
=3D=3D 0 ? 0 :
>> +                                                       =
errdetail("Signal sent by PID %lld, UID %lld.",
>> +                                                               (long =
long)proc_die_sender_pid, (long long)proc_die_sender_uid)
>> +                                               ));
>> ```
>>=20
>> Here errdetail is used twice. I guess the second conditional one =
should be errhint.
>>=20
>> 2 - syncrep.c
> [..]
>> Same as comment 1.
>=20
> You are right, apparently I copy/pasted code from =
src/backend/tcop/postgres.c
> way too fast... fixed.
>=20
>> 3
>> ```
>> +volatile uint32 proc_die_sender_pid =3D 0;
>> +volatile uint32 proc_die_sender_uid =3D 0;
>> ```
>>=20
>> These two globals are only written in the signal handler, I think =
they should be sig_atomic_t to ensure atomic writes.
>=20
> 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)
>=20
> Yet I moved those little below, so it's more aligned to the other =
uses.
>=20
>> 4
>> ```
>> -                                        errmsg("terminating =
walreceiver process due to administrator command")));
>> +                                        errmsg("terminating =
walreceiver process due to administrator command"),
>> +                                        proc_die_sender_pid =3D=3D 0 =
? 0 :
>> +                                               errdetail("Signal =
sent by PID %lld, UID %lld.",
>> +                                                       (long =
long)proc_die_sender_pid, (long long)proc_die_sender_uid)
>> +                                       ));
>> ```
>>=20
>> Why do we need to format pid and uid in =E2=80=9Clong long=E2=80=9D =
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=3D%d =
socket=3D%d",
>>                                                         =
GetBackendTypeDesc(bn->bkend_type),
>>                                                         (int) pid, =
(int) client_sock->sock)));
>> ```
>=20
> 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);
>=20
> v4 attached. Thanks again for the review.
>=20
> -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.=20
```
@@ -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 =3D=3D=
 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 =3D 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 =3D=3D=
 0 ? 0 :
+							errhint("Signal =
sent by PID %d, UID %d.",
+								=
proc_die_sender_pid, proc_die_sender_uid)
+						));
 			SyncRepCancelWait();
 			break;
 		}
```

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

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