Date: Tue, 10 Oct 2023 21:54:18 -0500
From: Nathan Bossart <nathandbossart@gmail.com>
To: Andres Freund <andres@anarazel.de>
Cc: Thomas Munro <thomas.munro@gmail.com>,
	Michael Paquier <michael@paquier.xyz>,
	Robert Haas <robertmhaas@gmail.com>, Tom Lane <tgl@sss.pgh.pa.us>,
	Fujii Masao <fujii@postgresql.org>,
	Postgres hackers <pgsql-hackers@lists.postgresql.org>
Subject: Re: stopgap fix for signal handling during restore_command
Message-ID: <20231011025418.GA845134@nathanxps13>
References: <20230225190742.lfopue3nkrvxxoie@awork3.anarazel.de>
 <20230225192825.GA868928@nathanxps13>
 <20230225195253.hzp6pyllhn4kw4g6@awork3.anarazel.de>
 <20230225220629.GA1021641@nathanxps13>
 <20230226180029.djk7cth4o3o5doom@awork3.anarazel.de>
 <20230226193900.GA1248604@nathanxps13>
 <20230226201227.cner3cdicropu4q5@awork3.anarazel.de>
 <20230301043603.GD1453450@nathanxps13>
 <20230301224751.GA1823946@nathanxps13>
 <20231010234028.iba6wotmp22poxoh@awork3.anarazel.de>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="jRHKVT23PllUwdXP"
Content-Disposition: inline
In-Reply-To: <20231010234028.iba6wotmp22poxoh@awork3.anarazel.de>
Archived-At: 
 <https://www.postgresql.org/message-id/20231011025418.GA845134%40nathanxps13>
Precedence: bulk


--jRHKVT23PllUwdXP
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

On Tue, Oct 10, 2023 at 04:40:28PM -0700, Andres Freund wrote:
> On 2023-03-01 14:47:51 -0800, Nathan Bossart wrote:
>> 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.

Right.  Should we write_stderr_signal_safe() and then abort() to keep these
paths async-signal-safe?

>> +/*
>> + * 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 :)

Yup.

>> -/*
>> - * 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?

I think it was because it's an exact copy of the comment above the function
in elog.c, and I didn't want to give the impression that it applied to the
signal-safe one, too.  I added it back along with a new comment for
write_stderr_signal_safe().

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

--jRHKVT23PllUwdXP
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
	filename="v11-0001-Move-extra-code-out-of-the-Pre-PostRestoreComman.patch"

From 0a5d6a420352ec6601bd9967321d82b8a7808d67 Mon Sep 17 00:00:00 2001
From: Nathan Bossart <nathandbossart@gmail.com>
Date: Thu, 23 Feb 2023 14:27:48 -0800
Subject: [PATCH v11 1/2] Move extra code out of the Pre/PostRestoreCommand()
 block.

If SIGTERM is received within this block, the startup process will
immediately proc_exit() in the signal handler, so it is inadvisable
to include any more code than is required in this section.  This
change moves the code recently added to this block (see 1b06d7b and
7fed801) to outside of the block.  This ensures that only system()
is called while proc_exit() might be called in the SIGTERM handler,
which is how this code worked from v8.4 to v14.
---
 src/backend/access/transam/xlogarchive.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/src/backend/access/transam/xlogarchive.c b/src/backend/access/transam/xlogarchive.c
index f3fb92c8f9..b998cd651c 100644
--- a/src/backend/access/transam/xlogarchive.c
+++ b/src/backend/access/transam/xlogarchive.c
@@ -159,20 +159,27 @@ RestoreArchivedFile(char *path, const char *xlogfname,
 			(errmsg_internal("executing restore command \"%s\"",
 							 xlogRestoreCmd)));
 
+	fflush(NULL);
+	pgstat_report_wait_start(WAIT_EVENT_RESTORE_COMMAND);
+
 	/*
-	 * Check signals before restore command and reset afterwards.
+	 * PreRestoreCommand() informs the SIGTERM handler for the startup process
+	 * that it should proc_exit() right away.  This is done for the duration of
+	 * the system() call because there isn't a good way to break out while it
+	 * is executing.  Since we might call proc_exit() in a signal handler, it
+	 * is best to put any additional logic before or after the
+	 * PreRestoreCommand()/PostRestoreCommand() section.
 	 */
 	PreRestoreCommand();
 
 	/*
 	 * Copy xlog from archival storage to XLOGDIR
 	 */
-	fflush(NULL);
-	pgstat_report_wait_start(WAIT_EVENT_RESTORE_COMMAND);
 	rc = system(xlogRestoreCmd);
-	pgstat_report_wait_end();
 
 	PostRestoreCommand();
+
+	pgstat_report_wait_end();
 	pfree(xlogRestoreCmd);
 
 	if (rc == 0)
-- 
2.25.1


--jRHKVT23PllUwdXP
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
	filename="v11-0002-Don-t-proc_exit-in-startup-s-SIGTERM-handler-if-.patch"

From ec9b038e8f4b1ecfece456e308b658ecdc8c8f9d Mon Sep 17 00:00:00 2001
From: Nathan Bossart <nathandbossart@gmail.com>
Date: Tue, 14 Feb 2023 09:44:53 -0800
Subject: [PATCH v11 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 checks in 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    |  4 ++++
 src/backend/storage/lmgr/proc.c  |  8 ++++++++
 src/backend/utils/error/elog.c   | 28 ++++++++++++++++++++++++++++
 src/include/utils/elog.h         |  6 ++++++
 5 files changed, 62 insertions(+), 1 deletion(-)

diff --git a/src/backend/postmaster/startup.c b/src/backend/postmaster/startup.c
index efc2580536..0e7de26bc2 100644
--- a/src/backend/postmaster/startup.c
+++ b/src/backend/postmaster/startup.c
@@ -19,6 +19,8 @@
  */
 #include "postgres.h"
 
+#include <unistd.h>
+
 #include "access/xlog.h"
 #include "access/xlogrecovery.h"
 #include "access/xlogutils.h"
@@ -121,7 +123,20 @@ StartupProcShutdownHandler(SIGNAL_ARGS)
 	int			save_errno = errno;
 
 	if (in_restore_command)
-		proc_exit(1);
+	{
+		/*
+		 * If we are in a child process (e.g., forked by system() in
+		 * RestoreArchivedFile()), we don't want to call any exit callbacks.
+		 * The parent will take care of that.
+		 */
+		if (MyProcPid == (int) getpid())
+			proc_exit(1);
+		else
+		{
+			write_stderr_signal_safe("StartupProcShutdownHandler() called in child process\n");
+			_exit(1);
+		}
+	}
 	else
 		shutdown_requested = true;
 	WakeupRecovery();
diff --git a/src/backend/storage/ipc/ipc.c b/src/backend/storage/ipc/ipc.c
index 1904d21795..6591b5d6a8 100644
--- a/src/backend/storage/ipc/ipc.c
+++ b/src/backend/storage/ipc/ipc.c
@@ -103,6 +103,10 @@ static int	on_proc_exit_index,
 void
 proc_exit(int code)
 {
+	/* not safe if forked by system(), etc. */
+	if (MyProcPid != (int) getpid())
+		elog(PANIC, "proc_exit() called in child process");
+
 	/* Clean up everything that must be cleaned up */
 	proc_exit_prepare(code);
 
diff --git a/src/backend/storage/lmgr/proc.c b/src/backend/storage/lmgr/proc.c
index 5b663a2997..e9e445bb21 100644
--- a/src/backend/storage/lmgr/proc.c
+++ b/src/backend/storage/lmgr/proc.c
@@ -806,6 +806,10 @@ ProcKill(int code, Datum arg)
 
 	Assert(MyProc != NULL);
 
+	/* not safe if forked by system(), etc. */
+	if (MyProc->pid != (int) getpid())
+		elog(PANIC, "ProcKill() called in child process");
+
 	/* Make sure we're out of the sync rep lists */
 	SyncRepCleanupAtProcExit();
 
@@ -926,6 +930,10 @@ AuxiliaryProcKill(int code, Datum arg)
 
 	Assert(proctype >= 0 && proctype < NUM_AUXILIARY_PROCS);
 
+	/* not safe if forked by system(), etc. */
+	if (MyProc->pid != (int) getpid())
+		elog(PANIC, "AuxiliaryProcKill() called in child process");
+
 	auxproc = &AuxiliaryProcs[proctype];
 
 	Assert(MyProc == auxproc);
diff --git a/src/backend/utils/error/elog.c b/src/backend/utils/error/elog.c
index 8e1f3e8521..eeb238331e 100644
--- a/src/backend/utils/error/elog.c
+++ b/src/backend/utils/error/elog.c
@@ -3733,6 +3733,34 @@ write_stderr(const char *fmt,...)
 }
 
 
+/*
+ * 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 *str)
+{
+	int			nwritten = 0;
+	int			ntotal = strlen(str);
+
+	while (nwritten < ntotal)
+	{
+		int			rc;
+
+		rc = write(STDERR_FILENO, str + nwritten, ntotal - nwritten);
+
+		/* Just give up on error.  There isn't much else we can do. */
+		if (rc == -1)
+			return;
+
+		nwritten += rc;
+	}
+}
+
+
 /*
  * Adjust the level of a recovery-related message per trace_recovery_messages.
  *
diff --git a/src/include/utils/elog.h b/src/include/utils/elog.h
index 4a9562fdaa..0292e88b4f 100644
--- a/src/include/utils/elog.h
+++ b/src/include/utils/elog.h
@@ -536,4 +536,10 @@ extern void write_jsonlog(ErrorData *edata);
  */
 extern void write_stderr(const char *fmt,...) pg_attribute_printf(1, 2);
 
+/*
+ * Write a message to STDERR using only async-signal-safe functions.  This can
+ * be used to safely emit a message from a signal handler.
+ */
+extern void write_stderr_signal_safe(const char *fmt);
+
 #endif							/* ELOG_H */
-- 
2.25.1


--jRHKVT23PllUwdXP--