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. I see
your point.
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? 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.)
If my understanding of this is correct, then I suggest:
1. Add a comment to the $gzip =~ s{\\}{/}g ... line.
2. Remove the $tar =~ s{\\}{/}g ... line.
3. Backpatch to PG15.
