Re: PATCH: Exclude additional directories in pg_basebackup

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.

--
-David
david@pgmasters.net