Common function for percent placeholder replacement

Started by Peter Eisentrautabout 3 years ago16 messages
#1Peter Eisentraut
peter.eisentraut@enterprisedb.com
1 attachment(s)

There are a number of places where a shell command is constructed with
percent-placeholders (like %x). First, it's obviously cumbersome to
have to open-code this several times. Second, each of those pieces of
code silently encodes some edge case behavior, such as what to do with
unrecognized placeholders. (I remember when I last did one of these, I
stared very hard at the existing code instances to figure out what they
would do.) We now also have a newer instance in basebackup_to_shell.c
that has different behavior in such cases. (Maybe it's better, but it
would be good to be explicit and consistent about this.)

This patch factors out this logic into a separate function. I have
documented to "old" error handling (which is to not handle them) and
brutally converted basebackup_to_shell.c to use that. We could also
adopt the new behavior; now there is only a single place to change for that.

Note that this is only used for shell commands with placeholders, not
for other places with placeholders, such as prompts and log line
prefixes, which would (IMO) need a different API that wouldn't be quite
as compact. This is explained in the code comments.

Attachments:

v1-0001-Common-function-for-percent-placeholder-replaceme.patchtext/plain; charset=UTF-8; name=v1-0001-Common-function-for-percent-placeholder-replaceme.patchDownload
From e0e44dbffafa50bf15bca6a03e98783a82ce072c Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter@eisentraut.org>
Date: Wed, 14 Dec 2022 08:17:25 +0100
Subject: [PATCH v1] Common function for percent placeholder replacement

There are a number of places where a shell command is constructed with
percent-placeholders (like %x).  It's cumbersome to have to open-code
this several times.  This factors out this logic into a separate
function.  This also allows us to ensure consistency for and document
some subtle behaviors, such as what to do with unrecognized
placeholders.
---
 .../basebackup_to_shell/basebackup_to_shell.c |  55 +--------
 src/backend/access/transam/xlogarchive.c      |  45 +-------
 src/backend/libpq/be-secure-common.c          |  38 ++----
 src/backend/postmaster/shell_archive.c        |  59 ++--------
 src/common/Makefile                           |   1 +
 src/common/archive.c                          |  81 ++-----------
 src/common/meson.build                        |   1 +
 src/common/percentrepl.c                      | 108 ++++++++++++++++++
 src/include/common/percentrepl.h              |  18 +++
 9 files changed, 161 insertions(+), 245 deletions(-)
 create mode 100644 src/common/percentrepl.c
 create mode 100644 src/include/common/percentrepl.h

diff --git a/contrib/basebackup_to_shell/basebackup_to_shell.c b/contrib/basebackup_to_shell/basebackup_to_shell.c
index bdaf67a4c8..9a3d57c64b 100644
--- a/contrib/basebackup_to_shell/basebackup_to_shell.c
+++ b/contrib/basebackup_to_shell/basebackup_to_shell.c
@@ -12,6 +12,7 @@
 
 #include "access/xact.h"
 #include "backup/basebackup_target.h"
+#include "common/percentrepl.h"
 #include "miscadmin.h"
 #include "storage/fd.h"
 #include "utils/acl.h"
@@ -208,59 +209,7 @@ static char *
 shell_construct_command(const char *base_command, const char *filename,
 						const char *target_detail)
 {
-	StringInfoData buf;
-	const char *c;
-
-	initStringInfo(&buf);
-	for (c = base_command; *c != '\0'; ++c)
-	{
-		/* Anything other than '%' is copied verbatim. */
-		if (*c != '%')
-		{
-			appendStringInfoChar(&buf, *c);
-			continue;
-		}
-
-		/* Any time we see '%' we eat the following character as well. */
-		++c;
-
-		/*
-		 * The following character determines what we insert here, or may
-		 * cause us to throw an error.
-		 */
-		if (*c == '%')
-		{
-			/* '%%' is replaced by a single '%' */
-			appendStringInfoChar(&buf, '%');
-		}
-		else if (*c == 'f')
-		{
-			/* '%f' is replaced by the filename */
-			appendStringInfoString(&buf, filename);
-		}
-		else if (*c == 'd')
-		{
-			/* '%d' is replaced by the target detail */
-			appendStringInfoString(&buf, target_detail);
-		}
-		else if (*c == '\0')
-		{
-			/* Incomplete escape sequence, expected a character afterward */
-			ereport(ERROR,
-					errcode(ERRCODE_SYNTAX_ERROR),
-					errmsg("shell command ends unexpectedly after escape character \"%%\""));
-		}
-		else
-		{
-			/* Unknown escape sequence */
-			ereport(ERROR,
-					errcode(ERRCODE_SYNTAX_ERROR),
-					errmsg("shell command contains unexpected escape sequence \"%c\"",
-						   *c));
-		}
-	}
-
-	return buf.data;
+	return replace_percent_placeholders(base_command, "df", (const char *[]){target_detail, filename});
 }
 
 /*
diff --git a/src/backend/access/transam/xlogarchive.c b/src/backend/access/transam/xlogarchive.c
index e2b7176f2f..0f74ccb49c 100644
--- a/src/backend/access/transam/xlogarchive.c
+++ b/src/backend/access/transam/xlogarchive.c
@@ -23,6 +23,7 @@
 #include "access/xlog_internal.h"
 #include "access/xlogarchive.h"
 #include "common/archive.h"
+#include "common/percentrepl.h"
 #include "miscadmin.h"
 #include "pgstat.h"
 #include "postmaster/startup.h"
@@ -291,11 +292,8 @@ void
 ExecuteRecoveryCommand(const char *command, const char *commandName,
 					   bool failOnSignal, uint32 wait_event_info)
 {
-	char		xlogRecoveryCmd[MAXPGPATH];
+	char	   *xlogRecoveryCmd;
 	char		lastRestartPointFname[MAXPGPATH];
-	char	   *dp;
-	char	   *endp;
-	const char *sp;
 	int			rc;
 	XLogSegNo	restartSegNo;
 	XLogRecPtr	restartRedoPtr;
@@ -316,42 +314,7 @@ ExecuteRecoveryCommand(const char *command, const char *commandName,
 	/*
 	 * construct the command to be executed
 	 */
-	dp = xlogRecoveryCmd;
-	endp = xlogRecoveryCmd + MAXPGPATH - 1;
-	*endp = '\0';
-
-	for (sp = command; *sp; sp++)
-	{
-		if (*sp == '%')
-		{
-			switch (sp[1])
-			{
-				case 'r':
-					/* %r: filename of last restartpoint */
-					sp++;
-					strlcpy(dp, lastRestartPointFname, endp - dp);
-					dp += strlen(dp);
-					break;
-				case '%':
-					/* convert %% to a single % */
-					sp++;
-					if (dp < endp)
-						*dp++ = *sp;
-					break;
-				default:
-					/* otherwise treat the % as not special */
-					if (dp < endp)
-						*dp++ = *sp;
-					break;
-			}
-		}
-		else
-		{
-			if (dp < endp)
-				*dp++ = *sp;
-		}
-	}
-	*dp = '\0';
+	xlogRecoveryCmd = replace_percent_placeholders(command, "r", (const char *[]){lastRestartPointFname});
 
 	ereport(DEBUG3,
 			(errmsg_internal("executing %s \"%s\"", commandName, command)));
@@ -364,6 +327,8 @@ ExecuteRecoveryCommand(const char *command, const char *commandName,
 	rc = system(xlogRecoveryCmd);
 	pgstat_report_wait_end();
 
+	pfree(xlogRecoveryCmd);
+
 	if (rc != 0)
 	{
 		/*
diff --git a/src/backend/libpq/be-secure-common.c b/src/backend/libpq/be-secure-common.c
index 2719ce1403..912f12d13e 100644
--- a/src/backend/libpq/be-secure-common.c
+++ b/src/backend/libpq/be-secure-common.c
@@ -22,6 +22,7 @@
 #include <sys/stat.h>
 #include <unistd.h>
 
+#include "common/percentrepl.h"
 #include "common/string.h"
 #include "libpq/libpq.h"
 #include "storage/fd.h"
@@ -39,8 +40,7 @@ int
 run_ssl_passphrase_command(const char *prompt, bool is_server_start, char *buf, int size)
 {
 	int			loglevel = is_server_start ? ERROR : LOG;
-	StringInfoData command;
-	char	   *p;
+	char	   *command;
 	FILE	   *fh;
 	int			pclose_rc;
 	size_t		len = 0;
@@ -49,37 +49,15 @@ run_ssl_passphrase_command(const char *prompt, bool is_server_start, char *buf,
 	Assert(size > 0);
 	buf[0] = '\0';
 
-	initStringInfo(&command);
+	command = replace_percent_placeholders(ssl_passphrase_command, "p", (const char *[]){prompt});
 
-	for (p = ssl_passphrase_command; *p; p++)
-	{
-		if (p[0] == '%')
-		{
-			switch (p[1])
-			{
-				case 'p':
-					appendStringInfoString(&command, prompt);
-					p++;
-					break;
-				case '%':
-					appendStringInfoChar(&command, '%');
-					p++;
-					break;
-				default:
-					appendStringInfoChar(&command, p[0]);
-			}
-		}
-		else
-			appendStringInfoChar(&command, p[0]);
-	}
-
-	fh = OpenPipeStream(command.data, "r");
+	fh = OpenPipeStream(command, "r");
 	if (fh == NULL)
 	{
 		ereport(loglevel,
 				(errcode_for_file_access(),
 				 errmsg("could not execute command \"%s\": %m",
-						command.data)));
+						command)));
 		goto error;
 	}
 
@@ -91,7 +69,7 @@ run_ssl_passphrase_command(const char *prompt, bool is_server_start, char *buf,
 			ereport(loglevel,
 					(errcode_for_file_access(),
 					 errmsg("could not read from command \"%s\": %m",
-							command.data)));
+							command)));
 			goto error;
 		}
 	}
@@ -111,7 +89,7 @@ run_ssl_passphrase_command(const char *prompt, bool is_server_start, char *buf,
 		ereport(loglevel,
 				(errcode_for_file_access(),
 				 errmsg("command \"%s\" failed",
-						command.data),
+						command),
 				 errdetail_internal("%s", wait_result_to_str(pclose_rc))));
 		goto error;
 	}
@@ -120,7 +98,7 @@ run_ssl_passphrase_command(const char *prompt, bool is_server_start, char *buf,
 	len = pg_strip_crlf(buf);
 
 error:
-	pfree(command.data);
+	pfree(command);
 	return len;
 }
 
diff --git a/src/backend/postmaster/shell_archive.c b/src/backend/postmaster/shell_archive.c
index 71d567065a..77c7b34535 100644
--- a/src/backend/postmaster/shell_archive.c
+++ b/src/backend/postmaster/shell_archive.c
@@ -18,6 +18,7 @@
 #include <sys/wait.h>
 
 #include "access/xlog.h"
+#include "common/percentrepl.h"
 #include "pgstat.h"
 #include "postmaster/pgarch.h"
 
@@ -44,58 +45,18 @@ shell_archive_configured(void)
 static bool
 shell_archive_file(const char *file, const char *path)
 {
-	char		xlogarchcmd[MAXPGPATH];
-	char	   *dp;
-	char	   *endp;
-	const char *sp;
+	char	   *xlogarchcmd;
+	char	   *nativePath = NULL;
 	int			rc;
 
-	/*
-	 * construct the command to be executed
-	 */
-	dp = xlogarchcmd;
-	endp = xlogarchcmd + MAXPGPATH - 1;
-	*endp = '\0';
-
-	for (sp = XLogArchiveCommand; *sp; sp++)
+	if (path)
 	{
-		if (*sp == '%')
-		{
-			switch (sp[1])
-			{
-				case 'p':
-					/* %p: relative path of source file */
-					sp++;
-					strlcpy(dp, path, endp - dp);
-					make_native_path(dp);
-					dp += strlen(dp);
-					break;
-				case 'f':
-					/* %f: filename of source file */
-					sp++;
-					strlcpy(dp, file, endp - dp);
-					dp += strlen(dp);
-					break;
-				case '%':
-					/* convert %% to a single % */
-					sp++;
-					if (dp < endp)
-						*dp++ = *sp;
-					break;
-				default:
-					/* otherwise treat the % as not special */
-					if (dp < endp)
-						*dp++ = *sp;
-					break;
-			}
-		}
-		else
-		{
-			if (dp < endp)
-				*dp++ = *sp;
-		}
+		nativePath = pstrdup(path);
+		make_native_path(nativePath);
 	}
-	*dp = '\0';
+
+	xlogarchcmd = replace_percent_placeholders(XLogArchiveCommand, "fp",
+											   (const char *[]){file, nativePath});
 
 	ereport(DEBUG3,
 			(errmsg_internal("executing archive command \"%s\"",
@@ -155,6 +116,8 @@ shell_archive_file(const char *file, const char *path)
 		return false;
 	}
 
+	pfree(xlogarchcmd);
+
 	elog(DEBUG1, "archived write-ahead log file \"%s\"", file);
 	return true;
 }
diff --git a/src/common/Makefile b/src/common/Makefile
index e9af7346c9..d88a3bf945 100644
--- a/src/common/Makefile
+++ b/src/common/Makefile
@@ -65,6 +65,7 @@ OBJS_COMMON = \
 	kwlookup.o \
 	link-canary.o \
 	md5_common.o \
+	percentrepl.o \
 	pg_get_line.o \
 	pg_lzcompress.o \
 	pg_prng.o \
diff --git a/src/common/archive.c b/src/common/archive.c
index 47fc877c90..a30146934d 100644
--- a/src/common/archive.c
+++ b/src/common/archive.c
@@ -20,7 +20,7 @@
 #endif
 
 #include "common/archive.h"
-#include "lib/stringinfo.h"
+#include "common/percentrepl.h"
 
 /*
  * BuildRestoreCommand
@@ -41,81 +41,14 @@ BuildRestoreCommand(const char *restoreCommand,
 					const char *xlogfname,
 					const char *lastRestartPointFname)
 {
-	StringInfoData result;
-	const char *sp;
+	char	   *nativePath = NULL;
 
-	/*
-	 * Build the command to be executed.
-	 */
-	initStringInfo(&result);
-
-	for (sp = restoreCommand; *sp; sp++)
+	if (xlogpath)
 	{
-		if (*sp == '%')
-		{
-			switch (sp[1])
-			{
-				case 'p':
-					{
-						char	   *nativePath;
-
-						/* %p: relative path of target file */
-						if (xlogpath == NULL)
-						{
-							pfree(result.data);
-							return NULL;
-						}
-						sp++;
-
-						/*
-						 * This needs to use a placeholder to not modify the
-						 * input with the conversion done via
-						 * make_native_path().
-						 */
-						nativePath = pstrdup(xlogpath);
-						make_native_path(nativePath);
-						appendStringInfoString(&result,
-											   nativePath);
-						pfree(nativePath);
-						break;
-					}
-				case 'f':
-					/* %f: filename of desired file */
-					if (xlogfname == NULL)
-					{
-						pfree(result.data);
-						return NULL;
-					}
-					sp++;
-					appendStringInfoString(&result, xlogfname);
-					break;
-				case 'r':
-					/* %r: filename of last restartpoint */
-					if (lastRestartPointFname == NULL)
-					{
-						pfree(result.data);
-						return NULL;
-					}
-					sp++;
-					appendStringInfoString(&result,
-										   lastRestartPointFname);
-					break;
-				case '%':
-					/* convert %% to a single % */
-					sp++;
-					appendStringInfoChar(&result, *sp);
-					break;
-				default:
-					/* otherwise treat the % as not special */
-					appendStringInfoChar(&result, *sp);
-					break;
-			}
-		}
-		else
-		{
-			appendStringInfoChar(&result, *sp);
-		}
+		nativePath = pstrdup(xlogpath);
+		make_native_path(nativePath);
 	}
 
-	return result.data;
+	return replace_percent_placeholders(restoreCommand, "frp",
+										(const char *[]){xlogfname, lastRestartPointFname, nativePath});
 }
diff --git a/src/common/meson.build b/src/common/meson.build
index f69d75e9c6..e0f60ee48e 100644
--- a/src/common/meson.build
+++ b/src/common/meson.build
@@ -15,6 +15,7 @@ common_sources = files(
   'kwlookup.c',
   'link-canary.c',
   'md5_common.c',
+  'percentrepl.c',
   'pg_get_line.c',
   'pg_lzcompress.c',
   'pg_prng.c',
diff --git a/src/common/percentrepl.c b/src/common/percentrepl.c
new file mode 100644
index 0000000000..d2590a7986
--- /dev/null
+++ b/src/common/percentrepl.c
@@ -0,0 +1,108 @@
+/*-------------------------------------------------------------------------
+ *
+ * percentrepl.c
+ *	  Common routines to replace percent placeholders in strings
+ *
+ * Portions Copyright (c) 1996-2022, PostgreSQL Global Development Group
+ * Portions Copyright (c) 1994, Regents of the University of California
+ *
+ *
+ * IDENTIFICATION
+ *	  src/common/percentrepl.c
+ *
+ *-------------------------------------------------------------------------
+ */
+
+#ifndef FRONTEND
+#include "postgres.h"
+#else
+#include "postgres_fe.h"
+#endif
+
+#include "common/percentrepl.h"
+#include "lib/stringinfo.h"
+
+/*
+ * replace_percent_placeholders
+ *
+ * Replace percent-letter placeholders in input string with the supplied
+ * values.  For example, to replace %f with foo and %b with bar, call
+ *
+ * replace_percent_placeholders(instr, "bf", (const char *[]){bar, foo});
+ *
+ * The return value is palloc'd.
+ *
+ * "%%" is replaced by a single "%".
+ *
+ * Behavior for erroneous input strings:
+ *
+ * - An unsupported letter (e.g., "%x") is left unchanged (outputs "%x").
+ *
+ * - A "%" at the end of the input string is copied to the output string.
+ *
+ * (This is just the way it's always been.  Other behaviors might be
+ * sensible.)
+ *
+ * A value may be NULL.  If the corresponding placeholder is found in the
+ * input string, the whole function returns NULL.
+ *
+ * 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
+ * strings.  It won't be a good match for things like log prefixes or prompts
+ * that use a mix of data types and any invocation will only use a few of the
+ * possible values.
+ */
+char *
+replace_percent_placeholders(const char *instr, const char *letters, const char *values[])
+{
+	StringInfoData result;
+
+	initStringInfo(&result);
+
+	for (const char *sp = instr; *sp; sp++)
+	{
+		if (*sp == '%')
+		{
+			if (sp[1] == '%')
+			{
+				/* convert %% to a single % */
+				sp++;
+				appendStringInfoChar(&result, *sp);
+			}
+			else
+			{
+				bool		found = false;
+
+				for (const char *lp = letters; *lp; lp++)
+				{
+					if (sp[1] == *lp)
+					{
+						int			num = lp - letters;
+
+						if (!values[num])
+						{
+							pfree(result.data);
+							return NULL;
+						}
+						sp++;
+						appendStringInfoString(&result, values[num]);
+						found = true;
+						break;
+					}
+				}
+				if (!found)
+				{
+					/* otherwise treat the % as not special */
+					appendStringInfoChar(&result, *sp);
+				}
+			}
+		}
+		else
+		{
+			appendStringInfoChar(&result, *sp);
+		}
+	}
+
+	return result.data;
+}
diff --git a/src/include/common/percentrepl.h b/src/include/common/percentrepl.h
new file mode 100644
index 0000000000..794da4e059
--- /dev/null
+++ b/src/include/common/percentrepl.h
@@ -0,0 +1,18 @@
+/*-------------------------------------------------------------------------
+ *
+ * percentrepl.h
+ *	  Common routines to replace percent placeholders in strings
+ *
+ * Portions Copyright (c) 1996-2022, PostgreSQL Global Development Group
+ * Portions Copyright (c) 1994, Regents of the University of California
+ *
+ * src/include/common/percentrepl.h
+ *
+ *-------------------------------------------------------------------------
+ */
+#ifndef PERCENTREPL_H
+#define PERCENTREPL_H
+
+extern char *replace_percent_placeholders(const char *instr, const char *letters, const char *values[]);
+
+#endif							/* PERCENTREPL_H */

base-commit: 60684dd834a222fefedd49b19d1f0a6189c1632e
-- 
2.38.1

#2Robert Haas
robertmhaas@gmail.com
In reply to: Peter Eisentraut (#1)
Re: Common function for percent placeholder replacement

On Wed, Dec 14, 2022 at 2:31 AM Peter Eisentraut
<peter.eisentraut@enterprisedb.com> wrote:

There are a number of places where a shell command is constructed with
percent-placeholders (like %x). First, it's obviously cumbersome to
have to open-code this several times. Second, each of those pieces of
code silently encodes some edge case behavior, such as what to do with
unrecognized placeholders. (I remember when I last did one of these, I
stared very hard at the existing code instances to figure out what they
would do.) We now also have a newer instance in basebackup_to_shell.c
that has different behavior in such cases. (Maybe it's better, but it
would be good to be explicit and consistent about this.)

Well, OK, I'll tentatively cast a vote in favor of adopting
basebackup_to_shell's approach elsewhere. Or to put that in plain
English: I think that if the input appears to be malformed, it's
better to throw an error than to guess what the user meant. In the
case of basebackup_to_shell there are potentially security
ramifications to the setting involved so it seemed like a bad idea to
take a laissez faire approach. But also, just in general, if somebody
supplies an ssl_passphrase_command or archive_command with %<something
unexpected>, I don't really see why we should treat that differently
than trying to start the server with work_mem=banana.

--
Robert Haas
EDB: http://www.enterprisedb.com

#3Justin Pryzby
pryzby@telsasoft.com
In reply to: Peter Eisentraut (#1)
Re: Common function for percent placeholder replacement

On Wed, Dec 14, 2022 at 08:31:02AM +0100, Peter Eisentraut wrote:

+ return replace_percent_placeholders(base_command, "df", (const char *[]){target_detail, filename});

This is a "compound literal", which I gather is required by C99.

But I don't think that's currently being exercised, so I wonder if it's
going to break some BF members.

--
Justin

#4Tom Lane
tgl@sss.pgh.pa.us
In reply to: Justin Pryzby (#3)
Re: Common function for percent placeholder replacement

Justin Pryzby <pryzby@telsasoft.com> writes:

On Wed, Dec 14, 2022 at 08:31:02AM +0100, Peter Eisentraut wrote:

+ return replace_percent_placeholders(base_command, "df", (const char *[]){target_detail, filename});

This is a "compound literal", which I gather is required by C99.

But I don't think that's currently being exercised, so I wonder if it's
going to break some BF members.

It's pretty illegible, whatever it is. Could we maybe expend a
few more keystrokes in favor of readability?

regards, tom lane

#5Peter Eisentraut
peter.eisentraut@enterprisedb.com
In reply to: Robert Haas (#2)
1 attachment(s)
Re: Common function for percent placeholder replacement

On 14.12.22 17:09, Robert Haas wrote:

Well, OK, I'll tentatively cast a vote in favor of adopting
basebackup_to_shell's approach elsewhere. Or to put that in plain
English: I think that if the input appears to be malformed, it's
better to throw an error than to guess what the user meant. In the
case of basebackup_to_shell there are potentially security
ramifications to the setting involved so it seemed like a bad idea to
take a laissez faire approach. But also, just in general, if somebody
supplies an ssl_passphrase_command or archive_command with %<something
unexpected>, I don't really see why we should treat that differently
than trying to start the server with work_mem=banana.

I agree. Here is an updated patch with the error checking included.

Attachments:

v2-0001-Common-function-for-percent-placeholder-replaceme.patchtext/plain; charset=UTF-8; name=v2-0001-Common-function-for-percent-placeholder-replaceme.patchDownload
From 8bb85ad7ed99d0255ea82306b08ef7d343ab8edb Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter@eisentraut.org>
Date: Mon, 19 Dec 2022 09:04:43 +0100
Subject: [PATCH v2] Common function for percent placeholder replacement

There are a number of places where a shell command is constructed with
percent-placeholders (like %x).  It's cumbersome to have to open-code
this several times.  This factors out this logic into a separate
function.  This also allows us to ensure consistency for and document
some subtle behaviors, such as what to do with unrecognized
placeholders.

Discussion: https://www.postgresql.org/message-id/flat/5238bbed-0b01-83a6-d4b2-7eb0562a054e%40enterprisedb.com
---
 .../basebackup_to_shell/basebackup_to_shell.c |  55 +-------
 src/backend/access/transam/xlogarchive.c      |  45 +------
 src/backend/libpq/be-secure-common.c          |  38 ++----
 src/backend/postmaster/shell_archive.c        |  59 ++-------
 src/common/Makefile                           |   1 +
 src/common/archive.c                          |  81 +-----------
 src/common/meson.build                        |   1 +
 src/common/percentrepl.c                      | 120 ++++++++++++++++++
 src/include/common/percentrepl.h              |  18 +++
 9 files changed, 173 insertions(+), 245 deletions(-)
 create mode 100644 src/common/percentrepl.c
 create mode 100644 src/include/common/percentrepl.h

diff --git a/contrib/basebackup_to_shell/basebackup_to_shell.c b/contrib/basebackup_to_shell/basebackup_to_shell.c
index bdaf67a4c8..9a3d57c64b 100644
--- a/contrib/basebackup_to_shell/basebackup_to_shell.c
+++ b/contrib/basebackup_to_shell/basebackup_to_shell.c
@@ -12,6 +12,7 @@
 
 #include "access/xact.h"
 #include "backup/basebackup_target.h"
+#include "common/percentrepl.h"
 #include "miscadmin.h"
 #include "storage/fd.h"
 #include "utils/acl.h"
@@ -208,59 +209,7 @@ static char *
 shell_construct_command(const char *base_command, const char *filename,
 						const char *target_detail)
 {
-	StringInfoData buf;
-	const char *c;
-
-	initStringInfo(&buf);
-	for (c = base_command; *c != '\0'; ++c)
-	{
-		/* Anything other than '%' is copied verbatim. */
-		if (*c != '%')
-		{
-			appendStringInfoChar(&buf, *c);
-			continue;
-		}
-
-		/* Any time we see '%' we eat the following character as well. */
-		++c;
-
-		/*
-		 * The following character determines what we insert here, or may
-		 * cause us to throw an error.
-		 */
-		if (*c == '%')
-		{
-			/* '%%' is replaced by a single '%' */
-			appendStringInfoChar(&buf, '%');
-		}
-		else if (*c == 'f')
-		{
-			/* '%f' is replaced by the filename */
-			appendStringInfoString(&buf, filename);
-		}
-		else if (*c == 'd')
-		{
-			/* '%d' is replaced by the target detail */
-			appendStringInfoString(&buf, target_detail);
-		}
-		else if (*c == '\0')
-		{
-			/* Incomplete escape sequence, expected a character afterward */
-			ereport(ERROR,
-					errcode(ERRCODE_SYNTAX_ERROR),
-					errmsg("shell command ends unexpectedly after escape character \"%%\""));
-		}
-		else
-		{
-			/* Unknown escape sequence */
-			ereport(ERROR,
-					errcode(ERRCODE_SYNTAX_ERROR),
-					errmsg("shell command contains unexpected escape sequence \"%c\"",
-						   *c));
-		}
-	}
-
-	return buf.data;
+	return replace_percent_placeholders(base_command, "df", (const char *[]){target_detail, filename});
 }
 
 /*
diff --git a/src/backend/access/transam/xlogarchive.c b/src/backend/access/transam/xlogarchive.c
index e2b7176f2f..0f74ccb49c 100644
--- a/src/backend/access/transam/xlogarchive.c
+++ b/src/backend/access/transam/xlogarchive.c
@@ -23,6 +23,7 @@
 #include "access/xlog_internal.h"
 #include "access/xlogarchive.h"
 #include "common/archive.h"
+#include "common/percentrepl.h"
 #include "miscadmin.h"
 #include "pgstat.h"
 #include "postmaster/startup.h"
@@ -291,11 +292,8 @@ void
 ExecuteRecoveryCommand(const char *command, const char *commandName,
 					   bool failOnSignal, uint32 wait_event_info)
 {
-	char		xlogRecoveryCmd[MAXPGPATH];
+	char	   *xlogRecoveryCmd;
 	char		lastRestartPointFname[MAXPGPATH];
-	char	   *dp;
-	char	   *endp;
-	const char *sp;
 	int			rc;
 	XLogSegNo	restartSegNo;
 	XLogRecPtr	restartRedoPtr;
@@ -316,42 +314,7 @@ ExecuteRecoveryCommand(const char *command, const char *commandName,
 	/*
 	 * construct the command to be executed
 	 */
-	dp = xlogRecoveryCmd;
-	endp = xlogRecoveryCmd + MAXPGPATH - 1;
-	*endp = '\0';
-
-	for (sp = command; *sp; sp++)
-	{
-		if (*sp == '%')
-		{
-			switch (sp[1])
-			{
-				case 'r':
-					/* %r: filename of last restartpoint */
-					sp++;
-					strlcpy(dp, lastRestartPointFname, endp - dp);
-					dp += strlen(dp);
-					break;
-				case '%':
-					/* convert %% to a single % */
-					sp++;
-					if (dp < endp)
-						*dp++ = *sp;
-					break;
-				default:
-					/* otherwise treat the % as not special */
-					if (dp < endp)
-						*dp++ = *sp;
-					break;
-			}
-		}
-		else
-		{
-			if (dp < endp)
-				*dp++ = *sp;
-		}
-	}
-	*dp = '\0';
+	xlogRecoveryCmd = replace_percent_placeholders(command, "r", (const char *[]){lastRestartPointFname});
 
 	ereport(DEBUG3,
 			(errmsg_internal("executing %s \"%s\"", commandName, command)));
@@ -364,6 +327,8 @@ ExecuteRecoveryCommand(const char *command, const char *commandName,
 	rc = system(xlogRecoveryCmd);
 	pgstat_report_wait_end();
 
+	pfree(xlogRecoveryCmd);
+
 	if (rc != 0)
 	{
 		/*
diff --git a/src/backend/libpq/be-secure-common.c b/src/backend/libpq/be-secure-common.c
index 2719ce1403..912f12d13e 100644
--- a/src/backend/libpq/be-secure-common.c
+++ b/src/backend/libpq/be-secure-common.c
@@ -22,6 +22,7 @@
 #include <sys/stat.h>
 #include <unistd.h>
 
+#include "common/percentrepl.h"
 #include "common/string.h"
 #include "libpq/libpq.h"
 #include "storage/fd.h"
@@ -39,8 +40,7 @@ int
 run_ssl_passphrase_command(const char *prompt, bool is_server_start, char *buf, int size)
 {
 	int			loglevel = is_server_start ? ERROR : LOG;
-	StringInfoData command;
-	char	   *p;
+	char	   *command;
 	FILE	   *fh;
 	int			pclose_rc;
 	size_t		len = 0;
@@ -49,37 +49,15 @@ run_ssl_passphrase_command(const char *prompt, bool is_server_start, char *buf,
 	Assert(size > 0);
 	buf[0] = '\0';
 
-	initStringInfo(&command);
+	command = replace_percent_placeholders(ssl_passphrase_command, "p", (const char *[]){prompt});
 
-	for (p = ssl_passphrase_command; *p; p++)
-	{
-		if (p[0] == '%')
-		{
-			switch (p[1])
-			{
-				case 'p':
-					appendStringInfoString(&command, prompt);
-					p++;
-					break;
-				case '%':
-					appendStringInfoChar(&command, '%');
-					p++;
-					break;
-				default:
-					appendStringInfoChar(&command, p[0]);
-			}
-		}
-		else
-			appendStringInfoChar(&command, p[0]);
-	}
-
-	fh = OpenPipeStream(command.data, "r");
+	fh = OpenPipeStream(command, "r");
 	if (fh == NULL)
 	{
 		ereport(loglevel,
 				(errcode_for_file_access(),
 				 errmsg("could not execute command \"%s\": %m",
-						command.data)));
+						command)));
 		goto error;
 	}
 
@@ -91,7 +69,7 @@ run_ssl_passphrase_command(const char *prompt, bool is_server_start, char *buf,
 			ereport(loglevel,
 					(errcode_for_file_access(),
 					 errmsg("could not read from command \"%s\": %m",
-							command.data)));
+							command)));
 			goto error;
 		}
 	}
@@ -111,7 +89,7 @@ run_ssl_passphrase_command(const char *prompt, bool is_server_start, char *buf,
 		ereport(loglevel,
 				(errcode_for_file_access(),
 				 errmsg("command \"%s\" failed",
-						command.data),
+						command),
 				 errdetail_internal("%s", wait_result_to_str(pclose_rc))));
 		goto error;
 	}
@@ -120,7 +98,7 @@ run_ssl_passphrase_command(const char *prompt, bool is_server_start, char *buf,
 	len = pg_strip_crlf(buf);
 
 error:
-	pfree(command.data);
+	pfree(command);
 	return len;
 }
 
diff --git a/src/backend/postmaster/shell_archive.c b/src/backend/postmaster/shell_archive.c
index 71d567065a..77c7b34535 100644
--- a/src/backend/postmaster/shell_archive.c
+++ b/src/backend/postmaster/shell_archive.c
@@ -18,6 +18,7 @@
 #include <sys/wait.h>
 
 #include "access/xlog.h"
+#include "common/percentrepl.h"
 #include "pgstat.h"
 #include "postmaster/pgarch.h"
 
@@ -44,58 +45,18 @@ shell_archive_configured(void)
 static bool
 shell_archive_file(const char *file, const char *path)
 {
-	char		xlogarchcmd[MAXPGPATH];
-	char	   *dp;
-	char	   *endp;
-	const char *sp;
+	char	   *xlogarchcmd;
+	char	   *nativePath = NULL;
 	int			rc;
 
-	/*
-	 * construct the command to be executed
-	 */
-	dp = xlogarchcmd;
-	endp = xlogarchcmd + MAXPGPATH - 1;
-	*endp = '\0';
-
-	for (sp = XLogArchiveCommand; *sp; sp++)
+	if (path)
 	{
-		if (*sp == '%')
-		{
-			switch (sp[1])
-			{
-				case 'p':
-					/* %p: relative path of source file */
-					sp++;
-					strlcpy(dp, path, endp - dp);
-					make_native_path(dp);
-					dp += strlen(dp);
-					break;
-				case 'f':
-					/* %f: filename of source file */
-					sp++;
-					strlcpy(dp, file, endp - dp);
-					dp += strlen(dp);
-					break;
-				case '%':
-					/* convert %% to a single % */
-					sp++;
-					if (dp < endp)
-						*dp++ = *sp;
-					break;
-				default:
-					/* otherwise treat the % as not special */
-					if (dp < endp)
-						*dp++ = *sp;
-					break;
-			}
-		}
-		else
-		{
-			if (dp < endp)
-				*dp++ = *sp;
-		}
+		nativePath = pstrdup(path);
+		make_native_path(nativePath);
 	}
-	*dp = '\0';
+
+	xlogarchcmd = replace_percent_placeholders(XLogArchiveCommand, "fp",
+											   (const char *[]){file, nativePath});
 
 	ereport(DEBUG3,
 			(errmsg_internal("executing archive command \"%s\"",
@@ -155,6 +116,8 @@ shell_archive_file(const char *file, const char *path)
 		return false;
 	}
 
+	pfree(xlogarchcmd);
+
 	elog(DEBUG1, "archived write-ahead log file \"%s\"", file);
 	return true;
 }
diff --git a/src/common/Makefile b/src/common/Makefile
index e9af7346c9..d88a3bf945 100644
--- a/src/common/Makefile
+++ b/src/common/Makefile
@@ -65,6 +65,7 @@ OBJS_COMMON = \
 	kwlookup.o \
 	link-canary.o \
 	md5_common.o \
+	percentrepl.o \
 	pg_get_line.o \
 	pg_lzcompress.o \
 	pg_prng.o \
diff --git a/src/common/archive.c b/src/common/archive.c
index 47fc877c90..a30146934d 100644
--- a/src/common/archive.c
+++ b/src/common/archive.c
@@ -20,7 +20,7 @@
 #endif
 
 #include "common/archive.h"
-#include "lib/stringinfo.h"
+#include "common/percentrepl.h"
 
 /*
  * BuildRestoreCommand
@@ -41,81 +41,14 @@ BuildRestoreCommand(const char *restoreCommand,
 					const char *xlogfname,
 					const char *lastRestartPointFname)
 {
-	StringInfoData result;
-	const char *sp;
+	char	   *nativePath = NULL;
 
-	/*
-	 * Build the command to be executed.
-	 */
-	initStringInfo(&result);
-
-	for (sp = restoreCommand; *sp; sp++)
+	if (xlogpath)
 	{
-		if (*sp == '%')
-		{
-			switch (sp[1])
-			{
-				case 'p':
-					{
-						char	   *nativePath;
-
-						/* %p: relative path of target file */
-						if (xlogpath == NULL)
-						{
-							pfree(result.data);
-							return NULL;
-						}
-						sp++;
-
-						/*
-						 * This needs to use a placeholder to not modify the
-						 * input with the conversion done via
-						 * make_native_path().
-						 */
-						nativePath = pstrdup(xlogpath);
-						make_native_path(nativePath);
-						appendStringInfoString(&result,
-											   nativePath);
-						pfree(nativePath);
-						break;
-					}
-				case 'f':
-					/* %f: filename of desired file */
-					if (xlogfname == NULL)
-					{
-						pfree(result.data);
-						return NULL;
-					}
-					sp++;
-					appendStringInfoString(&result, xlogfname);
-					break;
-				case 'r':
-					/* %r: filename of last restartpoint */
-					if (lastRestartPointFname == NULL)
-					{
-						pfree(result.data);
-						return NULL;
-					}
-					sp++;
-					appendStringInfoString(&result,
-										   lastRestartPointFname);
-					break;
-				case '%':
-					/* convert %% to a single % */
-					sp++;
-					appendStringInfoChar(&result, *sp);
-					break;
-				default:
-					/* otherwise treat the % as not special */
-					appendStringInfoChar(&result, *sp);
-					break;
-			}
-		}
-		else
-		{
-			appendStringInfoChar(&result, *sp);
-		}
+		nativePath = pstrdup(xlogpath);
+		make_native_path(nativePath);
 	}
 
-	return result.data;
+	return replace_percent_placeholders(restoreCommand, "frp",
+										(const char *[]){xlogfname, lastRestartPointFname, nativePath});
 }
diff --git a/src/common/meson.build b/src/common/meson.build
index f69d75e9c6..e0f60ee48e 100644
--- a/src/common/meson.build
+++ b/src/common/meson.build
@@ -15,6 +15,7 @@ common_sources = files(
   'kwlookup.c',
   'link-canary.c',
   'md5_common.c',
+  'percentrepl.c',
   'pg_get_line.c',
   'pg_lzcompress.c',
   'pg_prng.c',
diff --git a/src/common/percentrepl.c b/src/common/percentrepl.c
new file mode 100644
index 0000000000..d819d4a5fc
--- /dev/null
+++ b/src/common/percentrepl.c
@@ -0,0 +1,120 @@
+/*-------------------------------------------------------------------------
+ *
+ * percentrepl.c
+ *	  Common routines to replace percent placeholders in strings
+ *
+ * Portions Copyright (c) 1996-2022, PostgreSQL Global Development Group
+ * Portions Copyright (c) 1994, Regents of the University of California
+ *
+ *
+ * IDENTIFICATION
+ *	  src/common/percentrepl.c
+ *
+ *-------------------------------------------------------------------------
+ */
+
+#ifndef FRONTEND
+#include "postgres.h"
+#else
+#include "postgres_fe.h"
+#include "common/logging.h"
+#endif
+
+#include "common/percentrepl.h"
+#include "lib/stringinfo.h"
+
+/*
+ * replace_percent_placeholders
+ *
+ * Replace percent-letter placeholders in input string with the supplied
+ * values.  For example, to replace %f with foo and %b with bar, call
+ *
+ * replace_percent_placeholders(instr, "bf", (const char *[]){bar, foo});
+ *
+ * The return value is palloc'd.
+ *
+ * "%%" is replaced by a single "%".
+ *
+ * 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, the whole function returns NULL.
+ *
+ * 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
+ * strings.  It won't be a good match for things like log prefixes or prompts
+ * that use a mix of data types and any invocation will only use a few of the
+ * possible values.
+ */
+char *
+replace_percent_placeholders(const char *instr, const char *letters, const char *values[])
+{
+	StringInfoData result;
+
+	initStringInfo(&result);
+
+	for (const char *sp = instr; *sp; sp++)
+	{
+		if (*sp == '%')
+		{
+			if (sp[1] == '%')
+			{
+				/* convert %% to a single % */
+				sp++;
+				appendStringInfoChar(&result, *sp);
+			}
+			else if (sp[1] == '\0')
+			{
+				/* Incomplete escape sequence, expected a character afterward */
+#ifdef FRONTEND
+				pg_fatal("string ends unexpectedly after escape character \"%%\"");
+#else
+				ereport(ERROR,
+						errcode(ERRCODE_SYNTAX_ERROR),
+						errmsg("string ends unexpectedly after escape character \"%%\""));
+#endif
+			}
+			else
+			{
+				bool		found = false;
+
+				for (const char *lp = letters; *lp; lp++)
+				{
+					if (sp[1] == *lp)
+					{
+						int			num = lp - letters;
+
+						if (!values[num])
+						{
+							pfree(result.data);
+							return NULL;
+						}
+						sp++;
+						appendStringInfoString(&result, values[num]);
+						found = true;
+						break;
+					}
+				}
+				if (!found)
+				{
+					/* Unknown escape sequence */
+#ifdef FRONTEND
+					pg_fatal("string contains unexpected escape sequence \"%c\"", sp[1]);
+#else
+					ereport(ERROR,
+							errcode(ERRCODE_SYNTAX_ERROR),
+							errmsg("string contains unexpected escape sequence \"%c\"", sp[1]));
+#endif
+				}
+			}
+		}
+		else
+		{
+			appendStringInfoChar(&result, *sp);
+		}
+	}
+
+	return result.data;
+}
diff --git a/src/include/common/percentrepl.h b/src/include/common/percentrepl.h
new file mode 100644
index 0000000000..794da4e059
--- /dev/null
+++ b/src/include/common/percentrepl.h
@@ -0,0 +1,18 @@
+/*-------------------------------------------------------------------------
+ *
+ * percentrepl.h
+ *	  Common routines to replace percent placeholders in strings
+ *
+ * Portions Copyright (c) 1996-2022, PostgreSQL Global Development Group
+ * Portions Copyright (c) 1994, Regents of the University of California
+ *
+ * src/include/common/percentrepl.h
+ *
+ *-------------------------------------------------------------------------
+ */
+#ifndef PERCENTREPL_H
+#define PERCENTREPL_H
+
+extern char *replace_percent_placeholders(const char *instr, const char *letters, const char *values[]);
+
+#endif							/* PERCENTREPL_H */
-- 
2.39.0

#6Peter Eisentraut
peter.eisentraut@enterprisedb.com
In reply to: Justin Pryzby (#3)
Re: Common function for percent placeholder replacement

On 14.12.22 18:05, Justin Pryzby wrote:

On Wed, Dec 14, 2022 at 08:31:02AM +0100, Peter Eisentraut wrote:

+ return replace_percent_placeholders(base_command, "df", (const char *[]){target_detail, filename});

This is a "compound literal", which I gather is required by C99.

But I don't think that's currently being exercised, so I wonder if it's
going to break some BF members.

We already use this, for example in pg_dump.

#7Alvaro Herrera
alvherre@alvh.no-ip.org
In reply to: Peter Eisentraut (#6)
Re: Common function for percent placeholder replacement

On 2022-Dec-19, Peter Eisentraut wrote:

On 14.12.22 18:05, Justin Pryzby wrote:

On Wed, Dec 14, 2022 at 08:31:02AM +0100, Peter Eisentraut wrote:

+ return replace_percent_placeholders(base_command, "df", (const char *[]){target_detail, filename});

This is a "compound literal", which I gather is required by C99.

But I don't think that's currently being exercised, so I wonder if it's
going to break some BF members.

We already use this, for example in pg_dump.

Yeah, we have this

#define ARCHIVE_OPTS(...) &(ArchiveOpts){__VA_ARGS__}

which we then use like this

ArchiveEntry(fout,
dbCatId, /* catalog ID */
dbDumpId, /* dump ID */
ARCHIVE_OPTS(.tag = datname,
.owner = dba,
.description = "DATABASE",
.section = SECTION_PRE_DATA,
.createStmt = creaQry->data,
.dropStmt = delQry->data));

I think the new one is not great. I wish we could do something more
straightforward, maybe like

replace_percent_placeholders(base_command,
PERCENT_OPT("f", filename),
PERCENT_OPT("d", target_detail));

Is there a performance disadvantage to a variadic implementation?
Alternatively, have all these macro calls form an array.

--
Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
Subversion to GIT: the shortest path to happiness I've ever heard of
(Alexey Klyukin)

#8Robert Haas
robertmhaas@gmail.com
In reply to: Peter Eisentraut (#5)
Re: Common function for percent placeholder replacement

On Mon, Dec 19, 2022 at 3:13 AM Peter Eisentraut
<peter.eisentraut@enterprisedb.com> wrote:

I agree. Here is an updated patch with the error checking included.

Nice, but I think something in the error report needs to indicate what
caused the problem exactly. As coded, I think the user would have to
guess which GUC caused the problem. For basebackup_to_shell that might
not be too hard since you would have to try to initiate a backup to a
shell target to trigger the error, but for something that happens at
server start, you don't want to have to go search all of
postgresql.conf for possible causes.

--
Robert Haas
EDB: http://www.enterprisedb.com

#9Peter Eisentraut
peter.eisentraut@enterprisedb.com
In reply to: Alvaro Herrera (#7)
1 attachment(s)
Re: Common function for percent placeholder replacement

On 19.12.22 10:51, Alvaro Herrera wrote:

I think the new one is not great. I wish we could do something more
straightforward, maybe like

replace_percent_placeholders(base_command,
PERCENT_OPT("f", filename),
PERCENT_OPT("d", target_detail));

Is there a performance disadvantage to a variadic implementation?
Alternatively, have all these macro calls form an array.

How about this new one with variable arguments?

(Still need to think about Robert's comment about lack of error context.)

Attachments:

v3-0001-Common-function-for-percent-placeholder-replaceme.patchtext/plain; charset=UTF-8; name=v3-0001-Common-function-for-percent-placeholder-replaceme.patchDownload
From 76dbfd291313e610aadb0e01eb46f1b0113f2c0f Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter@eisentraut.org>
Date: Tue, 20 Dec 2022 06:27:42 +0100
Subject: [PATCH v3] Common function for percent placeholder replacement

There are a number of places where a shell command is constructed with
percent-placeholders (like %x).  It's cumbersome to have to open-code
this several times.  This factors out this logic into a separate
function.  This also allows us to ensure consistency for and document
some subtle behaviors, such as what to do with unrecognized
placeholders.

Discussion: https://www.postgresql.org/message-id/flat/5238bbed-0b01-83a6-d4b2-7eb0562a054e%40enterprisedb.com
---
 .../basebackup_to_shell/basebackup_to_shell.c |  55 +-------
 src/backend/access/transam/xlogarchive.c      |  45 +------
 src/backend/libpq/be-secure-common.c          |  38 ++----
 src/backend/postmaster/shell_archive.c        |  58 ++-------
 src/common/Makefile                           |   1 +
 src/common/archive.c                          |  81 +-----------
 src/common/meson.build                        |   1 +
 src/common/percentrepl.c                      | 123 ++++++++++++++++++
 src/include/common/percentrepl.h              |  18 +++
 9 files changed, 175 insertions(+), 245 deletions(-)
 create mode 100644 src/common/percentrepl.c
 create mode 100644 src/include/common/percentrepl.h

diff --git a/contrib/basebackup_to_shell/basebackup_to_shell.c b/contrib/basebackup_to_shell/basebackup_to_shell.c
index bdaf67a4c8..0dceab65f8 100644
--- a/contrib/basebackup_to_shell/basebackup_to_shell.c
+++ b/contrib/basebackup_to_shell/basebackup_to_shell.c
@@ -12,6 +12,7 @@
 
 #include "access/xact.h"
 #include "backup/basebackup_target.h"
+#include "common/percentrepl.h"
 #include "miscadmin.h"
 #include "storage/fd.h"
 #include "utils/acl.h"
@@ -208,59 +209,7 @@ static char *
 shell_construct_command(const char *base_command, const char *filename,
 						const char *target_detail)
 {
-	StringInfoData buf;
-	const char *c;
-
-	initStringInfo(&buf);
-	for (c = base_command; *c != '\0'; ++c)
-	{
-		/* Anything other than '%' is copied verbatim. */
-		if (*c != '%')
-		{
-			appendStringInfoChar(&buf, *c);
-			continue;
-		}
-
-		/* Any time we see '%' we eat the following character as well. */
-		++c;
-
-		/*
-		 * The following character determines what we insert here, or may
-		 * cause us to throw an error.
-		 */
-		if (*c == '%')
-		{
-			/* '%%' is replaced by a single '%' */
-			appendStringInfoChar(&buf, '%');
-		}
-		else if (*c == 'f')
-		{
-			/* '%f' is replaced by the filename */
-			appendStringInfoString(&buf, filename);
-		}
-		else if (*c == 'd')
-		{
-			/* '%d' is replaced by the target detail */
-			appendStringInfoString(&buf, target_detail);
-		}
-		else if (*c == '\0')
-		{
-			/* Incomplete escape sequence, expected a character afterward */
-			ereport(ERROR,
-					errcode(ERRCODE_SYNTAX_ERROR),
-					errmsg("shell command ends unexpectedly after escape character \"%%\""));
-		}
-		else
-		{
-			/* Unknown escape sequence */
-			ereport(ERROR,
-					errcode(ERRCODE_SYNTAX_ERROR),
-					errmsg("shell command contains unexpected escape sequence \"%c\"",
-						   *c));
-		}
-	}
-
-	return buf.data;
+	return replace_percent_placeholders(base_command, "df", target_detail, filename);
 }
 
 /*
diff --git a/src/backend/access/transam/xlogarchive.c b/src/backend/access/transam/xlogarchive.c
index e2b7176f2f..7b32f67331 100644
--- a/src/backend/access/transam/xlogarchive.c
+++ b/src/backend/access/transam/xlogarchive.c
@@ -23,6 +23,7 @@
 #include "access/xlog_internal.h"
 #include "access/xlogarchive.h"
 #include "common/archive.h"
+#include "common/percentrepl.h"
 #include "miscadmin.h"
 #include "pgstat.h"
 #include "postmaster/startup.h"
@@ -291,11 +292,8 @@ void
 ExecuteRecoveryCommand(const char *command, const char *commandName,
 					   bool failOnSignal, uint32 wait_event_info)
 {
-	char		xlogRecoveryCmd[MAXPGPATH];
+	char	   *xlogRecoveryCmd;
 	char		lastRestartPointFname[MAXPGPATH];
-	char	   *dp;
-	char	   *endp;
-	const char *sp;
 	int			rc;
 	XLogSegNo	restartSegNo;
 	XLogRecPtr	restartRedoPtr;
@@ -316,42 +314,7 @@ ExecuteRecoveryCommand(const char *command, const char *commandName,
 	/*
 	 * construct the command to be executed
 	 */
-	dp = xlogRecoveryCmd;
-	endp = xlogRecoveryCmd + MAXPGPATH - 1;
-	*endp = '\0';
-
-	for (sp = command; *sp; sp++)
-	{
-		if (*sp == '%')
-		{
-			switch (sp[1])
-			{
-				case 'r':
-					/* %r: filename of last restartpoint */
-					sp++;
-					strlcpy(dp, lastRestartPointFname, endp - dp);
-					dp += strlen(dp);
-					break;
-				case '%':
-					/* convert %% to a single % */
-					sp++;
-					if (dp < endp)
-						*dp++ = *sp;
-					break;
-				default:
-					/* otherwise treat the % as not special */
-					if (dp < endp)
-						*dp++ = *sp;
-					break;
-			}
-		}
-		else
-		{
-			if (dp < endp)
-				*dp++ = *sp;
-		}
-	}
-	*dp = '\0';
+	xlogRecoveryCmd = replace_percent_placeholders(command, "r", lastRestartPointFname);
 
 	ereport(DEBUG3,
 			(errmsg_internal("executing %s \"%s\"", commandName, command)));
@@ -364,6 +327,8 @@ ExecuteRecoveryCommand(const char *command, const char *commandName,
 	rc = system(xlogRecoveryCmd);
 	pgstat_report_wait_end();
 
+	pfree(xlogRecoveryCmd);
+
 	if (rc != 0)
 	{
 		/*
diff --git a/src/backend/libpq/be-secure-common.c b/src/backend/libpq/be-secure-common.c
index 2719ce1403..9fcffa714b 100644
--- a/src/backend/libpq/be-secure-common.c
+++ b/src/backend/libpq/be-secure-common.c
@@ -22,6 +22,7 @@
 #include <sys/stat.h>
 #include <unistd.h>
 
+#include "common/percentrepl.h"
 #include "common/string.h"
 #include "libpq/libpq.h"
 #include "storage/fd.h"
@@ -39,8 +40,7 @@ int
 run_ssl_passphrase_command(const char *prompt, bool is_server_start, char *buf, int size)
 {
 	int			loglevel = is_server_start ? ERROR : LOG;
-	StringInfoData command;
-	char	   *p;
+	char	   *command;
 	FILE	   *fh;
 	int			pclose_rc;
 	size_t		len = 0;
@@ -49,37 +49,15 @@ run_ssl_passphrase_command(const char *prompt, bool is_server_start, char *buf,
 	Assert(size > 0);
 	buf[0] = '\0';
 
-	initStringInfo(&command);
+	command = replace_percent_placeholders(ssl_passphrase_command, "p", prompt);
 
-	for (p = ssl_passphrase_command; *p; p++)
-	{
-		if (p[0] == '%')
-		{
-			switch (p[1])
-			{
-				case 'p':
-					appendStringInfoString(&command, prompt);
-					p++;
-					break;
-				case '%':
-					appendStringInfoChar(&command, '%');
-					p++;
-					break;
-				default:
-					appendStringInfoChar(&command, p[0]);
-			}
-		}
-		else
-			appendStringInfoChar(&command, p[0]);
-	}
-
-	fh = OpenPipeStream(command.data, "r");
+	fh = OpenPipeStream(command, "r");
 	if (fh == NULL)
 	{
 		ereport(loglevel,
 				(errcode_for_file_access(),
 				 errmsg("could not execute command \"%s\": %m",
-						command.data)));
+						command)));
 		goto error;
 	}
 
@@ -91,7 +69,7 @@ run_ssl_passphrase_command(const char *prompt, bool is_server_start, char *buf,
 			ereport(loglevel,
 					(errcode_for_file_access(),
 					 errmsg("could not read from command \"%s\": %m",
-							command.data)));
+							command)));
 			goto error;
 		}
 	}
@@ -111,7 +89,7 @@ run_ssl_passphrase_command(const char *prompt, bool is_server_start, char *buf,
 		ereport(loglevel,
 				(errcode_for_file_access(),
 				 errmsg("command \"%s\" failed",
-						command.data),
+						command),
 				 errdetail_internal("%s", wait_result_to_str(pclose_rc))));
 		goto error;
 	}
@@ -120,7 +98,7 @@ run_ssl_passphrase_command(const char *prompt, bool is_server_start, char *buf,
 	len = pg_strip_crlf(buf);
 
 error:
-	pfree(command.data);
+	pfree(command);
 	return len;
 }
 
diff --git a/src/backend/postmaster/shell_archive.c b/src/backend/postmaster/shell_archive.c
index 71d567065a..ce3be886b3 100644
--- a/src/backend/postmaster/shell_archive.c
+++ b/src/backend/postmaster/shell_archive.c
@@ -18,6 +18,7 @@
 #include <sys/wait.h>
 
 #include "access/xlog.h"
+#include "common/percentrepl.h"
 #include "pgstat.h"
 #include "postmaster/pgarch.h"
 
@@ -44,58 +45,17 @@ shell_archive_configured(void)
 static bool
 shell_archive_file(const char *file, const char *path)
 {
-	char		xlogarchcmd[MAXPGPATH];
-	char	   *dp;
-	char	   *endp;
-	const char *sp;
+	char	   *xlogarchcmd;
+	char	   *nativePath = NULL;
 	int			rc;
 
-	/*
-	 * construct the command to be executed
-	 */
-	dp = xlogarchcmd;
-	endp = xlogarchcmd + MAXPGPATH - 1;
-	*endp = '\0';
-
-	for (sp = XLogArchiveCommand; *sp; sp++)
+	if (path)
 	{
-		if (*sp == '%')
-		{
-			switch (sp[1])
-			{
-				case 'p':
-					/* %p: relative path of source file */
-					sp++;
-					strlcpy(dp, path, endp - dp);
-					make_native_path(dp);
-					dp += strlen(dp);
-					break;
-				case 'f':
-					/* %f: filename of source file */
-					sp++;
-					strlcpy(dp, file, endp - dp);
-					dp += strlen(dp);
-					break;
-				case '%':
-					/* convert %% to a single % */
-					sp++;
-					if (dp < endp)
-						*dp++ = *sp;
-					break;
-				default:
-					/* otherwise treat the % as not special */
-					if (dp < endp)
-						*dp++ = *sp;
-					break;
-			}
-		}
-		else
-		{
-			if (dp < endp)
-				*dp++ = *sp;
-		}
+		nativePath = pstrdup(path);
+		make_native_path(nativePath);
 	}
-	*dp = '\0';
+
+	xlogarchcmd = replace_percent_placeholders(XLogArchiveCommand, "fp", file, nativePath);
 
 	ereport(DEBUG3,
 			(errmsg_internal("executing archive command \"%s\"",
@@ -155,6 +115,8 @@ shell_archive_file(const char *file, const char *path)
 		return false;
 	}
 
+	pfree(xlogarchcmd);
+
 	elog(DEBUG1, "archived write-ahead log file \"%s\"", file);
 	return true;
 }
diff --git a/src/common/Makefile b/src/common/Makefile
index e9af7346c9..d88a3bf945 100644
--- a/src/common/Makefile
+++ b/src/common/Makefile
@@ -65,6 +65,7 @@ OBJS_COMMON = \
 	kwlookup.o \
 	link-canary.o \
 	md5_common.o \
+	percentrepl.o \
 	pg_get_line.o \
 	pg_lzcompress.o \
 	pg_prng.o \
diff --git a/src/common/archive.c b/src/common/archive.c
index 47fc877c90..e8d9759025 100644
--- a/src/common/archive.c
+++ b/src/common/archive.c
@@ -20,7 +20,7 @@
 #endif
 
 #include "common/archive.h"
-#include "lib/stringinfo.h"
+#include "common/percentrepl.h"
 
 /*
  * BuildRestoreCommand
@@ -41,81 +41,14 @@ BuildRestoreCommand(const char *restoreCommand,
 					const char *xlogfname,
 					const char *lastRestartPointFname)
 {
-	StringInfoData result;
-	const char *sp;
+	char	   *nativePath = NULL;
 
-	/*
-	 * Build the command to be executed.
-	 */
-	initStringInfo(&result);
-
-	for (sp = restoreCommand; *sp; sp++)
+	if (xlogpath)
 	{
-		if (*sp == '%')
-		{
-			switch (sp[1])
-			{
-				case 'p':
-					{
-						char	   *nativePath;
-
-						/* %p: relative path of target file */
-						if (xlogpath == NULL)
-						{
-							pfree(result.data);
-							return NULL;
-						}
-						sp++;
-
-						/*
-						 * This needs to use a placeholder to not modify the
-						 * input with the conversion done via
-						 * make_native_path().
-						 */
-						nativePath = pstrdup(xlogpath);
-						make_native_path(nativePath);
-						appendStringInfoString(&result,
-											   nativePath);
-						pfree(nativePath);
-						break;
-					}
-				case 'f':
-					/* %f: filename of desired file */
-					if (xlogfname == NULL)
-					{
-						pfree(result.data);
-						return NULL;
-					}
-					sp++;
-					appendStringInfoString(&result, xlogfname);
-					break;
-				case 'r':
-					/* %r: filename of last restartpoint */
-					if (lastRestartPointFname == NULL)
-					{
-						pfree(result.data);
-						return NULL;
-					}
-					sp++;
-					appendStringInfoString(&result,
-										   lastRestartPointFname);
-					break;
-				case '%':
-					/* convert %% to a single % */
-					sp++;
-					appendStringInfoChar(&result, *sp);
-					break;
-				default:
-					/* otherwise treat the % as not special */
-					appendStringInfoChar(&result, *sp);
-					break;
-			}
-		}
-		else
-		{
-			appendStringInfoChar(&result, *sp);
-		}
+		nativePath = pstrdup(xlogpath);
+		make_native_path(nativePath);
 	}
 
-	return result.data;
+	return replace_percent_placeholders(restoreCommand, "frp",
+										xlogfname, lastRestartPointFname, nativePath);
 }
diff --git a/src/common/meson.build b/src/common/meson.build
index f69d75e9c6..e0f60ee48e 100644
--- a/src/common/meson.build
+++ b/src/common/meson.build
@@ -15,6 +15,7 @@ common_sources = files(
   'kwlookup.c',
   'link-canary.c',
   'md5_common.c',
+  'percentrepl.c',
   'pg_get_line.c',
   'pg_lzcompress.c',
   'pg_prng.c',
diff --git a/src/common/percentrepl.c b/src/common/percentrepl.c
new file mode 100644
index 0000000000..36a750739f
--- /dev/null
+++ b/src/common/percentrepl.c
@@ -0,0 +1,123 @@
+/*-------------------------------------------------------------------------
+ *
+ * percentrepl.c
+ *	  Common routines to replace percent placeholders in strings
+ *
+ * Portions Copyright (c) 1996-2022, PostgreSQL Global Development Group
+ * Portions Copyright (c) 1994, Regents of the University of California
+ *
+ *
+ * IDENTIFICATION
+ *	  src/common/percentrepl.c
+ *
+ *-------------------------------------------------------------------------
+ */
+
+#ifndef FRONTEND
+#include "postgres.h"
+#else
+#include "postgres_fe.h"
+#include "common/logging.h"
+#endif
+
+#include "common/percentrepl.h"
+#include "lib/stringinfo.h"
+
+/*
+ * replace_percent_placeholders
+ *
+ * Replace percent-letter placeholders in input string with the supplied
+ * values.  For example, to replace %f with foo and %b with bar, call
+ *
+ * replace_percent_placeholders(instr, "bf", bar, foo);
+ *
+ * The return value is palloc'd.
+ *
+ * "%%" is replaced by a single "%".
+ *
+ * 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, the whole function returns NULL.
+ *
+ * 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
+ * strings.  It won't be a good match for things like log prefixes or prompts
+ * that use a mix of data types and any invocation will only use a few of the
+ * possible values.
+ */
+char *
+replace_percent_placeholders(const char *instr, const char *letters, ...)
+{
+	StringInfoData result;
+
+	initStringInfo(&result);
+
+	for (const char *sp = instr; *sp; sp++)
+	{
+		if (*sp == '%')
+		{
+			if (sp[1] == '%')
+			{
+				/* convert %% to a single % */
+				sp++;
+				appendStringInfoChar(&result, *sp);
+			}
+			else if (sp[1] == '\0')
+			{
+				/* Incomplete escape sequence, expected a character afterward */
+#ifdef FRONTEND
+				pg_fatal("string ends unexpectedly after escape character \"%%\"");
+#else
+				ereport(ERROR,
+						errcode(ERRCODE_SYNTAX_ERROR),
+						errmsg("string ends unexpectedly after escape character \"%%\""));
+#endif
+			}
+			else
+			{
+				bool		found = false;
+				va_list		ap;
+
+				va_start(ap, letters);
+				for (const char *lp = letters; *lp; lp++)
+				{
+					char	   *val = va_arg(ap, char *);
+
+					if (sp[1] == *lp)
+					{
+						if (!val)
+						{
+							pfree(result.data);
+							return NULL;
+						}
+						sp++;
+						appendStringInfoString(&result, val);
+						found = true;
+						break;
+					}
+				}
+				va_end(ap);
+				if (!found)
+				{
+					/* Unknown escape sequence */
+#ifdef FRONTEND
+					pg_fatal("string contains unexpected escape sequence \"%c\"", sp[1]);
+#else
+					ereport(ERROR,
+							errcode(ERRCODE_SYNTAX_ERROR),
+							errmsg("string contains unexpected escape sequence \"%c\"", sp[1]));
+#endif
+				}
+			}
+		}
+		else
+		{
+			appendStringInfoChar(&result, *sp);
+		}
+	}
+
+	return result.data;
+}
diff --git a/src/include/common/percentrepl.h b/src/include/common/percentrepl.h
new file mode 100644
index 0000000000..4190144798
--- /dev/null
+++ b/src/include/common/percentrepl.h
@@ -0,0 +1,18 @@
+/*-------------------------------------------------------------------------
+ *
+ * percentrepl.h
+ *	  Common routines to replace percent placeholders in strings
+ *
+ * Portions Copyright (c) 1996-2022, PostgreSQL Global Development Group
+ * Portions Copyright (c) 1994, Regents of the University of California
+ *
+ * src/include/common/percentrepl.h
+ *
+ *-------------------------------------------------------------------------
+ */
+#ifndef PERCENTREPL_H
+#define PERCENTREPL_H
+
+extern char *replace_percent_placeholders(const char *instr, const char *letters, ...);
+
+#endif							/* PERCENTREPL_H */
-- 
2.39.0

#10Corey Huinker
corey.huinker@gmail.com
In reply to: Peter Eisentraut (#9)
Re: Common function for percent placeholder replacement

How about this new one with variable arguments?

I like this a lot, but I also see merit in Alvaro's PERCENT_OPT variadic,
which at least avoids the two lists getting out of sync.

Initially, I was going to ask that we have shell-quote-safe equivalents of
whatever fixed parameters we baked in, but this allows the caller to do
that as needed. It seems we could now just copy quote_identifier and strip
out the keyword checks to get the desired effect. Has anyone else had a
need for quote-safe args in the shell commands?

#11Nathan Bossart
nathandbossart@gmail.com
In reply to: Peter Eisentraut (#9)
Re: Common function for percent placeholder replacement

In general, +1.

On Tue, Dec 20, 2022 at 06:30:40AM +0100, Peter Eisentraut wrote:

(Still need to think about Robert's comment about lack of error context.)

Would adding the name of the GUC be sufficient?

ereport(ERROR,
(errmsg("could not build %s", guc_name),
errdetail("string ends unexpectedly after escape character \"%%\"")));

+ * A value may be NULL.  If the corresponding placeholder is found in the
+ * input string, the whole function returns NULL.

This appears to be carried over from BuildRestoreCommand(), and AFAICT it
is only necessary because pg_rewind doesn't support %r in restore_command.
IMHO this behavior is counterintuitive and possibly error-prone and should
result in an ERROR instead. Since pg_rewind is the only special case, it
could independently check for %r before building the command.

--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

#12Peter Eisentraut
peter.eisentraut@enterprisedb.com
In reply to: Nathan Bossart (#11)
1 attachment(s)
Re: Common function for percent placeholder replacement

On 04.01.23 01:37, Nathan Bossart wrote:

In general, +1.

On Tue, Dec 20, 2022 at 06:30:40AM +0100, Peter Eisentraut wrote:

(Still need to think about Robert's comment about lack of error context.)

Would adding the name of the GUC be sufficient?

ereport(ERROR,
(errmsg("could not build %s", guc_name),
errdetail("string ends unexpectedly after escape character \"%%\"")));

done

The errors now basically look like an invalid GUC value.

+ * A value may be NULL.  If the corresponding placeholder is found in the
+ * input string, the whole function returns NULL.

This appears to be carried over from BuildRestoreCommand(), and AFAICT it
is only necessary because pg_rewind doesn't support %r in restore_command.
IMHO this behavior is counterintuitive and possibly error-prone and should
result in an ERROR instead. Since pg_rewind is the only special case, it
could independently check for %r before building the command.

Yeah, this annoyed me, too. I have now changed it so that a NULL
"value" is the same as an unsupported placeholder. This preserves the
existing behavior.

(Having pg_rewind check for %r itself would probably require replicating
most of the string processing logic (consider something like "%%r"), so
it didn't seem appealing.)

Attachments:

v4-0001-Common-function-for-percent-placeholder-replaceme.patchtext/plain; charset=UTF-8; name=v4-0001-Common-function-for-percent-placeholder-replaceme.patchDownload
From 3975b1296174218c5e25e2b02bca88cb3c26ee63 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter@eisentraut.org>
Date: Mon, 9 Jan 2023 09:03:30 +0100
Subject: [PATCH v4] Common function for percent placeholder replacement

There are a number of places where a shell command is constructed with
percent-placeholders (like %x).  It's cumbersome to have to open-code
this several times.  This factors out this logic into a separate
function.  This also allows us to ensure consistency for and document
some subtle behaviors, such as what to do with unrecognized
placeholders.

Discussion: https://www.postgresql.org/message-id/flat/5238bbed-0b01-83a6-d4b2-7eb0562a054e%40enterprisedb.com
---
 .../basebackup_to_shell/basebackup_to_shell.c |  56 +------
 src/backend/access/transam/xlogarchive.c      |  45 +-----
 src/backend/libpq/be-secure-common.c          |  38 +----
 src/backend/postmaster/shell_archive.c        |  58 ++------
 src/common/Makefile                           |   1 +
 src/common/archive.c                          |  81 +----------
 src/common/meson.build                        |   1 +
 src/common/percentrepl.c                      | 137 ++++++++++++++++++
 src/fe_utils/archive.c                        |   2 -
 src/include/common/percentrepl.h              |  18 +++
 10 files changed, 190 insertions(+), 247 deletions(-)
 create mode 100644 src/common/percentrepl.c
 create mode 100644 src/include/common/percentrepl.h

diff --git a/contrib/basebackup_to_shell/basebackup_to_shell.c b/contrib/basebackup_to_shell/basebackup_to_shell.c
index 8d583550b5..29f5069d42 100644
--- a/contrib/basebackup_to_shell/basebackup_to_shell.c
+++ b/contrib/basebackup_to_shell/basebackup_to_shell.c
@@ -12,6 +12,7 @@
 
 #include "access/xact.h"
 #include "backup/basebackup_target.h"
+#include "common/percentrepl.h"
 #include "miscadmin.h"
 #include "storage/fd.h"
 #include "utils/acl.h"
@@ -208,59 +209,8 @@ static char *
 shell_construct_command(const char *base_command, const char *filename,
 						const char *target_detail)
 {
-	StringInfoData buf;
-	const char *c;
-
-	initStringInfo(&buf);
-	for (c = base_command; *c != '\0'; ++c)
-	{
-		/* Anything other than '%' is copied verbatim. */
-		if (*c != '%')
-		{
-			appendStringInfoChar(&buf, *c);
-			continue;
-		}
-
-		/* Any time we see '%' we eat the following character as well. */
-		++c;
-
-		/*
-		 * The following character determines what we insert here, or may
-		 * cause us to throw an error.
-		 */
-		if (*c == '%')
-		{
-			/* '%%' is replaced by a single '%' */
-			appendStringInfoChar(&buf, '%');
-		}
-		else if (*c == 'f')
-		{
-			/* '%f' is replaced by the filename */
-			appendStringInfoString(&buf, filename);
-		}
-		else if (*c == 'd')
-		{
-			/* '%d' is replaced by the target detail */
-			appendStringInfoString(&buf, target_detail);
-		}
-		else if (*c == '\0')
-		{
-			/* Incomplete escape sequence, expected a character afterward */
-			ereport(ERROR,
-					errcode(ERRCODE_SYNTAX_ERROR),
-					errmsg("shell command ends unexpectedly after escape character \"%%\""));
-		}
-		else
-		{
-			/* Unknown escape sequence */
-			ereport(ERROR,
-					errcode(ERRCODE_SYNTAX_ERROR),
-					errmsg("shell command contains unexpected escape sequence \"%c\"",
-						   *c));
-		}
-	}
-
-	return buf.data;
+	return replace_percent_placeholders(base_command, "basebackup_to_shell.command",
+										"df", target_detail, filename);
 }
 
 /*
diff --git a/src/backend/access/transam/xlogarchive.c b/src/backend/access/transam/xlogarchive.c
index 76abc74c67..f911e8c3a6 100644
--- a/src/backend/access/transam/xlogarchive.c
+++ b/src/backend/access/transam/xlogarchive.c
@@ -23,6 +23,7 @@
 #include "access/xlog_internal.h"
 #include "access/xlogarchive.h"
 #include "common/archive.h"
+#include "common/percentrepl.h"
 #include "miscadmin.h"
 #include "pgstat.h"
 #include "postmaster/startup.h"
@@ -291,11 +292,8 @@ void
 ExecuteRecoveryCommand(const char *command, const char *commandName,
 					   bool failOnSignal, uint32 wait_event_info)
 {
-	char		xlogRecoveryCmd[MAXPGPATH];
+	char	   *xlogRecoveryCmd;
 	char		lastRestartPointFname[MAXPGPATH];
-	char	   *dp;
-	char	   *endp;
-	const char *sp;
 	int			rc;
 	XLogSegNo	restartSegNo;
 	XLogRecPtr	restartRedoPtr;
@@ -316,42 +314,7 @@ ExecuteRecoveryCommand(const char *command, const char *commandName,
 	/*
 	 * construct the command to be executed
 	 */
-	dp = xlogRecoveryCmd;
-	endp = xlogRecoveryCmd + MAXPGPATH - 1;
-	*endp = '\0';
-
-	for (sp = command; *sp; sp++)
-	{
-		if (*sp == '%')
-		{
-			switch (sp[1])
-			{
-				case 'r':
-					/* %r: filename of last restartpoint */
-					sp++;
-					strlcpy(dp, lastRestartPointFname, endp - dp);
-					dp += strlen(dp);
-					break;
-				case '%':
-					/* convert %% to a single % */
-					sp++;
-					if (dp < endp)
-						*dp++ = *sp;
-					break;
-				default:
-					/* otherwise treat the % as not special */
-					if (dp < endp)
-						*dp++ = *sp;
-					break;
-			}
-		}
-		else
-		{
-			if (dp < endp)
-				*dp++ = *sp;
-		}
-	}
-	*dp = '\0';
+	xlogRecoveryCmd = replace_percent_placeholders(command, commandName, "r", lastRestartPointFname);
 
 	ereport(DEBUG3,
 			(errmsg_internal("executing %s \"%s\"", commandName, command)));
@@ -364,6 +327,8 @@ ExecuteRecoveryCommand(const char *command, const char *commandName,
 	rc = system(xlogRecoveryCmd);
 	pgstat_report_wait_end();
 
+	pfree(xlogRecoveryCmd);
+
 	if (rc != 0)
 	{
 		/*
diff --git a/src/backend/libpq/be-secure-common.c b/src/backend/libpq/be-secure-common.c
index f6078c91c3..ab5e2dfa2b 100644
--- a/src/backend/libpq/be-secure-common.c
+++ b/src/backend/libpq/be-secure-common.c
@@ -22,6 +22,7 @@
 #include <sys/stat.h>
 #include <unistd.h>
 
+#include "common/percentrepl.h"
 #include "common/string.h"
 #include "libpq/libpq.h"
 #include "storage/fd.h"
@@ -39,8 +40,7 @@ int
 run_ssl_passphrase_command(const char *prompt, bool is_server_start, char *buf, int size)
 {
 	int			loglevel = is_server_start ? ERROR : LOG;
-	StringInfoData command;
-	char	   *p;
+	char	   *command;
 	FILE	   *fh;
 	int			pclose_rc;
 	size_t		len = 0;
@@ -49,37 +49,15 @@ run_ssl_passphrase_command(const char *prompt, bool is_server_start, char *buf,
 	Assert(size > 0);
 	buf[0] = '\0';
 
-	initStringInfo(&command);
+	command = replace_percent_placeholders(ssl_passphrase_command, "ssl_passphrase_command", "p", prompt);
 
-	for (p = ssl_passphrase_command; *p; p++)
-	{
-		if (p[0] == '%')
-		{
-			switch (p[1])
-			{
-				case 'p':
-					appendStringInfoString(&command, prompt);
-					p++;
-					break;
-				case '%':
-					appendStringInfoChar(&command, '%');
-					p++;
-					break;
-				default:
-					appendStringInfoChar(&command, p[0]);
-			}
-		}
-		else
-			appendStringInfoChar(&command, p[0]);
-	}
-
-	fh = OpenPipeStream(command.data, "r");
+	fh = OpenPipeStream(command, "r");
 	if (fh == NULL)
 	{
 		ereport(loglevel,
 				(errcode_for_file_access(),
 				 errmsg("could not execute command \"%s\": %m",
-						command.data)));
+						command)));
 		goto error;
 	}
 
@@ -91,7 +69,7 @@ run_ssl_passphrase_command(const char *prompt, bool is_server_start, char *buf,
 			ereport(loglevel,
 					(errcode_for_file_access(),
 					 errmsg("could not read from command \"%s\": %m",
-							command.data)));
+							command)));
 			goto error;
 		}
 	}
@@ -111,7 +89,7 @@ run_ssl_passphrase_command(const char *prompt, bool is_server_start, char *buf,
 		ereport(loglevel,
 				(errcode_for_file_access(),
 				 errmsg("command \"%s\" failed",
-						command.data),
+						command),
 				 errdetail_internal("%s", wait_result_to_str(pclose_rc))));
 		goto error;
 	}
@@ -120,7 +98,7 @@ run_ssl_passphrase_command(const char *prompt, bool is_server_start, char *buf,
 	len = pg_strip_crlf(buf);
 
 error:
-	pfree(command.data);
+	pfree(command);
 	return len;
 }
 
diff --git a/src/backend/postmaster/shell_archive.c b/src/backend/postmaster/shell_archive.c
index 6f98414a40..5d97c6df64 100644
--- a/src/backend/postmaster/shell_archive.c
+++ b/src/backend/postmaster/shell_archive.c
@@ -18,6 +18,7 @@
 #include <sys/wait.h>
 
 #include "access/xlog.h"
+#include "common/percentrepl.h"
 #include "pgstat.h"
 #include "postmaster/pgarch.h"
 
@@ -44,58 +45,17 @@ shell_archive_configured(void)
 static bool
 shell_archive_file(const char *file, const char *path)
 {
-	char		xlogarchcmd[MAXPGPATH];
-	char	   *dp;
-	char	   *endp;
-	const char *sp;
+	char	   *xlogarchcmd;
+	char	   *nativePath = NULL;
 	int			rc;
 
-	/*
-	 * construct the command to be executed
-	 */
-	dp = xlogarchcmd;
-	endp = xlogarchcmd + MAXPGPATH - 1;
-	*endp = '\0';
-
-	for (sp = XLogArchiveCommand; *sp; sp++)
+	if (path)
 	{
-		if (*sp == '%')
-		{
-			switch (sp[1])
-			{
-				case 'p':
-					/* %p: relative path of source file */
-					sp++;
-					strlcpy(dp, path, endp - dp);
-					make_native_path(dp);
-					dp += strlen(dp);
-					break;
-				case 'f':
-					/* %f: filename of source file */
-					sp++;
-					strlcpy(dp, file, endp - dp);
-					dp += strlen(dp);
-					break;
-				case '%':
-					/* convert %% to a single % */
-					sp++;
-					if (dp < endp)
-						*dp++ = *sp;
-					break;
-				default:
-					/* otherwise treat the % as not special */
-					if (dp < endp)
-						*dp++ = *sp;
-					break;
-			}
-		}
-		else
-		{
-			if (dp < endp)
-				*dp++ = *sp;
-		}
+		nativePath = pstrdup(path);
+		make_native_path(nativePath);
 	}
-	*dp = '\0';
+
+	xlogarchcmd = replace_percent_placeholders(XLogArchiveCommand, "archive_command", "fp", file, nativePath);
 
 	ereport(DEBUG3,
 			(errmsg_internal("executing archive command \"%s\"",
@@ -155,6 +115,8 @@ shell_archive_file(const char *file, const char *path)
 		return false;
 	}
 
+	pfree(xlogarchcmd);
+
 	elog(DEBUG1, "archived write-ahead log file \"%s\"", file);
 	return true;
 }
diff --git a/src/common/Makefile b/src/common/Makefile
index 898701fae1..113029bf7b 100644
--- a/src/common/Makefile
+++ b/src/common/Makefile
@@ -65,6 +65,7 @@ OBJS_COMMON = \
 	kwlookup.o \
 	link-canary.o \
 	md5_common.o \
+	percentrepl.o \
 	pg_get_line.o \
 	pg_lzcompress.o \
 	pg_prng.o \
diff --git a/src/common/archive.c b/src/common/archive.c
index 1466f67ea6..fb95006671 100644
--- a/src/common/archive.c
+++ b/src/common/archive.c
@@ -20,7 +20,7 @@
 #endif
 
 #include "common/archive.h"
-#include "lib/stringinfo.h"
+#include "common/percentrepl.h"
 
 /*
  * BuildRestoreCommand
@@ -41,81 +41,14 @@ BuildRestoreCommand(const char *restoreCommand,
 					const char *xlogfname,
 					const char *lastRestartPointFname)
 {
-	StringInfoData result;
-	const char *sp;
+	char	   *nativePath = NULL;
 
-	/*
-	 * Build the command to be executed.
-	 */
-	initStringInfo(&result);
-
-	for (sp = restoreCommand; *sp; sp++)
+	if (xlogpath)
 	{
-		if (*sp == '%')
-		{
-			switch (sp[1])
-			{
-				case 'p':
-					{
-						char	   *nativePath;
-
-						/* %p: relative path of target file */
-						if (xlogpath == NULL)
-						{
-							pfree(result.data);
-							return NULL;
-						}
-						sp++;
-
-						/*
-						 * This needs to use a placeholder to not modify the
-						 * input with the conversion done via
-						 * make_native_path().
-						 */
-						nativePath = pstrdup(xlogpath);
-						make_native_path(nativePath);
-						appendStringInfoString(&result,
-											   nativePath);
-						pfree(nativePath);
-						break;
-					}
-				case 'f':
-					/* %f: filename of desired file */
-					if (xlogfname == NULL)
-					{
-						pfree(result.data);
-						return NULL;
-					}
-					sp++;
-					appendStringInfoString(&result, xlogfname);
-					break;
-				case 'r':
-					/* %r: filename of last restartpoint */
-					if (lastRestartPointFname == NULL)
-					{
-						pfree(result.data);
-						return NULL;
-					}
-					sp++;
-					appendStringInfoString(&result,
-										   lastRestartPointFname);
-					break;
-				case '%':
-					/* convert %% to a single % */
-					sp++;
-					appendStringInfoChar(&result, *sp);
-					break;
-				default:
-					/* otherwise treat the % as not special */
-					appendStringInfoChar(&result, *sp);
-					break;
-			}
-		}
-		else
-		{
-			appendStringInfoChar(&result, *sp);
-		}
+		nativePath = pstrdup(xlogpath);
+		make_native_path(nativePath);
 	}
 
-	return result.data;
+	return replace_percent_placeholders(restoreCommand, "restore_command", "frp",
+										xlogfname, lastRestartPointFname, nativePath);
 }
diff --git a/src/common/meson.build b/src/common/meson.build
index a1fc398d8e..41bd58ebdf 100644
--- a/src/common/meson.build
+++ b/src/common/meson.build
@@ -17,6 +17,7 @@ common_sources = files(
   'kwlookup.c',
   'link-canary.c',
   'md5_common.c',
+  'percentrepl.c',
   'pg_get_line.c',
   'pg_lzcompress.c',
   'pg_prng.c',
diff --git a/src/common/percentrepl.c b/src/common/percentrepl.c
new file mode 100644
index 0000000000..25cfd0b2d4
--- /dev/null
+++ b/src/common/percentrepl.c
@@ -0,0 +1,137 @@
+/*-------------------------------------------------------------------------
+ *
+ * percentrepl.c
+ *	  Common routines to replace percent placeholders in strings
+ *
+ * Portions Copyright (c) 1996-2023, PostgreSQL Global Development Group
+ * Portions Copyright (c) 1994, Regents of the University of California
+ *
+ *
+ * IDENTIFICATION
+ *	  src/common/percentrepl.c
+ *
+ *-------------------------------------------------------------------------
+ */
+
+#ifndef FRONTEND
+#include "postgres.h"
+#else
+#include "postgres_fe.h"
+#include "common/logging.h"
+#endif
+
+#include "common/percentrepl.h"
+#include "lib/stringinfo.h"
+
+/*
+ * replace_percent_placeholders
+ *
+ * Replace percent-letter placeholders in input string with the supplied
+ * values.  For example, to replace %f with foo and %b with bar, call
+ *
+ * replace_percent_placeholders(instr, "bf", bar, foo);
+ *
+ * The return value is palloc'd.
+ *
+ * "%%" is replaced by a single "%".
+ *
+ * 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
+ * strings.  It won't be a good match for things like log prefixes or prompts
+ * that use a mix of data types and any invocation will only use a few of the
+ * possible values.
+ *
+ * param_name is the name of the underlying GUC parameter, for error
+ * reporting.  At the moment, this function is only used for GUC parameters.
+ * If other kinds of uses were added, the error reporting would need to be
+ * revised.
+ */
+char *
+replace_percent_placeholders(const char *instr, const char *param_name, const char *letters, ...)
+{
+	StringInfoData result;
+
+	initStringInfo(&result);
+
+	for (const char *sp = instr; *sp; sp++)
+	{
+		if (*sp == '%')
+		{
+			if (sp[1] == '%')
+			{
+				/* Convert %% to a single % */
+				sp++;
+				appendStringInfoChar(&result, *sp);
+			}
+			else if (sp[1] == '\0')
+			{
+				/* Incomplete escape sequence, expected a character afterward */
+#ifdef FRONTEND
+				pg_log_error("invalid value for parameter \"%s\": \"%s\"", param_name, instr);
+				pg_log_error_detail("String ends unexpectedly after escape character \"%%\".");
+				exit(1);
+#else
+				ereport(ERROR,
+						errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+						errmsg("invalid value for parameter \"%s\": \"%s\"", param_name, instr),
+						errdetail("String ends unexpectedly after escape character \"%%\"."));
+#endif
+			}
+			else
+			{
+				/* Look up placeholder character */
+				bool		found = false;
+				va_list		ap;
+
+				sp++;
+
+				va_start(ap, letters);
+				for (const char *lp = letters; *lp; lp++)
+				{
+					char	   *val = va_arg(ap, char *);
+
+					if (*sp == *lp)
+					{
+						if (val)
+						{
+							appendStringInfoString(&result, val);
+							found = true;
+						}
+						/* If val is NULL, we will report an error. */
+						break;
+					}
+				}
+				va_end(ap);
+				if (!found)
+				{
+					/* Unknown escape sequence */
+#ifdef FRONTEND
+					pg_log_error("invalid value for parameter \"%s\": \"%s\"", param_name, instr);
+					pg_log_error_detail("String contains unexpected escape sequence \"%c\".", *sp);
+					exit(1);
+#else
+					ereport(ERROR,
+							errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+							errmsg("invalid value for parameter \"%s\": \"%s\"", param_name, instr),
+							errdetail("String contains unexpected escape sequence \"%c\".", *sp));
+#endif
+				}
+			}
+		}
+		else
+		{
+			appendStringInfoChar(&result, *sp);
+		}
+	}
+
+	return result.data;
+}
diff --git a/src/fe_utils/archive.c b/src/fe_utils/archive.c
index aab813c102..eb1c930ae7 100644
--- a/src/fe_utils/archive.c
+++ b/src/fe_utils/archive.c
@@ -48,8 +48,6 @@ RestoreArchivedFile(const char *path, const char *xlogfname,
 
 	xlogRestoreCmd = BuildRestoreCommand(restoreCommand, xlogpath,
 										 xlogfname, NULL);
-	if (xlogRestoreCmd == NULL)
-		pg_fatal("cannot use restore_command with %%r placeholder");
 
 	/*
 	 * Execute restore_command, which should copy the missing file from
diff --git a/src/include/common/percentrepl.h b/src/include/common/percentrepl.h
new file mode 100644
index 0000000000..2dd88db144
--- /dev/null
+++ b/src/include/common/percentrepl.h
@@ -0,0 +1,18 @@
+/*-------------------------------------------------------------------------
+ *
+ * percentrepl.h
+ *	  Common routines to replace percent placeholders in strings
+ *
+ * Portions Copyright (c) 1996-2023, PostgreSQL Global Development Group
+ * Portions Copyright (c) 1994, Regents of the University of California
+ *
+ * src/include/common/percentrepl.h
+ *
+ *-------------------------------------------------------------------------
+ */
+#ifndef PERCENTREPL_H
+#define PERCENTREPL_H
+
+extern char *replace_percent_placeholders(const char *instr, const char *param_name, const char *letters, ...);
+
+#endif							/* PERCENTREPL_H */

base-commit: 3c569049b7b502bb4952483d19ce622ff0af5fd6
-- 
2.39.0

#13Nathan Bossart
nathandbossart@gmail.com
In reply to: Peter Eisentraut (#12)
Re: Common function for percent placeholder replacement

On Mon, Jan 09, 2023 at 09:36:12AM +0100, Peter Eisentraut wrote:

On 04.01.23 01:37, Nathan Bossart wrote:

On Tue, Dec 20, 2022 at 06:30:40AM +0100, Peter Eisentraut wrote:

+ * A value may be NULL.  If the corresponding placeholder is found in the
+ * input string, the whole function returns NULL.

This appears to be carried over from BuildRestoreCommand(), and AFAICT it
is only necessary because pg_rewind doesn't support %r in restore_command.
IMHO this behavior is counterintuitive and possibly error-prone and should
result in an ERROR instead. Since pg_rewind is the only special case, it
could independently check for %r before building the command.

Yeah, this annoyed me, too. I have now changed it so that a NULL "value" is
the same as an unsupported placeholder. This preserves the existing
behavior.

(Having pg_rewind check for %r itself would probably require replicating
most of the string processing logic (consider something like "%%r"), so it
didn't seem appealing.)

Sounds good to me.

+ nativePath = pstrdup(path);
+ make_native_path(nativePath);

+ nativePath = pstrdup(xlogpath);
+ make_native_path(nativePath);

Should these be freed?

--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

#14Peter Eisentraut
peter.eisentraut@enterprisedb.com
In reply to: Nathan Bossart (#13)
Re: Common function for percent placeholder replacement

On 09.01.23 18:53, Nathan Bossart wrote:

+ nativePath = pstrdup(path);
+ make_native_path(nativePath);

+ nativePath = pstrdup(xlogpath);
+ make_native_path(nativePath);

Should these be freed?

committed with that fixed

#15Nathan Bossart
nathandbossart@gmail.com
In reply to: Peter Eisentraut (#14)
1 attachment(s)
Re: Common function for percent placeholder replacement

On Wed, Jan 11, 2023 at 11:09:01AM +0100, Peter Eisentraut wrote:

committed with that fixed

While rebasing my recovery modules patch set, I noticed a couple of small
things that might be worth cleaning up.

--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

Attachments:

percentrepl_nitpicks.patchtext/x-diff; charset=us-asciiDownload
diff --git a/src/backend/access/transam/xlogarchive.c b/src/backend/access/transam/xlogarchive.c
index f911e8c3a6..fcc87ff44f 100644
--- a/src/backend/access/transam/xlogarchive.c
+++ b/src/backend/access/transam/xlogarchive.c
@@ -154,9 +154,6 @@ RestoreArchivedFile(char *path, const char *xlogfname,
 	xlogRestoreCmd = BuildRestoreCommand(recoveryRestoreCommand,
 										 xlogpath, xlogfname,
 										 lastRestartPointFname);
-	if (xlogRestoreCmd == NULL)
-		elog(ERROR, "could not build restore command \"%s\"",
-			 recoveryRestoreCommand);
 
 	ereport(DEBUG3,
 			(errmsg_internal("executing restore command \"%s\"",
diff --git a/src/common/archive.c b/src/common/archive.c
index de42e914f7..641a58ee88 100644
--- a/src/common/archive.c
+++ b/src/common/archive.c
@@ -33,7 +33,7 @@
  * 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 NULL is returned.
+ * by the caller, then an error is thrown.
  */
 char *
 BuildRestoreCommand(const char *restoreCommand,
#16Peter Eisentraut
peter.eisentraut@enterprisedb.com
In reply to: Nathan Bossart (#15)
Re: Common function for percent placeholder replacement

On 11.01.23 19:54, Nathan Bossart wrote:

On Wed, Jan 11, 2023 at 11:09:01AM +0100, Peter Eisentraut wrote:

committed with that fixed

While rebasing my recovery modules patch set, I noticed a couple of small
things that might be worth cleaning up.

committed, thanks