From c710f5a9e294b198ce6bb2e8d9404cb26a76b913 Mon Sep 17 00:00:00 2001 From: Nathan Bossart Date: Fri, 23 Dec 2022 16:53:38 -0800 Subject: [PATCH v8 1/2] Refactor code for restoring files via shell. Presently, restore_command uses a different code path than archive_cleanup_command and recovery_end_command. These code paths are similar and can be easily combined. --- src/backend/access/transam/shell_restore.c | 99 ++++++++++------------ src/backend/access/transam/xlogarchive.c | 1 - src/common/Makefile | 1 - src/common/archive.c | 60 ------------- src/common/meson.build | 1 - src/common/percentrepl.c | 13 +-- src/fe_utils/archive.c | 11 ++- src/include/common/archive.h | 21 ----- src/tools/msvc/Mkvcbuild.pm | 2 +- 9 files changed, 56 insertions(+), 153 deletions(-) delete mode 100644 src/common/archive.c delete mode 100644 src/include/common/archive.h diff --git a/src/backend/access/transam/shell_restore.c b/src/backend/access/transam/shell_restore.c index 7753a7d667..4752be0b9f 100644 --- a/src/backend/access/transam/shell_restore.c +++ b/src/backend/access/transam/shell_restore.c @@ -20,16 +20,14 @@ #include "access/xlogarchive.h" #include "access/xlogrecovery.h" -#include "common/archive.h" #include "common/percentrepl.h" #include "storage/ipc.h" #include "utils/wait_event.h" -static void ExecuteRecoveryCommand(const char *command, +static bool ExecuteRecoveryCommand(const char *command, const char *commandName, - bool failOnSignal, - uint32 wait_event_info, - const char *lastRestartPointFileName); + bool failOnSignal, bool exitOnSigterm, + uint32 wait_event_info, int fail_elevel); /* * Attempt to execute a shell-based restore command. @@ -40,25 +38,16 @@ bool shell_restore(const char *file, const char *path, const char *lastRestartPointFileName) { + char *nativePath = pstrdup(path); char *cmd; - int rc; + bool ret; /* Build the restore command to execute */ - cmd = BuildRestoreCommand(recoveryRestoreCommand, path, file, - lastRestartPointFileName); - - ereport(DEBUG3, - (errmsg_internal("executing restore command \"%s\"", cmd))); - - /* - * Copy xlog from archival storage to XLOGDIR - */ - fflush(NULL); - pgstat_report_wait_start(WAIT_EVENT_RESTORE_COMMAND); - rc = system(cmd); - pgstat_report_wait_end(); - - pfree(cmd); + 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 @@ -84,17 +73,11 @@ shell_restore(const char *file, const char *path, * * We treat hard shell errors such as "command not found" as fatal, too. */ - if (rc != 0) - { - 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", - file, wait_result_to_str(rc)))); - } + ret = ExecuteRecoveryCommand(cmd, "restore_command", true, true, + WAIT_EVENT_RESTORE_COMMAND, DEBUG2); + pfree(cmd); - return (rc == 0); + return ret; } /* @@ -103,9 +86,14 @@ shell_restore(const char *file, const char *path, void shell_archive_cleanup(const char *lastRestartPointFileName) { - ExecuteRecoveryCommand(archiveCleanupCommand, "archive_cleanup_command", - false, WAIT_EVENT_ARCHIVE_CLEANUP_COMMAND, - 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); } /* @@ -114,9 +102,14 @@ shell_archive_cleanup(const char *lastRestartPointFileName) void shell_recovery_end(const char *lastRestartPointFileName) { - ExecuteRecoveryCommand(recoveryEndCommand, "recovery_end_command", true, - WAIT_EVENT_RECOVERY_END_COMMAND, - 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); } /* @@ -124,27 +117,22 @@ shell_recovery_end(const char *lastRestartPointFileName) * * '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. + * '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. * - * This is currently used for recovery_end_command and archive_cleanup_command. + * Returns whether the command succeeded. */ -static void +static bool ExecuteRecoveryCommand(const char *command, const char *commandName, - bool failOnSignal, uint32 wait_event_info, - const char *lastRestartPointFileName) + bool failOnSignal, bool exitOnSigterm, + uint32 wait_event_info, int fail_elevel) { - char *xlogRecoveryCmd; int rc; Assert(command && commandName); - /* - * construct the command to be executed - */ - xlogRecoveryCmd = replace_percent_placeholders(command, commandName, "r", - lastRestartPointFileName); - ereport(DEBUG3, (errmsg_internal("executing %s \"%s\"", commandName, command))); @@ -153,18 +141,19 @@ ExecuteRecoveryCommand(const char *command, const char *commandName, */ fflush(NULL); pgstat_report_wait_start(wait_event_info); - rc = system(xlogRecoveryCmd); + rc = system(command); pgstat_report_wait_end(); - pfree(xlogRecoveryCmd); - 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 : WARNING, + 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 @@ -172,4 +161,6 @@ ExecuteRecoveryCommand(const char *command, const char *commandName, (errmsg("%s \"%s\": %s", commandName, command, wait_result_to_str(rc)))); } + + return (rc == 0); } diff --git a/src/backend/access/transam/xlogarchive.c b/src/backend/access/transam/xlogarchive.c index b5cb060d55..4b89addf97 100644 --- a/src/backend/access/transam/xlogarchive.c +++ b/src/backend/access/transam/xlogarchive.c @@ -22,7 +22,6 @@ #include "access/xlog.h" #include "access/xlog_internal.h" #include "access/xlogarchive.h" -#include "common/archive.h" #include "miscadmin.h" #include "pgstat.h" #include "postmaster/startup.h" diff --git a/src/common/Makefile b/src/common/Makefile index 113029bf7b..2f424a5735 100644 --- a/src/common/Makefile +++ b/src/common/Makefile @@ -46,7 +46,6 @@ 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 deleted file mode 100644 index 641a58ee88..0000000000 --- a/src/common/archive.c +++ /dev/null @@ -1,60 +0,0 @@ -/*------------------------------------------------------------------------- - * - * 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 41bd58ebdf..1caa1fed04 100644 --- a/src/common/meson.build +++ b/src/common/meson.build @@ -1,7 +1,6 @@ # Copyright (c) 2022-2023, PostgreSQL Global Development Group common_sources = files( - 'archive.c', 'base64.c', 'checksum_helper.c', 'compression.c', diff --git a/src/common/percentrepl.c b/src/common/percentrepl.c index d78571fec0..c083fd9b89 100644 --- a/src/common/percentrepl.c +++ b/src/common/percentrepl.c @@ -38,11 +38,6 @@ * This throws an error for an unsupported placeholder or a "%" at the end of * the input string. * - * A value may be NULL. If the corresponding placeholder is found in the - * input string, it will be treated as if an unsupported placeholder was used. - * This allows callers to share a "letters" specification but vary the - * actually supported placeholders at run time. - * * This functions is meant for cases where all the values are readily * available or cheap to compute and most invocations will use most values * (for example for archive_command). Also, it requires that all values are @@ -101,12 +96,8 @@ replace_percent_placeholders(const char *instr, const char *param_name, const ch if (*sp == *lp) { - if (val) - { - appendStringInfoString(&result, val); - found = true; - } - /* If val is NULL, we will report an error. */ + appendStringInfoString(&result, val); + found = true; break; } } diff --git a/src/fe_utils/archive.c b/src/fe_utils/archive.c index eb1c930ae7..c1ce250c90 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,13 +41,18 @@ 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); - xlogRestoreCmd = BuildRestoreCommand(restoreCommand, xlogpath, - xlogfname, NULL); + nativePath = pstrdup(xlogpath); + make_native_path(nativePath); + xlogRestoreCmd = replace_percent_placeholders(restoreCommand, + "restore_command", "fp", + xlogfname, nativePath); + pfree(nativePath); /* * Execute restore_command, which should copy the missing file from diff --git a/src/include/common/archive.h b/src/include/common/archive.h deleted file mode 100644 index 95196772c9..0000000000 --- a/src/include/common/archive.h +++ /dev/null @@ -1,21 +0,0 @@ -/*------------------------------------------------------------------------- - * - * 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 f1c9ddf4a0..ee49424d6f 100644 --- a/src/tools/msvc/Mkvcbuild.pm +++ b/src/tools/msvc/Mkvcbuild.pm @@ -133,7 +133,7 @@ sub mkvcbuild } our @pgcommonallfiles = qw( - archive.c base64.c checksum_helper.c compression.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 -- 2.25.1