Re: Common function for percent placeholder replacement
On 11.01.23 19:54, Nathan Bossart wrote: On Wed, Jan 11, 2023 at 11:09:01AM +0100, Peter Eisentraut wrote: committed with that fixed While rebasing my recovery modules patch set, I noticed a couple of small things that might be worth cleaning up. committed, thanks
Re: Common function for percent placeholder replacement
On Wed, Jan 11, 2023 at 11:09:01AM +0100, Peter Eisentraut wrote:
> committed with that fixed
While rebasing my recovery modules patch set, I noticed a couple of small
things that might be worth cleaning up.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
diff --git a/src/backend/access/transam/xlogarchive.c b/src/backend/access/transam/xlogarchive.c
index f911e8c3a6..fcc87ff44f 100644
--- a/src/backend/access/transam/xlogarchive.c
+++ b/src/backend/access/transam/xlogarchive.c
@@ -154,9 +154,6 @@ RestoreArchivedFile(char *path, const char *xlogfname,
xlogRestoreCmd = BuildRestoreCommand(recoveryRestoreCommand,
xlogpath, xlogfname,
lastRestartPointFname);
- if (xlogRestoreCmd == NULL)
- elog(ERROR, "could not build restore command \"%s\"",
- recoveryRestoreCommand);
ereport(DEBUG3,
(errmsg_internal("executing restore command \"%s\"",
diff --git a/src/common/archive.c b/src/common/archive.c
index de42e914f7..641a58ee88 100644
--- a/src/common/archive.c
+++ b/src/common/archive.c
@@ -33,7 +33,7 @@
* The result is a palloc'd string for the restore command built. The
* caller is responsible for freeing it. If any of the required arguments
* is NULL and that the corresponding alias is found in the command given
- * by the caller, then NULL is returned.
+ * by the caller, then an error is thrown.
*/
char *
BuildRestoreCommand(const char *restoreCommand,
Re: Common function for percent placeholder replacement
On 09.01.23 18:53, Nathan Bossart wrote: + nativePath = pstrdup(path); + make_native_path(nativePath); + nativePath = pstrdup(xlogpath); + make_native_path(nativePath); Should these be freed? committed with that fixed
Re: Common function for percent placeholder replacement
On Mon, Jan 09, 2023 at 09:36:12AM +0100, Peter Eisentraut wrote: > On 04.01.23 01:37, Nathan Bossart wrote: >> On Tue, Dec 20, 2022 at 06:30:40AM +0100, Peter Eisentraut wrote: >> > + * 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.) Sounds good to me. > + nativePath = pstrdup(path); > + make_native_path(nativePath); > + nativePath = pstrdup(xlogpath); > + make_native_path(nativePath); Should these be freed? -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
Re: Common function for percent placeholder replacement
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
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();
- for (c = base_command; *c != '\0'; ++c)
- {
- /* Anything other than '%' is copied verbatim. */
- if (*c != '%')
- {
- appendStringInfoChar(, *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(, '%');
- }
- else if (*c == 'f')
- {
- /* '%f' is replaced by the filename */
- appendStringInfoString(, filename);
- }
- else if (*c == 'd')
- {
- /* '%d' is replaced by the target detail */
- appendStringInfoString(, 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 \"%%\""));
-
Re: Common function for percent placeholder replacement
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 \"%%\"")));
> + * 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.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
Re: Common function for percent placeholder replacement
> > How about this new one with variable arguments? I like this a lot, but I also see merit in Alvaro's PERCENT_OPT variadic, which at least avoids the two lists getting out of sync. Initially, I was going to ask that we have shell-quote-safe equivalents of whatever fixed parameters we baked in, but this allows the caller to do that as needed. It seems we could now just copy quote_identifier and strip out the keyword checks to get the desired effect. Has anyone else had a need for quote-safe args in the shell commands?
Re: Common function for percent placeholder replacement
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 like
replace_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
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();
- for (c = base_command; *c != '\0'; ++c)
- {
- /* Anything other than '%' is copied verbatim. */
- if (*c != '%')
- {
- appendStringInfoChar(, *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(, '%');
- }
- else if (*c == 'f')
- {
- /* '%f' is replaced by the filename */
- appendStringInfoString(, filename);
- }
- else if (*c == 'd')
- {
- /* '%d' is replaced by the target detail */
- appendStringInfoString(, 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
Re: Common function for percent placeholder replacement
On Mon, Dec 19, 2022 at 3:13 AM Peter Eisentraut wrote: > I agree. Here is an updated patch with the error checking included. Nice, but I think something in the error report needs to indicate what caused the problem exactly. As coded, I think the user would have to guess which GUC caused the problem. For basebackup_to_shell that might not be too hard since you would have to try to initiate a backup to a shell target to trigger the error, but for something that happens at server start, you don't want to have to go search all of postgresql.conf for possible causes. -- Robert Haas EDB: http://www.enterprisedb.com
Re: Common function for percent placeholder replacement
On 2022-Dec-19, Peter Eisentraut wrote:
> On 14.12.22 18:05, Justin Pryzby wrote:
> > On Wed, Dec 14, 2022 at 08:31:02AM +0100, Peter Eisentraut wrote:
> > > + return replace_percent_placeholders(base_command, "df", (const char
> > > *[]){target_detail, filename});
> >
> > This is a "compound literal", which I gather is required by C99.
> >
> > But I don't think that's currently being exercised, so I wonder if it's
> > going to break some BF members.
>
> We already use this, for example in pg_dump.
Yeah, we have this
#define ARCHIVE_OPTS(...) &(ArchiveOpts){__VA_ARGS__}
which we then use like this
ArchiveEntry(fout,
dbCatId, /* catalog ID */
dbDumpId, /* dump ID */
ARCHIVE_OPTS(.tag = datname,
.owner = dba,
.description = "DATABASE",
.section = SECTION_PRE_DATA,
.createStmt = creaQry->data,
.dropStmt = delQry->data));
I think the new one is not great. I wish we could do something more
straightforward, maybe like
replace_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.
--
Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
Subversion to GIT: the shortest path to happiness I've ever heard of
(Alexey Klyukin)
Re: Common function for percent placeholder replacement
On 14.12.22 18:05, Justin Pryzby wrote:
On Wed, Dec 14, 2022 at 08:31:02AM +0100, Peter Eisentraut wrote:
+ return replace_percent_placeholders(base_command, "df", (const char
*[]){target_detail, filename});
This is a "compound literal", which I gather is required by C99.
But I don't think that's currently being exercised, so I wonder if it's
going to break some BF members.
We already use this, for example in pg_dump.
Re: Common function for percent placeholder replacement
On 14.12.22 17:09, Robert Haas wrote:
Well, OK, I'll tentatively cast a vote in favor of adopting
basebackup_to_shell's approach elsewhere. Or to put that in plain
English: I think that if the input appears to be malformed, it's
better to throw an error than to guess what the user meant. In the
case of basebackup_to_shell there are potentially security
ramifications to the setting involved so it seemed like a bad idea to
take a laissez faire approach. But also, just in general, if somebody
supplies an ssl_passphrase_command or archive_command with %, I don't really see why we should treat that differently
than trying to start the server with work_mem=banana.
I agree. Here is an updated patch with the error checking included.
From 8bb85ad7ed99d0255ea82306b08ef7d343ab8edb Mon Sep 17 00:00:00 2001
From: Peter Eisentraut
Date: Mon, 19 Dec 2022 09:04:43 +0100
Subject: [PATCH v2] 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| 59 ++---
src/common/Makefile | 1 +
src/common/archive.c | 81 +---
src/common/meson.build| 1 +
src/common/percentrepl.c | 120 ++
src/include/common/percentrepl.h | 18 +++
9 files changed, 173 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();
- for (c = base_command; *c != '\0'; ++c)
- {
- /* Anything other than '%' is copied verbatim. */
- if (*c != '%')
- {
- appendStringInfoChar(, *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(, '%');
- }
- else if (*c == 'f')
- {
- /* '%f' is replaced by the filename */
- appendStringInfoString(, filename);
- }
- else if (*c == 'd')
- {
- /* '%d' is replaced by the target detail */
- appendStringInfoString(, 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
Re: Common function for percent placeholder replacement
Justin Pryzby writes:
> On Wed, Dec 14, 2022 at 08:31:02AM +0100, Peter Eisentraut wrote:
>> +return replace_percent_placeholders(base_command, "df", (const char
>> *[]){target_detail, filename});
> This is a "compound literal", which I gather is required by C99.
> But I don't think that's currently being exercised, so I wonder if it's
> going to break some BF members.
It's pretty illegible, whatever it is. Could we maybe expend a
few more keystrokes in favor of readability?
regards, tom lane
Re: Common function for percent placeholder replacement
On Wed, Dec 14, 2022 at 08:31:02AM +0100, Peter Eisentraut wrote:
> + return replace_percent_placeholders(base_command, "df", (const char
> *[]){target_detail, filename});
This is a "compound literal", which I gather is required by C99.
But I don't think that's currently being exercised, so I wonder if it's
going to break some BF members.
--
Justin
Re: Common function for percent placeholder replacement
On Wed, Dec 14, 2022 at 2:31 AM Peter Eisentraut wrote: > 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.) Well, OK, I'll tentatively cast a vote in favor of adopting basebackup_to_shell's approach elsewhere. Or to put that in plain English: I think that if the input appears to be malformed, it's better to throw an error than to guess what the user meant. In the case of basebackup_to_shell there are potentially security ramifications to the setting involved so it seemed like a bad idea to take a laissez faire approach. But also, just in general, if somebody supplies an ssl_passphrase_command or archive_command with %, I don't really see why we should treat that differently than trying to start the server with work_mem=banana. -- Robert Haas EDB: http://www.enterprisedb.com
