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