v4-0001-Common-function-for-percent-placeholder-replaceme.patch

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