Re: stopgap fix for signal handling during restore_command - Mailing list pgsql-hackers

On 2023-03-01 14:47:51 -0800, Nathan Bossart wrote:
> Subject: [PATCH v10 1/2] Move extra code out of the Pre/PostRestoreCommand()
>  block.

LGTM


> From fb6957da01f11b75d1a1966f32b00e2e77c789a0 Mon Sep 17 00:00:00 2001
> From: Nathan Bossart <nathandbossart@gmail.com>
> Date: Tue, 14 Feb 2023 09:44:53 -0800
> Subject: [PATCH v10 2/2] Don't proc_exit() in startup's SIGTERM handler if
>  forked by system().
> 
> Instead, emit a message to STDERR and _exit() in this case.  This
> change also adds assertions to proc_exit(), ProcKill(), and
> AuxiliaryProcKill() to verify that these functions are not called
> by a process forked by system(), etc.
> ---
>  src/backend/postmaster/startup.c | 17 ++++++++++++++++-
>  src/backend/storage/ipc/ipc.c    |  3 +++
>  src/backend/storage/lmgr/proc.c  |  2 ++
>  src/backend/utils/error/elog.c   | 28 ++++++++++++++++++++++++++++
>  src/include/utils/elog.h         |  6 +-----
>  5 files changed, 50 insertions(+), 6 deletions(-)



> diff --git a/src/backend/storage/lmgr/proc.c b/src/backend/storage/lmgr/proc.c
> index 22b4278610..b9e2c3aafe 100644
> --- a/src/backend/storage/lmgr/proc.c
> +++ b/src/backend/storage/lmgr/proc.c
> @@ -805,6 +805,7 @@ ProcKill(int code, Datum arg)
>      dlist_head *procgloballist;
>  
>      Assert(MyProc != NULL);
> +    Assert(MyProc->pid == (int) getpid());  /* not safe if forked by system(), etc. */
>  
>      /* Make sure we're out of the sync rep lists */
>      SyncRepCleanupAtProcExit();
> @@ -925,6 +926,7 @@ AuxiliaryProcKill(int code, Datum arg)
>      PGPROC       *proc;
>  
>      Assert(proctype >= 0 && proctype < NUM_AUXILIARY_PROCS);
> +    Assert(MyProc->pid == (int) getpid());  /* not safe if forked by system(), etc. */
>  
>      auxproc = &AuxiliaryProcs[proctype];
>  

I'd make these elog(PANIC), I think. The paths are not performance critical
enough that a single branch hurts, so the overhead of the check is irrelevant,
and the consequences of calling ProcKill() twice for the same process are very
severe.


> +/*
> + * Write a message to STDERR using only async-signal-safe functions.  This can
> + * be used to safely emit a message from a signal handler.
> + *
> + * TODO: It is likely possible to safely do a limited amount of string
> + * interpolation (e.g., %s and %d), but that is not presently supported.
> + */
> +void
> +write_stderr_signal_safe(const char *fmt)

As is, this isn't a format, so I'd probably just name it s or str :)


> -/*
> - * Write errors to stderr (or by equal means when stderr is
> - * not available). Used before ereport/elog can be used
> - * safely (memory context, GUC load etc)
> - */
>  extern void write_stderr(const char *fmt,...) pg_attribute_printf(1, 2);
> +extern void write_stderr_signal_safe(const char *fmt);

Not sure why you removed the comment?

Greetings,

Andres Freund