Add progress reporting to pg_verifybackup

On Fri, Jan 06, 2023 at 04:28:42PM +0900, Masahiko Sawada wrote:

I've attached the simple patch to add the progress reporting option to
pg_verifybackup. The progress information is displayed with --progress
option only during the checksum verification, which is the most time
consuming task. It cannot be used together with --quiet option.

That looks helpful, particularly when a backup has many relation
files. Calculating the total size when browsing the file list looks
fine.

+   /* Complain if the specified arguments conflict */
+   if (show_progress && quiet)
+       pg_fatal("cannot specify both --progress and --quiet");

Nothing on HEAD proposes --progress and --quiet at the same time from
what I can see, so just disabling the combination is fine by me. For
the error message, I would recommend to switch to a more generic
pattern, as of:
"cannot specify both %s and %s", "-P/--progress", "-q/--quiet"

Could you add a check based on command_fails_like() in 004_options.pl,
at least? A second test to check after the output of --progress would
be a nice bonus, for example by sticking --progress into one of the
existing commands doing a command_like().
--
Michael

On Wed, Feb 1, 2023 at 10:25 AM Michael Paquier <michael@paquier.xyz> wrote:

On Fri, Jan 06, 2023 at 04:28:42PM +0900, Masahiko Sawada wrote:

I've attached the simple patch to add the progress reporting option to
pg_verifybackup. The progress information is displayed with --progress
option only during the checksum verification, which is the most time
consuming task. It cannot be used together with --quiet option.

That looks helpful, particularly when a backup has many relation
files. Calculating the total size when browsing the file list looks
fine.

+   /* Complain if the specified arguments conflict */
+   if (show_progress && quiet)
+       pg_fatal("cannot specify both --progress and --quiet");

Nothing on HEAD proposes --progress and --quiet at the same time from
what I can see, so just disabling the combination is fine by me. For
the error message, I would recommend to switch to a more generic
pattern, as of:
"cannot specify both %s and %s", "-P/--progress", "-q/--quiet"

Agreed.

Could you add a check based on command_fails_like() in 004_options.pl,
at least?

Agreed, done in v2 patch.

A second test to check after the output of --progress would
be a nice bonus, for example by sticking --progress into one of the
existing commands doing a command_like().

It seems that the --progress option doesn't work with command_like()
since the progress information is written in stderr but command_like()
doesn't want it.

Regards,

--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com

On Thu, Feb 02, 2023 at 02:57:44PM +0900, Masahiko Sawada wrote:

It seems that the --progress option doesn't work with command_like()
since the progress information is written in stderr but command_like()
doesn't want it.

What about command_checks_all()? It should check for stderr, stdout
as well as the expected error code.
--
Michael

On Thu, Feb 2, 2023 at 3:12 PM Michael Paquier <michael@paquier.xyz> wrote:

On Thu, Feb 02, 2023 at 02:57:44PM +0900, Masahiko Sawada wrote:

It seems that the --progress option doesn't work with command_like()
since the progress information is written in stderr but command_like()
doesn't want it.

What about command_checks_all()? It should check for stderr, stdout
as well as the expected error code.

Seems a good idea. Please find an attached patch.

Regards,

--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com

On Thu, Feb 02, 2023 at 05:56:47PM +0900, Masahiko Sawada wrote:

Seems a good idea. Please find an attached patch.

That seems rather OK seen from here. I'll see about getting that
applied except if there is an objection of any kind.
--
Michael

On Sat, Feb 04, 2023 at 12:32:15PM +0900, Michael Paquier wrote:

That seems rather OK seen from here. I'll see about getting that
applied except if there is an objection of any kind.

Okay, I have looked at that again this morning and I've spotted one
tiny issue: specifying --progress with --skip-checksums does not
really make sense.

Ignoring entries with a bad size would lead to incorrect progress
report (for example, say an entry in the manifest has a largely
oversized size number), so your approach on this side is correct. The
application of the ignore list via -i is also correct, as a patch
matching with should_ignore_relpath() does not compute an extra size
for total_size.

I was also wondering for a few minutes while on it whether it would
have been cleaner to move the check for should_ignore_relpath()
directly in verify_file_checksum() and verify_backup_file(), but
nobody has complained about that as being a problem, either.

What do you think about the updated version attached?
--
Michael

On Mon, Feb 6, 2023 at 9:35 AM Michael Paquier <michael@paquier.xyz> wrote:

On Sat, Feb 04, 2023 at 12:32:15PM +0900, Michael Paquier wrote:

That seems rather OK seen from here. I'll see about getting that
applied except if there is an objection of any kind.

Okay, I have looked at that again this morning and I've spotted one
tiny issue: specifying --progress with --skip-checksums does not
really make sense.

I thought that too, but I thought it's better to ignore it, instead of
erroring out. For example, we can specify both --disable and
--progress options to pg_checksum commands, but we don't write any
progress information in this case.

Regards,

--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com

On Mon, Feb 06, 2023 at 12:27:51PM +0900, Masahiko Sawada wrote:

I thought that too, but I thought it's better to ignore it, instead of
erroring out. For example, we can specify both --disable and
--progress options to pg_checksum commands, but we don't write any
progress information in this case.

Well, if you don't feel strongly about that, that's fine by me as
well, so I have applied your v3 with the tweaks I posted previously,
without the restriction on --skip-checksums.
--
Michael

On Mon, Feb 6, 2023 at 2:45 PM Michael Paquier <michael@paquier.xyz> wrote:

On Mon, Feb 06, 2023 at 12:27:51PM +0900, Masahiko Sawada wrote:

I thought that too, but I thought it's better to ignore it, instead of
erroring out. For example, we can specify both --disable and
--progress options to pg_checksum commands, but we don't write any
progress information in this case.

Well, if you don't feel strongly about that, that's fine by me as
well, so I have applied your v3 with the tweaks I posted previously,
without the restriction on --skip-checksums.

Thank you!

Regards,

--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com