Common function for percent placeholder replacement
authorPeter Eisentraut <peter@eisentraut.org>
Wed, 11 Jan 2023 06:22:51 +0000 (07:22 +0100)
committerPeter Eisentraut <peter@eisentraut.org>
Wed, 11 Jan 2023 09:42:35 +0000 (10:42 +0100)
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.

The unified handling is now that incorrect and unknown placeholders
are an error, where previously in most cases they were skipped or
ignored.  This affects the following settings:

- archive_cleanup_command
- archive_command
- recovery_end_command
- restore_command
- ssl_passphrase_command

The following settings are part of this refactoring but already had
stricter error handling and should be unchanged in their behavior:

- basebackup_to_shell.command

Reviewed-by: Nathan Bossart <nathandbossart@gmail.com>
Discussion: https://www.postgresql.org/message-id/flat/5238bbed-0b01-83a6-d4b2-7eb0562a054e%40enterprisedb.com

contrib/basebackup_to_shell/basebackup_to_shell.c
src/backend/access/transam/xlogarchive.c
src/backend/libpq/be-secure-common.c
src/backend/postmaster/shell_archive.c
src/common/Makefile
src/common/archive.c
src/common/meson.build
src/common/percentrepl.c [new file with mode: 0644]
src/fe_utils/archive.c
src/include/common/percentrepl.h [new file with mode: 0644]

index 8d583550b5062d4f9226ba8f0b802d67333b9049..29f5069d427baaa0f4063809ddc560b835612b0a 100644 (file)
@@ -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);
 }
 
 /*
index 76abc74c67801187fed1e90577daab52965f2ccb..f911e8c3a6ff8287272cda103efee8cde81061ac 100644 (file)
@@ -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)
    {
        /*
index f6078c91c34bc31296a44817c19b79e6a8d85033..ab5e2dfa2bffe9ff17a7958971be5cd8039c022e 100644 (file)
@@ -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;
 }
 
index 6f98414a4083b460d9e8f2c988afd968eed5c0df..806b81c3f227fda5bbaac61da5702b688e8e8bce 100644 (file)
@@ -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,20 @@ 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);
+
+   if (nativePath)
+       pfree(nativePath);
 
    ereport(DEBUG3,
            (errmsg_internal("executing archive command \"%s\"",
@@ -155,6 +118,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;
 }
index 898701fae1c17c0733204dc0105824fad2ff9b35..113029bf7b91519746144f3a4d4a31a23c664714 100644 (file)
@@ -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 \
index 1466f67ea69f522a1480b0238ec2b9c79e015f7a..de42e914f749b0ec19666e81a0c30a0bac86cb8f 100644 (file)
@@ -20,7 +20,7 @@
 #endif
 
 #include "common/archive.h"
-#include "lib/stringinfo.h"
+#include "common/percentrepl.h"
 
 /*
  * BuildRestoreCommand
@@ -41,81 +41,20 @@ BuildRestoreCommand(const char *restoreCommand,
                    const char *xlogfname,
                    const char *lastRestartPointFname)
 {
-   StringInfoData result;
-   const char *sp;
+   char       *nativePath = NULL;
+   char       *result;
 
-   /*
-    * Build the command to be executed.
-    */
-   initStringInfo(&result);
-
-   for (sp = restoreCommand; *sp; sp++)
+   if (xlogpath)
    {
-       if (*sp == '%')
-       {
-           switch (sp[1])
-           {
-               case 'p':
-                   {
-                       char       *nativePath;
+       nativePath = pstrdup(xlogpath);
+       make_native_path(nativePath);
+   }
 
-                       /* %p: relative path of target file */
-                       if (xlogpath == NULL)
-                       {
-                           pfree(result.data);
-                           return NULL;
-                       }
-                       sp++;
+   result = replace_percent_placeholders(restoreCommand, "restore_command", "frp",
+                                         xlogfname, lastRestartPointFname, nativePath);
 
-                       /*
-                        * 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);
-       }
-   }
+   if (nativePath)
+       pfree(nativePath);
 
-   return result.data;
+   return result;
 }
index a1fc398d8e4495d5031f7b9f7b2b7d8e95743e24..41bd58ebdf19a5c4df3940f30340ed0dde0f2c61 100644 (file)
@@ -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 (file)
index 0000000..d78571f
--- /dev/null
@@ -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, "param_name", "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;
+}
index aab813c10271b8d8d87c94dd08d30f642dffc75f..eb1c930ae7f27e1f7152a1d23e68911ef1025f09 100644 (file)
@@ -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 (file)
index 0000000..0efb6ec
--- /dev/null
@@ -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 */