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