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.From e0e44dbffafa50bf15bca6a03e98783a82ce072c Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <pe...@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
