PG-1862 Use single argument for wrapped command

AndersAstrand · jeltz · AndersAstrand · commit 0924dc58f0c9 · 2025-08-12T14:23:10.000+02:00
Use a single argument for the wrapped command in the archivation wrappers.

Instead of giving all of the arguments of the command separately and trying
to figure out which one should be replaced by the path to the unencrypted
WAL segment, we take a single argument and do % parameter replacement similar
to what postgres does with archive_command and restore_command.

This also mean that we can simplify by using system() instead of exec().

Co-authored-by: Andreas Karlsson &lt;andreas.karlsson@percona.com&gt;
diff --git a/contrib/pg_tde/src/bin/pg_tde_archive_decrypt.c b/contrib/pg_tde/src/bin/pg_tde_archive_decrypt.c
@@ -1,12 +1,9 @@
 #include "postgres_fe.h"
 
-#include <fcntl.h>
-#include <sys/wait.h>
-#include <unistd.h>
-
 #include "access/xlog_internal.h"
 #include "access/xlog_smgr.h"
 #include "common/logging.h"
+#include "common/percentrepl.h"
 
 #include "access/pg_tde_fe_init.h"
 #include "access/pg_tde_xlog_smgr.h"
@@ -116,26 +113,38 @@ write_decrypted_segment(const char *segpath, const char *segname, const char *tm
 static void
 usage(const char *progname)
 {
-	printf(_("%s wraps an archive command to make it archive unencrypted WAL.\n\n"), progname);
-	printf(_("Usage:\n  %s %%p <archive command>\n\n"), progname);
-	printf(_("Options:\n"));
-	printf(_("  -V, --version output version information, then exit\n"));
-	printf(_("  -?, --help    show this help, then exit\n"));
+	printf(_("%s wraps an archive command to give the command unencrypted WAL.\n\n"), progname);
+	printf(_("Usage:\n"));
+	printf(_("  %s [OPTION]\n"), progname);
+	printf(_("  %s SOURCE-PATH ARCHIVE-COMMAND\n"), progname);
+	printf(_("\nOptions:\n"));
+	printf(_("  -V, --version   output version information, then exit\n"));
+	printf(_("  -?, --help      show this help, then exit\n"));
+	printf(_("  SOURCE-PATH     path of the source WAL segment to decrypt\n"));
+	printf(_("  ARCHIVE-COMMAND archive command to wrap, %%d will be replaced with the\n"
+			 "                  absolute path of the decrypted WAL segment.\n"));
+	printf(_("\n"));
+	printf(_("Note that any %%d parameter in ARCHIVE-COMMAND will have to be escaped as\n"
+			 "%%%%d if used as archive_command in postgresql.conf.\n"
+			 "e.g.\n"
+			 "  archive_command='%s %%p \"cp %%%%d /mnt/server/archivedir/%%f\"'\n"
+			 "or\n"
+			 "  archive_command='%s %%p \"pgbackrest --stanza=your_stanza archive-push %%%%d\"'\n"
+			 "In these examples %%f and %%p will be replaced as usual by PostgreSQL before\n"
+			 "%s is called.\n\n"), progname, progname, progname);
 }
 
 int
 main(int argc, char *argv[])
 {
 	const char *progname;
 	char	   *sourcepath;
+	char	   *command;
 	char	   *sep;
 	char	   *sourcename;
 	char		tmpdir[MAXPGPATH] = TMPFS_DIRECTORY "/pg_tde_archiveXXXXXX";
 	char		tmppath[MAXPGPATH];
 	bool		issegment;
-	pid_t		child;
-	int			status;
-	int			r;
 
 	pg_logging_init(argv[0]);
 	progname = get_progname(argv[0]);
@@ -154,14 +163,15 @@ main(int argc, char *argv[])
 		}
 	}
 
-	if (argc < 3)
+	if (argc != 3)
 	{
-		pg_log_error("too few arguments");
+		pg_log_error("wrong number of arguments, 2 expected");
 		pg_log_error_detail("Try \"%s --help\" for more information.", progname);
 		exit(1);
 	}
 
 	sourcepath = argv[1];
+	command = argv[2];
 
 	pg_tde_fe_init("pg_tde");
 	TDEXLogSmgrInit();
@@ -186,35 +196,19 @@ main(int argc, char *argv[])
 		s = stpcpy(s, "/");
 		stpcpy(s, sourcename);
 
-		for (int i = 2; i < argc; i++)
-			if (strcmp(sourcepath, argv[i]) == 0)
-				argv[i] = tmppath;
+		command = replace_percent_placeholders(command,
+											   "ARCHIVE-COMMAND", "d",
+											   tmppath);
 
 		write_decrypted_segment(sourcepath, sourcename, tmppath);
 	}
+	else
+		command = replace_percent_placeholders(command,
+											   "ARCHIVE-COMMAND", "d",
+											   sourcepath);
 
-	child = fork();
-	if (child == 0)
-	{
-		if (execvp(argv[2], argv + 2) < 0)
-			pg_fatal("exec failed: %m");
-	}
-	else if (child < 0)
-		pg_fatal("could not create background process: %m");
-
-	r = waitpid(child, &status, 0);
-	if (r == (pid_t) -1)
-		pg_fatal("could not wait for child process: %m");
-	if (r != child)
-		pg_fatal("child %d died, expected %d", (int) r, (int) child);
-	if (status != 0)
-	{
-		char	   *reason = wait_result_to_str(status);
-
-		pg_fatal("%s", reason);
-		/* keep lsan happy */
-		free(reason);
-	}
+	if (system(command) != 0)
+		pg_fatal("ARCHIVE-COMMAND \"%s\" failed: %m", command);
 
 	if (issegment)
 	{
diff --git a/contrib/pg_tde/src/bin/pg_tde_restore_encrypt.c b/contrib/pg_tde/src/bin/pg_tde_restore_encrypt.c
@@ -1,12 +1,9 @@
 #include "postgres_fe.h"
 
-#include <fcntl.h>
-#include <sys/wait.h>
-#include <unistd.h>
-
 #include "access/xlog_internal.h"
 #include "access/xlog_smgr.h"
 #include "common/logging.h"
+#include "common/percentrepl.h"
 
 #include "access/pg_tde_fe_init.h"
 #include "access/pg_tde_xlog_smgr.h"
@@ -110,27 +107,38 @@ write_encrypted_segment(const char *segpath, const char *segname, const char *tm
 static void
 usage(const char *progname)
 {
-	printf(_("%s wraps a restore command to make it write encrypted WAL to pg_wal.\n\n"), progname);
-	printf(_("Usage:\n  %s %%f %%p <restore command>\n\n"), progname);
-	printf(_("Options:\n"));
-	printf(_("  -V, --version output version information, then exit\n"));
-	printf(_("  -?, --help    show this help, then exit\n"));
+	printf(_("%s wraps a restore command to encrypt its returned WAL.\n\n"), progname);
+	printf(_("Usage:\n"));
+	printf(_("  %s [OPTION]\n"), progname);
+	printf(_("  %s DEST-PATH RESTORE-COMMAND\n"), progname);
+	printf(_("\nOptions:\n"));
+	printf(_("  -V, --version   output version information, then exit\n"));
+	printf(_("  -?, --help      show this help, then exit\n"));
+	printf(_("  DEST-PATH       path where the encrypted WAL segment should be written\n"));
+	printf(_("  RESTORE-COMMAND restore command to wrap, %%d will be replaced with the\n"
+			 "                  path where it should write the unencrypted WAL segment\n"));
+	printf(_("\n"));
+	printf(_("Note that any %%d parameter in RESTORE-COMMAND will have to be escaped as\n"
+			 "%%%%d if used as restore_command in postgresql.conf.\n"
+			 "e.g.\n"
+			 "  restore_command='%s %%p \"cp /mnt/server/archivedir/%%f %%%%d\"'\n"
+			 "or\n"
+			 "  restore_command='%s %%p \"pgbackrest --stanza=your_stanza archive-get %%f \\\"%%%%d\\\"\"'\n"
+			 "In these examples %%f and %%p will be replaced as usual by PostgreSQL before\n"
+			 "%s is called.\n\n"), progname, progname, progname);
 }
 
 int
 main(int argc, char *argv[])
 {
 	const char *progname;
-	char	   *sourcename;
+	char	   *command;
 	char	   *targetpath;
 	char	   *sep;
 	char	   *targetname;
 	char		tmpdir[MAXPGPATH] = TMPFS_DIRECTORY "/pg_tde_restoreXXXXXX";
 	char		tmppath[MAXPGPATH];
 	bool		issegment;
-	pid_t		child;
-	int			status;
-	int			r;
 
 	pg_logging_init(argv[0]);
 	progname = get_progname(argv[0]);
@@ -149,15 +157,15 @@ main(int argc, char *argv[])
 		}
 	}
 
-	if (argc < 4)
+	if (argc != 3)
 	{
-		pg_log_error("too few arguments");
+		pg_log_error("wrong number of arguments, 2 expected");
 		pg_log_error_detail("Try \"%s --help\" for more information.", progname);
 		exit(1);
 	}
 
-	sourcename = argv[1];
-	targetpath = argv[2];
+	targetpath = argv[1];
+	command = argv[2];
 
 	pg_tde_fe_init("pg_tde");
 	TDEXLogSmgrInit();
@@ -169,7 +177,7 @@ main(int argc, char *argv[])
 	else
 		targetname = targetpath;
 
-	issegment = is_segment(sourcename);
+	issegment = is_segment(targetname);
 
 	if (issegment)
 	{
@@ -182,37 +190,21 @@ main(int argc, char *argv[])
 		s = stpcpy(s, "/");
 		stpcpy(s, targetname);
 
-		for (int i = 2; i < argc; i++)
-			if (strcmp(targetpath, argv[i]) == 0)
-				argv[i] = tmppath;
-	}
-
-	child = fork();
-	if (child == 0)
-	{
-		if (execvp(argv[3], argv + 3) < 0)
-			pg_fatal("exec failed: %m");
+		command = replace_percent_placeholders(command,
+											   "RESTORE-COMMAND", "d",
+											   tmppath);
 	}
-	else if (child < 0)
-		pg_fatal("could not create background process: %m");
-
-	r = waitpid(child, &status, 0);
-	if (r == (pid_t) -1)
-		pg_fatal("could not wait for child process: %m");
-	if (r != child)
-		pg_fatal("child %d died, expected %d", (int) r, (int) child);
-	if (status != 0)
-	{
-		char	   *reason = wait_result_to_str(status);
+	else
+		command = replace_percent_placeholders(command,
+											   "RESTORE-COMMAND", "d",
+											   targetpath);
 
-		pg_fatal("%s", reason);
-		/* keep lsan happy */
-		free(reason);
-	}
+	if (system(command) != 0)
+		pg_fatal("RESTORE-COMMAND \"%s\" failed: %m", command);
 
 	if (issegment)
 	{
-		write_encrypted_segment(targetpath, sourcename, tmppath);
+		write_encrypted_segment(targetpath, targetname, tmppath);
 
 		if (unlink(tmppath) < 0)
 			pg_log_warning("could not remove file \"%s\": %m", tmppath);
diff --git a/contrib/pg_tde/t/wal_archiving.pl b/contrib/pg_tde/t/wal_archiving.pl
@@ -21,7 +21,8 @@
 $primary->append_conf('postgresql.conf', "checkpoint_timeout = 1h");
 $primary->append_conf('postgresql.conf', "archive_mode = on");
 $primary->append_conf('postgresql.conf',
-	"archive_command = 'pg_tde_archive_decrypt %p cp %p $archive_dir/%f'");
+	"archive_command = 'pg_tde_archive_decrypt %p \"cp %%d $archive_dir/%f\"'"
+);
 $primary->start;
 
 $primary->safe_psql('postgres', "CREATE EXTENSION pg_tde;");
@@ -75,7 +76,7 @@
 my $replica = PostgreSQL::Test::Cluster->new('replica');
 $replica->init_from_backup($primary, 'backup');
 $replica->append_conf('postgresql.conf',
-	"restore_command = 'pg_tde_restore_encrypt %f %p cp $archive_dir/%f %p'");
+	"restore_command = 'pg_tde_restore_encrypt %p \"cp $archive_dir/%f %%d\"'");
 $replica->append_conf('postgresql.conf', "recovery_target_action = promote");
 $replica->set_recovery_mode;
 $replica->start;
