Re: Add system identifier to backup manifest

On Thu, Feb 1, 2024 at 3:06 AM Robert Haas <robertmhaas(at)gmail(dot)com> wrote:

> On Thu, Jan 25, 2024 at 2:52 AM Amul Sul <sulamul(at)gmail(dot)com> wrote:
> > Thank you for the review-comments, updated version attached.
>
> I generally agree with 0001. I spent a long time thinking about your
> decision to make verifier_context contain a pointer to manifest_data
> instead of, as it does currently, a pointer to manifest_files_hash. I
> don't think that's a horrible idea, but it also doesn't seem to be
> used anywhere currently. One advantage of the current approach is that
> we know that none of the code downstream of verify_backup_directory()
> or verify_backup_checksums() actually cares about anything other than
> the manifest_files_hash. That's kind of nice. If we didn't change this
> as you have done here, then we would need to continue passing the WAL
> ranges to parse_required_walI() and the system identifier would have
> to be passed explicitly to the code that checks the system identifier,
> but that's not such a bad thing, either. It makes it clear which
> functions are using which information.
>

I intended to minimize the out param of parse_manifest_file(), which
currently
returns manifest_files_hash and manifest_wal_range, and I need two more --
manifest versions and the system identifier.

But before you go change anything there, exactly when should 0002 be
> checking the system identifier in the control file? What happens now
> is that we first walk over the directory tree and make sure we have
> the files (verify_backup_directory) and then go through and verify
> checksums in a second pass (verify_backup_checksums). We do this
> because it lets us report problems that can be detected cheaply --
> like missing files -- relatively quickly, and problems that are more
> expensive to detect -- like mismatching checksums -- only after we've
> reported all the cheap-to-detect problems. At what stage should we
> verify the control file? I don't really like verifying it first, as
> you've done, because I think the error message change in
> 004_options.pl is a clear regression. When the whole directory is
> missing, it's much more pleasant to complain about the directory being
> missing than some file inside the directory being missing.
>
> What I'd be inclined to suggest is that you have verify_backup_file()
> notice when the file it's being asked to verify is the control file,
> and have it check the system identifier at that stage. I think if you
> do that, then the error message change in 004_options.pl goes away.
> Now, to do that, you'd need to have the whole manifest_data available
> from the context, not just the manifest_files_hash, so that you can
> see the expected system identifier. And, interestingly, if you take
> this approach, then it appears to me that 0001 is correct as-is and
> doesn't need any changes.
>

Yeah, we can do that, but I think it is a bit inefficient to have strcmp()
check for the pg_control file on each verify_backup_file() call, despite, we
know that path. Also, I think, we need additional handling to ensure that
the
system identifier has been verified in verify_backup_file(), what if the
pg_control file itself missing from the backup -- might be a rare case, but
possible.

For now, we can do the system identifier validation after
verify_backup_directory().

Regards,
Amul