From: Nathan Bossart <nathandbossart@gmail.com>
To: Thomas Munro <thomas.munro@gmail.com>
Cc: Michael Paquier <michael@paquier.xyz>
Cc: Andres Freund <andres@anarazel.de>
Cc: Robert Haas <robertmhaas@gmail.com>
Cc: Tom Lane <tgl@sss.pgh.pa.us>
Cc: Fujii Masao <fujii@postgresql.org>
Cc: Postgres hackers <pgsql-hackers@lists.postgresql.org>
Subject: stopgap fix for signal handling during restore_command
Date: Thu, 23 Feb 2023 15:15:03 -0800
Message-ID: <20230223231503.GA743455@nathanxps13> (raw)
On Wed, Feb 22, 2023 at 09:48:10PM +1300, Thomas Munro wrote:
> On Tue, Feb 21, 2023 at 5:50 PM Nathan Bossart <nathandbossart@gmail.com> wrote:
>> I'm happy to create a new thread if needed, but I can't tell if there is
>> any interest in this stopgap/back-branch fix. Perhaps we should just jump
>> straight to the long-term fix that Thomas is looking into.
>
> Unfortunately the latch-friendly subprocess module proposal I was
> talking about would be for 17. I may post a thread fairly soon with
> design ideas + list of problems and decision points as I see them, and
> hopefully some sketch code, but it won't be a proposal for [/me checks
> calendar] next week's commitfest and probably wouldn't be appropriate
> in a final commitfest anyway, and I also have some other existing
> stuff to clear first. So please do continue with the stopgap ideas.
Okay, here is a new thread...
Since v8.4, the startup process will proc_exit() immediately within its
SIGTERM handler while the restore_command executes via system(). Some
recent changes added unsafe code to the section where this behavior is
enabled [0]. The long-term fix likely includes moving away from system()
completely, but we may want to have a stopgap/back-branch fix while that is
under development.
I've attached a patch set for a proposed stopgap fix. 0001 simply moves
the extra code outside of the Pre/PostRestoreCommand() block so that only
system() is executed while the SIGTERM handler might proc_exit(). This
restores the behavior that was in place from v8.4 to v14, so I don't expect
it to be too controversial. 0002 adds code to startup's SIGTERM handler to
call _exit() instead of proc_exit() if we are in a forked process from
system(), etc. It also adds assertions to ensure proc_exit(), ProcKill(),
and AuxiliaryProcKill() are not called within such forked processes.
Thoughts?
[0] https://postgr.es/m/20230201105514.rsjl4bnhb65giyvo%40alap3.anarazel.de
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.comAttachments:
[text/x-diff] v6-0001-Move-extra-code-out-of-the-Pre-PostRestoreCommand.patch (2.0K, ../20230223231503.GA743455@nathanxps13/2-v6-0001-Move-extra-code-out-of-the-Pre-PostRestoreCommand.patch)
download | inline diff:
From aee0df5e44cfd631fb601e18ab50469a662ced19 Mon Sep 17 00:00:00 2001
From: Nathan Bossart <nathandbossart@gmail.com>
Date: Thu, 23 Feb 2023 14:27:48 -0800
Subject: [PATCH v6 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.cindex fcc87ff44f..41684418b6 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
[text/x-diff] v6-0002-Don-t-proc_exit-in-startup-s-SIGTERM-handler-if-f.patch (2.8K, ../20230223231503.GA743455@nathanxps13/3-v6-0002-Don-t-proc_exit-in-startup-s-SIGTERM-handler-if-f.patch)
download | inline diff:
From 76e4f00c1c08513893780bc50709b5434dc3470f Mon Sep 17 00:00:00 2001
From: Nathan Bossart <nathandbossart@gmail.com>
Date: Tue, 14 Feb 2023 09:44:53 -0800
Subject: [PATCH v6 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 | 20 +++++++++++++++++++-
src/backend/storage/ipc/ipc.c | 3 +++
src/backend/storage/lmgr/proc.c | 2 ++
3 files changed, 24 insertions(+), 1 deletion(-)
diff --git a/src/backend/postmaster/startup.c b/src/backend/postmaster/startup.cindex efc2580536..de2b56c2fa 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,23 @@ 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+ {+ const char msg[] = "StartupProcShutdownHandler() called in child process";+ int rc pg_attribute_unused();++ rc = write(STDERR_FILENO, msg, sizeof(msg));+ _exit(1);+ }+ }
else
shutdown_requested = true;
WakeupRecovery();
diff --git a/src/backend/storage/ipc/ipc.c b/src/backend/storage/ipc/ipc.cindex 1904d21795..d5097dc008 100644--- a/src/backend/storage/ipc/ipc.c+++ b/src/backend/storage/ipc/ipc.c@@ -103,6 +103,9 @@ static int on_proc_exit_index,
void
proc_exit(int code)
{
+ /* proc_exit() is not safe in forked processes from system(), etc. */+ Assert(MyProcPid == (int) getpid());+
/* 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.cindex 22b4278610..e3da0622d7 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(MyProcPid == (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(MyProcPid == (int) getpid()); /* not safe if forked by system(), etc. */
auxproc = &AuxiliaryProcs[proctype];
--
2.25.1
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: nathandbossart@gmail.com, thomas.munro@gmail.com, michael@paquier.xyz, andres@anarazel.de, robertmhaas@gmail.com, tgl@sss.pgh.pa.us, fujii@postgresql.org, pgsql-hackers@lists.postgresql.org
Subject: Re: stopgap fix for signal handling during restore_command
In-Reply-To: <20230223231503.GA743455@nathanxps13>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox