From 2f6e15ac93c58c1140e4a4affe61e78f7346497a Mon Sep 17 00:00:00 2001 From: Michael Paquier Date: Mon, 6 Feb 2023 08:28:42 +0900 Subject: [PATCH] Revert refactoring of restore command code to shell_restore.c This reverts commits 24c35ec and 57169ad. PreRestoreCommand() and PostRestoreCommand() need to be put closer to the system() call calling a restore_command, as they enable in_restore_command for the startup process which would in turn trigger an immediate proc_exit() in the SIGTERM handler. Perhaps we could get rid of this behavior entirely, but 24c35ec has made the window where the flag is enabled much larger than it was, and any Postgres-like actions (palloc, etc.) taken by code paths while the flag is enabled could lead to more severe issues in the shutdown processing. Note that curculio has showed that there are much more problems in this area, unrelated to this change, actually, hence the issues related to that had better be addressed first. Keeping the code of HEAD in line with the stable branches should make that a bit easier. Per discussion with Andres Freund and Nathan Bossart. Discussion: https://postgr.es/m/Y979NR3U5VnWrTwB@paquier.xyz --- src/backend/access/transam/Makefile | 1 - src/backend/access/transam/meson.build | 1 - src/backend/access/transam/shell_restore.c | 171 --------------------- src/backend/access/transam/xlog.c | 37 +---- src/backend/access/transam/xlogarchive.c | 121 ++++++++++++++- src/common/Makefile | 1 + src/common/archive.c | 60 ++++++++ src/common/meson.build | 1 + src/fe_utils/archive.c | 11 +- src/include/access/xlogarchive.h | 7 +- src/include/common/archive.h | 21 +++ src/tools/msvc/Mkvcbuild.pm | 2 +- 12 files changed, 215 insertions(+), 219 deletions(-) delete mode 100644 src/backend/access/transam/shell_restore.c create mode 100644 src/common/archive.c create mode 100644 src/include/common/archive.h diff --git a/src/backend/access/transam/Makefile b/src/backend/access/transam/Makefile index 099c315d03..661c55a9db 100644 --- a/src/backend/access/transam/Makefile +++ b/src/backend/access/transam/Makefile @@ -19,7 +19,6 @@ OBJS = \ multixact.o \ parallel.o \ rmgr.o \ - shell_restore.o \ slru.o \ subtrans.o \ timeline.o \ diff --git a/src/backend/access/transam/meson.build b/src/backend/access/transam/meson.build index 3031c2f6cf..8920c1bfce 100644 --- a/src/backend/access/transam/meson.build +++ b/src/backend/access/transam/meson.build @@ -7,7 +7,6 @@ backend_sources += files( 'multixact.c', 'parallel.c', 'rmgr.c', - 'shell_restore.c', 'slru.c', 'subtrans.c', 'timeline.c', diff --git a/src/backend/access/transam/shell_restore.c b/src/backend/access/transam/shell_restore.c deleted file mode 100644 index 8458209f49..0000000000 --- a/src/backend/access/transam/shell_restore.c +++ /dev/null @@ -1,171 +0,0 @@ -/*------------------------------------------------------------------------- - * - * shell_restore.c - * Recovery functions for a user-specified shell command. - * - * These recovery functions use a user-specified shell command (e.g. based - * on the GUC restore_command). - * - * Portions Copyright (c) 1996-2023, PostgreSQL Global Development Group - * Portions Copyright (c) 1994, Regents of the University of California - * - * src/backend/access/transam/shell_restore.c - * - *------------------------------------------------------------------------- - */ - -#include "postgres.h" - -#include - -#include "access/xlogarchive.h" -#include "access/xlogrecovery.h" -#include "common/percentrepl.h" -#include "storage/ipc.h" -#include "utils/wait_event.h" - -static bool ExecuteRecoveryCommand(const char *command, - const char *commandName, - bool failOnSignal, - bool exitOnSigterm, - uint32 wait_event_info, - int fail_elevel); - -/* - * Attempt to execute a shell-based restore command. - * - * Returns true if the command has succeeded, false otherwise. - */ -bool -shell_restore(const char *file, const char *path, - const char *lastRestartPointFileName) -{ - char *nativePath = pstrdup(path); - char *cmd; - bool ret; - - /* Build the restore command to execute */ - make_native_path(nativePath); - cmd = replace_percent_placeholders(recoveryRestoreCommand, - "restore_command", "frp", file, - lastRestartPointFileName, - nativePath); - pfree(nativePath); - - /* - * Remember, we rollforward UNTIL the restore fails so failure here is - * just part of the process... that makes it difficult to determine - * whether the restore failed because there isn't an archive to restore, - * or because the administrator has specified the restore program - * incorrectly. We have to assume the former. - * - * However, if the failure was due to any sort of signal, it's best to - * punt and abort recovery. (If we "return false" here, upper levels will - * assume that recovery is complete and start up the database!) It's - * essential to abort on child SIGINT and SIGQUIT, because per spec - * system() ignores SIGINT and SIGQUIT while waiting; if we see one of - * those it's a good bet we should have gotten it too. - * - * On SIGTERM, assume we have received a fast shutdown request, and exit - * cleanly. It's pure chance whether we receive the SIGTERM first, or the - * child process. If we receive it first, the signal handler will call - * proc_exit, otherwise we do it here. If we or the child process received - * SIGTERM for any other reason than a fast shutdown request, postmaster - * will perform an immediate shutdown when it sees us exiting - * unexpectedly. - * - * We treat hard shell errors such as "command not found" as fatal, too. - */ - ret = ExecuteRecoveryCommand(cmd, "restore_command", - true, /* failOnSignal */ - true, /* exitOnSigterm */ - WAIT_EVENT_RESTORE_COMMAND, DEBUG2); - pfree(cmd); - - return ret; -} - -/* - * Attempt to execute a shell-based archive cleanup command. - */ -void -shell_archive_cleanup(const char *lastRestartPointFileName) -{ - char *cmd; - - cmd = replace_percent_placeholders(archiveCleanupCommand, - "archive_cleanup_command", - "r", lastRestartPointFileName); - (void) ExecuteRecoveryCommand(cmd, "archive_cleanup_command", false, false, - WAIT_EVENT_ARCHIVE_CLEANUP_COMMAND, WARNING); - pfree(cmd); -} - -/* - * Attempt to execute a shell-based end-of-recovery command. - */ -void -shell_recovery_end(const char *lastRestartPointFileName) -{ - char *cmd; - - cmd = replace_percent_placeholders(recoveryEndCommand, - "recovery_end_command", - "r", lastRestartPointFileName); - (void) ExecuteRecoveryCommand(cmd, "recovery_end_command", true, false, - WAIT_EVENT_RECOVERY_END_COMMAND, WARNING); - pfree(cmd); -} - -/* - * Attempt to execute an external shell command during recovery. - * - * 'command' is the shell command to be executed, 'commandName' is a - * 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. - * - * Returns whether the command succeeded. - */ -static bool -ExecuteRecoveryCommand(const char *command, const char *commandName, - bool failOnSignal, bool exitOnSigterm, - uint32 wait_event_info, int fail_elevel) -{ - int rc; - - Assert(command && commandName); - - ereport(DEBUG3, - (errmsg_internal("executing %s \"%s\"", commandName, command))); - - /* - * execute the constructed command - */ - fflush(NULL); - pgstat_report_wait_start(wait_event_info); - rc = system(command); - pgstat_report_wait_end(); - - if (rc != 0) - { - if (exitOnSigterm && wait_result_is_signal(rc, SIGTERM)) - proc_exit(1); - - /* - * If the failure was due to any sort of signal, it's best to punt and - * abort recovery. See comments in shell_restore(). - */ - ereport((failOnSignal && wait_result_is_any_signal(rc, true)) ? FATAL : fail_elevel, - /*------ - translator: First %s represents a postgresql.conf parameter name like - "recovery_end_command", the 2nd is the value of that parameter, the - third an already translated error message. */ - (errmsg("%s \"%s\": %s", commandName, - command, wait_result_to_str(rc)))); - } - - return (rc == 0); -} diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c index fb4c860bde..f9f0f6db8d 100644 --- a/src/backend/access/transam/xlog.c +++ b/src/backend/access/transam/xlog.c @@ -692,7 +692,6 @@ static char *GetXLogBuffer(XLogRecPtr ptr, TimeLineID tli); static XLogRecPtr XLogBytePosToRecPtr(uint64 bytepos); static XLogRecPtr XLogBytePosToEndRecPtr(uint64 bytepos); static uint64 XLogRecPtrToBytePos(XLogRecPtr ptr); -static void GetOldestRestartPointFileName(char *fname); static void WALInsertLockAcquire(void); static void WALInsertLockAcquireExclusive(void); @@ -4890,12 +4889,10 @@ CleanupAfterArchiveRecovery(TimeLineID EndOfLogTLI, XLogRecPtr EndOfLog, * Execute the recovery_end_command, if any. */ if (recoveryEndCommand && strcmp(recoveryEndCommand, "") != 0) - { - char lastRestartPointFname[MAXFNAMELEN]; - - GetOldestRestartPointFileName(lastRestartPointFname); - shell_recovery_end(lastRestartPointFname); - } + ExecuteRecoveryCommand(recoveryEndCommand, + "recovery_end_command", + true, + WAIT_EVENT_RECOVERY_END_COMMAND); /* * We switched to a new timeline. Clean up segments on the old timeline. @@ -7312,12 +7309,10 @@ CreateRestartPoint(int flags) * Finally, execute archive_cleanup_command, if any. */ if (archiveCleanupCommand && strcmp(archiveCleanupCommand, "") != 0) - { - char lastRestartPointFname[MAXFNAMELEN]; - - GetOldestRestartPointFileName(lastRestartPointFname); - shell_archive_cleanup(lastRestartPointFname); - } + ExecuteRecoveryCommand(archiveCleanupCommand, + "archive_cleanup_command", + false, + WAIT_EVENT_ARCHIVE_CLEANUP_COMMAND); return true; } @@ -8894,22 +8889,6 @@ GetOldestRestartPoint(XLogRecPtr *oldrecptr, TimeLineID *oldtli) LWLockRelease(ControlFileLock); } -/* - * Returns the WAL file name for the last checkpoint or restartpoint. This is - * the oldest WAL file that we still need if we have to restart recovery. - */ -static void -GetOldestRestartPointFileName(char *fname) -{ - XLogRecPtr restartRedoPtr; - TimeLineID restartTli; - XLogSegNo restartSegNo; - - GetOldestRestartPoint(&restartRedoPtr, &restartTli); - XLByteToSeg(restartRedoPtr, restartSegNo, wal_segment_size); - XLogFileName(fname, restartTli, restartSegNo, wal_segment_size); -} - /* Thin wrapper around ShutdownWalRcv(). */ void XLogShutdownWalRcv(void) diff --git a/src/backend/access/transam/xlogarchive.c b/src/backend/access/transam/xlogarchive.c index 4b89addf97..fcc87ff44f 100644 --- a/src/backend/access/transam/xlogarchive.c +++ b/src/backend/access/transam/xlogarchive.c @@ -22,6 +22,8 @@ #include "access/xlog.h" #include "access/xlog_internal.h" #include "access/xlogarchive.h" +#include "common/archive.h" +#include "common/percentrepl.h" #include "miscadmin.h" #include "pgstat.h" #include "postmaster/startup.h" @@ -55,8 +57,9 @@ RestoreArchivedFile(char *path, const char *xlogfname, bool cleanupEnabled) { char xlogpath[MAXPGPATH]; + char *xlogRestoreCmd; char lastRestartPointFname[MAXPGPATH]; - bool ret; + int rc; struct stat stat_buf; XLogSegNo restartSegNo; XLogRecPtr restartRedoPtr; @@ -147,6 +150,15 @@ RestoreArchivedFile(char *path, const char *xlogfname, else XLogFileName(lastRestartPointFname, 0, 0L, wal_segment_size); + /* Build the restore command to execute */ + xlogRestoreCmd = BuildRestoreCommand(recoveryRestoreCommand, + xlogpath, xlogfname, + lastRestartPointFname); + + ereport(DEBUG3, + (errmsg_internal("executing restore command \"%s\"", + xlogRestoreCmd))); + /* * Check signals before restore command and reset afterwards. */ @@ -155,11 +167,15 @@ RestoreArchivedFile(char *path, const char *xlogfname, /* * Copy xlog from archival storage to XLOGDIR */ - ret = shell_restore(xlogfname, xlogpath, lastRestartPointFname); + fflush(NULL); + pgstat_report_wait_start(WAIT_EVENT_RESTORE_COMMAND); + rc = system(xlogRestoreCmd); + pgstat_report_wait_end(); PostRestoreCommand(); + pfree(xlogRestoreCmd); - if (ret) + if (rc == 0) { /* * command apparently succeeded, but let's make sure the file is @@ -215,6 +231,37 @@ RestoreArchivedFile(char *path, const char *xlogfname, } } + /* + * Remember, we rollforward UNTIL the restore fails so failure here is + * just part of the process... that makes it difficult to determine + * whether the restore failed because there isn't an archive to restore, + * or because the administrator has specified the restore program + * incorrectly. We have to assume the former. + * + * However, if the failure was due to any sort of signal, it's best to + * punt and abort recovery. (If we "return false" here, upper levels will + * assume that recovery is complete and start up the database!) It's + * essential to abort on child SIGINT and SIGQUIT, because per spec + * system() ignores SIGINT and SIGQUIT while waiting; if we see one of + * those it's a good bet we should have gotten it too. + * + * On SIGTERM, assume we have received a fast shutdown request, and exit + * cleanly. It's pure chance whether we receive the SIGTERM first, or the + * child process. If we receive it first, the signal handler will call + * proc_exit, otherwise we do it here. If we or the child process received + * SIGTERM for any other reason than a fast shutdown request, postmaster + * will perform an immediate shutdown when it sees us exiting + * unexpectedly. + * + * We treat hard shell errors such as "command not found" as fatal, too. + */ + if (wait_result_is_signal(rc, SIGTERM)) + proc_exit(1); + + ereport(wait_result_is_any_signal(rc, true) ? FATAL : DEBUG2, + (errmsg("could not restore file \"%s\" from archive: %s", + xlogfname, wait_result_to_str(rc)))); + not_available: /* @@ -228,6 +275,74 @@ not_available: return false; } +/* + * Attempt to execute an external shell command during recovery. + * + * 'command' is the shell command to be executed, 'commandName' is a + * 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 a WARNING is emitted. + * + * This is currently used for recovery_end_command and archive_cleanup_command. + */ +void +ExecuteRecoveryCommand(const char *command, const char *commandName, + bool failOnSignal, uint32 wait_event_info) +{ + char *xlogRecoveryCmd; + char lastRestartPointFname[MAXPGPATH]; + int rc; + XLogSegNo restartSegNo; + XLogRecPtr restartRedoPtr; + TimeLineID restartTli; + + Assert(command && commandName); + + /* + * Calculate the archive file cutoff point for use during log shipping + * replication. All files earlier than this point can be deleted from the + * archive, though there is no requirement to do so. + */ + GetOldestRestartPoint(&restartRedoPtr, &restartTli); + XLByteToSeg(restartRedoPtr, restartSegNo, wal_segment_size); + XLogFileName(lastRestartPointFname, restartTli, restartSegNo, + wal_segment_size); + + /* + * construct the command to be executed + */ + xlogRecoveryCmd = replace_percent_placeholders(command, commandName, "r", lastRestartPointFname); + + ereport(DEBUG3, + (errmsg_internal("executing %s \"%s\"", commandName, command))); + + /* + * execute the constructed command + */ + fflush(NULL); + pgstat_report_wait_start(wait_event_info); + rc = system(xlogRecoveryCmd); + pgstat_report_wait_end(); + + pfree(xlogRecoveryCmd); + + if (rc != 0) + { + /* + * If the failure was due to any sort of signal, it's best to punt and + * abort recovery. See comments in RestoreArchivedFile(). + */ + ereport((failOnSignal && wait_result_is_any_signal(rc, true)) ? FATAL : WARNING, + /*------ + translator: First %s represents a postgresql.conf parameter name like + "recovery_end_command", the 2nd is the value of that parameter, the + third an already translated error message. */ + (errmsg("%s \"%s\": %s", commandName, + command, wait_result_to_str(rc)))); + } +} + + /* * A file was restored from the archive under a temporary filename (path), * and now we want to keep it. Rename it under the permanent filename in diff --git a/src/common/Makefile b/src/common/Makefile index 2f424a5735..113029bf7b 100644 --- a/src/common/Makefile +++ b/src/common/Makefile @@ -46,6 +46,7 @@ LIBS += $(PTHREAD_LIBS) # If you add objects here, see also src/tools/msvc/Mkvcbuild.pm OBJS_COMMON = \ + archive.o \ base64.o \ checksum_helper.o \ compression.o \ diff --git a/src/common/archive.c b/src/common/archive.c new file mode 100644 index 0000000000..641a58ee88 --- /dev/null +++ b/src/common/archive.c @@ -0,0 +1,60 @@ +/*------------------------------------------------------------------------- + * + * archive.c + * Common WAL archive routines + * + * Portions Copyright (c) 1996-2023, PostgreSQL Global Development Group + * Portions Copyright (c) 1994, Regents of the University of California + * + * + * IDENTIFICATION + * src/common/archive.c + * + *------------------------------------------------------------------------- + */ + +#ifndef FRONTEND +#include "postgres.h" +#else +#include "postgres_fe.h" +#endif + +#include "common/archive.h" +#include "common/percentrepl.h" + +/* + * BuildRestoreCommand + * + * Builds a restore command to retrieve a file from WAL archives, replacing + * the supported aliases with values supplied by the caller as defined by + * the GUC parameter restore_command: xlogpath for %p, xlogfname for %f and + * lastRestartPointFname for %r. + * + * The result is a palloc'd string for the restore command built. The + * caller is responsible for freeing it. If any of the required arguments + * is NULL and that the corresponding alias is found in the command given + * by the caller, then an error is thrown. + */ +char * +BuildRestoreCommand(const char *restoreCommand, + const char *xlogpath, + const char *xlogfname, + const char *lastRestartPointFname) +{ + char *nativePath = NULL; + char *result; + + if (xlogpath) + { + nativePath = pstrdup(xlogpath); + make_native_path(nativePath); + } + + result = replace_percent_placeholders(restoreCommand, "restore_command", "frp", + xlogfname, lastRestartPointFname, nativePath); + + if (nativePath) + pfree(nativePath); + + return result; +} diff --git a/src/common/meson.build b/src/common/meson.build index 1caa1fed04..41bd58ebdf 100644 --- a/src/common/meson.build +++ b/src/common/meson.build @@ -1,6 +1,7 @@ # Copyright (c) 2022-2023, PostgreSQL Global Development Group common_sources = files( + 'archive.c', 'base64.c', 'checksum_helper.c', 'compression.c', diff --git a/src/fe_utils/archive.c b/src/fe_utils/archive.c index c1ce250c90..eb1c930ae7 100644 --- a/src/fe_utils/archive.c +++ b/src/fe_utils/archive.c @@ -19,8 +19,8 @@ #include #include "access/xlog_internal.h" +#include "common/archive.h" #include "common/logging.h" -#include "common/percentrepl.h" #include "fe_utils/archive.h" @@ -41,18 +41,13 @@ RestoreArchivedFile(const char *path, const char *xlogfname, { char xlogpath[MAXPGPATH]; char *xlogRestoreCmd; - char *nativePath; int rc; struct stat stat_buf; snprintf(xlogpath, MAXPGPATH, "%s/" XLOGDIR "/%s", path, xlogfname); - nativePath = pstrdup(xlogpath); - make_native_path(nativePath); - xlogRestoreCmd = replace_percent_placeholders(restoreCommand, - "restore_command", "fp", - xlogfname, nativePath); - pfree(nativePath); + xlogRestoreCmd = BuildRestoreCommand(restoreCommand, xlogpath, + xlogfname, NULL); /* * Execute restore_command, which should copy the missing file from diff --git a/src/include/access/xlogarchive.h b/src/include/access/xlogarchive.h index 299304703e..31ff206034 100644 --- a/src/include/access/xlogarchive.h +++ b/src/include/access/xlogarchive.h @@ -20,6 +20,8 @@ extern bool RestoreArchivedFile(char *path, const char *xlogfname, const char *recovername, off_t expectedSize, bool cleanupEnabled); +extern void ExecuteRecoveryCommand(const char *command, const char *commandName, + bool failOnSignal, uint32 wait_event_info); extern void KeepFileRestoredFromArchive(const char *path, const char *xlogfname); extern void XLogArchiveNotify(const char *xlog); extern void XLogArchiveNotifySeg(XLogSegNo segno, TimeLineID tli); @@ -30,9 +32,4 @@ extern bool XLogArchiveIsReady(const char *xlog); extern bool XLogArchiveIsReadyOrDone(const char *xlog); extern void XLogArchiveCleanup(const char *xlog); -extern bool shell_restore(const char *file, const char *path, - const char *lastRestartPointFileName); -extern void shell_archive_cleanup(const char *lastRestartPointFileName); -extern void shell_recovery_end(const char *lastRestartPointFileName); - #endif /* XLOG_ARCHIVE_H */ diff --git a/src/include/common/archive.h b/src/include/common/archive.h new file mode 100644 index 0000000000..95196772c9 --- /dev/null +++ b/src/include/common/archive.h @@ -0,0 +1,21 @@ +/*------------------------------------------------------------------------- + * + * archive.h + * Common WAL archive routines + * + * Portions Copyright (c) 1996-2023, PostgreSQL Global Development Group + * Portions Copyright (c) 1994, Regents of the University of California + * + * src/include/common/archive.h + * + *------------------------------------------------------------------------- + */ +#ifndef ARCHIVE_H +#define ARCHIVE_H + +extern char *BuildRestoreCommand(const char *restoreCommand, + const char *xlogpath, /* %p */ + const char *xlogfname, /* %f */ + const char *lastRestartPointFname); /* %r */ + +#endif /* ARCHIVE_H */ diff --git a/src/tools/msvc/Mkvcbuild.pm b/src/tools/msvc/Mkvcbuild.pm index ee49424d6f..f1c9ddf4a0 100644 --- a/src/tools/msvc/Mkvcbuild.pm +++ b/src/tools/msvc/Mkvcbuild.pm @@ -133,7 +133,7 @@ sub mkvcbuild } our @pgcommonallfiles = qw( - base64.c checksum_helper.c compression.c + archive.c base64.c checksum_helper.c compression.c config_info.c controldata_utils.c d2s.c encnames.c exec.c f2s.c file_perm.c file_utils.c hashfn.c ip.c jsonapi.c keywords.c kwlookup.c link-canary.c md5_common.c percentrepl.c