Обсуждение: Add progress reporting to pg_verifybackup

Hi all,

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.

Feedback is very welcome.

Regards,

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

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