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
From 08a33cc650fb3cd58e742a79b232dbd5d9843008 Mon Sep 17 00:00:00 2001
From: Michael Paquier <mich...@paquier.xyz>
Date: Mon, 6 Feb 2023 09:34:19 +0900
Subject: [PATCH v4] Add progress reporting to pg_verifybackup
This adds a new option to pg_verifybackup called -P/--progress,
showing every second some information about the state of checksum
verification.
Similarly to what is done for pg_rewind and pg_basebackup, the
information printed in the progress report consists of the current
amount of data computed and the total amount of data to compute. This
could be extended later on.
---
src/bin/pg_verifybackup/pg_verifybackup.c | 86 ++++++++++++++++++++++-
src/bin/pg_verifybackup/t/004_options.pl | 21 ++++--
doc/src/sgml/ref/pg_verifybackup.sgml | 15 ++++
3 files changed, 116 insertions(+), 6 deletions(-)
diff --git a/src/bin/pg_verifybackup/pg_verifybackup.c b/src/bin/pg_verifybackup/pg_verifybackup.c
index 7634dfc285..8d5cb1c72b 100644
--- a/src/bin/pg_verifybackup/pg_verifybackup.c
+++ b/src/bin/pg_verifybackup/pg_verifybackup.c
@@ -16,12 +16,14 @@
#include <dirent.h>
#include <fcntl.h>
#include <sys/stat.h>
+#include <time.h>
#include "common/hashfn.h"
#include "common/logging.h"
#include "fe_utils/simple_list.h"
#include "getopt_long.h"
#include "parse_manifest.h"
+#include "pgtime.h"
/*
* For efficiency, we'd like our hash table containing information about the
@@ -57,6 +59,8 @@ typedef struct manifest_file
bool matched;
bool bad;
} manifest_file;
+#define should_check_checksums(m) \
+ (((m)->matched) && !((m)->bad) && (((m)->checksum_type) != CHECKSUM_TYPE_NONE))
/*
* Define a hash table which we can use to store information about the files
@@ -147,10 +151,18 @@ static void report_fatal_error(const char *pg_restrict fmt,...)
pg_attribute_printf(1, 2) pg_attribute_noreturn();
static bool should_ignore_relpath(verifier_context *context, char *relpath);
+static void progress_report(bool finished);
static void usage(void);
static const char *progname;
+/* options */
+static bool show_progress = false;
+
+/* Progress indicators */
+static uint64 total_size = 0;
+static uint64 done_size = 0;
+
/*
* Main entry point.
*/
@@ -162,6 +174,7 @@ main(int argc, char **argv)
{"ignore", required_argument, NULL, 'i'},
{"manifest-path", required_argument, NULL, 'm'},
{"no-parse-wal", no_argument, NULL, 'n'},
+ {"progress", no_argument, NULL, 'P'},
{"quiet", no_argument, NULL, 'q'},
{"skip-checksums", no_argument, NULL, 's'},
{"wal-directory", required_argument, NULL, 'w'},
@@ -219,7 +232,7 @@ main(int argc, char **argv)
simple_string_list_append(&context.ignore_list, "recovery.signal");
simple_string_list_append(&context.ignore_list, "standby.signal");
- while ((c = getopt_long(argc, argv, "ei:m:nqsw:", long_options, NULL)) != -1)
+ while ((c = getopt_long(argc, argv, "ei:m:nPqsw:", long_options, NULL)) != -1)
{
switch (c)
{
@@ -241,6 +254,9 @@ main(int argc, char **argv)
case 'n':
no_parse_wal = true;
break;
+ case 'P':
+ show_progress = true;
+ break;
case 'q':
quiet = true;
break;
@@ -277,6 +293,14 @@ main(int argc, char **argv)
exit(1);
}
+ /* Complain if the specified arguments conflict */
+ if (show_progress && quiet)
+ pg_fatal("cannot specify both %s and %s",
+ "-P/--progress", "-q/--quiet");
+ if (show_progress && skip_checksums)
+ pg_fatal("cannot specify both %s and %s",
+ "-P/--progress", "-s/--skip-checksums");
+
/* Unless --no-parse-wal was specified, we will need pg_waldump. */
if (!no_parse_wal)
{
@@ -638,6 +662,10 @@ verify_backup_file(verifier_context *context, char *relpath, char *fullpath)
m->bad = true;
}
+ /* Update statistics for progress report, if necessary */
+ if (show_progress && should_check_checksums(m))
+ total_size += m->size;
+
/*
* We don't verify checksums at this stage. We first finish verifying that
* we have the expected set of files with the expected sizes, and only
@@ -675,10 +703,12 @@ verify_backup_checksums(verifier_context *context)
manifest_files_iterator it;
manifest_file *m;
+ progress_report(false);
+
manifest_files_start_iterate(context->ht, &it);
while ((m = manifest_files_iterate(context->ht, &it)) != NULL)
{
- if (m->matched && !m->bad && m->checksum_type != CHECKSUM_TYPE_NONE &&
+ if (should_check_checksums(m) &&
!should_ignore_relpath(context, m->pathname))
{
char *fullpath;
@@ -694,6 +724,8 @@ verify_backup_checksums(verifier_context *context)
pfree(fullpath);
}
}
+
+ progress_report(true);
}
/*
@@ -740,6 +772,10 @@ verify_file_checksum(verifier_context *context, manifest_file *m,
close(fd);
return;
}
+
+ /* Report progress */
+ done_size += rc;
+ progress_report(false);
}
if (rc < 0)
report_backup_error(context, "could not read file \"%s\": %m",
@@ -894,6 +930,51 @@ hash_string_pointer(char *s)
return hash_bytes(ss, strlen(s));
}
+/*
+ * Print a progress report based on the global variables.
+ *
+ * Progress report is written at maximum once per second, unless the finished
+ * parameter is set to true.
+ *
+ * If finished is set to true, this is the last progress report. The cursor
+ * is moved to the next line.
+ */
+static void
+progress_report(bool finished)
+{
+ static pg_time_t last_progress_report = 0;
+ pg_time_t now;
+ int percent_size = 0;
+ char totalsize_str[32];
+ char donesize_str[32];
+
+ if (!show_progress)
+ return;
+
+ now = time(NULL);
+ if (now == last_progress_report && !finished)
+ return; /* Max once per second */
+
+ last_progress_report = now;
+ percent_size = total_size ? (int) ((done_size * 100 / total_size)) : 0;
+
+ snprintf(totalsize_str, sizeof(totalsize_str), UINT64_FORMAT,
+ total_size / 1024);
+ snprintf(donesize_str, sizeof(donesize_str), UINT64_FORMAT,
+ done_size / 1024);
+
+ fprintf(stderr,
+ _("%*s/%s kB (%d%%) verified"),
+ (int) strlen(totalsize_str),
+ donesize_str, totalsize_str, percent_size);
+
+ /*
+ * Stay on the same line if reporting to a terminal and we're not done
+ * yet.
+ */
+ fputc((!finished && isatty(fileno(stderr))) ? '\r' : '\n', stderr);
+}
+
/*
* Print out usage information and exit.
*/
@@ -907,6 +988,7 @@ usage(void)
printf(_(" -i, --ignore=RELATIVE_PATH ignore indicated path\n"));
printf(_(" -m, --manifest-path=PATH use specified path for manifest\n"));
printf(_(" -n, --no-parse-wal do not try to parse WAL files\n"));
+ printf(_(" -P, --progress show progress information\n"));
printf(_(" -q, --quiet do not print any output, except for errors\n"));
printf(_(" -s, --skip-checksums skip checksum verification\n"));
printf(_(" -w, --wal-directory=PATH use specified path for WAL files\n"));
diff --git a/src/bin/pg_verifybackup/t/004_options.pl b/src/bin/pg_verifybackup/t/004_options.pl
index 25c485e0ee..654cba743b 100644
--- a/src/bin/pg_verifybackup/t/004_options.pl
+++ b/src/bin/pg_verifybackup/t/004_options.pl
@@ -28,6 +28,16 @@ ok($result, "-q succeeds: exit code 0");
is($stdout, '', "-q succeeds: no stdout");
is($stderr, '', "-q succeeds: no stderr");
+# Test invalid options
+command_fails_like(
+ [ 'pg_verifybackup', '--progress', '--quiet', $backup_path ],
+ qr{cannot specify both -P/--progress and -q/--quiet},
+ 'cannot use --progress and --quiet at the same time');
+command_fails_like(
+ [ 'pg_verifybackup', '--progress', '--skip-checksums', $backup_path ],
+ qr{cannot specify both -P/--progress and -s/--skip-checksums},
+ 'cannot use options --progress and --skip-checksums at the same time');
+
# Corrupt the PG_VERSION file.
my $version_pathname = "$backup_path/PG_VERSION";
my $version_contents = slurp_file($version_pathname);
@@ -48,10 +58,13 @@ command_like(
qr/backup successfully verified/,
'-s skips checksumming');
-# Validation should succeed if we ignore the problem file.
-command_like(
- [ 'pg_verifybackup', '-i', 'PG_VERSION', $backup_path ],
- qr/backup successfully verified/,
+# Validation should succeed if we ignore the problem file. Also, check
+# the progress information.
+command_checks_all(
+ [ 'pg_verifybackup', '-P', '-i', 'PG_VERSION', $backup_path ],
+ 0,
+ [qr/backup successfully verified/],
+ [qr{(\d+/\d+ kB \(\d+%\) verified)+}],
'-i ignores problem file');
# PG_VERSION is already corrupt; let's try also removing all of pg_xact.
diff --git a/doc/src/sgml/ref/pg_verifybackup.sgml b/doc/src/sgml/ref/pg_verifybackup.sgml
index 5f83c98706..b4e0a52ad3 100644
--- a/doc/src/sgml/ref/pg_verifybackup.sgml
+++ b/doc/src/sgml/ref/pg_verifybackup.sgml
@@ -178,6 +178,21 @@ PostgreSQL documentation
</listitem>
</varlistentry>
+ <varlistentry>
+ <term><option>-P</option></term>
+ <term><option>--progress</option></term>
+ <listitem>
+ <para>
+ Enable progress reporting. Turning this on will deliver a progress
+ report while verifying checksums.
+ </para>
+ <para>
+ This option cannot be used together with the option
+ <option>--quiet</option> or <option>--skip-checksums</option>.
+ </para>
+ </listitem>
+ </varlistentry>
+
<varlistentry>
<term><option>-q</option></term>
<term><option>--quiet</option></term>
--
2.39.1
signature.asc
Description: PGP signature
