Re: PATCH: Exclude additional directories in pg_basebackup

On Sat, Sep 24, 2016 at 3:26 AM, David Steele <david(at)pgmasters(dot)net> wrote:
> On 9/12/16 2:50 PM, Peter Eisentraut wrote:
>> The comments on excludeDirContents are somewhat imprecise. For
>> example, none of those directories are actually removed or recreated
>> on startup, only the contents are.
>
> Fixed.
>
>> The comment for pg_replslot is incorrect. I think you can copy
>> replication slots just fine, but you usually don't want to.
>
> Fixed.
>
>> What is the 2 for in this code?
>>
>> if (strcmp(pathbuf + 2, excludeFile[excludeIdx]) == 0)
>
> This should be basepathlen + 1 (i.e., $PGDATA/). Fixed.
>
>> I would keep the signature of _tarWriteDir() similar to
>> _tarWriteHeader(). So don't pass sizeonly in there, or do pass in
>> sizeonly but do the same with _tarWriteHeader().
>
> I'm not sure how much more similar they can be, but I do agree that
> sizeonly logic should be wrapped in _tarWriteHeader(). Done.
>
>> The documentation in pg_basebackup.sgml and protocol.sgml should be
>> updated.
>
> Done. I moved a few things to the protocol docs to avoid repetition.
>
>> Add some tests. At least test that one entry from the directory list
>> and one entry from the files list is not contained in the backup
>> result.
>
> Done for all files and directories.
>
>> Testing the symlink handling would also be good.
>
> Done using pg_replslot for the test.
>
>> (The
>> pg_basebackup tests claim that Windows doesn't support symlinks and
>> therefore skip all the symlink tests. That seems a bit at odds with
>> your code handling symlinks on Windows.)
>
> Hopefully Michael's response down-thread answered your question.

Just a nit:
<para>
- <filename>postmaster.pid</>
+ <filename>postmaster.pid</> and <filename>postmaster.opts</>
</para>
Having one <para> block for each file would be better.

+const char *excludeFile[] =
excludeFiles[]?

+# Move pg_replslot out of $pgdata and create a symlink to it
+rename("$pgdata/pg_replslot", "$tempdir/pg_replslot")
+ or die "unable to move $pgdata/pg_replslot";
+symlink("$tempdir/pg_replslot", "$pgdata/pg_replslot");
This will blow up on Windows. Those tests need to be included in the
SKIP block. Even if your code needs to support junction points on
Windows, as perl does not offer an equivalent for it we just cannot
test it on this platform.

Except that, the patch looks pretty good to me.
--
Michael