| From: | Jakub Wartak <jakub(dot)wartak(at)enterprisedb(dot)com> |
|---|---|
| To: | Chao Li <li(dot)evan(dot)chao(at)gmail(dot)com> |
| Cc: | Andrew Dunstan <andrew(at)dunslane(dot)net>, Jim Jones <jim(dot)jones(at)uni-muenster(dot)de>, PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org> |
| Subject: | Re: Add errdetail() with PID and UID about source of termination signal |
| Date: | 2026-02-25 10:45:12 |
| Message-ID: | CAKZiRmzmV6H4Cg=Yn=-i0myrTp2qhK_vuNAtFJppiQXmPmgy3A@mail.gmail.com |
| Views: | Whole Thread | Raw Message | Download mbox | Resend email |
| Thread: | |
| Lists: | pgsql-hackers |
On Wed, Feb 25, 2026 at 10:15 AM Chao Li <li(dot)evan(dot)chao(at)gmail(dot)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.
| Attachment | Content-Type | Size |
|---|---|---|
| v4-0001-Add-errdetail-with-PID-and-UID-about-source-of-te.patch | text/x-patch | 13.2 KB |
| From | Date | Subject | |
|---|---|---|---|
| Next Message | Zsolt Parragi | 2026-02-25 10:54:34 | Re: More speedups for tuple deformation |
| Previous Message | shveta malik | 2026-02-25 10:31:34 | Re: Skipping schema changes in publication |