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