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: <jygesyr7mwg7ovdbxpmjvvbi3hccptpkcreqb645h7f56puwbz@hmkkwi3melfe>
Date: Thu, 9 Apr 2026 11:11:08 +0800
Cc: Jim Jones <jim.jones@uni-muenster.de>,
 Andrew Dunstan <andrew@dunslane.net>,
 Jakub Wartak <jakub.wartak@enterprisedb.com>,
 PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Content-Transfer-Encoding: quoted-printable
Message-Id: <4D19BF33-3C16-49A3-918E-148369ADBCB2@gmail.com>
References: <CFD6C011-7D95-418B-B7BA-5F864F682C83@gmail.com>
 <CAKZiRmxvdYXhO1JrVLW3LkuoaTjYpKmhOzzOCOhw4wDM3rYRMQ@mail.gmail.com>
 <1ba07c75-cc4e-410c-9af1-076feeeb9594@dunslane.net>
 <cwyyryh2veejuxbj5ifzyaejw7jhhqc5mrdeq56xckknsdecn2@6hzfcxde2nm5>
 <a076e20d-fc56-4b5d-aa47-a52b4e7a5061@dunslane.net>
 <j7gm4f7bx7hprsc6j4tl47nd5r2vfv5aqevco76rsdawvkwfnd@nsbig7xqlqj5>
 <9179378a-42db-4315-9ff6-b849ad5eefaf@dunslane.net>
 <6cj4oxgdpavltoqdbydw2pgxfdbcejc26x3mitasrnswipxne5@7gkaeoibos4l>
 <71930E58-82A8-4DDC-BA8C-5E394331E463@gmail.com>
 <43ab327c-3ec7-4424-88de-22c7906e5529@uni-muenster.de>
 <jygesyr7mwg7ovdbxpmjvvbi3hccptpkcreqb645h7f56puwbz@hmkkwi3melfe>
To: Andres Freund <andres@anarazel.de>
Archived-At: 
 <https://www.postgresql.org/message-id/4D19BF33-3C16-49A3-918E-148369ADBCB2%40gmail.com>
Precedence: bulk


> On Apr 9, 2026, at 01:01, Andres Freund <andres@anarazel.de> wrote:
>=20
> Hi,
>=20
> Attached is a very rough first draft for how I think this needs to =
look like.
>=20
> 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.
>=20
> With that die() (and also StatementCancelHandler, ...) can just set =
whatever
> globals it wants, without pqsignal.c needing to know about it.
>=20
> 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.
>=20
> 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.
>=20
>=20
> 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:
>=20
>> +#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
>=20
> 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.
>=20
>=20
>> .. and it should be rather done in the source (c.h) where SIGNAL_ARGS =
is
>> defined:
>>=20
>> #ifndef SIGNAL_ARGS
>> #define SIGNAL_ARGS  int postgres_signal_arg
>> #endif
>>=20
>> Something like:
>>=20
>> #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
>>=20
>> But wouldn't it mean that all handlers need to be updated as well, =
since
>> they'd get new parameters?
>=20
> All the signal handlers actually use SIGNAL_ARGS themselves, so that =
part is
> not a problem.
>=20
> 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.
>=20
> Hence in my prototype it's hidden behind a platform indepenedent =
struct of our
> own.
>=20
>=20
>> If this is the case, the change can be quite substantial.
>=20
> It's a bit annoying to do all the s/\b(SIG_(IGN|DFL)/PG_$1/, but it's =
not that
> bad, I think?
>=20
> 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.
>=20
> Greetings,
>=20
> 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 =E2=80=9Cpg_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 =
=E2=80=9Cunknown=E2=80=9D 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/