Re: Add errdetail() with PID and UID about source of termination signal - Mailing list pgsql-hackers

Hi,

On 2026-04-06 12:51:40 -0400, Andrew Dunstan wrote:
> From 450b68d29f02ee1f5bf71db708b380ab389a30c6 Mon Sep 17 00:00:00 2001
> From: Andrew Dunstan <andrew@dunslane.net>
> Date: Mon, 6 Apr 2026 12:39:14 -0400
> Subject: [PATCH v6] Add errdetail() with PID and UID about source of
>  termination signal.
>
> When a backend is terminated via pg_terminate_backend() or an external
> SIGTERM, the error message now includes the sender's PID and UID as
> errdetail, making it easier to identify the source of unexpected
> terminations in multi-user environments.
>
> On platforms that support SA_SIGINFO (Linux, FreeBSD, and most modern
> Unix systems), the signal handler captures si_pid and si_uid from the
> siginfo_t structure.  On platforms without SA_SIGINFO, the detail is
> simply omitted.
>
> Author: Jakub Wartak <jakub.wartak@enterprisedb.com>
> Reviewed-by: Andrew Dunstan <andrew@dunslane.net>
> Reviewed-by: Chao Li <1356863904@qq.com>
> Discussion: https://postgr.es/m/CAKZiRmyrOWovZSdixpLd3PGMQXuQL_zw2Ght5XhHCkQ1uDsxjw@mail.gmail.com


> +++ b/src/backend/replication/syncrep.c
> @@ -303,7 +303,11 @@ SyncRepWaitForLSN(XLogRecPtr lsn, bool commit)
>              ereport(WARNING,
>                      (errcode(ERRCODE_ADMIN_SHUTDOWN),
>                       errmsg("canceling the wait for synchronous replication and terminating connection due to
administratorcommand"),
 
> -                     errdetail("The transaction has already committed locally, but might not have been replicated to
thestandby.")));
 
> +                     errdetail("The transaction has already committed locally, but might not have been replicated to
thestandby.%s",
 
> +                               ProcDieSenderPid == 0 ? "" :
> +                               psprintf("\nSignal sent by PID %d, UID %d.",
> +                                        (int) ProcDieSenderPid,
> +                                        (int) ProcDieSenderUid))));
>              whereToSendOutput = DestNone;
>              SyncRepCancelWait();
>              break;

Pretty sure this is broken from a translateability POV?

It's also somewhat ugly.


> +++ b/src/port/pqsignal.c
> @@ -82,10 +82,19 @@ static volatile pqsigfunc pqsignal_handlers[PG_NSIG];
>   *
>   * This wrapper also handles restoring the value of errno.
>   */
> +#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
>  {
>      int            save_errno = errno;
> +#if !defined(FRONTEND) && defined(HAVE_SA_SIGINFO)
> +    /* SA_SIGINFO signature uses signo, not SIGNAL_ARGS macro */
> +    int            postgres_signal_arg = signo;
> +#endif

Seems that you then should change what SIGNAL_ARGS means, not randomly hack
around it in one place?


>      Assert(postgres_signal_arg > 0);
>      Assert(postgres_signal_arg < PG_NSIG);
> @@ -105,6 +114,14 @@ wrapper_handler(SIGNAL_ARGS)
>          raise(postgres_signal_arg);
>          return;
>      }
> +
> +#ifdef HAVE_SA_SIGINFO
> +    if (signo == SIGTERM && info)
> +    {
> +        ProcDieSenderPid = info->si_pid;
> +        ProcDieSenderUid = info->si_uid;
> +    }
> +#endif
>  #endif
>
>      (*pqsignal_handlers[postgres_signal_arg]) (postgres_signal_arg);

This seems completely wrong from a layering POV.  The wrapper has no business
whatsoever to know that how SIGTERM is interpreted and thus no business
setting variables like ProcDieSenderPid.

Pretty sure have some sigterm handlers that shouldn't set ProcDieSenderPid.


A more correct answer here would be to forward information about the sender of
a signal to the signal handlers and let them interpret the information if
available.



I think this is nowhere near ready to have been committed.


Greetings,

Andres Freund