Re: fairywren is generating bogus BASE_BACKUP commands

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> "server" is a valid backup target, but "server;C" is not. And I think
> this must be a bug on the client side, because the server logs the
> generated query:

> 2022-01-21 20:53:11.618 UTC [8404:10] 010_pg_basebackup.pl LOG:
> received replication command: BASE_BACKUP ( LABEL 'pg_basebackup base
> backup', PROGRESS, CHECKPOINT 'fast', MANIFEST 'yes',
> TABLESPACE_MAP, TARGET 'server;C', TARGET_DETAIL
> '\\tools\\msys64\\home\\pgrunner\\bf\\root\\HEAD\\pgsql.build\\src\\bin\\pg_basebackup\\tmp_check\\tmp_test_Ag8r\\backuponserver')

> So it's not that the server parses the query and introduces gibberish
> -- the client has already introduced gibberish when constructing the
> query. But the code to do that is pretty straightforward -- we just
> call strchr to find the colon in the backup target, and then
> pnstrdup() the part before the colon and use the latter part as-is. If
> pnstrdup were failing to add a terminating \0 then this would be quite
> plausible, but I think it shouldn't. Unless the operating sytem's
> strnlen() is buggy? That seems like a stretch, so feel free to tell me
> what obvious stupid thing I did here and am not seeing...

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.

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.)

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.

regards, tom lane