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

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