Horton

Re: pg_verifybackup: TAR format backup verification

Amul Sul <sulamul@gmail.com>

View thread View on postgr.es
From: Amul Sul <sulamul@gmail.com>
To: Robert Haas <robertmhaas@gmail.com>
Cc: Sravan Kumar <sravanvcybage@gmail.com>, PostgreSQL-development <pgsql-hackers@postgresql.org>
Date: 2024-08-21T11:07:49Z
Lists: pgsql-hackers

Attachments

On Tue, Aug 20, 2024 at 3:56 PM Amul Sul <sulamul@gmail.com> wrote:
>
> On Sat, Aug 17, 2024 at 1:34 AM Robert Haas <robertmhaas@gmail.com> wrote:
> >
> > On Fri, Aug 16, 2024 at 3:53 PM Robert Haas <robertmhaas@gmail.com> wrote:
[...]
> > There's probably more to look at here but I'm running out of energy for today.
> >
>
> Thank you for the review and committing 0004 and 0006 patches.
>

I have reworked a few comments, revised error messages, and made some
minor tweaks in the attached version.

Additionally, I would like to discuss a couple of concerns regarding
error placement and function names to gather your suggestions.

0007 patch: Regarding error placement:

1. I'm a bit unsure about the (bytes_read != m->size) check that I
placed in verify_checksum() and whether it's in the right place. Per
our previous discussion, this check is applicable to plain backup
files since they can change while being read, but not for files
belonging to tar backups. For consistency, I included the check for
tar backups as well, as it doesn't cause any harm. Is it okay to keep
this check in verify_checksum(), or should I move it back to
verify_file_checksum() and apply it only to the plain backup format?

2. For the verify_checksum() function, I kept the argument name as
bytes_read. Should we rename it to something more meaningful like
computed_bytes, computed_size, or checksum_computed_size?


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.

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.

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.

Regards,
Amul

Commits

  1. Adjust json_manifest_per_file_callback API in one more place.

  2. File size in a backup manifest should use uint64, not size_t.

  3. Fix some pg_verifybackup issues reported by Coverity.

  4. pg_verifybackup: Move some declarations to new pg_verifybackup.h

  5. pg_verifybackup: Move skip_checksums into verifier_context.

  6. Improve file header comments for astramer code.

  7. Move astreamer (except astreamer_inject) to fe_utils.

  8. Move recovery injector astreamer to a separate header file.

  9. Rename bbstreamer to astreamer.