From c710f5a9e294b198ce6bb2e8d9404cb26a76b913 Mon Sep 17 00:00:00 2001
From: Nathan Bossart <nathandbossart@gmail.com>
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 <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
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

