Re: pg_verifybackup: TAR format backup verification

On Tue, Aug 13, 2024 at 10:49 PM Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>
> On Mon, Aug 12, 2024 at 5:13 AM Amul Sul <sulamul(at)gmail(dot)com> wrote:
> > I tried this in the attached version and made a few additional changes
> > based on Sravan's off-list comments regarding function names and
> > descriptions.
> >
> > Now, verification happens in two passes. The first pass simply
> > verifies the file names, determines their compression types, and
> > returns a list of valid tar files whose contents need to be verified
> > in the second pass. The second pass is called at the end of
> > verify_backup_directory() after all files in that directory have been
> > scanned. I named the functions for pass 1 and pass 2 as
> > verify_tar_file_name() and verify_tar_file_contents(), respectively.
> > The rest of the code flow is similar as in the previous version.
> >
> > In the attached patch set, I abandoned the changes, touching the
> > progress reporting code of plain backups by dropping the previous 0009
> > patch. The new 0009 patch adds missing APIs to simple_list.c to
> > destroy SimplePtrList. The rest of the patch numbers remain unchanged.
>
> I think you've entangled the code paths here for plain-format backup
> and tar-format backup in a way that is not very nice. I suggest
> refactoring things so that verify_backup_directory() is only called
> for plain-format backups, and you have some completely separate
> function (perhaps verify_tar_backup) that is called for tar-format
> backups. I don't think verify_backup_file() should be shared between
> tar-format and plain-format backups either. Let that be just for
> plain-format backups, and have separate logic for tar-format backups.
> Right now you've got "if" statements in various places trying to get
> all the cases correct, but I think you've missed some (and there's
> also the issue of updating all the comments).
>
> For instance, verify_backup_file() recurses into subdirectories, but
> that behavior is inappropriate for a tar format backup, where
> subdirectories should instead be treated like stray files: complain
> that they exist. pg_verify_backup() does this:
>
> /* If it's a directory, just recurse. */
> if (S_ISDIR(sb.st_mode))
> {
> verify_backup_directory(context, relpath, fullpath);
> return;
> }
>
> /* If it's not a directory, it should be a plain file. */
> if (!S_ISREG(sb.st_mode))
> {
> report_backup_error(context,
> "\"%s\" is not a file or directory",
> relpath);
> return;
> }
>
> For a plain format backup, this whole thing should be just:
>
> /* In a tar format backup, we expect only plain files. */
> if (!S_ISREG(sb.st_mode))
> {
> report_backup_error(context,
> "\"%s\" is not a plain file",
> relpath);
> return;
> }
>
> Also, immediately above, you do
> simple_string_list_append(&context->ignore_list, relpath), but that is
> pointless in the tar-file case, and arguably wrong, if -i is going to
> ignore both pathnames in the base directory and also pathnames inside
> the tar files, because we could add something to the ignore list here
> -- accomplishing nothing useful -- and then that ignore-list entry
> could cause us to disregard a stray file with the same name present
> inside one of the tar files -- which is silly. Note that the actual
> point of this logic is to make sure that if we can't stat() a certain
> directory, we don't go off and issue a million complaints about all
> the files in that directory being missing. But this doesn't accomplish
> that goal for a tar-format backup. For a tar-format backup, you'd want
> to figure out which files in the manifest we don't expect to see based
> on this file being inaccessible, and then arrange to suppress future
> complaints about all of those files. But you can't implement that
> here, because you haven't parsed the file name yet. That happens
> later, in verify_tar_file_name().
>
> You could add a whole bunch more if statements here and try to work
> around these issues, but it seems pretty obviously a dead end to me.
> Almost the entire function is going to end up being inside of an
> if-statement. Essentially the only thing in verify_backup_file() that
> should actually be the same in the plain and tar-format cases is that
> you should call stat() either way and check whether it throws an
> error. But that is not enough justification for trying to share the
> whole function.
>

I agree with keeping verify_backup_file() separate, but I'm hesitant
about doing the same for verify_backup_directory(). Otherwise, we
might end up with nearly duplicate functions that are very similar.
Since the changes in verify_backup_directory() are minimal, I've left
it as is, let me know your thoughts on that. I’ve kept
verify_backup_file() separated for plain backup files and added a new
function, verify_tar_backup_file(), in patch 0011. To maintain
consistency, I also renamed verify_backup_file() to
verify_plain_backup_file() in patch 0006.

> I find the logic in verify_tar_file_name() to be somewhat tortured as
> well. The strstr() calls will match those strings anywhere in the
> filename, not just at the end. But also, even if you fixed that, why
> work backward from the end of the filename toward the beginning? It
> would seem like it would more sense to (1) first check if the string
> starts with "base" and set suffix equal to pathname+4, (2) if not,
> strtol(pathname, &suffix, 10) and complain if we didn't eat at least
> one character or got 0 or something too big to be an OID, (3) check
> whether suffix is .tar, .tar.gz, etc.
>

Ok, did it this way.

> In verify_member_checksum(), you set mystreamer->verify_checksum =
> false. That would be correct if there could only ever be one call to
> verify_member_checksum() per member file, but there is no such rule.
> There can be (and, I think, normally will be) more than one
> ASTREAMER_MEMBER_CONTENTS chunk. I'm a little confused as to how this
> code passes any kind of testing.
>

I did that to avoid adding the line for every error case where we
return. The flag is re-enabled when the file contents are yet to be
received in mystreamer->received_bytes < m->size.

> Also in verify_member_checksum(), the mystreamer->received_bytes <
> m->size seems strange. I don't think this is the right way to do
> something when you reach the end of an archive member. The right way
> to do that is to do it when the ASTREAMER_MEMBER_TRAILER chunk shows
> up.
>

Ok, I've split this into two parts: the first part handles incremental
computation at ASTREAMER_MEMBER_CONTENTS, and the second part performs
the final verification at the ASTREAMER_MEMBER_TRAILER stage.

> In verify_member_control_data(), you use astreamer_buffer_untIl(). But
> that's the same buffer that is being used by verify_member_checksum(),
> so I don't really understand how you expect this to work. If this code
> path were ever taken, verify_member_checksum() would see the same data
> more than once.
>

No, for checksum calculation, we directly compute the checksum on the
received content (which is the caller's buffer) without copying it.
However, for control file verification, we need the entire file, so we
first copy it into a local buffer within myastremer. This local buffer
is used solely for storing control file data.

I've made some adjustments to align this code style with checksum
verification: copying will occur during the ASTREAMER_MEMBER_CONTENTS
stage, and final verification will be performed in the
ASTREAMER_MEMBER_TRAILER stage.

> The call to pg_log_debug() in this function also seems quite random.
> In a plain-format backup, we'd actually be doing something different
> for pg_controldata vs. other files, namely reading it during the
> initial directory scan. But here we're reading the file in exactly the
> same sense as we're reading every other file, neither more nor less,
> so why mention this file and not all of the others? And why at this
> exact spot in the code?
>

Agreed, it was added without much thinking.

> I suspect that the report_fatal_error("%s: could not read control
> file: read %d of %zu", ...) call is unreachable. I agree that you need
> to handle the case where the control file inside the tar file is not
> the expected length, and in fact I think you should probably write a
> TAP test for that exact scenario to make sure it works. I bet this
> doesn't. Even if it did, the error message makes no sense in context.
> In the plain-format backup, this error would come from code reading
> the actual bytes off the disk -- i.e. the complaint about not being
> able to read the control file would come from the read() system call.
> Here it doesn't.
>

Agreed. Replace that with a check for an expected file size.

In addition to the mentioned changes, I have renamed functions in
astreamer_verify.c to ensure a consistent naming format.

Regards,
Amul.