Hi,
On 2022-07-07 12:09:32 +0200, Peter Eisentraut wrote:
> On 06.07.22 15:21, Andres Freund wrote:
> > > Here is my rough assessment of where we are with this patch set:
> > >
> > > 08b4330ded prereq: deal with \ paths in basebackup_to_shell tests.
> > >
> > > This still needs clarification, per my previous review.
> > Hm. I thought I had explained that bit, but apparently not. Well, it's pretty
> > simple - without this, the test fail on windows for me, as soon as one of the
> > binaries is in a directory with spaces (which is common on windows). Iimagine
> > what happens with e.g.
> > qq{$gzip --fast > "$escaped_backup_path\\\\%f.gz"}
> > if $gzip contains spaces.
> >
> >
> > This doesn't happen currently on CI because nothing runs these tests on
> > windows yet.
>
> Hmm, maybe this patch looked different the last time I saw it.
Don't think it had changed since I wrote it first.
> The quoting of "$gzip" is clearly necessary.
>
> What about the backslash replacements s{\\}{/}g ? Is that also required for
> passing the path through the shell?
I don't recall the details, but it's definitely needed for embedding it into
postgresql.conf. We could double the escapes instead, but that doesn't seem an
improvement (and wouldn't work when passing it to the shell anymore).
> If so, the treatment of $tar in that
> way doesn't seem necessary, since that doesn't get called through an
> intermediate shell. (That would then also explain why $gzip in the
> pg_basebackup tests doesn't require that treatment, which had previously
> confused me.)
Yea, it doesn't immediately look like it's needed, and the test passes without
it. I guess I might just have tried to be complete...
Greetings,
Andres Freund