Re: pg_verifybackup: TAR format backup verification

On Sat, Aug 24, 2024 at 2:02 AM Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>
> On Wed, Aug 21, 2024 at 7:08 AM Amul Sul <sulamul(at)gmail(dot)com> wrote:
> [....]
> Then the result verifies. But I feel like we should have some test
> cases that do this kind of stuff so that there is automated
> verification. In fact, the current patch seems to have no negative
> test cases at all. I think we should test all the cases in
> 003_corruption.pl with tar format backups as well as with plain format
> backups, which we could do by untarring one of the archives, messing
> something up, and then retarring it. I also think we should have some
> negative test case specific to tar-format backup. I suggest running a
> coverage analysis and trying to craft test cases that hit as much of
> the code as possible. There will probably be some errors you can't
> hit, but you should try to hit the ones you can.
>

Done. I’ve added a few tests that extract, modify, and repack the tar
files, mainly base.tar and skipping tablespace.tar since it mostly
duplicate tests. I’ve also updated 002_algorithm.pl to cover
tests for tar backups.

>
> > 0011 patch: Regarding function names:
> >
> > 1. named the function verify_tar_backup_file() to align with
> > verify_plain_backup_file(), but it does not perform the complete
> > verification as verify_plain_backup_file does. Not sure if it is the
> > right name.
>
> I was thinking of something like precheck_tar_backup_file().

Done.

> > 2. verify_tar_file_contents() is the second and final part of tar
> > backup verification. Should its name be aligned with
> > verify_tar_backup_file()? I’m unsure what the best name would be.
> > Perhaps verify_tar_backup_file_final(), but then
> > verify_tar_backup_file() would need to be renamed to something like
> > verify_tar_backup_file_initial(), which might be too lengthy.
>
> verify_tar_file_contents() actually verifies the contents of all the
> tar files, not just one, so the name is a bit confusing. Maybe
> verify_all_tar_files().
>

Done.

>
> > 3. verify_tar_contents() is the core of verify_tar_file_contents()
> > that handles the actual verification. I’m unsure about the current
> > naming. Should we rename it to something like
> > verify_tar_contents_core()? It wouldn’t be an issue if we renamed
> > verify_tar_file_contents() as pointed in point #2.
>
> verify_one_tar_file()?
>

Done.

>
> But with those renames, I think you really start to see why I'm not
> very comfortable with verify_backup_directory(). The tar and plain
> format cases aren't really doing the same thing. We're just gluing
> them into a single function anyway.

Agreed. I can see the uncomfortness -- added a new function.

>
> I am also still uncomfortable with the way you've refactored some of
> this so that we end up with very small amounts of code far away from
> other code that they influence. Like you end up with this:
>
> /* Check the backup manifest entry for this file. */
> m = verify_manifest_entry(context, relpath, sb.st_size);
>
> /* Validate the pg_control information */
> if (should_verify_control_data(context->manifest, m))
> ...
> if (show_progress && !context->skip_checksums &&
> should_verify_checksum(m))
>
> But verify_manifest_entry can return NULL or it can set m->bad and
> either of those change the result of should_verify_control_data() and
> should_verify_checksum(), but none of that is obvious when you just
> look at this. Admittedly, the code in master isn't brilliant in terms
> of making it obvious what's happening either, but I think this is
> worse. I'm not really sure what I think we should do about that yet,
> but I'm uncomfortable with it.

I am not sure if I fully understand the concern, but I see it
differently. The verify_manifest_entry function returns an entry, m,
that the caller doesn't need to worry about, as it simply passes it to
subsequent routines or macros that are aware of the possible inputs --
whether it's NULL, m->bad, or something else.

Regards,
Amul