Thread: 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
Attachment
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
Attachment
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
Attachment
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
Attachment
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
Attachment
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
Attachment
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
Attachment
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
Attachment
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