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-06T05:57:08Z
Lists: pgsql-hackers

Attachments

On Mon, Aug 5, 2024 at 10:29 PM Robert Haas <robertmhaas@gmail.com> wrote:
>
> On Fri, Aug 2, 2024 at 7:43 AM Amul Sul <sulamul@gmail.com> wrote:
> > Please consider the attached version for the review.
>
> Thanks. I committed 0001-0003. The only thing that I changed was that
> in 0001, you forgot to pgindent, which actually mattered quite a bit,
> because astreamer is one character shorter than bbstreamer.
>

Understood. Thanks for tidying up and committing the patches.

> Before we proceed with the rest of this patch series, I think we
> should fix up the comments for some of the astreamer files. Proposed
> patch for that attached; please review.
>

Looks good to me, except for the following typo that I have fixed in
the attached version:

s/astreamer_plan_writer/astreamer_plain_writer/

> I also noticed that cfbot was unhappy about this patch set:
>
> [10:37:55.075] pg_verifybackup.c:100:7: error: no previous extern
> declaration for non-static variable 'format'
> [-Werror,-Wmissing-variable-declarations]
> [10:37:55.075] char            format = '\0';          /* p(lain)/t(ar) */
> [10:37:55.075]                 ^
> [10:37:55.075] pg_verifybackup.c:100:1: note: declare 'static' if the
> variable is not intended to be used outside of this translation unit
> [10:37:55.075] char            format = '\0';          /* p(lain)/t(ar) */
> [10:37:55.075] ^
> [10:37:55.075] pg_verifybackup.c:101:23: error: no previous extern
> declaration for non-static variable 'compress_algorithm'
> [-Werror,-Wmissing-variable-declarations]
> [10:37:55.075] pg_compress_algorithm compress_algorithm = PG_COMPRESSION_NONE;
> [10:37:55.075]                       ^
> [10:37:55.075] pg_verifybackup.c:101:1: note: declare 'static' if the
> variable is not intended to be used outside of this translation unit
> [10:37:55.075] pg_compress_algorithm compress_algorithm = PG_COMPRESSION_NONE;
> [10:37:55.075] ^
> [10:37:55.075] 2 errors generated.
>

Fixed in the attached version.

> Please fix and, after posting future versions of the patch set, try to
> remember to check http://cfbot.cputube.org/amul-sul.html

Sure. I used to rely on that earlier, but after Cirrus CI in the
GitHub repo, I assumed the workflow would be the same as cfbot and
started overlooking it. However, cfbot reported a warning that didn't
appear in my GitHub run. From now on, I'll make sure to check cfbot as
well.

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.