Re: pg_verifybackup: TAR format backup verification

On Wed, Aug 14, 2024 at 12:44 PM Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Wed, Aug 14, 2024 at 9:20 AM Amul Sul <sulamul(at)gmail(dot)com> wrote:
> > I agree with keeping verify_backup_file() separate, but I'm hesitant
> > about doing the same for verify_backup_directory().
>
> I don't have time today to go through your whole email or re-review
> the code, but I plan to circle back to that at a later time, However,
> I want to respond to this point in the meanwhile.

I have committed 0004 (except that I changed a comment) and 0005
(except that I didn't move READ_CHUNK_SIZE).

Looking at the issue mentioned above again, I agree that the changes
in verify_backup_directory() in this version don't look overly
invasive in this version. I'm still not 100% convinced it's the right
call, but it doesn't seem bad.

You have a spurious comment change to the header of verify_plain_backup_file().

I feel like the naming of tarFile and tarFiles is not consistent with
the overall style of this file.

I don't like this:

[robert.haas ~]$ pg_verifybackup btar
pg_verifybackup: error: pg_waldump does not support parsing WAL files
from a tar archive.
pg_verifybackup: hint: Try "pg_verifybackup --help" to skip parse WAL
files option.

The hint seems like it isn't really correct grammar, and I also don't
see why we can't be more specific. How about "You must use -n,
--no-parse-wal when verifying a tar-format backup."?

The primary message seems a bit redundant, because parsing WAL files
is the only thing pg_waldump does. How about "pg_waldump cannot read
from a tar archive"? Note that primary error messages should not end
in a period (while hint and detail messages should).

+ int64 num = strtoi64(relpath, &suffix, 10);

+ if (suffix == NULL || (num <= 0) || (num > OID_MAX))

Seems like too many parentheses.

--
Robert Haas
EDB: http://www.enterprisedb.com