On 3/27/20 1:53 PM, Robert Haas wrote:
On Thu, Mar 26, 2020 at 4:37 PM David Steele <da...@pgmasters.net> wrote:
I know you and Stephen have agreed on a number of doc changes, would it
be possible to get a new patch with those included? I finally have time
to do a review of this tomorrow. I saw some mistakes in the docs in the
current patch but I know those patches are not current.
Hi David,
Here's a new version with some fixes:
- Fixes for doc typos noted by Stephen Frost and Andres Freund.
- Replace a doc paragraph about the advantages and disadvantages of
CRC-32C with one by Stephen Frost, with a slightly change by me that I
thought made it sound more grammatical.
- Change the pg_validatebackup documentation so that it makes no
mention of compatible tools, per Stephen.
- Reword the discussion of the exclude list in the pg_validatebackup
documentation, per discussion between Stephen and myself.
- Try to make the documentation more clear about the fact that we
check for both extra and missing files.
- Incorporate a fix from Amit Kapila to make 003_corruption.pl pass on Windows.
Thanks!
There appear to be conflicts with 67e0adfb3f98:
$ git apply -3
../download/v14-0002-Generate-backup-manifests-for-base-backups-and-v.patch
../download/v14-0002-Generate-backup-manifests-for-base-backups-and-v.patch:3396:
trailing whitespace.
sub cleanup_search_directory_fails
error: patch failed: src/backend/replication/basebackup.c:258
Falling back to three-way merge...
Applied patch to 'src/backend/replication/basebackup.c' with conflicts.
U src/backend/replication/basebackup.c
warning: 1 line adds whitespace errors.
> + Specifies the algorithm that should be used to checksum
each file
> + for purposes of the backup manifest. Currently, the available
perhaps "for inclusion in the backup manifest"? Anyway, I think this
sentence is awkward.
> + Specifies the algorithm that should be used to checksum each
file
> + for purposes of the backup manifest. Currently, the available
And again.
> + because the files themselves do not need to read.
should be "need to be read".
> + the manifest itself will always contain a
<literal>SHA256</literal>
I think just "the manifest will always contain" is fine.
> + manifeste itself, and is therefore ignored. Note that the
manifest
typo "manifeste", perhaps remove itself.
> { "Path": "backup_label", "Size": 224, "Last-Modified": "2020-03-27
18:33:18 GMT", "Checksum-Algorithm": "CRC32C", "Checksum": "b914bec9" },
Storing the checksum type with each file seems pretty redundant.
Perhaps that could go in the header? You could always override if a
specific file had a different checksum type, though that seems unlikely.
In general it might be good to go with shorter keys: "mod", "chk", etc.
Manifests can get pretty big and that's a lot of extra bytes.
I'm also partial to using epoch time in the manifest because it is
generally easier for programs to work with. But, human-readable doesn't
suck, either.
> if (maxrate > 0)
> maxrate_clause = psprintf("MAX_RATE %u", maxrate);
> + if (manifest)
A linefeed here would be nice.
> + manifestfile *tabent;
This is an odd name. A holdover from the tab-delimited version?
> + printf(_("Usage:\n %s [OPTION]... BACKUPDIR\n\n"), progname);
When I ran pg_validatebackup I expected to use -D to specify the backup
dir since pg_basebackup does. On the other hand -D is weird because I
*really* expect that to be the pg data dir.
But, do we want this to be different from pg_basebackup?
> + checksum_length = checksum_string_length / 2;
This check is defeated if a single character is added the to checksum.
Not too big a deal since you still get an error, but still.
> + * Verify that the manifest checksum is correct.
This is not working the way I would expect -- I could freely modify the
manifest without getting a checksum error on the manifest. For example:
$ /home/vagrant/test/pg/bin/pg_validatebackup test/backup3
pg_validatebackup: fatal: invalid checksum for file "backup_label":
"408901e0814f40f8ceb7796309a59c7248458325a21941e7c55568e381f53831?"
So, if I deleted the entry above, I got a manifest checksum error. But
if I just modified the checksum I get a file checksum error with no
manifest checksum error.
I would prefer a manifest checksum error in all cases where it is wrong,
unless --exit-on-error is specified.
--
-David
da...@pgmasters.net
