|From:||David Steele <david(at)pgmasters(dot)net>|
|To:||Peter Eisentraut <peter(dot)eisentraut(at)2ndquadrant(dot)com>, Michael Paquier <michael(dot)paquier(at)gmail(dot)com>|
|Cc:||Pg Hackers <pgsql-hackers(at)postgresql(dot)org>, Ashutosh Sharma <ashu(dot)coek88(at)gmail(dot)com>|
|Subject:||Re: PATCH: Exclude additional directories in pg_basebackup|
|Views:||Raw Message | Whole Thread | Download mbox | Resend email|
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.
> The comment for pg_replslot is incorrect. I think you can copy
> replication slots just fine, but you usually don't want to.
> 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
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
Done for all files and directories.
> Testing the symlink handling would also be good.
Done using pg_replslot for the test.
> 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.
|Next Message||Robert Haas||2016-09-23 19:10:20||Re: Rename max_parallel_degree?|
|Previous Message||David Steele||2016-09-23 18:19:23||Re: pg_basebackup creates a corrupt file for pg_stat_tmp and pg_replslot on a backup location|