agora inbox for pgsql-hackers@postgresql.orghelp / color / mirror / Atom feed
[PATCH v1 1/1] stopgap fix for restore_command 5+ messages / 1 participants [nested] [flat]
* [PATCH v1 1/1] stopgap fix for restore_command @ 2023-02-01 22:32 Nathan Bossart <nathandbossart@gmail.com> 0 siblings, 0 replies; 5+ messages in thread From: Nathan Bossart @ 2023-02-01 22:32 UTC (permalink / raw) --- src/backend/access/transam/shell_restore.c | 22 ++++++++++++++++++++-- src/backend/access/transam/xlogarchive.c | 7 ------- 2 files changed, 20 insertions(+), 9 deletions(-) diff --git a/src/backend/access/transam/shell_restore.c b/src/backend/access/transam/shell_restore.c index 8458209f49..abec023c1a 100644 --- a/src/backend/access/transam/shell_restore.c +++ b/src/backend/access/transam/shell_restore.c @@ -21,6 +21,7 @@ #include "access/xlogarchive.h" #include "access/xlogrecovery.h" #include "common/percentrepl.h" +#include "postmaster/startup.h" #include "storage/ipc.h" #include "utils/wait_event.h" @@ -124,8 +125,7 @@ shell_recovery_end(const char *lastRestartPointFileName) * human-readable name describing the command emitted in the logs. If * 'failOnSignal' is true and the command is killed by a signal, a FATAL * error is thrown. Otherwise, 'fail_elevel' is used for the log message. - * If 'exitOnSigterm' is true and the command is killed by SIGTERM, we exit - * immediately. + * If 'exitOnSigterm' is true and SIGTERM is received, we exit immediately. * * Returns whether the command succeeded. */ @@ -146,7 +146,25 @@ ExecuteRecoveryCommand(const char *command, const char *commandName, */ fflush(NULL); pgstat_report_wait_start(wait_event_info); + + /* + * PreRestoreCommand() is used to tell the SIGTERM handler for the startup + * process that it is okay to proc_exit() right away on SIGTERM. 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 here, it is extremely important that nothing but the + * system() call happens between the calls to PreRestoreCommand() and + * PostRestoreCommand(). Any additional code must go before or after this + * section. + */ + if (exitOnSigterm) + PreRestoreCommand(); + rc = system(command); + + if (exitOnSigterm) + PostRestoreCommand(); + pgstat_report_wait_end(); if (rc != 0) diff --git a/src/backend/access/transam/xlogarchive.c b/src/backend/access/transam/xlogarchive.c index 4b89addf97..66312c816b 100644 --- a/src/backend/access/transam/xlogarchive.c +++ b/src/backend/access/transam/xlogarchive.c @@ -147,18 +147,11 @@ RestoreArchivedFile(char *path, const char *xlogfname, else XLogFileName(lastRestartPointFname, 0, 0L, wal_segment_size); - /* - * Check signals before restore command and reset afterwards. - */ - PreRestoreCommand(); - /* * Copy xlog from archival storage to XLOGDIR */ ret = shell_restore(xlogfname, xlogpath, lastRestartPointFname); - PostRestoreCommand(); - if (ret) { /* -- 2.25.1 --nFreZHaLTZJo0R7j-- ^ permalink raw reply [nested|flat] 5+ messages in thread
* [PATCH v4 1/3] stopgap fix for restore_command @ 2023-02-01 22:32 Nathan Bossart <nathandbossart@gmail.com> 0 siblings, 0 replies; 5+ messages in thread From: Nathan Bossart @ 2023-02-01 22:32 UTC (permalink / raw) --- src/backend/access/transam/shell_restore.c | 22 ++++++++++++++++++++-- src/backend/access/transam/xlogarchive.c | 7 ------- 2 files changed, 20 insertions(+), 9 deletions(-) diff --git a/src/backend/access/transam/shell_restore.c b/src/backend/access/transam/shell_restore.c index 8458209f49..abec023c1a 100644 --- a/src/backend/access/transam/shell_restore.c +++ b/src/backend/access/transam/shell_restore.c @@ -21,6 +21,7 @@ #include "access/xlogarchive.h" #include "access/xlogrecovery.h" #include "common/percentrepl.h" +#include "postmaster/startup.h" #include "storage/ipc.h" #include "utils/wait_event.h" @@ -124,8 +125,7 @@ shell_recovery_end(const char *lastRestartPointFileName) * human-readable name describing the command emitted in the logs. If * 'failOnSignal' is true and the command is killed by a signal, a FATAL * error is thrown. Otherwise, 'fail_elevel' is used for the log message. - * If 'exitOnSigterm' is true and the command is killed by SIGTERM, we exit - * immediately. + * If 'exitOnSigterm' is true and SIGTERM is received, we exit immediately. * * Returns whether the command succeeded. */ @@ -146,7 +146,25 @@ ExecuteRecoveryCommand(const char *command, const char *commandName, */ fflush(NULL); pgstat_report_wait_start(wait_event_info); + + /* + * PreRestoreCommand() is used to tell the SIGTERM handler for the startup + * process that it is okay to proc_exit() right away on SIGTERM. 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 here, it is extremely important that nothing but the + * system() call happens between the calls to PreRestoreCommand() and + * PostRestoreCommand(). Any additional code must go before or after this + * section. + */ + if (exitOnSigterm) + PreRestoreCommand(); + rc = system(command); + + if (exitOnSigterm) + PostRestoreCommand(); + pgstat_report_wait_end(); if (rc != 0) diff --git a/src/backend/access/transam/xlogarchive.c b/src/backend/access/transam/xlogarchive.c index 4b89addf97..66312c816b 100644 --- a/src/backend/access/transam/xlogarchive.c +++ b/src/backend/access/transam/xlogarchive.c @@ -147,18 +147,11 @@ RestoreArchivedFile(char *path, const char *xlogfname, else XLogFileName(lastRestartPointFname, 0, 0L, wal_segment_size); - /* - * Check signals before restore command and reset afterwards. - */ - PreRestoreCommand(); - /* * Copy xlog from archival storage to XLOGDIR */ ret = shell_restore(xlogfname, xlogpath, lastRestartPointFname); - PostRestoreCommand(); - if (ret) { /* -- 2.25.1 --6TrnltStXW4iwmi0 Content-Type: text/x-diff; charset=us-ascii Content-Disposition: attachment; filename="v4-0002-do-not-call-proc_exit-in-process-forked-for-resto.patch" ^ permalink raw reply [nested|flat] 5+ messages in thread
* [PATCH v2 1/1] stopgap fix for restore_command @ 2023-02-02 20:04 Nathan Bossart <nathandbossart@gmail.com> 0 siblings, 0 replies; 5+ messages in thread From: Nathan Bossart @ 2023-02-02 20:04 UTC (permalink / raw) --- src/backend/access/transam/shell_restore.c | 15 +++++++++++- src/backend/access/transam/xlogarchive.c | 7 ------ src/backend/postmaster/startup.c | 27 +++++++++++++++++----- src/include/postmaster/startup.h | 3 +-- 4 files changed, 36 insertions(+), 16 deletions(-) diff --git a/src/backend/access/transam/shell_restore.c b/src/backend/access/transam/shell_restore.c index 8458209f49..8fc3e86a10 100644 --- a/src/backend/access/transam/shell_restore.c +++ b/src/backend/access/transam/shell_restore.c @@ -21,6 +21,8 @@ #include "access/xlogarchive.h" #include "access/xlogrecovery.h" #include "common/percentrepl.h" +#include "miscadmin.h" +#include "postmaster/startup.h" #include "storage/ipc.h" #include "utils/wait_event.h" @@ -146,7 +148,18 @@ ExecuteRecoveryCommand(const char *command, const char *commandName, */ fflush(NULL); pgstat_report_wait_start(wait_event_info); - rc = system(command); + + /* + * When exitOnSigterm is set and we are in the startup process, use the + * special wrapper for system() that enables exiting immediately upon + * receiving SIGTERM. This ensures we can break out of system() if + * required. + */ + if (exitOnSigterm && MyBackendType == B_STARTUP) + rc = RunInterruptibleShellCommand(command); + else + rc = system(command); + pgstat_report_wait_end(); if (rc != 0) diff --git a/src/backend/access/transam/xlogarchive.c b/src/backend/access/transam/xlogarchive.c index 4b89addf97..66312c816b 100644 --- a/src/backend/access/transam/xlogarchive.c +++ b/src/backend/access/transam/xlogarchive.c @@ -147,18 +147,11 @@ RestoreArchivedFile(char *path, const char *xlogfname, else XLogFileName(lastRestartPointFname, 0, 0L, wal_segment_size); - /* - * Check signals before restore command and reset afterwards. - */ - PreRestoreCommand(); - /* * Copy xlog from archival storage to XLOGDIR */ ret = shell_restore(xlogfname, xlogpath, lastRestartPointFname); - PostRestoreCommand(); - if (ret) { /* diff --git a/src/backend/postmaster/startup.c b/src/backend/postmaster/startup.c index 8786186898..aa94430c6f 100644 --- a/src/backend/postmaster/startup.c +++ b/src/backend/postmaster/startup.c @@ -273,9 +273,24 @@ StartupProcessMain(void) proc_exit(0); } -void -PreRestoreCommand(void) +/* + * This is a wrapper for system() that enables exiting immediately on SIGTERM. + * It is intended for use with restore_command since there isn't a good way to + * break out while it is executing. Note that this behavior only works in the + * startup process. + * + * NB: Since we might call proc_exit() in a signal handler here, it is + * imperative that that nothing but the system() call happens between setting + * and resetting in_restore_command. Any additional code must go before or + * after this section. + */ +int +RunInterruptibleShellCommand(const char *command) { + int ret; + + Assert(MyBackendType == B_STARTUP); + /* * Set in_restore_command to tell the signal handler that we should exit * right away on SIGTERM. We know that we're at a safe point to do that. @@ -285,12 +300,12 @@ PreRestoreCommand(void) in_restore_command = true; if (shutdown_requested) proc_exit(1); -} -void -PostRestoreCommand(void) -{ + ret = system(command); + in_restore_command = false; + + return ret; } bool diff --git a/src/include/postmaster/startup.h b/src/include/postmaster/startup.h index dd957f9291..5188f49d21 100644 --- a/src/include/postmaster/startup.h +++ b/src/include/postmaster/startup.h @@ -27,8 +27,7 @@ extern PGDLLIMPORT int log_startup_progress_interval; extern void HandleStartupProcInterrupts(void); extern void StartupProcessMain(void) pg_attribute_noreturn(); -extern void PreRestoreCommand(void); -extern void PostRestoreCommand(void); +extern int RunInterruptibleShellCommand(const char *command); extern bool IsPromoteSignaled(void); extern void ResetPromoteSignaled(void); -- 2.25.1 --LQksG6bCIzRHxTLp-- ^ permalink raw reply [nested|flat] 5+ messages in thread
* [PATCH v5 1/1] stopgap fix for restore_command @ 2023-02-14 17:44 Nathan Bossart <nathandbossart@gmail.com> 0 siblings, 0 replies; 5+ messages in thread From: Nathan Bossart @ 2023-02-14 17:44 UTC (permalink / raw) --- src/backend/access/transam/xlogarchive.c | 15 +++++++++++---- src/backend/postmaster/startup.c | 20 +++++++++++++++++++- src/backend/storage/ipc/ipc.c | 3 +++ src/backend/storage/lmgr/proc.c | 2 ++ 4 files changed, 35 insertions(+), 5 deletions(-) diff --git a/src/backend/access/transam/xlogarchive.c b/src/backend/access/transam/xlogarchive.c index 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) diff --git a/src/backend/postmaster/startup.c b/src/backend/postmaster/startup.c index 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.c index 1904d21795..6796cabc3e 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 == 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.c index 22b4278610..ae845e8249 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 == 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 == getpid()); /* not safe if forked by system(), etc. */ auxproc = &AuxiliaryProcs[proctype]; -- 2.25.1 --opJtzjQTFsWo+cga-- ^ permalink raw reply [nested|flat] 5+ messages in thread
* [PATCH v12 1/6] stopgap fix for restore_command @ 2023-02-14 17:44 Nathan Bossart <nathandbossart@gmail.com> 0 siblings, 0 replies; 5+ messages in thread From: Nathan Bossart @ 2023-02-14 17:44 UTC (permalink / raw) --- src/backend/access/transam/xlogarchive.c | 15 +++++++++++---- src/backend/postmaster/startup.c | 20 +++++++++++++++++++- src/backend/storage/ipc/ipc.c | 3 +++ src/backend/storage/lmgr/proc.c | 2 ++ 4 files changed, 35 insertions(+), 5 deletions(-) diff --git a/src/backend/access/transam/xlogarchive.c b/src/backend/access/transam/xlogarchive.c index 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) diff --git a/src/backend/postmaster/startup.c b/src/backend/postmaster/startup.c index 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.c index 1904d21795..6796cabc3e 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 == 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.c index 22b4278610..ae845e8249 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 == 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 == getpid()); /* not safe if forked by system(), etc. */ auxproc = &AuxiliaryProcs[proctype]; -- 2.25.1 --YiEDa0DAkWCtVeE4 Content-Type: text/x-diff; charset=us-ascii Content-Disposition: attachment; filename="v12-0002-introduce-routine-for-checking-mutually-exclusiv.patch" ^ permalink raw reply [nested|flat] 5+ messages in thread
end of thread, other threads:[~2023-02-14 17:44 UTC | newest] Thread overview: 5+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2023-02-01 22:32 [PATCH v1 1/1] stopgap fix for restore_command Nathan Bossart <nathandbossart@gmail.com> 2023-02-01 22:32 [PATCH v4 1/3] stopgap fix for restore_command Nathan Bossart <nathandbossart@gmail.com> 2023-02-02 20:04 [PATCH v2 1/1] stopgap fix for restore_command Nathan Bossart <nathandbossart@gmail.com> 2023-02-14 17:44 [PATCH v5 1/1] stopgap fix for restore_command Nathan Bossart <nathandbossart@gmail.com> 2023-02-14 17:44 [PATCH v12 1/6] stopgap fix for restore_command Nathan Bossart <nathandbossart@gmail.com>
This inbox is served by agora; see mirroring instructions for how to clone and mirror all data and code used for this inbox