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: Sravan Kumar <sravanvcybage@gmail.com>

Cc: PostgreSQL-development <pgsql-hackers@postgresql.org>

Date: 2024-07-22T11:52:07Z

Lists: pgsql-hackers

Attachments

v2-0007-Refactor-split-verify_control_file.patch (application/octet-stream) patch v2-0007
v2-0008-pg_verifybackup-Add-backup-format-and-compression.patch (application/octet-stream) patch v2-0008
v2-0006-Refactor-split-verify_file_checksum-function.patch (application/octet-stream) patch v2-0006
v2-0010-pg_verifybackup-Tests-and-document.patch (application/octet-stream) patch v2-0010
v2-0009-pg_verifybackup-Read-tar-files-and-verify-its-con.patch (application/octet-stream) patch v2-0009
v2-0003-Refactor-move-astreamer-files-to-fe_utils-to-make.patch (application/octet-stream) patch v2-0003
v2-0004-Refactor-move-some-part-of-pg_verifybackup.c-to-p.patch (application/octet-stream) patch v2-0004
v2-0002-Refactor-Add-astreamer_inject.h-and-move-related-.patch (application/octet-stream) patch v2-0002
v2-0005-Refactor-split-verify_backup_file-function.patch (application/octet-stream) patch v2-0005
v2-0001-Refactor-Rename-all-bbstreamer-references-to-astr.patch (application/octet-stream) patch v2-0001

On Mon, Jul 22, 2024 at 8:29 AM Sravan Kumar <sravanvcybage@gmail.com> wrote:
>
> Hi Amul,
>      thanks for working on this.
>

Thanks, for your review.

>> + file_name_len = strlen(relpath);
>> + if (file_name_len < file_extn_len ||
>> + strcmp(relpath + file_name_len - file_extn_len, file_extn) != 0)
>> + {
>> + if (compress_algorithm == PG_COMPRESSION_NONE)
>> + report_backup_error(context,
>> + "\"%s\" is not a valid file, expecting tar file",
>> + relpath);
>> + else
>> + report_backup_error(context,
>> + "\"%s\" is not a valid file, expecting \"%s\" compressed tar file",
>> + relpath,
>> + get_compress_algorithm_name(compress_algorithm));
>> + return;
>> + }
>
>
> I believe pg_verifybackup needs to exit after reporting a failure here since it could not figure out a streamer to allocate.
>
The intention here is to continue the verification of the remaining tar files
instead of exiting immediately in case of an error. If the user prefers an
immediate exit, they can use the --exit-on-error option of pg_verifybackup.


> Also, v1-0002 removes #include "pqexpbuffer.h" from astreamer.h and adds it to the new .h file and in v1-0004 it
> reverts the change. So this can be avoided altogether.
>
Fix in the attached version.

Regards,
Amul

Commits

Adjust json_manifest_per_file_callback API in one more place.
- 554d3a18f362 18.0 landed
File size in a backup manifest should use uint64, not size_t.
- d94cf5ca7fad 18.0 landed
Fix some pg_verifybackup issues reported by Coverity.
- fc1b2ce0ee9c 18.0 landed
pg_verifybackup: Move some declarations to new pg_verifybackup.h
- aa2d6b15d6d6 18.0 landed
pg_verifybackup: Move skip_checksums into verifier_context.
- af99d44a889f 18.0 landed
Improve file header comments for astramer code.
- 22b4a1b561f8 18.0 landed
Move astreamer (except astreamer_inject) to fe_utils.
- f80b09bac87d 18.0 landed
Move recovery injector astreamer to a separate header file.
- 53b2c921a0f9 18.0 landed
Rename bbstreamer to astreamer.
- 3c905698114d 18.0 landed