Thread: Common function for percent placeholder replacement
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.
Attachment
On Wed, Dec 14, 2022 at 2:31 AM Peter Eisentraut <peter.eisentraut@enterprisedb.com> 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 %<something unexpected>, 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
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
Justin Pryzby <pryzby@telsasoft.com> 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
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 %<something > unexpected>, 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.
Attachment
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.
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)
On Mon, Dec 19, 2022 at 3:13 AM Peter Eisentraut <peter.eisentraut@enterprisedb.com> 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
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.)
Attachment
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?
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?
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
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.)
Attachment
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
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
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
Attachment
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