On Wed, Aug 14, 2024 at 12:44 PM Robert Haas <robertmh...@gmail.com> wrote: > On Wed, Aug 14, 2024 at 9:20 AM Amul Sul <sula...@gmail.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
