Re: stopgap fix for signal handling during restore_command

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(at)gmail(dot)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