PG-1862 Use single argument for wrapped command

AndersAstrand · jeltz · AndersAstrand · commit 7a825ad7c583 · 2025-08-12T17:43:58.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().

We also clean up usage instructions and make the two wrappers more
symmetrical by requiring the same parameters.

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,40 @@ 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 DEST-NAME 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(_("  DEST-NAME       name of the WAL file to send to archive\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 %%f %%p \"cp %%%%d /mnt/server/archivedir/%%f\"'\n"
+			 "or\n"
+			 "  archive_command='%s %%f %%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	   *targetname;
+	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 +165,16 @@ main(int argc, char *argv[])
 		}
 	}
 
-	if (argc < 3)
+	if (argc != 4)
 	{
-		pg_log_error("too few arguments");
+		pg_log_error("wrong number of arguments, 3 expected");
 		pg_log_error_detail("Try \"%s --help\" for more information.", progname);
 		exit(1);
 	}
 
-	sourcepath = argv[1];
+	targetname = argv[1];
+	sourcepath = argv[2];
+	command = argv[3];
 
 	pg_tde_fe_init("pg_tde");
 	TDEXLogSmgrInit();
@@ -173,7 +186,7 @@ main(int argc, char *argv[])
 	else
 		sourcename = sourcepath;
 
-	issegment = is_segment(sourcename);
+	issegment = is_segment(targetname);
 
 	if (issegment)
 	{
@@ -186,35 +199,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;
-
-		write_decrypted_segment(sourcepath, sourcename, tmppath);
-	}
+		command = replace_percent_placeholders(command,
+											   "ARCHIVE-COMMAND", "d",
+											   tmppath);
 
-	child = fork();
-	if (child == 0)
-	{
-		if (execvp(argv[2], argv + 2) < 0)
-			pg_fatal("exec failed: %m");
+		write_decrypted_segment(sourcepath, targetname, 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,
+											   "ARCHIVE-COMMAND", "d",
+											   sourcepath);
 
-		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,11 +107,26 @@ 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 SOURCE-NAME 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(_("  SOURCE-NAME     name of the WAL file to retrieve from archive\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 %%f %%p \"cp /mnt/server/archivedir/%%f %%%%d\"'\n"
+			 "or\n"
+			 "  restore_command='%s %%f %%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
@@ -123,14 +135,12 @@ main(int argc, char *argv[])
 	const char *progname;
 	char	   *sourcename;
 	char	   *targetpath;
+	char	   *command;
 	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 +159,16 @@ main(int argc, char *argv[])
 		}
 	}
 
-	if (argc < 4)
+	if (argc != 4)
 	{
-		pg_log_error("too few arguments");
+		pg_log_error("wrong number of arguments, 3 expected");
 		pg_log_error_detail("Try \"%s --help\" for more information.", progname);
 		exit(1);
 	}
 
 	sourcename = argv[1];
 	targetpath = argv[2];
+	command = argv[3];
 
 	pg_tde_fe_init("pg_tde");
 	TDEXLogSmgrInit();
@@ -182,33 +193,17 @@ 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)
 	{
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 %f %p \"cp %%d $archive_dir/%f\"'"
+);
 $primary->start;
 
 $primary->safe_psql('postgres', "CREATE EXTENSION pg_tde;");
@@ -75,7 +76,8 @@
 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 %f %p \"cp $archive_dir/%f %%d\"'"
+);
 $replica->append_conf('postgresql.conf', "recovery_target_action = promote");
 $replica->set_recovery_mode;
 $replica->start;
