Refactor code for restoring files via shell commands
authorMichael Paquier <michael@paquier.xyz>
Wed, 18 Jan 2023 02:15:48 +0000 (11:15 +0900)
committerMichael Paquier <michael@paquier.xyz>
Wed, 18 Jan 2023 02:15:48 +0000 (11:15 +0900)
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, as long as it is possible to
identify if a command should:
- Issue a FATAL on signal.
- Exit immediately on SIGTERM.

While on it, this removes src/common/archive.c and its associated
header.  Since the introduction of c96de2c, BuildRestoreCommand() has
become a simple wrapper of replace_percent_placeholders() able to call
make_native_path().  This simplifies shell_restore.c as long as
RestoreArchivedFile() includes a call to make_native_path().

Author: Nathan Bossart
Reviewed-by: Andres Freund, Michael Paquier
Discussion: https://postgr.es/m/20221227192449.GA3672473@nathanxps13

src/backend/access/transam/shell_restore.c
src/backend/access/transam/xlogarchive.c
src/common/Makefile
src/common/archive.c [deleted file]
src/common/meson.build
src/fe_utils/archive.c
src/include/common/archive.h [deleted file]
src/tools/msvc/Mkvcbuild.pm

index 7753a7d667f3c14b58867f031bdf20baea5d61c7..8458209f490ba374d3a3be6662a38ba3ebee46ea 100644 (file)
 
 #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,
+                                  bool exitOnSigterm,
                                   uint32 wait_event_info,
-                                  const char *lastRestartPointFileName);
+                                  int fail_elevel);
 
 /*
  * Attempt to execute a shell-based restore command.
@@ -40,25 +40,17 @@ 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 +76,13 @@ 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,  /* failOnSignal */
+                                true,  /* exitOnSigterm */
+                                WAIT_EVENT_RESTORE_COMMAND, DEBUG2);
+   pfree(cmd);
 
-   return (rc == 0);
+   return ret;
 }
 
 /*
@@ -103,9 +91,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 +107,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);
 }
 
 /*
@@ -125,26 +123,21 @@ 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.
+ * 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 +146,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 +166,6 @@ ExecuteRecoveryCommand(const char *command, const char *commandName,
                (errmsg("%s \"%s\": %s", commandName,
                        command, wait_result_to_str(rc))));
    }
+
+   return (rc == 0);
 }
index b5cb060d55635609a4ffb098baaf190e22b4f3ed..4b89addf9762d2d9b39de80909e297f4a6a8c572 100644 (file)
@@ -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"
index 113029bf7b91519746144f3a4d4a31a23c664714..2f424a57353327c2a6e5171737d78bf6d2dc819e 100644 (file)
@@ -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 (file)
index 641a58e..0000000
+++ /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;
-}
index 41bd58ebdf19a5c4df3940f30340ed0dde0f2c61..1caa1fed0432cdacb460b359a942b8c0db2b1cb9 100644 (file)
@@ -1,7 +1,6 @@
 # Copyright (c) 2022-2023, PostgreSQL Global Development Group
 
 common_sources = files(
-  'archive.c',
   'base64.c',
   'checksum_helper.c',
   'compression.c',
index eb1c930ae7f27e1f7152a1d23e68911ef1025f09..c1ce250c907e4523a90bf13d4a47859320e3a289 100644 (file)
@@ -19,8 +19,8 @@
 #include <sys/stat.h>
 
 #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 (file)
index 9519677..0000000
+++ /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 */
index f1c9ddf4a025e1381b092154f589706e4134f0a4..ee49424d6fc7ee0b959d323babe62b686a327a9c 100644 (file)
@@ -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