Re: backup manifests

On Wed, Apr 1, 2020 at 4:47 PM Robert Haas <robertmhaas@gmail.com> wrote:
> Here's a new patch set. I haven't fixed the things in your latest
> round of review comments yet, but I did rewrite the documentation for
> pg_validatebackup, add documentation for the new pg_waldump option,
> and add regression tests for the new WAL-checking facility of
> pg_validatebackup.
>
> 0001 - add pg_waldump -q
> 0002 - add checksum helpers
> 0003 - core backup manifest patch, now with WAL verification included

And here's another new patch set. After some experimentation, I was
able to manually test the timeline-switch-during-a-base-backup case
and found that it had bugs in both pg_validatebackup and the code I
added to the backend's basebackup.c. So I fixed those. It would be
nice to have automated tests, but you need a large database (so that
backing it up takes non-trivial time) and a load on the primary (so
that WAL is being replayed during the backup) and there's a race
condition (because the backup has to not finish before the cascading
standby learns that the upstream has been promoted), so I don't at
present see a practical way to automate that. I did verify, in manual
testing, that a problem with WAL files on either timeline caused a
validation failure. I also verified that the LSNs at which the standby
began replay and reached consistency matched what was stored in the
manifest.

I also implemented Noah's suggestion that we should write the backup
manifest under a temporary name and then rename it afterward.
Stephen's original complaint that you could end up with a backup that
validates successfully even though we died before we got the WAL is,
at this point, moot, because pg_validatebackup is now capable of
noticing that the WAL is missing. Nevertheless, this seems like a nice
belt-and-suspenders check. I was able to position the rename *after*
we fsync() the backup directory, as well as after we get all of the
WAL, so unless those steps complete you'll have backup_manifest.tmp
rather than backup_manifest. It's true that, if we suffered an OS
crash before the fsync() completed and lost some files or some file
data, pg_validatebackup ought to fail anyway, but this way it is
absolutely certain to fail, and to do so immediately. Likewise for a
failure while fetching WAL that manages to leave the output directory
behind.

This version has also had a visit from the pgindent police.

I think this responds to pretty much all of the complaints that I know
about and upon which we have a reasonable degree of consensus. There
are still some things that not everybody is happy about. In
particular, Stephen and David are unhappy about using CRC-32C as the
default algorithm, but Andres and Noah both think it's a reasonable
choice, even if not as robust as everybody will want. As I agree, I'm
going to stick with that choice.

Also, there is still some debate about what the tool ought to be
called. My previous suggestion to rename this from pg_validatebackup
to pg_validatemanifest seems wrong now that WAL validation has been
added; in fact, given that we now have two independent sanity checks
on a backup, I'm going to argue that it would be reasonable to extend
that by adding more kinds of backup validation, perhaps even including
the permissions check that Andres suggested before. I don't plan to
pursue that at present, though. There remains the idea of merging this
with some other tool, but I still don't like that. On the one hand,
it's been suggested that it could be merged into pg_checksums, but I
think that is less appealing now that it seems to be growing into a
general-purpose backup validation tool. It may do things that have
nothing to do with checksums. On the other hand, it's been suggested
that it ought to be called pg_validate and that pg_checksums ought to
eventually be merged into it, but I don't think we have sufficient
consensus here to commit the project to such a plan. Nobody
responsible for the pg_checksums work has endorsed it, for example.
Moreover, pg_checksums does things other than validation, such as
enabling and disabling checksums. Therefore, I think it's unclear that
such a plan would achieve a sufficient degree of consensus.

For my part, I think this is a general issue that is not really this
patch's problem to solve. We have had multiple discussions over the
years about reducing the number of binaries that we ship. We could
have a general binary called "pg" or similar and use subcommands: pg
createdb, pg basebackup, pg validatebackup, etc. I think such an
approach is worth considering, though it would certainly be an
adjustment for everyone. Or we might do something else. But I don't
want to deal with that in this patch.

A couple of other minor suggestions have been made: (1) rejigger
things to avoid message duplication related to launching external
binaries, (2) maybe use appendShellString, and (3) change some details
of error-reporting related to manifest parsing. I don't believe anyone
views these as blockers; (1) and (2) are preexisting issues that this
patch extends to one new case.

Considering all the foregoing, I would like to go ahead and commit this stuff.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company