> On Feb 25, 2026, at 16:26, Jakub Wartak <[email protected]> wrote:
> 
> On Tue, Feb 24, 2026 at 5:15 PM Andrew Dunstan <[email protected]> wrote:
>> 
>> 
>> On 2026-02-24 Tu 5:05 AM, Jakub Wartak wrote:
>>> On Tue, Feb 24, 2026 at 9:40 AM Chao Li <[email protected]> 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/






Reply via email to