Re: fairywren is generating bogus BASE_BACKUP commands

On Fri, Jan 21, 2022 at 5:09 PM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> I think the backup_target string was already corrupted that way when
> pg_basebackup absorbed it from optarg. It's pretty hard to believe that
> the strchr/pnstrdup stanza got it wrong. However, comparing the
> TARGET_DETAIL to what the TAP test says it issued:
>
> # Running: pg_basebackup --no-sync -cfast --target server:/home/pgrunner/bf/root/HEAD/pgsql.build/src/bin/pg_basebackup/tmp_check/tmp_test_Ag8r/backuponserver -X none
> pg_basebackup: error: could not initiate base backup: ERROR: unrecognized target: "server;C"
>
> it's absolutely clear that something decided to munge the target string.
> Given that colon is reserved in Windows filename syntax, it's not
> so surprising if it munged it wrong, or at least more aggressively
> than you expected.

Nothing in the Perl code tells it that the particular argument in
question is a path rather than anything else, so it must be applying
some heuristic to decide whether to munge it. That's horrible.

> I kinda wonder if this notation for the target was well-chosen.
> Keeping the file name strictly separate from the "type" keyword
> might be a wiser move. Quite aside from Windows-isms, there
> are going to be usages where this is hard to tell from a URL.
> (If memory serves, double leading slash is significant to some
> networked file systems.)

Maybe. I think it's important that the notation is not ridiculously
verbose, and -t server --target-detail $PATH is a LOT more typing.

> While we're on the subject of ill-chosen option syntax: "-cfast"
> with non double dashes? Really? That's horribly ambiguous.
> Most programs would parse something like that as five single-letter
> options, and most users who know Unix would expect it to mean that.

I'm not sure whether you're complaining that we accept that syntax or
using it, but AFAIK I'm responsible for neither. I think the syntax
has been accepted since pg_basebackup was added in 2011, and Andres
added it to this test case earlier this week (with -cfast in the
subject line of the commit message). FWIW, though, I've been aware of
that syntax for a long time and never thought it was a problem. I
usually spell the option in exactly that way when I use it, and I'm
relatively sure that things I've given to customers would break if we
removed support for it. I don't know how we'd do that anyway, since
all that's happening here is a call to getopt_long().

--
Robert Haas
EDB: http://www.enterprisedb.com