Progress reporting for pg_verify_checksums
Hi,
my colleague Bernd Helmle recently added progress reporting to our
pg_checksums application[1]https://github.com/credativ/pg_checksums/commit/2b691 -- Michael Banck Projektleiter / Senior Berater Tel.: +49 2166 9901-171 Fax: +49 2166 9901-100 Email: michael.banck@credativ.de. I have now forward ported this to
pg_verify_checksums for the September commitfest, please see the
attached patch.
Here's the description:
This optionally prints the progress of pg_verify_checksums via read
kilobytes to the terminal with the new command line argument -P.
If -P was forgotten and pg_verify_checksums operates on a large cluster,
the caller can send SIGUSR1 to pg_verify_checksums to turn progress
status reporting on during runtime.
Michael
[1]: https://github.com/credativ/pg_checksums/commit/2b691 -- Michael Banck Projektleiter / Senior Berater Tel.: +49 2166 9901-171 Fax: +49 2166 9901-100 Email: michael.banck@credativ.de
--
Michael Banck
Projektleiter / Senior Berater
Tel.: +49 2166 9901-171
Fax: +49 2166 9901-100
Email: michael.banck@credativ.de
credativ GmbH, HRB Mönchengladbach 12080
USt-ID-Nummer: DE204566209
Trompeterallee 108, 41189 Mönchengladbach
Geschäftsführung: Dr. Michael Meskes, Jörg Folz, Sascha Heuer
Unser Umgang mit personenbezogenen Daten unterliegt
folgenden Bestimmungen: https://www.credativ.de/datenschutz
Attachments:
pg_verify_checksums_progress.patchtext/x-patch; charset=UTF-8; name=pg_verify_checksums_progress.patchDownload
commit ad69b43a24a6eef8a7deef4c9810ee312f3496fb
Author: Michael Banck <michael.banck@credativ.de>
Date: Fri Aug 31 13:04:59 2018 +0200
Add progress reporting to pg_verify_checksums.
This optionally prints the progress of pg_verify_checksums via read kilobytes
to the terminal with the new command line argument -P.
If -P was forgotten and pg_verify_checksums operates on a large cluster, the
caller can send SIGUSR1 to pg_verify_checksums to turn progress status
reporting on during runtime.
Author: Bernd Helmle
diff --git a/src/bin/pg_verify_checksums/pg_verify_checksums.c b/src/bin/pg_verify_checksums/pg_verify_checksums.c
index a941236563..27b1103902 100644
--- a/src/bin/pg_verify_checksums/pg_verify_checksums.c
+++ b/src/bin/pg_verify_checksums/pg_verify_checksums.c
@@ -21,6 +21,8 @@
#include <sys/stat.h>
#include <dirent.h>
#include <unistd.h>
+#include <signal.h>
+#include <time.h>
#include "pg_getopt.h"
@@ -32,9 +34,18 @@ static ControlFileData *ControlFile;
static char *only_relfilenode = NULL;
static bool verbose = false;
+static bool show_progress = false;
static const char *progname;
+/*
+ * Progress status information.
+ */
+int64 total_size = 0;
+int64 current_size = 0;
+pg_time_t last_progress_update;
+pg_time_t scan_started;
+
static void
usage()
{
@@ -45,6 +56,7 @@ usage()
printf(_(" [-D, --pgdata=]DATADIR data directory\n"));
printf(_(" -v, --verbose output verbose messages\n"));
printf(_(" -r RELFILENODE check only relation with specified relfilenode\n"));
+ printf(_(" -P show progress information\n"));
printf(_(" -V, --version output version information, then exit\n"));
printf(_(" -?, --help show this help, then exit\n"));
printf(_("\nIf no data directory (DATADIR) is specified, "
@@ -60,6 +72,69 @@ static const char *skip[] = {
NULL,
};
+static void
+enable_progress_report(int signo,
+ siginfo_t * siginfo,
+ void *context)
+{
+
+ /* we handle SIGUSR1 only */
+ if (signo == SIGUSR1)
+ {
+ show_progress = true;
+ }
+
+}
+
+/*
+ * Report current progress status. Parts borrowed from
+ * PostgreSQLs' src/bin/pg_basebackup.c
+ */
+static void
+report_progress(bool force)
+{
+ pg_time_t now = time(NULL);
+ int total_percent = 0;
+
+ char totalstr[32];
+ char currentstr[32];
+ char currspeedstr[32];
+
+ /* Make sure we just report at least once a second */
+ if ((now == last_progress_update) && !force)
+ return;
+
+ /* Save current time */
+ last_progress_update = now;
+
+ /*
+ * Calculate current percent done, based on KiB...
+ */
+ total_percent = total_size ? (int64) ((current_size / 1024) * 100 / (total_size / 1024)) : 0;
+
+ /* Don't display larger than 100% */
+ if (total_percent > 100)
+ total_percent = 100;
+
+ /* The same for total size */
+ if (current_size > total_size)
+ total_size = current_size / 1024;
+
+ snprintf(totalstr, sizeof(totalstr), INT64_FORMAT,
+ total_size / 1024);
+ snprintf(currentstr, sizeof(currentstr), INT64_FORMAT,
+ current_size / 1024);
+ snprintf(currspeedstr, sizeof(currspeedstr), INT64_FORMAT,
+ (current_size / 1024) / (((time(NULL) - scan_started) == 0) ? 1 : (time(NULL) - scan_started)));
+ fprintf(stderr, "%s/%s kB (%d%%, %s kB/s)",
+ currentstr, totalstr, total_percent, currspeedstr);
+
+ if (isatty(fileno(stderr)))
+ fprintf(stderr, "\r");
+ else
+ fprintf(stderr, "\n");
+}
+
static bool
skipfile(char *fn)
{
@@ -76,7 +151,7 @@ skipfile(char *fn)
}
static void
-scan_file(char *fn, int segmentno)
+scan_file(char *fn, int segmentno, bool sizeonly)
{
char buf[BLCKSZ];
PageHeader header = (PageHeader) buf;
@@ -113,6 +188,7 @@ scan_file(char *fn, int segmentno)
continue;
csum = pg_checksum_page(buf, blockno + segmentno * RELSEG_SIZE);
+ current_size += r;
if (csum != header->pd_checksum)
{
if (ControlFile->data_checksum_version == PG_DATA_CHECKSUM_VERSION)
@@ -120,6 +196,9 @@ scan_file(char *fn, int segmentno)
progname, fn, blockno, csum, header->pd_checksum);
badblocks++;
}
+
+ if (show_progress)
+ report_progress(false);
}
if (verbose)
@@ -129,9 +208,10 @@ scan_file(char *fn, int segmentno)
close(f);
}
-static void
-scan_directory(char *basedir, char *subdir)
+static int64
+scan_directory(char *basedir, char *subdir, bool sizeonly)
{
+ int64 dirsize = 0;
char path[MAXPGPATH];
DIR *dir;
struct dirent *de;
@@ -192,16 +272,22 @@ scan_directory(char *basedir, char *subdir)
/* Relfilenode not to be included */
continue;
- scan_file(fn, segmentno);
+ dirsize += st.st_size;
+
+ if (!sizeonly)
+ {
+ scan_file(fn, segmentno, sizeonly);
+ }
}
#ifndef WIN32
else if (S_ISDIR(st.st_mode) || S_ISLNK(st.st_mode))
#else
else if (S_ISDIR(st.st_mode) || pgwin32_is_junction(fn))
#endif
- scan_directory(path, de->d_name);
+ dirsize += scan_directory(path, de->d_name, sizeonly);
}
closedir(dir);
+ return dirsize;
}
int
@@ -218,6 +304,8 @@ main(int argc, char *argv[])
int option_index;
bool crc_ok;
+ struct sigaction act; /* to turn progress status info on */
+
set_pglocale_pgservice(argv[0], PG_TEXTDOMAIN("pg_verify_checksums"));
progname = get_progname(argv[0]);
@@ -236,7 +324,7 @@ main(int argc, char *argv[])
}
}
- while ((c = getopt_long(argc, argv, "D:r:v", long_options, &option_index)) != -1)
+ while ((c = getopt_long(argc, argv, "D:r:vP", long_options, &option_index)) != -1)
{
switch (c)
{
@@ -254,6 +342,9 @@ main(int argc, char *argv[])
}
only_relfilenode = pstrdup(optarg);
break;
+ case 'P':
+ show_progress = true;
+ break;
default:
fprintf(stderr, _("Try \"%s --help\" for more information.\n"), progname);
exit(1);
@@ -307,10 +398,56 @@ main(int argc, char *argv[])
exit(1);
}
+
+ /*
+ * Assign SIGUSR1 signal handler to turn progress status information
+ * on in case someone has forgotten -P (and doesn't want to
+ * restart)...
+ */
+ memset(&act, '\0', sizeof(act));
+ act.sa_sigaction = &enable_progress_report;
+ act.sa_flags = SA_SIGINFO;
+
+ /*
+ * Enable signal handler, but don't treat it as severe if we don't
+ * succeed here. Just give a message on STDERR.
+ */
+ if (sigaction(SIGUSR1, &act, NULL) < 0)
+ {
+ fprintf(stderr, _("%s: could not set signal handler to turn on progress bar\n"),
+ progname;
+ }
+
+ /*
+ * Iff progress status information is requested, we need to scan the
+ * directory tree(s) twice, once to get the idea how much data we need
+ * to scan and finally to do the real legwork.
+ */
+ total_size = scan_directory(DataDir, "global", true);
+ total_size += scan_directory(DataDir, "base", true);
+ total_size += scan_directory(DataDir, "pg_tblspc", true);
+
+ /*
+ * Remember start time. Required to calculate the current speed in
+ * report_progress()
+ */
+ scan_started = time(NULL);
+
/* Scan all files */
- scan_directory(DataDir, "global");
- scan_directory(DataDir, "base");
- scan_directory(DataDir, "pg_tblspc");
+ scan_directory(DataDir, "global", false);
+ scan_directory(DataDir, "base", false);
+ scan_directory(DataDir, "pg_tblspc", false);
+
+ /*
+ * Done. Move to next line in case progress information was shown.
+ * Otherwise we clutter the summary output.
+ */
+ if (show_progress)
+ {
+ report_progress(true);
+ if (isatty(fileno(stderr)))
+ fprintf(stderr, "\n");
+ }
printf(_("Checksum scan completed\n"));
printf(_("Data checksum version: %d\n"), ControlFile->data_checksum_version);
Hallo Michael,
my colleague Bernd Helmle recently added progress reporting to our
pg_checksums application[1]. I have now forward ported this to
pg_verify_checksums for the September commitfest, please see the
attached patch.
It seems that the patch does not apply cleanly on master, neither with
"git apply" nor "patch". Could you rebase it?
Here's the description:
This optionally prints the progress of pg_verify_checksums via read
kilobytes
MB? GB?
to the terminal with the new command line argument -P.
If -P was forgotten and pg_verify_checksums operates on a large cluster,
the caller can send SIGUSR1 to pg_verify_checksums to turn progress
status reporting on during runtime.
Hmmm. I cannot say I like the signal feature much. Would it make sense for
the progress to be on by default, and to have a quiet option instead?
--
Fabien.
On 2018-Sep-01, Fabien COELHO wrote:
If -P was forgotten and pg_verify_checksums operates on a large cluster,
the caller can send SIGUSR1 to pg_verify_checksums to turn progress
status reporting on during runtime.Hmmm. I cannot say I like the signal feature much. Would it make sense for
the progress to be on by default, and to have a quiet option instead?
Hmm, I recall this technique being used elsewhere and is sometimes
useful. Can't remember where though -- by manpages, it's not rsync nor
pv ...
How about making it a toggle? Default off, enable-able by option,
toggleable by signal. (If you enable it via the signal, what's the rate
to report at?)
--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Hi,
On Mon, Sep 03, 2018 at 11:21:32AM -0300, Alvaro Herrera wrote:
On 2018-Sep-01, Fabien COELHO wrote:
If -P was forgotten and pg_verify_checksums operates on a large cluster,
the caller can send SIGUSR1 to pg_verify_checksums to turn progress
status reporting on during runtime.Hmmm. I cannot say I like the signal feature much. Would it make sense for
the progress to be on by default, and to have a quiet option instead?Hmm, I recall this technique being used elsewhere and is sometimes
useful. Can't remember where though -- by manpages, it's not rsync nor
pv ...
e2fsck does it. And I think it would be useful for pg_basebackup as
well, but that's for another time.
In any case, pg_basebackup has the same option and it is off by default
as well. I guess the rationale for not having it on by default for
pg_basebackup is the fact is does not have any other output normally.
pg_verify_checksums writes a report at the end however, but maybe that
one could be demoted to verbose mode now that we have it?
How about making it a toggle? Default off, enable-able by option,
toggleable by signal.
That sounds like a good idea.
(If you enable it via the signal, what's the rate to report at?)
You mean how often it reports progress (that seems independent of the
signal)? From testing, it's roughly every second or so.
Michael
--
Michael Banck
Projektleiter / Senior Berater
Tel.: +49 2166 9901-171
Fax: +49 2166 9901-100
Email: michael.banck@credativ.de
credativ GmbH, HRB M�nchengladbach 12080
USt-ID-Nummer: DE204566209
Trompeterallee 108, 41189 M�nchengladbach
Gesch�ftsf�hrung: Dr. Michael Meskes, J�rg Folz, Sascha Heuer
Unser Umgang mit personenbezogenen Daten unterliegt
folgenden Bestimmungen: https://www.credativ.de/datenschutz
Hi,
Am Freitag, den 31.08.2018, 14:50 +0200 schrieb Michael Banck:
my colleague Bernd Helmle recently added progress reporting to our
pg_checksums application[1]. I have now forward ported this to
pg_verify_checksums for the September commitfest, please see the
attached patch.Here's the description:
This optionally prints the progress of pg_verify_checksums via read
kilobytes to the terminal with the new command line argument -P.If -P was forgotten and pg_verify_checksums operates on a large cluster,
the caller can send SIGUSR1 to pg_verify_checksums to turn progress
status reporting on during runtime.
Version 2 of the patch is attached. This is rebased to master as of
422952ee and changes the signal handling to be a toggle as suggested by
Alvaro.
Michael
--
Michael Banck
Projektleiter / Senior Berater
Tel.: +49 2166 9901-171
Fax: +49 2166 9901-100
Email: michael.banck@credativ.de
credativ GmbH, HRB Mönchengladbach 12080
USt-ID-Nummer: DE204566209
Trompeterallee 108, 41189 Mönchengladbach
Geschäftsführung: Dr. Michael Meskes, Jörg Folz, Sascha Heuer
Unser Umgang mit personenbezogenen Daten unterliegt
folgenden Bestimmungen: https://www.credativ.de/datenschutz
Attachments:
pg_verify_checksums_progress_V2.patchtext/x-patch; charset=UTF-8; name=pg_verify_checksums_progress_V2.patchDownload
diff --git a/src/bin/pg_verify_checksums/pg_verify_checksums.c b/src/bin/pg_verify_checksums/pg_verify_checksums.c
index 1bc020ab6c..24ac99d204 100644
--- a/src/bin/pg_verify_checksums/pg_verify_checksums.c
+++ b/src/bin/pg_verify_checksums/pg_verify_checksums.c
@@ -10,6 +10,8 @@
#include "postgres_fe.h"
#include <dirent.h>
+#include <signal.h>
+#include <time.h>
#include <sys/stat.h>
#include <unistd.h>
@@ -21,7 +23,6 @@
#include "storage/checksum.h"
#include "storage/checksum_impl.h"
-
static int64 files = 0;
static int64 blocks = 0;
static int64 badblocks = 0;
@@ -29,9 +30,18 @@ static ControlFileData *ControlFile;
static char *only_relfilenode = NULL;
static bool verbose = false;
+static bool show_progress = false;
static const char *progname;
+/*
+ * Progress status information.
+ */
+int64 total_size = 0;
+int64 current_size = 0;
+pg_time_t last_progress_update;
+pg_time_t scan_started;
+
static void
usage(void)
{
@@ -42,6 +52,7 @@ usage(void)
printf(_(" [-D, --pgdata=]DATADIR data directory\n"));
printf(_(" -v, --verbose output verbose messages\n"));
printf(_(" -r RELFILENODE check only relation with specified relfilenode\n"));
+ printf(_(" -P, --progress show progress information\n"));
printf(_(" -V, --version output version information, then exit\n"));
printf(_(" -?, --help show this help, then exit\n"));
printf(_("\nIf no data directory (DATADIR) is specified, "
@@ -57,6 +68,72 @@ static const char *const skip[] = {
NULL,
};
+static void
+toggle_progress_report(int signo,
+ siginfo_t * siginfo,
+ void *context)
+{
+
+ /* we handle SIGUSR1 only */
+ if (signo == SIGUSR1)
+ {
+ if (show_progress)
+ show_progress = false;
+ else
+ show_progress = true;
+ }
+
+}
+
+/*
+ * Report current progress status. Parts borrowed from
+ * PostgreSQLs' src/bin/pg_basebackup.c
+ */
+static void
+report_progress(bool force)
+{
+ pg_time_t now = time(NULL);
+ int total_percent = 0;
+
+ char totalstr[32];
+ char currentstr[32];
+ char currspeedstr[32];
+
+ /* Make sure we just report at least once a second */
+ if ((now == last_progress_update) && !force)
+ return;
+
+ /* Save current time */
+ last_progress_update = now;
+
+ /*
+ * Calculate current percent done, based on KiB...
+ */
+ total_percent = total_size ? (int64) ((current_size / 1024) * 100 / (total_size / 1024)) : 0;
+
+ /* Don't display larger than 100% */
+ if (total_percent > 100)
+ total_percent = 100;
+
+ /* The same for total size */
+ if (current_size > total_size)
+ total_size = current_size / 1024;
+
+ snprintf(totalstr, sizeof(totalstr), INT64_FORMAT,
+ total_size / 1024);
+ snprintf(currentstr, sizeof(currentstr), INT64_FORMAT,
+ current_size / 1024);
+ snprintf(currspeedstr, sizeof(currspeedstr), INT64_FORMAT,
+ (current_size / 1024) / (((time(NULL) - scan_started) == 0) ? 1 : (time(NULL) - scan_started)));
+ fprintf(stderr, "%s/%s kB (%d%%, %s kB/s)",
+ currentstr, totalstr, total_percent, currspeedstr);
+
+ if (isatty(fileno(stderr)))
+ fprintf(stderr, "\r");
+ else
+ fprintf(stderr, "\n");
+}
+
static bool
skipfile(const char *fn)
{
@@ -73,7 +150,7 @@ skipfile(const char *fn)
}
static void
-scan_file(const char *fn, BlockNumber segmentno)
+scan_file(const char *fn, BlockNumber segmentno, bool sizeonly)
{
PGAlignedBlock buf;
PageHeader header = (PageHeader) buf.data;
@@ -110,6 +187,7 @@ scan_file(const char *fn, BlockNumber segmentno)
continue;
csum = pg_checksum_page(buf.data, blockno + segmentno * RELSEG_SIZE);
+ current_size += r;
if (csum != header->pd_checksum)
{
if (ControlFile->data_checksum_version == PG_DATA_CHECKSUM_VERSION)
@@ -117,6 +195,9 @@ scan_file(const char *fn, BlockNumber segmentno)
progname, fn, blockno, csum, header->pd_checksum);
badblocks++;
}
+
+ if (show_progress)
+ report_progress(false);
}
if (verbose)
@@ -126,9 +207,10 @@ scan_file(const char *fn, BlockNumber segmentno)
close(f);
}
-static void
-scan_directory(const char *basedir, const char *subdir)
+static int64
+scan_directory(const char *basedir, const char *subdir, bool sizeonly)
{
+ int64 dirsize = 0;
char path[MAXPGPATH];
DIR *dir;
struct dirent *de;
@@ -191,16 +273,22 @@ scan_directory(const char *basedir, const char *subdir)
/* Relfilenode not to be included */
continue;
- scan_file(fn, segmentno);
+ dirsize += st.st_size;
+
+ if (!sizeonly)
+ {
+ scan_file(fn, segmentno, sizeonly);
+ }
}
#ifndef WIN32
else if (S_ISDIR(st.st_mode) || S_ISLNK(st.st_mode))
#else
else if (S_ISDIR(st.st_mode) || pgwin32_is_junction(fn))
#endif
- scan_directory(path, de->d_name);
+ dirsize += scan_directory(path, de->d_name, sizeonly);
}
closedir(dir);
+ return dirsize;
}
int
@@ -208,6 +296,7 @@ main(int argc, char *argv[])
{
static struct option long_options[] = {
{"pgdata", required_argument, NULL, 'D'},
+ {"progress", no_argument, NULL, 'P'},
{"verbose", no_argument, NULL, 'v'},
{NULL, 0, NULL, 0}
};
@@ -217,6 +306,8 @@ main(int argc, char *argv[])
int option_index;
bool crc_ok;
+ struct sigaction act; /* to turn progress status info on */
+
set_pglocale_pgservice(argv[0], PG_TEXTDOMAIN("pg_verify_checksums"));
progname = get_progname(argv[0]);
@@ -235,7 +326,7 @@ main(int argc, char *argv[])
}
}
- while ((c = getopt_long(argc, argv, "D:r:v", long_options, &option_index)) != -1)
+ while ((c = getopt_long(argc, argv, "D:r:vP", long_options, &option_index)) != -1)
{
switch (c)
{
@@ -253,6 +344,9 @@ main(int argc, char *argv[])
}
only_relfilenode = pstrdup(optarg);
break;
+ case 'P':
+ show_progress = true;
+ break;
default:
fprintf(stderr, _("Try \"%s --help\" for more information.\n"), progname);
exit(1);
@@ -306,10 +400,54 @@ main(int argc, char *argv[])
exit(1);
}
+
+ /*
+ * Assign SIGUSR1 signal handler to toggle progress status information
+ */
+ memset(&act, '\0', sizeof(act));
+ act.sa_sigaction = &toggle_progress_report;
+ act.sa_flags = SA_SIGINFO;
+
+ /*
+ * Enable signal handler, but don't treat it as severe if we don't
+ * succeed here. Just give a message on STDERR.
+ */
+ if (sigaction(SIGUSR1, &act, NULL) < 0)
+ {
+ fprintf(stderr, _("%s: could not set signal handler to toggle progress\n"),
+ progname);
+ }
+
+ /*
+ * Iff progress status information is requested, we need to scan the
+ * directory tree(s) twice, once to get the idea how much data we need
+ * to scan and finally to do the real legwork.
+ */
+ total_size = scan_directory(DataDir, "global", true);
+ total_size += scan_directory(DataDir, "base", true);
+ total_size += scan_directory(DataDir, "pg_tblspc", true);
+
+ /*
+ * Remember start time. Required to calculate the current speed in
+ * report_progress()
+ */
+ scan_started = time(NULL);
+
/* Scan all files */
- scan_directory(DataDir, "global");
- scan_directory(DataDir, "base");
- scan_directory(DataDir, "pg_tblspc");
+ scan_directory(DataDir, "global", false);
+ scan_directory(DataDir, "base", false);
+ scan_directory(DataDir, "pg_tblspc", false);
+
+ /*
+ * Done. Move to next line in case progress information was shown.
+ * Otherwise we clutter the summary output.
+ */
+ if (show_progress)
+ {
+ report_progress(true);
+ if (isatty(fileno(stderr)))
+ fprintf(stderr, "\n");
+ }
printf(_("Checksum scan completed\n"));
printf(_("Data checksum version: %d\n"), ControlFile->data_checksum_version);
Hallo Michael,
This optionally prints the progress of pg_verify_checksums via read
kilobytes to the terminal with the new command line argument -P.If -P was forgotten and pg_verify_checksums operates on a large cluster,
the caller can send SIGUSR1 to pg_verify_checksums to turn progress
status reporting on during runtime.Version 2 of the patch is attached. This is rebased to master as of
422952ee and changes the signal handling to be a toggle as suggested by
Alvaro.
Patch applies cleanly and compiles.
About tests: "make check" is okay, but ITSM that the command is not
started once, ever, in any test:-( It is unclear whether any test triggers
checksums anyway...
The time() granularity to the second makes the result awkward on small
tests:
8/1554552 kB (0%, 8 kB/s)
1040856/1554552 kB (66%, 1040856 kB/s)
1554552/1554552 kB (100%, 1554552 kB/s)
I'd suggest to reuse the "portability/instr_time.h" stuff which allows a
much better precision.
The "kB" kilo-byte symbols stands for 1000 bytes, but you are displaying
1024-bytes kilos, which symbol is either "KiB" or "KB". Given that we are
about storage which is usually measured in powers of 1,000, so I'd suggest
to use thousands.
Another reserve I have is that on any hardware it is likely that there
will be a lot of kilos, so maybe megas (MB) should be used instead.
I'm wondering whether the bandwidth display should be local (from the last
display) or global (from the start of the command), but for the last
forced one. This is an open question.
Maybe it would be nice to show elapsed time and expected completion time
based on the current speed.
I would be in favor or having this turned on by default, and silenced with
some option. I'm not sure other people would agree, though, so it is an
open question as well.
About the code:
+ if (show_progress)
+ show_progress = false;
+ else
+ show_progress = true;
Why not a simple:
/* invert current state */
show_progress = !show_progress;
I do not see much advantage in using intermediate string buffers for
values:
+ snprintf(totalstr, sizeof(totalstr), INT64_FORMAT,
+ total_size / 1024);
+ snprintf(currentstr, sizeof(currentstr), INT64_FORMAT,
+ current_size / 1024);
+ snprintf(currspeedstr, sizeof(currspeedstr), INT64_FORMAT,
+ (current_size / 1024) / (((time(NULL) - scan_started) == 0) ? 1 : (time(NULL) - scan_started)));
+ fprintf(stderr, "%s/%s kB (%d%%, %s kB/s)",
+ currentstr, totalstr, total_percent, currspeedstr);
ISTM that just one simple fprintf would be fine:
fprintf(stderr, INT64_FORMAT "/" INT64_FORMAT " ...",
formulas for each slot...);
ISTM that this line overwriting trick deserves a comment:
+ if (isatty(fileno(stderr)))
+ fprintf(stderr, "\r");
+ else
+ fprintf(stderr, "\n");
And it runs a little amok with "-v".
+ memset(&act, '\0', sizeof(act));
pg uses 0 instead of '\0' everywhere else.
+ /* Make sure we just report at least once a second */
+ if ((now == last_progress_update) && !force) return;
The comments seems contradictory, I understand the code makes sure that it
is "at most" once a second. Pgbench uses "-P XXX" to strigger a progress
report every XXX second. I'm unsure whether it would be useful to allow
the user to change the 1 second display interval.
I'm not sure why you removed one unrelated line:
#include "storage/checksum.h"
#include "storage/checksum_impl.h"
-
static int64 files = 0;
static int64 blocks = 0;
static int64 badblocks = 0;
There is a problem in the scan_file code: The added sizeonly parameter is
not used. It should be removed.
--
Fabien.
Hi,
thanks for the review!
On Wed, Sep 19, 2018 at 05:17:05PM +0200, Fabien COELHO wrote:
This optionally prints the progress of pg_verify_checksums via read
kilobytes to the terminal with the new command line argument -P.If -P was forgotten and pg_verify_checksums operates on a large cluster,
the caller can send SIGUSR1 to pg_verify_checksums to turn progress
status reporting on during runtime.Version 2 of the patch is attached. This is rebased to master as of
422952ee and changes the signal handling to be a toggle as suggested by
Alvaro.Patch applies cleanly and compiles.
About tests: "make check" is okay, but ITSM that the command is not started
once, ever, in any test:-( It is unclear whether any test triggers checksums
anyway...
Yeah, this was discussed on another thread and some basic tap tests for
pg_verify_checksums were proposed (also by me), but this hasn't been
further addressed.
Personally, I think this would be a good thing to add to v11 even.
In any case, this particular feature might not be very easy to tap test,
but I am open to suggestions, of course.
The time() granularity to the second makes the result awkward on small
tests:8/1554552 kB (0%, 8 kB/s)
1040856/1554552 kB (66%, 1040856 kB/s)
1554552/1554552 kB (100%, 1554552 kB/s)I'd suggest to reuse the "portability/instr_time.h" stuff which allows a
much better precision.The "kB" kilo-byte symbols stands for 1000 bytes, but you are displaying
1024-bytes kilos, which symbol is either "KiB" or "KB". Given that we are
about storage which is usually measured in powers of 1,000, so I'd suggest
to use thousands.Another reserve I have is that on any hardware it is likely that there will
be a lot of kilos, so maybe megas (MB) should be used instead.
The display is exactly the same as in pg_basebackup (except the
bandwith is shown as well), so right now I think it is more useful to be
consistent here. So if we change that, pg_basebackup should be changed
as well I think.
Maybe the code could be factored out to some common file in the future.
I'm wondering whether the bandwidth display should be local (from the last
display) or global (from the start of the command), but for the last forced
one. This is an open question.
Maybe it would be nice to show elapsed time and expected completion time
based on the current speed.
Maybe; this could be added to the factored out common code I mentioned
above.
I would be in favor or having this turned on by default, and silenced with
some option. I'm not sure other people would agree, though, so it is an open
question as well.
If this runs in a script, it would be pretty annoying, so everybody
would have to add --no-progress so I am not convinced. Also, both
pg_basebackup and pgbench don't show progress by default.
About the code:
+ if (show_progress) + show_progress = false; + else + show_progress = true;Why not a simple:
/* invert current state */
show_progress = !show_progress;
Fair enough.
I do not see much advantage in using intermediate string buffers for values:
+ snprintf(totalstr, sizeof(totalstr), INT64_FORMAT, + total_size / 1024); + snprintf(currentstr, sizeof(currentstr), INT64_FORMAT, + current_size / 1024); + snprintf(currspeedstr, sizeof(currspeedstr), INT64_FORMAT, + (current_size / 1024) / (((time(NULL) - scan_started) == 0) ? 1 : (time(NULL) - scan_started))); + fprintf(stderr, "%s/%s kB (%d%%, %s kB/s)", + currentstr, totalstr, total_percent, currspeedstr);ISTM that just one simple fprintf would be fine:
fprintf(stderr, INT64_FORMAT "/" INT64_FORMAT " ...",
formulas for each slot...);
That code is adopted from pg_basebackup.c and the comment there says:
| * Separate step to keep platform-dependent format code out of
| * translatable strings. And we only test for INT64_FORMAT
| * availability in snprintf, not fprintf.
So that sounds legit to me.
ISTM that this line overwriting trick deserves a comment:
+ if (isatty(fileno(stderr))) + fprintf(stderr, "\r"); + else + fprintf(stderr, "\n");
I agree a comment should be in there. But that code is also taken
verbatim from pg_basebackup.c (but this time, there's no comment there,
either).
How about this:
| * If we are reporting to a terminal, send a carriage return so that
| * we stay on the same line. If not, send a newline.
And it runs a little amok with "-v".
Do you suggest we should make those mutually exlusive? I agree that in
general, -P is not very useful if -v is on, but if you have a really big
table, it should still be, no?
+ memset(&act, '\0', sizeof(act));
pg uses 0 instead of '\0' everywhere else.
Ok.
+ /* Make sure we just report at least once a second */ + if ((now == last_progress_update) && !force) return;The comments seems contradictory, I understand the code makes sure that it
is "at most" once a second.
I guess you're right as the identical code in pg_basebackup.c has a
comment saying "Max once per second".
Pgbench uses "-P XXX" to strigger a progress
report every XXX second. I'm unsure whether it would be useful to allow the
user to change the 1 second display interval.
I think pgbench writes a new line for each report? In that case, having
it every second for longer runs might be annoying and I can see the
point in having in configurable.
In the case of pg_basebackup/pg_verify_checksums, I guess 1 second is
fine.
I'm not sure why you removed one unrelated line:
#include "storage/checksum.h"
#include "storage/checksum_impl.h"-
static int64 files = 0;
static int64 blocks = 0;
static int64 badblocks = 0;
Merge error on my side I guess, will fix.
There is a problem in the scan_file code: The added sizeonly parameter is
not used. It should be removed.
Right, this might have been a leftover from an earlier version of the
code, I'll check back with Bernd to make sure that was not a
porting/merge error on my side.
I've attached V3 of the patch, addressing some of your comments.
Michael
--
Michael Banck
Projektleiter / Senior Berater
Tel.: +49 2166 9901-171
Fax: +49 2166 9901-100
Email: michael.banck@credativ.de
credativ GmbH, HRB M�nchengladbach 12080
USt-ID-Nummer: DE204566209
Trompeterallee 108, 41189 M�nchengladbach
Gesch�ftsf�hrung: Dr. Michael Meskes, J�rg Folz, Sascha Heuer
Unser Umgang mit personenbezogenen Daten unterliegt
folgenden Bestimmungen: https://www.credativ.de/datenschutz
Attachments:
pg_verify_checksums_progress_V3.patchtext/x-diff; charset=us-asciiDownload
diff --git a/src/bin/pg_verify_checksums/pg_verify_checksums.c b/src/bin/pg_verify_checksums/pg_verify_checksums.c
index 1bc020ab6c..2e8b14dcb7 100644
--- a/src/bin/pg_verify_checksums/pg_verify_checksums.c
+++ b/src/bin/pg_verify_checksums/pg_verify_checksums.c
@@ -10,6 +10,8 @@
#include "postgres_fe.h"
#include <dirent.h>
+#include <signal.h>
+#include <time.h>
#include <sys/stat.h>
#include <unistd.h>
@@ -29,9 +31,18 @@ static ControlFileData *ControlFile;
static char *only_relfilenode = NULL;
static bool verbose = false;
+static bool show_progress = false;
static const char *progname;
+/*
+ * Progress status information.
+ */
+int64 total_size = 0;
+int64 current_size = 0;
+pg_time_t last_progress_update;
+pg_time_t scan_started;
+
static void
usage(void)
{
@@ -42,6 +53,7 @@ usage(void)
printf(_(" [-D, --pgdata=]DATADIR data directory\n"));
printf(_(" -v, --verbose output verbose messages\n"));
printf(_(" -r RELFILENODE check only relation with specified relfilenode\n"));
+ printf(_(" -P, --progress show progress information\n"));
printf(_(" -V, --version output version information, then exit\n"));
printf(_(" -?, --help show this help, then exit\n"));
printf(_("\nIf no data directory (DATADIR) is specified, "
@@ -57,6 +69,71 @@ static const char *const skip[] = {
NULL,
};
+static void
+toggle_progress_report(int signo,
+ siginfo_t * siginfo,
+ void *context)
+{
+
+ /* we handle SIGUSR1 only, and toggle the value of show_progress */
+ if (signo == SIGUSR1)
+ show_progress = !show_progress;
+
+}
+
+/*
+ * Report current progress status. Parts borrowed from
+ * PostgreSQLs' src/bin/pg_basebackup.c
+ */
+static void
+report_progress(bool force)
+{
+ pg_time_t now = time(NULL);
+ int total_percent = 0;
+
+ char totalstr[32];
+ char currentstr[32];
+ char currspeedstr[32];
+
+ /* Make sure we report at most once a second */
+ if ((now == last_progress_update) && !force)
+ return;
+
+ /* Save current time */
+ last_progress_update = now;
+
+ /*
+ * Calculate current percent done, based on KiB...
+ */
+ total_percent = total_size ? (int64) ((current_size / 1024) * 100 / (total_size / 1024)) : 0;
+
+ /* Don't display larger than 100% */
+ if (total_percent > 100)
+ total_percent = 100;
+
+ /* The same for total size */
+ if (current_size > total_size)
+ total_size = current_size / 1024;
+
+ snprintf(totalstr, sizeof(totalstr), INT64_FORMAT,
+ total_size / 1024);
+ snprintf(currentstr, sizeof(currentstr), INT64_FORMAT,
+ current_size / 1024);
+ snprintf(currspeedstr, sizeof(currspeedstr), INT64_FORMAT,
+ (current_size / 1024) / (((time(NULL) - scan_started) == 0) ? 1 : (time(NULL) - scan_started)));
+ fprintf(stderr, "%s/%s kB (%d%%, %s kB/s)",
+ currentstr, totalstr, total_percent, currspeedstr);
+
+ /*
+ * If we are reporting to a terminal, send a carriage return so that we
+ * stay on the same line. If not, send a newline.
+ */
+ if (isatty(fileno(stderr)))
+ fprintf(stderr, "\r");
+ else
+ fprintf(stderr, "\n");
+}
+
static bool
skipfile(const char *fn)
{
@@ -110,6 +187,7 @@ scan_file(const char *fn, BlockNumber segmentno)
continue;
csum = pg_checksum_page(buf.data, blockno + segmentno * RELSEG_SIZE);
+ current_size += r;
if (csum != header->pd_checksum)
{
if (ControlFile->data_checksum_version == PG_DATA_CHECKSUM_VERSION)
@@ -117,6 +195,9 @@ scan_file(const char *fn, BlockNumber segmentno)
progname, fn, blockno, csum, header->pd_checksum);
badblocks++;
}
+
+ if (show_progress)
+ report_progress(false);
}
if (verbose)
@@ -126,9 +207,10 @@ scan_file(const char *fn, BlockNumber segmentno)
close(f);
}
-static void
-scan_directory(const char *basedir, const char *subdir)
+static int64
+scan_directory(const char *basedir, const char *subdir, bool sizeonly)
{
+ int64 dirsize = 0;
char path[MAXPGPATH];
DIR *dir;
struct dirent *de;
@@ -191,16 +273,22 @@ scan_directory(const char *basedir, const char *subdir)
/* Relfilenode not to be included */
continue;
- scan_file(fn, segmentno);
+ dirsize += st.st_size;
+
+ if (!sizeonly)
+ {
+ scan_file(fn, segmentno);
+ }
}
#ifndef WIN32
else if (S_ISDIR(st.st_mode) || S_ISLNK(st.st_mode))
#else
else if (S_ISDIR(st.st_mode) || pgwin32_is_junction(fn))
#endif
- scan_directory(path, de->d_name);
+ dirsize += scan_directory(path, de->d_name, sizeonly);
}
closedir(dir);
+ return dirsize;
}
int
@@ -208,6 +296,7 @@ main(int argc, char *argv[])
{
static struct option long_options[] = {
{"pgdata", required_argument, NULL, 'D'},
+ {"progress", no_argument, NULL, 'P'},
{"verbose", no_argument, NULL, 'v'},
{NULL, 0, NULL, 0}
};
@@ -217,6 +306,8 @@ main(int argc, char *argv[])
int option_index;
bool crc_ok;
+ struct sigaction act; /* to turn progress status info on */
+
set_pglocale_pgservice(argv[0], PG_TEXTDOMAIN("pg_verify_checksums"));
progname = get_progname(argv[0]);
@@ -235,7 +326,7 @@ main(int argc, char *argv[])
}
}
- while ((c = getopt_long(argc, argv, "D:r:v", long_options, &option_index)) != -1)
+ while ((c = getopt_long(argc, argv, "D:r:vP", long_options, &option_index)) != -1)
{
switch (c)
{
@@ -253,6 +344,9 @@ main(int argc, char *argv[])
}
only_relfilenode = pstrdup(optarg);
break;
+ case 'P':
+ show_progress = true;
+ break;
default:
fprintf(stderr, _("Try \"%s --help\" for more information.\n"), progname);
exit(1);
@@ -306,10 +400,54 @@ main(int argc, char *argv[])
exit(1);
}
+
+ /*
+ * Assign SIGUSR1 signal handler to toggle progress status information
+ */
+ memset(&act, '0', sizeof(act));
+ act.sa_sigaction = &toggle_progress_report;
+ act.sa_flags = SA_SIGINFO;
+
+ /*
+ * Enable signal handler, but don't treat it as severe if we don't
+ * succeed here. Just give a message on STDERR.
+ */
+ if (sigaction(SIGUSR1, &act, NULL) < 0)
+ {
+ fprintf(stderr, _("%s: could not set signal handler to toggle progress\n"),
+ progname);
+ }
+
+ /*
+ * Iff progress status information is requested, we need to scan the
+ * directory tree(s) twice, once to get the idea how much data we need
+ * to scan and finally to do the real legwork.
+ */
+ total_size = scan_directory(DataDir, "global", true);
+ total_size += scan_directory(DataDir, "base", true);
+ total_size += scan_directory(DataDir, "pg_tblspc", true);
+
+ /*
+ * Remember start time. Required to calculate the current speed in
+ * report_progress()
+ */
+ scan_started = time(NULL);
+
/* Scan all files */
- scan_directory(DataDir, "global");
- scan_directory(DataDir, "base");
- scan_directory(DataDir, "pg_tblspc");
+ scan_directory(DataDir, "global", false);
+ scan_directory(DataDir, "base", false);
+ scan_directory(DataDir, "pg_tblspc", false);
+
+ /*
+ * Done. Move to next line in case progress information was shown.
+ * Otherwise we clutter the summary output.
+ */
+ if (show_progress)
+ {
+ report_progress(true);
+ if (isatty(fileno(stderr)))
+ fprintf(stderr, "\n");
+ }
printf(_("Checksum scan completed\n"));
printf(_("Data checksum version: %d\n"), ControlFile->data_checksum_version);
On Tue, Sep 4, 2018 at 2:21 AM Alvaro Herrera <alvherre@2ndquadrant.com> wrote:
On 2018-Sep-01, Fabien COELHO wrote:
If -P was forgotten and pg_verify_checksums operates on a large cluster,
the caller can send SIGUSR1 to pg_verify_checksums to turn progress
status reporting on during runtime.Hmmm. I cannot say I like the signal feature much. Would it make sense for
the progress to be on by default, and to have a quiet option instead?Hmm, I recall this technique being used elsewhere and is sometimes
useful. Can't remember where though -- by manpages, it's not rsync nor
pv ...
On BSD-derived systems including macOS, you can hit ^T to deliver
SIGINFO to the foreground process (much like ^C delivers SIGINT).
Many well behaved programs including rsync (at least on FreeBSD) then
show you some kind of progress or status report:
munro@asterix $ sleep 60
load: 0.11 cmd: sleep 62234 [nanslp] 1.22r 0.00u 0.00s 0% 2032k
sleep: about 58 second(s) left out of the original 60
I've contemplated various crackpot ideas about teaching psql to show
information about what my backend is doing when I hit ^T (perhaps via
a second connection, similar to the way it cancels queries?)...
There was an interesting thread[1]/messages/by-id/20140623101501.GN16260@awork2.anarazel.de about dumping various things from
backends on (multiplexed) SIGUSR1. Also if memory serves, JVMs also
dump internal stuff when you signal them, so you can confirm that they
really did eat all 64GB of your RAM. But none of these things are
examples of enabling/disabling an on-going status reporting mode,
which is perhaps what you meant, they're more like one-off pokes to
dump information.
[1]: /messages/by-id/20140623101501.GN16260@awork2.anarazel.de
--
Thomas Munro
http://www.enterprisedb.com
Hallo Michael,
About patch v3: applies cleanly and compiles.
The xml documentation should be updated! (It is kind of hard to notice
what is not there:-)
See "doc/src/sgml/ref/pg_verify_checksums.sgml".
The time() granularity to the second makes the result awkward on small
tests:8/1554552 kB (0%, 8 kB/s)
1040856/1554552 kB (66%, 1040856 kB/s)
1554552/1554552 kB (100%, 1554552 kB/s)I'd suggest to reuse the "portability/instr_time.h" stuff which allows a
much better precision.
I still think it would make sense to use that instead of low-precision
time().
The "kB" kilo-byte symbols stands for 1000 bytes, but you are displaying
1024-bytes kilos, which symbol is either "KiB" or "KB". Given that we are
about storage which is usually measured in powers of 1,000, so I'd suggest
to use thousands.Another reserve I have is that on any hardware it is likely that there will
be a lot of kilos, so maybe megas (MB) should be used instead.The display is exactly the same as in pg_basebackup (except the
bandwith is shown as well), so right now I think it is more useful to be
consistent here.
Hmmm... units are units, and the display is wrong. The fact that other
commands are wrong as well does not look like a good argument to persist
in an error.
So if we change that, pg_basebackup should be changed as well I think.
I'm okay with fixing pg_basebackup as well! I'm unsure about the best
place to put such a function, though. Probably under "src/common/" where
there is already some front-end specific code ("fe_memutils.c").
Maybe the code could be factored out to some common file in the future.
I would be okay with a progress printing function shared between some
commands. It just needs some place. pg_rewind also has a --rewind option.
Maybe it would be nice to show elapsed time and expected completion time
based on the current speed.Maybe; this could be added to the factored out common code I mentioned
above.
Yep.
I would be in favor or having this turned on by default, and silenced with
some option. I'm not sure other people would agree, though, so it is an open
question as well.If this runs in a script, it would be pretty annoying, so everybody
would have to add --no-progress so I am not convinced. Also, both
pg_basebackup and pgbench don't show progress by default.
Ok.
I do not see much advantage in using intermediate string buffers for values:
+ snprintf(totalstr, sizeof(totalstr), INT64_FORMAT, + total_size / 1024); + snprintf(currentstr, sizeof(currentstr), INT64_FORMAT, + current_size / 1024); + snprintf(currspeedstr, sizeof(currspeedstr), INT64_FORMAT, + (current_size / 1024) / (((time(NULL) - scan_started) == 0) ? 1 : (time(NULL) - scan_started))); + fprintf(stderr, "%s/%s kB (%d%%, %s kB/s)", + currentstr, totalstr, total_percent, currspeedstr);ISTM that just one simple fprintf would be fine:
fprintf(stderr, INT64_FORMAT "/" INT64_FORMAT " ...",
formulas for each slot...);That code is adopted from pg_basebackup.c and the comment there says:
| * Separate step to keep platform-dependent format code out of
| * translatable strings. And we only test for INT64_FORMAT
| * availability in snprintf, not fprintf.So that sounds legit to me.
Hmmm. Translation. Not sure I like much the idea of translating units,
but why not.
| * If we are reporting to a terminal, send a carriage return so that
| * we stay on the same line. If not, send a newline.And it runs a little amok with "-v".
Do you suggest we should make those mutually exlusive? I agree that in
general, -P is not very useful if -v is on, but if you have a really big
table, it should still be, no?
Yep. I was just mentionning that they interfere on a terminal, but I agree
that there is no obvious fix.
+ memset(&act, '\0', sizeof(act));
pg uses 0 instead of '\0' everywhere else.
Ok.
Not '0', plain 0 (the integer of value zero).
--
Fabien.
Hi,
On Wed, Sep 26, 2018 at 10:46:06AM +0200, Fabien COELHO wrote:
The xml documentation should be updated! (It is kind of hard to notice what
is not there:-)See "doc/src/sgml/ref/pg_verify_checksums.sgml".
Right, I've added a paragraph.
The time() granularity to the second makes the result awkward on small
tests:8/1554552 kB (0%, 8 kB/s)
1040856/1554552 kB (66%, 1040856 kB/s)
1554552/1554552 kB (100%, 1554552 kB/s)I'd suggest to reuse the "portability/instr_time.h" stuff which allows a
much better precision.I still think it would make sense to use that instead of low-precision
time().
As the use-case of this is not for small tests, I don't think it is
useful to make the code more complicated for this.
The "kB" kilo-byte symbols stands for 1000 bytes, but you are displaying
1024-bytes kilos, which symbol is either "KiB" or "KB". Given that we are
about storage which is usually measured in powers of 1,000, so I'd suggest
to use thousands.Another reserve I have is that on any hardware it is likely that there will
be a lot of kilos, so maybe megas (MB) should be used instead.The display is exactly the same as in pg_basebackup (except the
bandwith is shown as well), so right now I think it is more useful to be
consistent here.Hmmm... units are units, and the display is wrong. The fact that other
commands are wrong as well does not look like a good argument to persist in
an error.
I've had a look around, and "kB" seems to be a common unit for 1024
bytes, e.g. in pg_test_fsync etc., unless I am mistaken?
So if we change that, pg_basebackup should be changed as well I think.
I'm okay with fixing pg_basebackup as well! I'm unsure about the best place
to put such a function, though. Probably under "src/common/" where there is
already some front-end specific code ("fe_memutils.c").Maybe the code could be factored out to some common file in the future.
I would be okay with a progress printing function shared between some
commands. It just needs some place. pg_rewind also has a --rewind option.
I guess you mean pg_rewind also has a --progress option.
I do agree it makes sense to refactor that, but I don't think this
should be part of this patch.
+ memset(&act, '\0', sizeof(act));
pg uses 0 instead of '\0' everywhere else.
Ok.
Not '0', plain 0 (the integer of value zero).
Oops, thanks for spotting that.
I've attached v4 of the patch.
Michael
--
Michael Banck
Projektleiter / Senior Berater
Tel.: +49 2166 9901-171
Fax: +49 2166 9901-100
Email: michael.banck@credativ.de
credativ GmbH, HRB M�nchengladbach 12080
USt-ID-Nummer: DE204566209
Trompeterallee 108, 41189 M�nchengladbach
Gesch�ftsf�hrung: Dr. Michael Meskes, J�rg Folz, Sascha Heuer
Unser Umgang mit personenbezogenen Daten unterliegt
folgenden Bestimmungen: https://www.credativ.de/datenschutz
Attachments:
pg_verify_checksums_progress_V4.patchtext/x-diff; charset=us-asciiDownload
diff --git a/doc/src/sgml/ref/pg_verify_checksums.sgml b/doc/src/sgml/ref/pg_verify_checksums.sgml
index 905b8f1222..5550b7b2bf 100644
--- a/doc/src/sgml/ref/pg_verify_checksums.sgml
+++ b/doc/src/sgml/ref/pg_verify_checksums.sgml
@@ -80,6 +80,19 @@ PostgreSQL documentation
</varlistentry>
<varlistentry>
+ <term><option>-P</option></term>
+ <term><option>--progress</option></term>
+ <listitem>
+ <para>
+ Enable progress reporting. Turning this on will deliver an approximate
+ progress report during the checksum verification. Sending the
+ <systemitem>SIGUSR1</systemitem> signal will toggle progress reporting
+ on or off during the verification run.
+ </para>
+ </listitem>
+ </varlistentry>
+
+ <varlistentry>
<term><option>-V</option></term>
<term><option>--version</option></term>
<listitem>
diff --git a/src/bin/pg_verify_checksums/pg_verify_checksums.c b/src/bin/pg_verify_checksums/pg_verify_checksums.c
index 1bc020ab6c..ed0dec2325 100644
--- a/src/bin/pg_verify_checksums/pg_verify_checksums.c
+++ b/src/bin/pg_verify_checksums/pg_verify_checksums.c
@@ -10,6 +10,8 @@
#include "postgres_fe.h"
#include <dirent.h>
+#include <signal.h>
+#include <time.h>
#include <sys/stat.h>
#include <unistd.h>
@@ -29,9 +31,18 @@ static ControlFileData *ControlFile;
static char *only_relfilenode = NULL;
static bool verbose = false;
+static bool show_progress = false;
static const char *progname;
+/*
+ * Progress status information.
+ */
+int64 total_size = 0;
+int64 current_size = 0;
+pg_time_t last_progress_update;
+pg_time_t scan_started;
+
static void
usage(void)
{
@@ -42,6 +53,7 @@ usage(void)
printf(_(" [-D, --pgdata=]DATADIR data directory\n"));
printf(_(" -v, --verbose output verbose messages\n"));
printf(_(" -r RELFILENODE check only relation with specified relfilenode\n"));
+ printf(_(" -P, --progress show progress information\n"));
printf(_(" -V, --version output version information, then exit\n"));
printf(_(" -?, --help show this help, then exit\n"));
printf(_("\nIf no data directory (DATADIR) is specified, "
@@ -57,6 +69,71 @@ static const char *const skip[] = {
NULL,
};
+static void
+toggle_progress_report(int signo,
+ siginfo_t * siginfo,
+ void *context)
+{
+
+ /* we handle SIGUSR1 only, and toggle the value of show_progress */
+ if (signo == SIGUSR1)
+ show_progress = !show_progress;
+
+}
+
+/*
+ * Report current progress status. Parts borrowed from
+ * PostgreSQLs' src/bin/pg_basebackup.c
+ */
+static void
+report_progress(bool force)
+{
+ pg_time_t now = time(NULL);
+ int total_percent = 0;
+
+ char totalstr[32];
+ char currentstr[32];
+ char currspeedstr[32];
+
+ /* Make sure we report at most once a second */
+ if ((now == last_progress_update) && !force)
+ return;
+
+ /* Save current time */
+ last_progress_update = now;
+
+ /*
+ * Calculate current percent done, based on KiB...
+ */
+ total_percent = total_size ? (int64) ((current_size / 1024) * 100 / (total_size / 1024)) : 0;
+
+ /* Don't display larger than 100% */
+ if (total_percent > 100)
+ total_percent = 100;
+
+ /* The same for total size */
+ if (current_size > total_size)
+ total_size = current_size / 1024;
+
+ snprintf(totalstr, sizeof(totalstr), INT64_FORMAT,
+ total_size / 1024);
+ snprintf(currentstr, sizeof(currentstr), INT64_FORMAT,
+ current_size / 1024);
+ snprintf(currspeedstr, sizeof(currspeedstr), INT64_FORMAT,
+ (current_size / 1024) / (((time(NULL) - scan_started) == 0) ? 1 : (time(NULL) - scan_started)));
+ fprintf(stderr, "%s/%s kB (%d%%, %s kB/s)",
+ currentstr, totalstr, total_percent, currspeedstr);
+
+ /*
+ * If we are reporting to a terminal, send a carriage return so that we
+ * stay on the same line. If not, send a newline.
+ */
+ if (isatty(fileno(stderr)))
+ fprintf(stderr, "\r");
+ else
+ fprintf(stderr, "\n");
+}
+
static bool
skipfile(const char *fn)
{
@@ -110,6 +187,7 @@ scan_file(const char *fn, BlockNumber segmentno)
continue;
csum = pg_checksum_page(buf.data, blockno + segmentno * RELSEG_SIZE);
+ current_size += r;
if (csum != header->pd_checksum)
{
if (ControlFile->data_checksum_version == PG_DATA_CHECKSUM_VERSION)
@@ -117,6 +195,9 @@ scan_file(const char *fn, BlockNumber segmentno)
progname, fn, blockno, csum, header->pd_checksum);
badblocks++;
}
+
+ if (show_progress)
+ report_progress(false);
}
if (verbose)
@@ -126,9 +207,10 @@ scan_file(const char *fn, BlockNumber segmentno)
close(f);
}
-static void
-scan_directory(const char *basedir, const char *subdir)
+static int64
+scan_directory(const char *basedir, const char *subdir, bool sizeonly)
{
+ int64 dirsize = 0;
char path[MAXPGPATH];
DIR *dir;
struct dirent *de;
@@ -191,16 +273,22 @@ scan_directory(const char *basedir, const char *subdir)
/* Relfilenode not to be included */
continue;
- scan_file(fn, segmentno);
+ dirsize += st.st_size;
+
+ if (!sizeonly)
+ {
+ scan_file(fn, segmentno);
+ }
}
#ifndef WIN32
else if (S_ISDIR(st.st_mode) || S_ISLNK(st.st_mode))
#else
else if (S_ISDIR(st.st_mode) || pgwin32_is_junction(fn))
#endif
- scan_directory(path, de->d_name);
+ dirsize += scan_directory(path, de->d_name, sizeonly);
}
closedir(dir);
+ return dirsize;
}
int
@@ -208,6 +296,7 @@ main(int argc, char *argv[])
{
static struct option long_options[] = {
{"pgdata", required_argument, NULL, 'D'},
+ {"progress", no_argument, NULL, 'P'},
{"verbose", no_argument, NULL, 'v'},
{NULL, 0, NULL, 0}
};
@@ -217,6 +306,8 @@ main(int argc, char *argv[])
int option_index;
bool crc_ok;
+ struct sigaction act; /* to turn progress status info on */
+
set_pglocale_pgservice(argv[0], PG_TEXTDOMAIN("pg_verify_checksums"));
progname = get_progname(argv[0]);
@@ -235,7 +326,7 @@ main(int argc, char *argv[])
}
}
- while ((c = getopt_long(argc, argv, "D:r:v", long_options, &option_index)) != -1)
+ while ((c = getopt_long(argc, argv, "D:r:vP", long_options, &option_index)) != -1)
{
switch (c)
{
@@ -253,6 +344,9 @@ main(int argc, char *argv[])
}
only_relfilenode = pstrdup(optarg);
break;
+ case 'P':
+ show_progress = true;
+ break;
default:
fprintf(stderr, _("Try \"%s --help\" for more information.\n"), progname);
exit(1);
@@ -306,10 +400,54 @@ main(int argc, char *argv[])
exit(1);
}
+
+ /*
+ * Assign SIGUSR1 signal handler to toggle progress status information
+ */
+ memset(&act, 0, sizeof(act));
+ act.sa_sigaction = &toggle_progress_report;
+ act.sa_flags = SA_SIGINFO;
+
+ /*
+ * Enable signal handler, but don't treat it as severe if we don't
+ * succeed here. Just give a message on STDERR.
+ */
+ if (sigaction(SIGUSR1, &act, NULL) < 0)
+ {
+ fprintf(stderr, _("%s: could not set signal handler to toggle progress\n"),
+ progname);
+ }
+
+ /*
+ * Iff progress status information is requested, we need to scan the
+ * directory tree(s) twice, once to get the idea how much data we need
+ * to scan and finally to do the real legwork.
+ */
+ total_size = scan_directory(DataDir, "global", true);
+ total_size += scan_directory(DataDir, "base", true);
+ total_size += scan_directory(DataDir, "pg_tblspc", true);
+
+ /*
+ * Remember start time. Required to calculate the current speed in
+ * report_progress()
+ */
+ scan_started = time(NULL);
+
/* Scan all files */
- scan_directory(DataDir, "global");
- scan_directory(DataDir, "base");
- scan_directory(DataDir, "pg_tblspc");
+ scan_directory(DataDir, "global", false);
+ scan_directory(DataDir, "base", false);
+ scan_directory(DataDir, "pg_tblspc", false);
+
+ /*
+ * Done. Move to next line in case progress information was shown.
+ * Otherwise we clutter the summary output.
+ */
+ if (show_progress)
+ {
+ report_progress(true);
+ if (isatty(fileno(stderr)))
+ fprintf(stderr, "\n");
+ }
printf(_("Checksum scan completed\n"));
printf(_("Data checksum version: %d\n"), ControlFile->data_checksum_version);
The time() granularity to the second makes the result awkward on small
tests:8/1554552 kB (0%, 8 kB/s)
1040856/1554552 kB (66%, 1040856 kB/s)
1554552/1554552 kB (100%, 1554552 kB/s)I'd suggest to reuse the "portability/instr_time.h" stuff which allows a
much better precision.I still think it would make sense to use that instead of low-precision
time().As the use-case of this is not for small tests,
Even for a long run, the induce error by the 1 second imprecision on both
points is significant at the beginning of the scan.
I don't think it is useful to make the code more complicated for this.
I do not think that it would be much more complicated to use the
portability macros to get a precise time.
The display is exactly the same as in pg_basebackup (except the
bandwith is shown as well), so right now I think it is more useful to be
consistent here.Hmmm... units are units, and the display is wrong. The fact that other
commands are wrong as well does not look like a good argument to persist in
an error.I've had a look around, and "kB" seems to be a common unit for 1024
bytes, e.g. in pg_test_fsync etc., unless I am mistaken?
I can only repeat my above comment: the fact that postgres is wrong
elsewhere is not a good reason to persist in reproducing an error.
See the mother of all knowledge https://en.wikipedia.org/wiki/Kilobyte
- SI (decimal, 1000): kB, MB, GB, ...
- IEC (binary 1024): KiB, MiB, GiB, ...
- JEDEC (binary 1024, used for memory): KB, MB, GB.
Summary:
- 1 kB = 1000 bytes
- 1 KB = 1 KiB = 1024 bytes
Decimal is used for storage (HDD, SSD), binary for memory. That is life,
and IMHO Postgres code is not the place to invent new units.
Moreover, I still think that the progress should use MB defined as 1000000
bytes for showing the progress.
I would be okay with a progress printing function shared between some
commands. It just needs some place. pg_rewind also has a --rewind option.I guess you mean pg_rewind also has a --progress option.
Indeed.
I do agree it makes sense to refactor that, but I don't think this
should be part of this patch.
That's a point. I'd suggest to put the new progress function directly in
the common part, and other patches could take advantage of it for other
commands when someone feels like it.
Other comments:
function toggle_progress_report has empty lines after { and before },
but this style is not used elsewhere, I think that they should be removed.
The report_progress API should be ready to be used by another client
application, which suggest that global variables should be avoided.
Maybe:
void report_progress(current, total, force)
and the scan start and last times could be a static variable inside the
function initialized on the first call, which would suggest to call the
function at the beginning of the scan, probably with current == 0.
Or maybe:
time_type report_progress(current, total, start, last, force)
Which would return the last time, and the caller has responsability for
initializing a start time.
--
Fabien.
On Sat, Sep 29, 2018 at 1:07 AM Michael Banck <michael.banck@credativ.de> wrote:
I've attached v4 of the patch.
Hi Michael,
Windows doesn't like sigaction:
https://ci.appveyor.com/project/postgresql-cfbot/postgresql/build/1.0.15189
I'm not sure if we classify this as a "frontend" program. Should it
be using pqsignal() from src/port/pqsignal.c? Or perhaps just
sigaction as you have it (pqsignal.c says that we require sigaction on
all Unices), but #ifndef WIN32 around that stuff, since SIGUSR1 is
never going to work anyway.
--
Thomas Munro
http://www.enterprisedb.com
On Wed, Oct 3, 2018 at 12:51 AM Thomas Munro <thomas.munro@enterprisedb.com> wrote:
On Sat, Sep 29, 2018 at 1:07 AM Michael Banck <michael.banck@credativ.de> wrote:
I've attached v4 of the patch.
Hi Michael,
Windows doesn't like sigaction:
https://ci.appveyor.com/project/postgresql-cfbot/postgresql/build/1.0.15189
I'm not sure if we classify this as a "frontend" program. Should it
be using pqsignal() from src/port/pqsignal.c? Or perhaps just
sigaction as you have it (pqsignal.c says that we require sigaction on
all Unices), but #ifndef WIN32 around that stuff, since SIGUSR1 is
never going to work anyway.
Unfortunately, patch also has some conflicts with the current master. I'll move
it to the next CF.
Hi,
On Wed, Oct 03, 2018 at 11:50:36AM +1300, Thomas Munro wrote:
On Sat, Sep 29, 2018 at 1:07 AM Michael Banck <michael.banck@credativ.de> wrote:
Windows doesn't like sigaction:https://ci.appveyor.com/project/postgresql-cfbot/postgresql/build/1.0.15189
Thanks for the report and sorry for taking so long to reply.
I'm not sure if we classify this as a "frontend" program. Should it
be using pqsignal() from src/port/pqsignal.c? Or perhaps just
sigaction as you have it (pqsignal.c says that we require sigaction on
all Unices), but #ifndef WIN32 around that stuff, since SIGUSR1 is
never going to work anyway.
I've used pqsignal now, and disabled that feature on Windows.
I've also updated one comment and added an additional regression test.
V5 attached.
Michael
--
Michael Banck
Projektleiter / Senior Berater
Tel.: +49 2166 9901-171
Fax: +49 2166 9901-100
Email: michael.banck@credativ.de
credativ GmbH, HRB M�nchengladbach 12080
USt-ID-Nummer: DE204566209
Trompeterallee 108, 41189 M�nchengladbach
Gesch�ftsf�hrung: Dr. Michael Meskes, J�rg Folz, Sascha Heuer
Unser Umgang mit personenbezogenen Daten unterliegt
folgenden Bestimmungen: https://www.credativ.de/datenschutz
Attachments:
pg_verify_checksums_progress_V5.patchtext/x-diff; charset=us-asciiDownload
diff --git a/doc/src/sgml/ref/pg_verify_checksums.sgml b/doc/src/sgml/ref/pg_verify_checksums.sgml
index 905b8f1222..b470c19c08 100644
--- a/doc/src/sgml/ref/pg_verify_checksums.sgml
+++ b/doc/src/sgml/ref/pg_verify_checksums.sgml
@@ -80,6 +80,19 @@ PostgreSQL documentation
</varlistentry>
<varlistentry>
+ <term><option>-P</option></term>
+ <term><option>--progress</option></term>
+ <listitem>
+ <para>
+ Enable progress reporting. Turning this on will deliver an approximate
+ progress report during the checksum verification. Sending the
+ <systemitem>SIGUSR1</systemitem> signal will toggle progress reporting
+ on or off during the verification run (not available on Windows).
+ </para>
+ </listitem>
+ </varlistentry>
+
+ <varlistentry>
<term><option>-V</option></term>
<term><option>--version</option></term>
<listitem>
diff --git a/src/bin/pg_verify_checksums/pg_verify_checksums.c b/src/bin/pg_verify_checksums/pg_verify_checksums.c
index 6444fc9ca4..b049ea17b3 100644
--- a/src/bin/pg_verify_checksums/pg_verify_checksums.c
+++ b/src/bin/pg_verify_checksums/pg_verify_checksums.c
@@ -10,6 +10,8 @@
#include "postgres_fe.h"
#include <dirent.h>
+#include <signal.h>
+#include <time.h>
#include <sys/stat.h>
#include <unistd.h>
@@ -30,9 +32,18 @@ static ControlFileData *ControlFile;
static char *only_relfilenode = NULL;
static bool verbose = false;
+static bool show_progress = false;
static const char *progname;
+/*
+ * Progress status information.
+ */
+int64 total_size = 0;
+int64 current_size = 0;
+pg_time_t last_progress_update;
+pg_time_t scan_started;
+
static void
usage(void)
{
@@ -43,6 +54,7 @@ usage(void)
printf(_(" [-D, --pgdata=]DATADIR data directory\n"));
printf(_(" -v, --verbose output verbose messages\n"));
printf(_(" -r RELFILENODE check only relation with specified relfilenode\n"));
+ printf(_(" -P, --progress show progress information\n"));
printf(_(" -V, --version output version information, then exit\n"));
printf(_(" -?, --help show this help, then exit\n"));
printf(_("\nIf no data directory (DATADIR) is specified, "
@@ -67,6 +79,69 @@ static const char *const skip[] = {
NULL,
};
+static void
+toggle_progress_report(int signum)
+{
+
+ /* we handle SIGUSR1 only, and toggle the value of show_progress */
+ if (signum == SIGUSR1)
+ show_progress = !show_progress;
+
+}
+
+/*
+ * Report current progress status. Parts borrowed from
+ * PostgreSQLs' src/bin/pg_basebackup.c
+ */
+static void
+report_progress(bool force)
+{
+ pg_time_t now = time(NULL);
+ int total_percent = 0;
+
+ char totalstr[32];
+ char currentstr[32];
+ char currspeedstr[32];
+
+ /* Make sure we report at most once a second */
+ if ((now == last_progress_update) && !force)
+ return;
+
+ /* Save current time */
+ last_progress_update = now;
+
+ /*
+ * Calculate current percent done, based on KiB...
+ */
+ total_percent = total_size ? (int64) ((current_size / 1024) * 100 / (total_size / 1024)) : 0;
+
+ /* Don't display larger than 100% */
+ if (total_percent > 100)
+ total_percent = 100;
+
+ /* The same for total size */
+ if (current_size > total_size)
+ total_size = current_size / 1024;
+
+ snprintf(totalstr, sizeof(totalstr), INT64_FORMAT,
+ total_size / 1024);
+ snprintf(currentstr, sizeof(currentstr), INT64_FORMAT,
+ current_size / 1024);
+ snprintf(currspeedstr, sizeof(currspeedstr), INT64_FORMAT,
+ (current_size / 1024) / (((time(NULL) - scan_started) == 0) ? 1 : (time(NULL) - scan_started)));
+ fprintf(stderr, "%s/%s kB (%d%%, %s kB/s)",
+ currentstr, totalstr, total_percent, currspeedstr);
+
+ /*
+ * If we are reporting to a terminal, send a carriage return so that we
+ * stay on the same line. If not, send a newline.
+ */
+ if (isatty(fileno(stderr)))
+ fprintf(stderr, "\r");
+ else
+ fprintf(stderr, "\n");
+}
+
static bool
skipfile(const char *fn)
{
@@ -117,6 +192,7 @@ scan_file(const char *fn, BlockNumber segmentno)
continue;
csum = pg_checksum_page(buf.data, blockno + segmentno * RELSEG_SIZE);
+ current_size += r;
if (csum != header->pd_checksum)
{
if (ControlFile->data_checksum_version == PG_DATA_CHECKSUM_VERSION)
@@ -124,6 +200,9 @@ scan_file(const char *fn, BlockNumber segmentno)
progname, fn, blockno, csum, header->pd_checksum);
badblocks++;
}
+
+ if (show_progress)
+ report_progress(false);
}
if (verbose)
@@ -133,9 +212,10 @@ scan_file(const char *fn, BlockNumber segmentno)
close(f);
}
-static void
-scan_directory(const char *basedir, const char *subdir)
+static int64
+scan_directory(const char *basedir, const char *subdir, bool sizeonly)
{
+ int64 dirsize = 0;
char path[MAXPGPATH];
DIR *dir;
struct dirent *de;
@@ -214,16 +294,22 @@ scan_directory(const char *basedir, const char *subdir)
/* Relfilenode not to be included */
continue;
- scan_file(fn, segmentno);
+ dirsize += st.st_size;
+
+ if (!sizeonly)
+ {
+ scan_file(fn, segmentno);
+ }
}
#ifndef WIN32
else if (S_ISDIR(st.st_mode) || S_ISLNK(st.st_mode))
#else
else if (S_ISDIR(st.st_mode) || pgwin32_is_junction(fn))
#endif
- scan_directory(path, de->d_name);
+ dirsize += scan_directory(path, de->d_name, sizeonly);
}
closedir(dir);
+ return dirsize;
}
int
@@ -231,6 +317,7 @@ main(int argc, char *argv[])
{
static struct option long_options[] = {
{"pgdata", required_argument, NULL, 'D'},
+ {"progress", no_argument, NULL, 'P'},
{"verbose", no_argument, NULL, 'v'},
{NULL, 0, NULL, 0}
};
@@ -258,7 +345,7 @@ main(int argc, char *argv[])
}
}
- while ((c = getopt_long(argc, argv, "D:r:v", long_options, &option_index)) != -1)
+ while ((c = getopt_long(argc, argv, "D:r:vP", long_options, &option_index)) != -1)
{
switch (c)
{
@@ -276,6 +363,9 @@ main(int argc, char *argv[])
}
only_relfilenode = pstrdup(optarg);
break;
+ case 'P':
+ show_progress = true;
+ break;
default:
fprintf(stderr, _("Try \"%s --help\" for more information.\n"), progname);
exit(1);
@@ -329,10 +419,44 @@ main(int argc, char *argv[])
exit(1);
}
+
+#ifndef WIN32
+ /*
+ * Assign SIGUSR1 signal handler to toggle progress status information
+ */
+ pqsignal(SIGUSR1, toggle_progress_report);
+#endif
+
+ /*
+ * As progress status information may be requested, we need to scan the
+ * directory tree(s) twice, once to get the idea how much data we need to
+ * scan and finally to do the real legwork.
+ */
+ total_size = scan_directory(DataDir, "global", true);
+ total_size += scan_directory(DataDir, "base", true);
+ total_size += scan_directory(DataDir, "pg_tblspc", true);
+
+ /*
+ * Remember start time. Required to calculate the current speed in
+ * report_progress()
+ */
+ scan_started = time(NULL);
+
/* Scan all files */
- scan_directory(DataDir, "global");
- scan_directory(DataDir, "base");
- scan_directory(DataDir, "pg_tblspc");
+ scan_directory(DataDir, "global", false);
+ scan_directory(DataDir, "base", false);
+ scan_directory(DataDir, "pg_tblspc", false);
+
+ /*
+ * Done. Move to next line in case progress information was shown.
+ * Otherwise we clutter the summary output.
+ */
+ if (show_progress)
+ {
+ report_progress(true);
+ if (isatty(fileno(stderr)))
+ fprintf(stderr, "\n");
+ }
printf(_("Checksum scan completed\n"));
printf(_("Data checksum version: %d\n"), ControlFile->data_checksum_version);
diff --git a/src/bin/pg_verify_checksums/t/002_actions.pl b/src/bin/pg_verify_checksums/t/002_actions.pl
index 5250b5a728..283e987fca 100644
--- a/src/bin/pg_verify_checksums/t/002_actions.pl
+++ b/src/bin/pg_verify_checksums/t/002_actions.pl
@@ -5,7 +5,7 @@ use strict;
use warnings;
use PostgresNode;
use TestLib;
-use Test::More tests => 45;
+use Test::More tests => 46;
# Utility routine to create and check a table with corrupted checksums
@@ -104,6 +104,10 @@ append_to_file "$pgdata/global/pgsql_tmp/1.1", "foo";
command_ok(['pg_verify_checksums', '-D', $pgdata],
"succeeds with offline cluster");
+# Checksums pass with progress-reporting on
+command_ok(['pg_verify_checksums', '-P', '-D', $pgdata],
+ "succeeds with progress reporting");
+
# Checks cannot happen with an online cluster
$node->start;
command_fails(['pg_verify_checksums', '-D', $pgdata],
Hallo Michael,
V5 attached.
Patch applies cleanly, compiles, global & local make check are ok.
Given that the specific output is not checked, I do not think that the -P
check deserves a test on its own, I think that the -P option could simply
be added to any of the existing tests.
I'm still unhappy that "kB" is used for 1024 bytes in the output, contrary
to all existing standards (1 kB = 1000 bytes -- SI, 1 KB = 1 KiB = 1024
bytes -- IEC & JEDEC). The fact that this is also used wrongly elsewhere
in pg is not relevant, these are bugs to be fixed, not to be replicated.
Given the speed of verifying checksums and its storage-oriented status, I
also still think that a (possibly fractional) MB (1,000,000 bytes), or
even GB, is the right unit to use for reporting this progress. On my
laptop (SSD), verifying runs at least at 1.26 GB/s (on one small test),
there is no point in displaying kilobytes progress.
I still think that using a more precise time than time(), eg with existing
macros from "instr_time.h", would not cost anything more and result in a
better precision output. It would also allow to remove the check used to
avoid a division-by-zero by switching to double.
If the check is performed while online (other patch in queue), then the
size may change thus it may not reach or go beyond 100%. No big deal.
I'd consider inverting the sizeonly boolean, so that true does the check
and false does only the size collection. It seems more logical to me if it
performs more with true than with false.
--
Fabien.
Given the speed of verifying checksums and its storage-oriented status, I
also still think that a (possibly fractional) MB (1,000,000 bytes), or even
GB, is the right unit to use for reporting this progress. On my laptop (SSD),
verifying runs at least at 1.26 GB/s (on one small test), there is no point
in displaying kilobytes progress.
Obviously the file is cached by the system at such speed, but still most
disks should provides dozens of MB per second of read bandwidth. If GB is
used, it should use fractional display (eg 1.25 GB) though.
--
Fabien.
Hi,
Am Dienstag, den 25.12.2018, 12:12 +0100 schrieb Fabien COELHO:
Given the speed of verifying checksums and its storage-oriented status, I
also still think that a (possibly fractional) MB (1,000,000 bytes), or even
GB, is the right unit to use for reporting this progress. On my laptop (SSD),
verifying runs at least at 1.26 GB/s (on one small test), there is no point
in displaying kilobytes progress.Obviously the file is cached by the system at such speed, but still most
disks should provides dozens of MB per second of read bandwidth. If GB is
used, it should use fractional display (eg 1.25 GB) though.
I think MB indeed makes more sense than kB, so I have changed that now
in V7, per attached.
Michael
--
Michael Banck
Projektleiter / Senior Berater
Tel.: +49 2166 9901-171
Fax: +49 2166 9901-100
Email: michael.banck@credativ.de
credativ GmbH, HRB Mönchengladbach 12080
USt-ID-Nummer: DE204566209
Trompeterallee 108, 41189 Mönchengladbach
Geschäftsführung: Dr. Michael Meskes, Jörg Folz, Sascha Heuer
Unser Umgang mit personenbezogenen Daten unterliegt
folgenden Bestimmungen: https://www.credativ.de/datenschutz
Attachments:
pg_verify_checksums_progress_V6.patchtext/x-patch; charset=UTF-8; name=pg_verify_checksums_progress_V6.patchDownload
diff --git a/doc/src/sgml/ref/pg_verify_checksums.sgml b/doc/src/sgml/ref/pg_verify_checksums.sgml
index 905b8f1222..b470c19c08 100644
--- a/doc/src/sgml/ref/pg_verify_checksums.sgml
+++ b/doc/src/sgml/ref/pg_verify_checksums.sgml
@@ -80,6 +80,19 @@ PostgreSQL documentation
</varlistentry>
<varlistentry>
+ <term><option>-P</option></term>
+ <term><option>--progress</option></term>
+ <listitem>
+ <para>
+ Enable progress reporting. Turning this on will deliver an approximate
+ progress report during the checksum verification. Sending the
+ <systemitem>SIGUSR1</systemitem> signal will toggle progress reporting
+ on or off during the verification run (not available on Windows).
+ </para>
+ </listitem>
+ </varlistentry>
+
+ <varlistentry>
<term><option>-V</option></term>
<term><option>--version</option></term>
<listitem>
diff --git a/src/bin/pg_verify_checksums/pg_verify_checksums.c b/src/bin/pg_verify_checksums/pg_verify_checksums.c
index 6444fc9ca4..4cecb64727 100644
--- a/src/bin/pg_verify_checksums/pg_verify_checksums.c
+++ b/src/bin/pg_verify_checksums/pg_verify_checksums.c
@@ -10,6 +10,8 @@
#include "postgres_fe.h"
#include <dirent.h>
+#include <signal.h>
+#include <time.h>
#include <sys/stat.h>
#include <unistd.h>
@@ -30,9 +32,18 @@ static ControlFileData *ControlFile;
static char *only_relfilenode = NULL;
static bool verbose = false;
+static bool show_progress = false;
static const char *progname;
+/*
+ * Progress status information.
+ */
+int64 total_size = 0;
+int64 current_size = 0;
+pg_time_t last_progress_update;
+pg_time_t scan_started;
+
static void
usage(void)
{
@@ -43,6 +54,7 @@ usage(void)
printf(_(" [-D, --pgdata=]DATADIR data directory\n"));
printf(_(" -v, --verbose output verbose messages\n"));
printf(_(" -r RELFILENODE check only relation with specified relfilenode\n"));
+ printf(_(" -P, --progress show progress information\n"));
printf(_(" -V, --version output version information, then exit\n"));
printf(_(" -?, --help show this help, then exit\n"));
printf(_("\nIf no data directory (DATADIR) is specified, "
@@ -67,6 +79,69 @@ static const char *const skip[] = {
NULL,
};
+static void
+toggle_progress_report(int signum)
+{
+
+ /* we handle SIGUSR1 only, and toggle the value of show_progress */
+ if (signum == SIGUSR1)
+ show_progress = !show_progress;
+
+}
+
+/*
+ * Report current progress status. Parts borrowed from
+ * PostgreSQLs' src/bin/pg_basebackup.c
+ */
+static void
+report_progress(bool force)
+{
+ pg_time_t now = time(NULL);
+ int total_percent = 0;
+
+ char totalstr[32];
+ char currentstr[32];
+ char currspeedstr[32];
+
+ /* Make sure we report at most once a second */
+ if ((now == last_progress_update) && !force)
+ return;
+
+ /* Save current time */
+ last_progress_update = now;
+
+ /*
+ * Calculate current percent done, based on MB...
+ */
+ total_percent = total_size ? (int64) ((current_size / 1048576) * 100 / (total_size / 1048576)) : 0;
+
+ /* Don't display larger than 100% */
+ if (total_percent > 100)
+ total_percent = 100;
+
+ /* The same for total size */
+ if (current_size > total_size)
+ total_size = current_size / 1048576;
+
+ snprintf(totalstr, sizeof(totalstr), INT64_FORMAT,
+ total_size / 1048576);
+ snprintf(currentstr, sizeof(currentstr), INT64_FORMAT,
+ current_size / 1048576);
+ snprintf(currspeedstr, sizeof(currspeedstr), INT64_FORMAT,
+ (current_size / 1048576) / (((time(NULL) - scan_started) == 0) ? 1 : (time(NULL) - scan_started)));
+ fprintf(stderr, "%s/%s MB (%d%%, %s MB/s)",
+ currentstr, totalstr, total_percent, currspeedstr);
+
+ /*
+ * If we are reporting to a terminal, send a carriage return so that we
+ * stay on the same line. If not, send a newline.
+ */
+ if (isatty(fileno(stderr)))
+ fprintf(stderr, "\r");
+ else
+ fprintf(stderr, "\n");
+}
+
static bool
skipfile(const char *fn)
{
@@ -117,6 +192,7 @@ scan_file(const char *fn, BlockNumber segmentno)
continue;
csum = pg_checksum_page(buf.data, blockno + segmentno * RELSEG_SIZE);
+ current_size += r;
if (csum != header->pd_checksum)
{
if (ControlFile->data_checksum_version == PG_DATA_CHECKSUM_VERSION)
@@ -124,6 +200,9 @@ scan_file(const char *fn, BlockNumber segmentno)
progname, fn, blockno, csum, header->pd_checksum);
badblocks++;
}
+
+ if (show_progress)
+ report_progress(false);
}
if (verbose)
@@ -133,9 +212,10 @@ scan_file(const char *fn, BlockNumber segmentno)
close(f);
}
-static void
-scan_directory(const char *basedir, const char *subdir)
+static int64
+scan_directory(const char *basedir, const char *subdir, bool sizeonly)
{
+ int64 dirsize = 0;
char path[MAXPGPATH];
DIR *dir;
struct dirent *de;
@@ -167,7 +247,7 @@ scan_directory(const char *basedir, const char *subdir)
if (strncmp(de->d_name,
PG_TEMP_FILES_DIR,
strlen(PG_TEMP_FILES_DIR)) == 0)
- return;
+ continue;
snprintf(fn, sizeof(fn), "%s/%s", path, de->d_name);
if (lstat(fn, &st) < 0)
@@ -214,16 +294,22 @@ scan_directory(const char *basedir, const char *subdir)
/* Relfilenode not to be included */
continue;
- scan_file(fn, segmentno);
+ dirsize += st.st_size;
+
+ if (!sizeonly)
+ {
+ scan_file(fn, segmentno);
+ }
}
#ifndef WIN32
else if (S_ISDIR(st.st_mode) || S_ISLNK(st.st_mode))
#else
else if (S_ISDIR(st.st_mode) || pgwin32_is_junction(fn))
#endif
- scan_directory(path, de->d_name);
+ dirsize += scan_directory(path, de->d_name, sizeonly);
}
closedir(dir);
+ return dirsize;
}
int
@@ -231,6 +317,7 @@ main(int argc, char *argv[])
{
static struct option long_options[] = {
{"pgdata", required_argument, NULL, 'D'},
+ {"progress", no_argument, NULL, 'P'},
{"verbose", no_argument, NULL, 'v'},
{NULL, 0, NULL, 0}
};
@@ -258,7 +345,7 @@ main(int argc, char *argv[])
}
}
- while ((c = getopt_long(argc, argv, "D:r:v", long_options, &option_index)) != -1)
+ while ((c = getopt_long(argc, argv, "D:r:vP", long_options, &option_index)) != -1)
{
switch (c)
{
@@ -276,6 +363,9 @@ main(int argc, char *argv[])
}
only_relfilenode = pstrdup(optarg);
break;
+ case 'P':
+ show_progress = true;
+ break;
default:
fprintf(stderr, _("Try \"%s --help\" for more information.\n"), progname);
exit(1);
@@ -329,10 +419,44 @@ main(int argc, char *argv[])
exit(1);
}
+
+#ifndef WIN32
+ /*
+ * Assign SIGUSR1 signal handler to toggle progress status information
+ */
+ pqsignal(SIGUSR1, toggle_progress_report);
+#endif
+
+ /*
+ * As progress status information may be requested, we need to scan the
+ * directory tree(s) twice, once to get the idea how much data we need to
+ * scan and finally to do the real legwork.
+ */
+ total_size = scan_directory(DataDir, "global", true);
+ total_size += scan_directory(DataDir, "base", true);
+ total_size += scan_directory(DataDir, "pg_tblspc", true);
+
+ /*
+ * Remember start time. Required to calculate the current speed in
+ * report_progress()
+ */
+ scan_started = time(NULL);
+
/* Scan all files */
- scan_directory(DataDir, "global");
- scan_directory(DataDir, "base");
- scan_directory(DataDir, "pg_tblspc");
+ scan_directory(DataDir, "global", false);
+ scan_directory(DataDir, "base", false);
+ scan_directory(DataDir, "pg_tblspc", false);
+
+ /*
+ * Done. Move to next line in case progress information was shown.
+ * Otherwise we clutter the summary output.
+ */
+ if (show_progress)
+ {
+ report_progress(true);
+ if (isatty(fileno(stderr)))
+ fprintf(stderr, "\n");
+ }
printf(_("Checksum scan completed\n"));
printf(_("Data checksum version: %d\n"), ControlFile->data_checksum_version);
diff --git a/src/bin/pg_verify_checksums/t/002_actions.pl b/src/bin/pg_verify_checksums/t/002_actions.pl
index 5250b5a728..283e987fca 100644
--- a/src/bin/pg_verify_checksums/t/002_actions.pl
+++ b/src/bin/pg_verify_checksums/t/002_actions.pl
@@ -5,7 +5,7 @@ use strict;
use warnings;
use PostgresNode;
use TestLib;
-use Test::More tests => 45;
+use Test::More tests => 46;
# Utility routine to create and check a table with corrupted checksums
@@ -104,6 +104,10 @@ append_to_file "$pgdata/global/pgsql_tmp/1.1", "foo";
command_ok(['pg_verify_checksums', '-D', $pgdata],
"succeeds with offline cluster");
+# Checksums pass with progress-reporting on
+command_ok(['pg_verify_checksums', '-P', '-D', $pgdata],
+ "succeeds with progress reporting");
+
# Checks cannot happen with an online cluster
$node->start;
command_fails(['pg_verify_checksums', '-D', $pgdata],
I think MB indeed makes more sense than kB, so I have changed that now
in V7, per attached.
You use 1024ᅵ bytes. What about 1000000 bytes per MB, as the unit is about
stored files?
Also, you did not answer to my other points:
- use "instr_time.h" for better precision
- invert sizeonly
- reuse a test
--
Fabien.
On Tue, Dec 25, 2018 at 07:05:30PM +0100, Fabien COELHO wrote:
You use 1024² bytes. What about 1000000 bytes per MB, as the unit is about
stored files?Also, you did not answer to my other points:
- use "instr_time.h" for better precision
- invert sizeonly
- reuse a test
It seems to me that the SIGUSR1 business is not necessary for the
basic layer of this feature, so I would rather rip that off now. If
necessary we could always discuss that later on. My take about the
option is that --progress should not be the default, but that reports
should only be provided if the caller wants them.
--quiet may have some value by itself, but that seems like a separate
discussion to me.
+ /*
+ * If we are reporting to a terminal, send a carriage return so that we
+ * stay on the same line. If not, send a newline.
+ */
+ if (isatty(fileno(stderr)))
+ fprintf(stderr, "\r");
+ else
+ fprintf(stderr, "\n");
This bit is not really elegant, why not just '\r'?
+ /* The same for total size */
+ if (current_size > total_size)
+ total_size = current_size / 1048576;
Let's use that in a variable and not hardcode the number.
What's introduced here is very similar to what progress_report() does
in pg_rewind/logging.c. The report depends on the context so this
cannot be a common routine logic but perhaps we could keep a
consistent output for both tools?
--
Michael
On 2018-Dec-26, Michael Paquier wrote:
+ /* + * If we are reporting to a terminal, send a carriage return so that we + * stay on the same line. If not, send a newline. + */ + if (isatty(fileno(stderr))) + fprintf(stderr, "\r"); + else + fprintf(stderr, "\n"); This bit is not really elegant, why not just '\r'?
Umm, this is established coding pattern in pg_basebackup.c.
Stylistically I'd change all those cases to "fprintf(stderr,
isatty(fileno(stderr)) ? "\r" : "\n")" but leave the string alone, since
AFAIR it took some time to figure out what to do. (I'd also make the
comment one line instead of four, say "Stay on the same line if
reporting to a terminal". That makes the whole stanza two lines rather
than eight, which is the appropriate amount of space for it).
--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Tue, Dec 25, 2018 at 11:45:09PM -0300, Alvaro Herrera wrote:
Umm, this is established coding pattern in pg_basebackup.c.
Stylistically I'd change all those cases to "fprintf(stderr,
isatty(fileno(stderr)) ? "\r" : "\n")" but leave the string alone, since
AFAIR it took some time to figure out what to do. (I'd also make the
comment one line instead of four, say "Stay on the same line if
reporting to a terminal". That makes the whole stanza two lines rather
than eight, which is the appropriate amount of space for it).
There has been no input from the author for a couple of weeks now, so
I have marked the patch as returned with feedback.
--
Michael
Hi,
Am Mittwoch, den 26.12.2018, 11:14 +0900 schrieb Michael Paquier:
On Tue, Dec 25, 2018 at 07:05:30PM +0100, Fabien COELHO wrote:
You use 1024² bytes. What about 1000000 bytes per MB, as the
unit is about stored files?
I haven't changed that as Ihave not been pointed to a clear project
guideline that 1 MB should be 1000000 and not 1024x1024.
Also, you did not answer to my other points:
- use "instr_time.h" for better precision
Both pg_rewind and pg_basebackup are using time() as well, so I think
there's precedence for this.
- invert sizeonly
The current way is consistent with src/backend/replication/basebackup.c
so I'd prefer to keep it that way.
- reuse a test
I've removed the test.
It seems to me that the SIGUSR1 business is not necessary for the
basic layer of this feature, so I would rather rip that off now.
But is it gaining all so much if it is ripped out? The patch will be a
dozen lines shorter, but those are all isolated.
And contrary to pg_basebackup, pg_verify_checksums might be run in the
foreground much more often - right now it has to be done while the
cluster is offline, so if you have large instance and you forgot to pass
-P then you're glad you can signal it so you roughly know when you can
expect to startup the instance again.
If necessary we could always discuss that later on.
If you insist we can rip it out, of course.
My take about the option is that --progress should not be the default,
but that reports should only be provided if the caller wants them.--quiet may have some value by itself, but that seems like a separate
discussion to me.
Right now --progress isn't default so I think that's how you like it?
+ /* + * If we are reporting to a terminal, send a carriage return so that we + * stay on the same line. If not, send a newline. + */ + if (isatty(fileno(stderr))) + fprintf(stderr, "\r"); + else + fprintf(stderr, "\n"); This bit is not really elegant, why not just '\r'?
I've changed it as Alvaro suggested.
+ /* The same for total size */ + if (current_size > total_size) + total_size = current_size / 1048576; Let's use that in a variable and not hardcode the number.
Done so, I called it MEGABYTES.
What's introduced here is very similar to what progress_report() does
in pg_rewind/logging.c. The report depends on the context so this
cannot be a common routine logic but perhaps we could keep a
consistent output for both tools?
Well, pg_rewind is also using kB, so should I switch it back to that?
It looks like the progress reporting is otherwise quite similar, so what
exactly did you have in mind?
New patch attached.
Michael
--
Michael Banck
Projektleiter / Senior Berater
Tel.: +49 2166 9901-171
Fax: +49 2166 9901-100
Email: michael.banck@credativ.de
credativ GmbH, HRB Mönchengladbach 12080
USt-ID-Nummer: DE204566209
Trompeterallee 108, 41189 Mönchengladbach
Geschäftsführung: Dr. Michael Meskes, Jörg Folz, Sascha Heuer
Unser Umgang mit personenbezogenen Daten unterliegt
folgenden Bestimmungen: https://www.credativ.de/datenschutz
Attachments:
pg_verify_checksums_progress_V7.patchtext/x-patch; charset=UTF-8; name=pg_verify_checksums_progress_V7.patchDownload
diff --git a/doc/src/sgml/ref/pg_verify_checksums.sgml b/doc/src/sgml/ref/pg_verify_checksums.sgml
index 905b8f1222..b470c19c08 100644
--- a/doc/src/sgml/ref/pg_verify_checksums.sgml
+++ b/doc/src/sgml/ref/pg_verify_checksums.sgml
@@ -80,6 +80,19 @@ PostgreSQL documentation
</varlistentry>
<varlistentry>
+ <term><option>-P</option></term>
+ <term><option>--progress</option></term>
+ <listitem>
+ <para>
+ Enable progress reporting. Turning this on will deliver an approximate
+ progress report during the checksum verification. Sending the
+ <systemitem>SIGUSR1</systemitem> signal will toggle progress reporting
+ on or off during the verification run (not available on Windows).
+ </para>
+ </listitem>
+ </varlistentry>
+
+ <varlistentry>
<term><option>-V</option></term>
<term><option>--version</option></term>
<listitem>
diff --git a/src/bin/pg_verify_checksums/pg_verify_checksums.c b/src/bin/pg_verify_checksums/pg_verify_checksums.c
index 511262ab5f..9705f7dd57 100644
--- a/src/bin/pg_verify_checksums/pg_verify_checksums.c
+++ b/src/bin/pg_verify_checksums/pg_verify_checksums.c
@@ -10,6 +10,8 @@
#include "postgres_fe.h"
#include <dirent.h>
+#include <signal.h>
+#include <time.h>
#include <sys/stat.h>
#include <unistd.h>
@@ -30,9 +32,18 @@ static ControlFileData *ControlFile;
static char *only_relfilenode = NULL;
static bool verbose = false;
+static bool show_progress = false;
static const char *progname;
+/*
+ * Progress status information.
+ */
+int64 total_size = 0;
+int64 current_size = 0;
+pg_time_t last_progress_update;
+pg_time_t scan_started;
+
static void
usage(void)
{
@@ -43,6 +54,7 @@ usage(void)
printf(_(" [-D, --pgdata=]DATADIR data directory\n"));
printf(_(" -v, --verbose output verbose messages\n"));
printf(_(" -r RELFILENODE check only relation with specified relfilenode\n"));
+ printf(_(" -P, --progress show progress information\n"));
printf(_(" -V, --version output version information, then exit\n"));
printf(_(" -?, --help show this help, then exit\n"));
printf(_("\nIf no data directory (DATADIR) is specified, "
@@ -67,6 +79,65 @@ static const char *const skip[] = {
NULL,
};
+static void
+toggle_progress_report(int signum)
+{
+
+ /* we handle SIGUSR1 only, and toggle the value of show_progress */
+ if (signum == SIGUSR1)
+ show_progress = !show_progress;
+
+}
+
+/*
+ * Report current progress status. Parts borrowed from
+ * PostgreSQLs' src/bin/pg_basebackup.c
+ */
+static void
+report_progress(bool force)
+{
+ pg_time_t now = time(NULL);
+ int total_percent = 0;
+
+ char totalstr[32];
+ char currentstr[32];
+ char currspeedstr[32];
+
+ /* Make sure we report at most once a second */
+ if ((now == last_progress_update) && !force)
+ return;
+
+ /* Save current time */
+ last_progress_update = now;
+
+#define MEGABYTES 1048576
+
+ /*
+ * Calculate current percent done, based on MB...
+ */
+ total_percent = total_size ? (int64) ((current_size / MEGABYTES) * 100 / (total_size / MEGABYTES)) : 0;
+
+ /* Don't display larger than 100% */
+ if (total_percent > 100)
+ total_percent = 100;
+
+ /* The same for total size */
+ if (current_size > total_size)
+ total_size = current_size / MEGABYTES;
+
+ snprintf(totalstr, sizeof(totalstr), INT64_FORMAT,
+ total_size / MEGABYTES);
+ snprintf(currentstr, sizeof(currentstr), INT64_FORMAT,
+ current_size / MEGABYTES);
+ snprintf(currspeedstr, sizeof(currspeedstr), INT64_FORMAT,
+ (current_size / MEGABYTES) / (((time(NULL) - scan_started) == 0) ? 1 : (time(NULL) - scan_started)));
+ fprintf(stderr, "%s/%s MB (%d%%, %s MB/s)",
+ currentstr, totalstr, total_percent, currspeedstr);
+
+ /* Stay on the same line if reporting to a terminal */
+ fprintf(stderr, isatty(fileno(stderr)) ? "\r" : "\n");
+}
+
static bool
skipfile(const char *fn)
{
@@ -117,6 +188,7 @@ scan_file(const char *fn, BlockNumber segmentno)
continue;
csum = pg_checksum_page(buf.data, blockno + segmentno * RELSEG_SIZE);
+ current_size += r;
if (csum != header->pd_checksum)
{
if (ControlFile->data_checksum_version == PG_DATA_CHECKSUM_VERSION)
@@ -124,6 +196,9 @@ scan_file(const char *fn, BlockNumber segmentno)
progname, fn, blockno, csum, header->pd_checksum);
badblocks++;
}
+
+ if (show_progress)
+ report_progress(false);
}
if (verbose)
@@ -133,9 +208,10 @@ scan_file(const char *fn, BlockNumber segmentno)
close(f);
}
-static void
-scan_directory(const char *basedir, const char *subdir)
+static int64
+scan_directory(const char *basedir, const char *subdir, bool sizeonly)
{
+ int64 dirsize = 0;
char path[MAXPGPATH];
DIR *dir;
struct dirent *de;
@@ -167,7 +243,7 @@ scan_directory(const char *basedir, const char *subdir)
if (strncmp(de->d_name,
PG_TEMP_FILES_DIR,
strlen(PG_TEMP_FILES_DIR)) == 0)
- return;
+ continue;
snprintf(fn, sizeof(fn), "%s/%s", path, de->d_name);
if (lstat(fn, &st) < 0)
@@ -214,16 +290,22 @@ scan_directory(const char *basedir, const char *subdir)
/* Relfilenode not to be included */
continue;
- scan_file(fn, segmentno);
+ dirsize += st.st_size;
+
+ if (!sizeonly)
+ {
+ scan_file(fn, segmentno);
+ }
}
#ifndef WIN32
else if (S_ISDIR(st.st_mode) || S_ISLNK(st.st_mode))
#else
else if (S_ISDIR(st.st_mode) || pgwin32_is_junction(fn))
#endif
- scan_directory(path, de->d_name);
+ dirsize += scan_directory(path, de->d_name, sizeonly);
}
closedir(dir);
+ return dirsize;
}
int
@@ -231,6 +313,7 @@ main(int argc, char *argv[])
{
static struct option long_options[] = {
{"pgdata", required_argument, NULL, 'D'},
+ {"progress", no_argument, NULL, 'P'},
{"verbose", no_argument, NULL, 'v'},
{NULL, 0, NULL, 0}
};
@@ -258,7 +341,7 @@ main(int argc, char *argv[])
}
}
- while ((c = getopt_long(argc, argv, "D:r:v", long_options, &option_index)) != -1)
+ while ((c = getopt_long(argc, argv, "D:r:vP", long_options, &option_index)) != -1)
{
switch (c)
{
@@ -276,6 +359,9 @@ main(int argc, char *argv[])
}
only_relfilenode = pstrdup(optarg);
break;
+ case 'P':
+ show_progress = true;
+ break;
default:
fprintf(stderr, _("Try \"%s --help\" for more information.\n"), progname);
exit(1);
@@ -329,10 +415,44 @@ main(int argc, char *argv[])
exit(1);
}
+
+#ifndef WIN32
+ /*
+ * Assign SIGUSR1 signal handler to toggle progress status information
+ */
+ pqsignal(SIGUSR1, toggle_progress_report);
+#endif
+
+ /*
+ * As progress status information may be requested, we need to scan the
+ * directory tree(s) twice, once to get the idea how much data we need to
+ * scan and finally to do the real legwork.
+ */
+ total_size = scan_directory(DataDir, "global", true);
+ total_size += scan_directory(DataDir, "base", true);
+ total_size += scan_directory(DataDir, "pg_tblspc", true);
+
+ /*
+ * Remember start time. Required to calculate the current speed in
+ * report_progress()
+ */
+ scan_started = time(NULL);
+
/* Scan all files */
- scan_directory(DataDir, "global");
- scan_directory(DataDir, "base");
- scan_directory(DataDir, "pg_tblspc");
+ scan_directory(DataDir, "global", false);
+ scan_directory(DataDir, "base", false);
+ scan_directory(DataDir, "pg_tblspc", false);
+
+ /*
+ * Done. Move to next line in case progress information was shown.
+ * Otherwise we clutter the summary output.
+ */
+ if (show_progress)
+ {
+ report_progress(true);
+ if (isatty(fileno(stderr)))
+ fprintf(stderr, "\n");
+ }
printf(_("Checksum scan completed\n"));
printf(_("Data checksum version: %d\n"), ControlFile->data_checksum_version);
On Sun, Feb 17, 2019 at 09:00:29PM +0100, Michael Banck wrote:
Well, pg_rewind is also using kB, so should I switch it back to that?
I am not sure which one is better, sorry :)
There is an argument for switching pg_rewind to use MB as well. I
don't expect pg_rewind to transfer gigs of data, but it could make the
maths harder for a newcomer if it transfers a lot of data.
It looks like the progress reporting is otherwise quite similar, so what
exactly did you have in mind?
I was thinking about a routine in src/common/ or such at this time.
The tools have separate goals though, so it looks sensible as well to
keep two routines. I am fine to let that up to you.
--
Michael
Hi,
Am Montag, den 18.02.2019, 12:24 +0900 schrieb Michael Paquier:
On Sun, Feb 17, 2019 at 09:00:29PM +0100, Michael Banck wrote:
Well, pg_rewind is also using kB, so should I switch it back to that?
I am not sure which one is better, sorry :)
There is an argument for switching pg_rewind to use MB as well. I
don't expect pg_rewind to transfer gigs of data, but it could make the
maths harder for a newcomer if it transfers a lot of data.
Right, I think it makes more sense for pg_basebackup and
pg_verify_checksums as they go over the whole cluster than for
pg_rewind, but being consistent is also worth something.
It looks like the progress reporting is otherwise quite similar, so what
exactly did you have in mind?I was thinking about a routine in src/common/ or such at this time.
The tools have separate goals though, so it looks sensible as well to
keep two routines. I am fine to let that up to you.
I'll leave it as-is for now, but will see whether this can be improved
in a follow-up patch.
Michael
--
Michael Banck
Projektleiter / Senior Berater
Tel.: +49 2166 9901-171
Fax: +49 2166 9901-100
Email: michael.banck@credativ.de
credativ GmbH, HRB Mönchengladbach 12080
USt-ID-Nummer: DE204566209
Trompeterallee 108, 41189 Mönchengladbach
Geschäftsführung: Dr. Michael Meskes, Jörg Folz, Sascha Heuer
Unser Umgang mit personenbezogenen Daten unterliegt
folgenden Bestimmungen: https://www.credativ.de/datenschutz
On 2019-Feb-17, Michael Banck wrote:
+ /* + * As progress status information may be requested, we need to scan the + * directory tree(s) twice, once to get the idea how much data we need to + * scan and finally to do the real legwork. + */ + total_size = scan_directory(DataDir, "global", true); + total_size += scan_directory(DataDir, "base", true); + total_size += scan_directory(DataDir, "pg_tblspc", true);
Surely we know at that point whether this first scan is needed, and we
can skip it if not?
--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Hi,
Am Montag, den 18.02.2019, 11:18 -0300 schrieb Alvaro Herrera:
On 2019-Feb-17, Michael Banck wrote:
+ /* + * As progress status information may be requested, we need to scan the + * directory tree(s) twice, once to get the idea how much data we need to + * scan and finally to do the real legwork. + */ + total_size = scan_directory(DataDir, "global", true); + total_size += scan_directory(DataDir, "base", true); + total_size += scan_directory(DataDir, "pg_tblspc", true);Surely we know at that point whether this first scan is needed, and we
can skip it if not?
Yeah - new patch attached.
Michael
--
Michael Banck
Projektleiter / Senior Berater
Tel.: +49 2166 9901-171
Fax: +49 2166 9901-100
Email: michael.banck@credativ.de
credativ GmbH, HRB Mönchengladbach 12080
USt-ID-Nummer: DE204566209
Trompeterallee 108, 41189 Mönchengladbach
Geschäftsführung: Dr. Michael Meskes, Jörg Folz, Sascha Heuer
Unser Umgang mit personenbezogenen Daten unterliegt
folgenden Bestimmungen: https://www.credativ.de/datenschutz
Attachments:
pg_verify_checksums_progress_V8.patchtext/x-patch; charset=UTF-8; name=pg_verify_checksums_progress_V8.patchDownload
diff --git a/doc/src/sgml/ref/pg_verify_checksums.sgml b/doc/src/sgml/ref/pg_verify_checksums.sgml
index 905b8f1222..b470c19c08 100644
--- a/doc/src/sgml/ref/pg_verify_checksums.sgml
+++ b/doc/src/sgml/ref/pg_verify_checksums.sgml
@@ -80,6 +80,19 @@ PostgreSQL documentation
</varlistentry>
<varlistentry>
+ <term><option>-P</option></term>
+ <term><option>--progress</option></term>
+ <listitem>
+ <para>
+ Enable progress reporting. Turning this on will deliver an approximate
+ progress report during the checksum verification. Sending the
+ <systemitem>SIGUSR1</systemitem> signal will toggle progress reporting
+ on or off during the verification run (not available on Windows).
+ </para>
+ </listitem>
+ </varlistentry>
+
+ <varlistentry>
<term><option>-V</option></term>
<term><option>--version</option></term>
<listitem>
diff --git a/src/bin/pg_verify_checksums/pg_verify_checksums.c b/src/bin/pg_verify_checksums/pg_verify_checksums.c
index 511262ab5f..06e233b501 100644
--- a/src/bin/pg_verify_checksums/pg_verify_checksums.c
+++ b/src/bin/pg_verify_checksums/pg_verify_checksums.c
@@ -10,6 +10,8 @@
#include "postgres_fe.h"
#include <dirent.h>
+#include <signal.h>
+#include <time.h>
#include <sys/stat.h>
#include <unistd.h>
@@ -30,9 +32,18 @@ static ControlFileData *ControlFile;
static char *only_relfilenode = NULL;
static bool verbose = false;
+static bool show_progress = false;
static const char *progname;
+/*
+ * Progress status information.
+ */
+int64 total_size = 0;
+int64 current_size = 0;
+pg_time_t last_progress_update;
+pg_time_t scan_started;
+
static void
usage(void)
{
@@ -43,6 +54,7 @@ usage(void)
printf(_(" [-D, --pgdata=]DATADIR data directory\n"));
printf(_(" -v, --verbose output verbose messages\n"));
printf(_(" -r RELFILENODE check only relation with specified relfilenode\n"));
+ printf(_(" -P, --progress show progress information\n"));
printf(_(" -V, --version output version information, then exit\n"));
printf(_(" -?, --help show this help, then exit\n"));
printf(_("\nIf no data directory (DATADIR) is specified, "
@@ -67,6 +79,65 @@ static const char *const skip[] = {
NULL,
};
+static void
+toggle_progress_report(int signum)
+{
+
+ /* we handle SIGUSR1 only, and toggle the value of show_progress */
+ if (signum == SIGUSR1)
+ show_progress = !show_progress;
+
+}
+
+/*
+ * Report current progress status. Parts borrowed from
+ * PostgreSQLs' src/bin/pg_basebackup.c
+ */
+static void
+report_progress(bool force)
+{
+ pg_time_t now = time(NULL);
+ int total_percent = 0;
+
+ char totalstr[32];
+ char currentstr[32];
+ char currspeedstr[32];
+
+ /* Make sure we report at most once a second */
+ if ((now == last_progress_update) && !force)
+ return;
+
+ /* Save current time */
+ last_progress_update = now;
+
+#define MEGABYTES 1048576
+
+ /*
+ * Calculate current percent done, based on MB...
+ */
+ total_percent = total_size ? (int64) ((current_size / MEGABYTES) * 100 / (total_size / MEGABYTES)) : 0;
+
+ /* Don't display larger than 100% */
+ if (total_percent > 100)
+ total_percent = 100;
+
+ /* The same for total size */
+ if (current_size > total_size)
+ total_size = current_size / MEGABYTES;
+
+ snprintf(totalstr, sizeof(totalstr), INT64_FORMAT,
+ total_size / MEGABYTES);
+ snprintf(currentstr, sizeof(currentstr), INT64_FORMAT,
+ current_size / MEGABYTES);
+ snprintf(currspeedstr, sizeof(currspeedstr), INT64_FORMAT,
+ (current_size / MEGABYTES) / (((time(NULL) - scan_started) == 0) ? 1 : (time(NULL) - scan_started)));
+ fprintf(stderr, "%s/%s MB (%d%%, %s MB/s)",
+ currentstr, totalstr, total_percent, currspeedstr);
+
+ /* Stay on the same line if reporting to a terminal */
+ fprintf(stderr, isatty(fileno(stderr)) ? "\r" : "\n");
+}
+
static bool
skipfile(const char *fn)
{
@@ -117,6 +188,7 @@ scan_file(const char *fn, BlockNumber segmentno)
continue;
csum = pg_checksum_page(buf.data, blockno + segmentno * RELSEG_SIZE);
+ current_size += r;
if (csum != header->pd_checksum)
{
if (ControlFile->data_checksum_version == PG_DATA_CHECKSUM_VERSION)
@@ -124,6 +196,9 @@ scan_file(const char *fn, BlockNumber segmentno)
progname, fn, blockno, csum, header->pd_checksum);
badblocks++;
}
+
+ if (show_progress)
+ report_progress(false);
}
if (verbose)
@@ -133,9 +208,10 @@ scan_file(const char *fn, BlockNumber segmentno)
close(f);
}
-static void
-scan_directory(const char *basedir, const char *subdir)
+static int64
+scan_directory(const char *basedir, const char *subdir, bool sizeonly)
{
+ int64 dirsize = 0;
char path[MAXPGPATH];
DIR *dir;
struct dirent *de;
@@ -167,7 +243,7 @@ scan_directory(const char *basedir, const char *subdir)
if (strncmp(de->d_name,
PG_TEMP_FILES_DIR,
strlen(PG_TEMP_FILES_DIR)) == 0)
- return;
+ continue;
snprintf(fn, sizeof(fn), "%s/%s", path, de->d_name);
if (lstat(fn, &st) < 0)
@@ -214,16 +290,22 @@ scan_directory(const char *basedir, const char *subdir)
/* Relfilenode not to be included */
continue;
- scan_file(fn, segmentno);
+ dirsize += st.st_size;
+
+ if (!sizeonly)
+ {
+ scan_file(fn, segmentno);
+ }
}
#ifndef WIN32
else if (S_ISDIR(st.st_mode) || S_ISLNK(st.st_mode))
#else
else if (S_ISDIR(st.st_mode) || pgwin32_is_junction(fn))
#endif
- scan_directory(path, de->d_name);
+ dirsize += scan_directory(path, de->d_name, sizeonly);
}
closedir(dir);
+ return dirsize;
}
int
@@ -231,6 +313,7 @@ main(int argc, char *argv[])
{
static struct option long_options[] = {
{"pgdata", required_argument, NULL, 'D'},
+ {"progress", no_argument, NULL, 'P'},
{"verbose", no_argument, NULL, 'v'},
{NULL, 0, NULL, 0}
};
@@ -258,7 +341,7 @@ main(int argc, char *argv[])
}
}
- while ((c = getopt_long(argc, argv, "D:r:v", long_options, &option_index)) != -1)
+ while ((c = getopt_long(argc, argv, "D:r:vP", long_options, &option_index)) != -1)
{
switch (c)
{
@@ -276,6 +359,9 @@ main(int argc, char *argv[])
}
only_relfilenode = pstrdup(optarg);
break;
+ case 'P':
+ show_progress = true;
+ break;
default:
fprintf(stderr, _("Try \"%s --help\" for more information.\n"), progname);
exit(1);
@@ -329,10 +415,47 @@ main(int argc, char *argv[])
exit(1);
}
+
+#ifndef WIN32
+ /*
+ * Assign SIGUSR1 signal handler to toggle progress status information
+ */
+ pqsignal(SIGUSR1, toggle_progress_report);
+#endif
+
+ /*
+ * If progress status information is requested, we first need to scan the
+ * directory tree(s) twice, once to get the idea how much data we need
+ * to scan and finally to do the real legwork.
+ */
+ if (show_progress)
+ {
+ total_size = scan_directory(DataDir, "global", true);
+ total_size += scan_directory(DataDir, "base", true);
+ total_size += scan_directory(DataDir, "pg_tblspc", true);
+ }
+
+ /*
+ * Remember start time. Required to calculate the current speed in
+ * report_progress()
+ */
+ scan_started = time(NULL);
+
/* Scan all files */
- scan_directory(DataDir, "global");
- scan_directory(DataDir, "base");
- scan_directory(DataDir, "pg_tblspc");
+ scan_directory(DataDir, "global", false);
+ scan_directory(DataDir, "base", false);
+ scan_directory(DataDir, "pg_tblspc", false);
+
+ /*
+ * Done. Move to next line in case progress information was shown.
+ * Otherwise we clutter the summary output.
+ */
+ if (show_progress)
+ {
+ report_progress(true);
+ if (isatty(fileno(stderr)))
+ fprintf(stderr, "\n");
+ }
printf(_("Checksum scan completed\n"));
printf(_("Data checksum version: %d\n"), ControlFile->data_checksum_version);
Am Montag, den 18.02.2019, 16:52 +0100 schrieb Michael Banck:
Surely we know at that point whether this first scan is needed, and
we
can skip it if not?
Yeah - new patch attached.
Maybe i'm wrong, but my thought is that this breaks the SIGUSR1
business, since there seems no code path which calculates total_size in
this case?
Bernd
On 2019-Feb-18, Bernd Helmle wrote:
Am Montag, den 18.02.2019, 16:52 +0100 schrieb Michael Banck:
Surely we know at that point whether this first scan is needed, and
we
can skip it if not?
Yeah - new patch attached.
Maybe i'm wrong, but my thought is that this breaks the SIGUSR1
business, since there seems no code path which calculates total_size in
this case?
Oh, yeah, it does. In that case, a comment explaining that is needed.
--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Hi,
Am Montag, den 18.02.2019, 13:42 -0300 schrieb Alvaro Herrera:
On 2019-Feb-18, Bernd Helmle wrote:
Am Montag, den 18.02.2019, 16:52 +0100 schrieb Michael Banck:
Surely we know at that point whether this first scan is needed, and
we can skip it if not?Yeah - new patch attached.
Maybe i'm wrong, but my thought is that this breaks the SIGUSR1
business, since there seems no code path which calculates total_size in
this case?Oh, yeah, it does. In that case, a comment explaining that is needed.
New patch attached.
Michael
--
Michael Banck
Projektleiter / Senior Berater
Tel.: +49 2166 9901-171
Fax: +49 2166 9901-100
Email: michael.banck@credativ.de
credativ GmbH, HRB Mönchengladbach 12080
USt-ID-Nummer: DE204566209
Trompeterallee 108, 41189 Mönchengladbach
Geschäftsführung: Dr. Michael Meskes, Jörg Folz, Sascha Heuer
Unser Umgang mit personenbezogenen Daten unterliegt
folgenden Bestimmungen: https://www.credativ.de/datenschutz
Attachments:
pg_verify_checksums_progress_V9.patchtext/x-patch; charset=UTF-8; name=pg_verify_checksums_progress_V9.patchDownload
diff --git a/doc/src/sgml/ref/pg_verify_checksums.sgml b/doc/src/sgml/ref/pg_verify_checksums.sgml
index 905b8f1222..b470c19c08 100644
--- a/doc/src/sgml/ref/pg_verify_checksums.sgml
+++ b/doc/src/sgml/ref/pg_verify_checksums.sgml
@@ -80,6 +80,19 @@ PostgreSQL documentation
</varlistentry>
<varlistentry>
+ <term><option>-P</option></term>
+ <term><option>--progress</option></term>
+ <listitem>
+ <para>
+ Enable progress reporting. Turning this on will deliver an approximate
+ progress report during the checksum verification. Sending the
+ <systemitem>SIGUSR1</systemitem> signal will toggle progress reporting
+ on or off during the verification run (not available on Windows).
+ </para>
+ </listitem>
+ </varlistentry>
+
+ <varlistentry>
<term><option>-V</option></term>
<term><option>--version</option></term>
<listitem>
diff --git a/src/bin/pg_verify_checksums/pg_verify_checksums.c b/src/bin/pg_verify_checksums/pg_verify_checksums.c
index 511262ab5f..448ecd7325 100644
--- a/src/bin/pg_verify_checksums/pg_verify_checksums.c
+++ b/src/bin/pg_verify_checksums/pg_verify_checksums.c
@@ -10,6 +10,8 @@
#include "postgres_fe.h"
#include <dirent.h>
+#include <signal.h>
+#include <time.h>
#include <sys/stat.h>
#include <unistd.h>
@@ -30,9 +32,18 @@ static ControlFileData *ControlFile;
static char *only_relfilenode = NULL;
static bool verbose = false;
+static bool show_progress = false;
static const char *progname;
+/*
+ * Progress status information.
+ */
+int64 total_size = 0;
+int64 current_size = 0;
+pg_time_t last_progress_update;
+pg_time_t scan_started;
+
static void
usage(void)
{
@@ -43,6 +54,7 @@ usage(void)
printf(_(" [-D, --pgdata=]DATADIR data directory\n"));
printf(_(" -v, --verbose output verbose messages\n"));
printf(_(" -r RELFILENODE check only relation with specified relfilenode\n"));
+ printf(_(" -P, --progress show progress information\n"));
printf(_(" -V, --version output version information, then exit\n"));
printf(_(" -?, --help show this help, then exit\n"));
printf(_("\nIf no data directory (DATADIR) is specified, "
@@ -67,6 +79,65 @@ static const char *const skip[] = {
NULL,
};
+static void
+toggle_progress_report(int signum)
+{
+
+ /* we handle SIGUSR1 only, and toggle the value of show_progress */
+ if (signum == SIGUSR1)
+ show_progress = !show_progress;
+
+}
+
+/*
+ * Report current progress status. Parts borrowed from
+ * PostgreSQLs' src/bin/pg_basebackup.c
+ */
+static void
+report_progress(bool force)
+{
+ pg_time_t now = time(NULL);
+ int total_percent = 0;
+
+ char totalstr[32];
+ char currentstr[32];
+ char currspeedstr[32];
+
+ /* Make sure we report at most once a second */
+ if ((now == last_progress_update) && !force)
+ return;
+
+ /* Save current time */
+ last_progress_update = now;
+
+#define MEGABYTES 1048576
+
+ /*
+ * Calculate current percent done, based on MB...
+ */
+ total_percent = total_size ? (int64) ((current_size / MEGABYTES) * 100 / (total_size / MEGABYTES)) : 0;
+
+ /* Don't display larger than 100% */
+ if (total_percent > 100)
+ total_percent = 100;
+
+ /* The same for total size */
+ if (current_size > total_size)
+ total_size = current_size / MEGABYTES;
+
+ snprintf(totalstr, sizeof(totalstr), INT64_FORMAT,
+ total_size / MEGABYTES);
+ snprintf(currentstr, sizeof(currentstr), INT64_FORMAT,
+ current_size / MEGABYTES);
+ snprintf(currspeedstr, sizeof(currspeedstr), INT64_FORMAT,
+ (current_size / MEGABYTES) / (((time(NULL) - scan_started) == 0) ? 1 : (time(NULL) - scan_started)));
+ fprintf(stderr, "%s/%s MB (%d%%, %s MB/s)",
+ currentstr, totalstr, total_percent, currspeedstr);
+
+ /* Stay on the same line if reporting to a terminal */
+ fprintf(stderr, isatty(fileno(stderr)) ? "\r" : "\n");
+}
+
static bool
skipfile(const char *fn)
{
@@ -117,6 +188,7 @@ scan_file(const char *fn, BlockNumber segmentno)
continue;
csum = pg_checksum_page(buf.data, blockno + segmentno * RELSEG_SIZE);
+ current_size += r;
if (csum != header->pd_checksum)
{
if (ControlFile->data_checksum_version == PG_DATA_CHECKSUM_VERSION)
@@ -124,6 +196,9 @@ scan_file(const char *fn, BlockNumber segmentno)
progname, fn, blockno, csum, header->pd_checksum);
badblocks++;
}
+
+ if (show_progress)
+ report_progress(false);
}
if (verbose)
@@ -133,9 +208,10 @@ scan_file(const char *fn, BlockNumber segmentno)
close(f);
}
-static void
-scan_directory(const char *basedir, const char *subdir)
+static int64
+scan_directory(const char *basedir, const char *subdir, bool sizeonly)
{
+ int64 dirsize = 0;
char path[MAXPGPATH];
DIR *dir;
struct dirent *de;
@@ -167,7 +243,7 @@ scan_directory(const char *basedir, const char *subdir)
if (strncmp(de->d_name,
PG_TEMP_FILES_DIR,
strlen(PG_TEMP_FILES_DIR)) == 0)
- return;
+ continue;
snprintf(fn, sizeof(fn), "%s/%s", path, de->d_name);
if (lstat(fn, &st) < 0)
@@ -214,16 +290,22 @@ scan_directory(const char *basedir, const char *subdir)
/* Relfilenode not to be included */
continue;
- scan_file(fn, segmentno);
+ dirsize += st.st_size;
+
+ if (!sizeonly)
+ {
+ scan_file(fn, segmentno);
+ }
}
#ifndef WIN32
else if (S_ISDIR(st.st_mode) || S_ISLNK(st.st_mode))
#else
else if (S_ISDIR(st.st_mode) || pgwin32_is_junction(fn))
#endif
- scan_directory(path, de->d_name);
+ dirsize += scan_directory(path, de->d_name, sizeonly);
}
closedir(dir);
+ return dirsize;
}
int
@@ -231,6 +313,7 @@ main(int argc, char *argv[])
{
static struct option long_options[] = {
{"pgdata", required_argument, NULL, 'D'},
+ {"progress", no_argument, NULL, 'P'},
{"verbose", no_argument, NULL, 'v'},
{NULL, 0, NULL, 0}
};
@@ -258,7 +341,7 @@ main(int argc, char *argv[])
}
}
- while ((c = getopt_long(argc, argv, "D:r:v", long_options, &option_index)) != -1)
+ while ((c = getopt_long(argc, argv, "D:r:vP", long_options, &option_index)) != -1)
{
switch (c)
{
@@ -276,6 +359,9 @@ main(int argc, char *argv[])
}
only_relfilenode = pstrdup(optarg);
break;
+ case 'P':
+ show_progress = true;
+ break;
default:
fprintf(stderr, _("Try \"%s --help\" for more information.\n"), progname);
exit(1);
@@ -329,10 +415,45 @@ main(int argc, char *argv[])
exit(1);
}
+
+#ifndef WIN32
+ /*
+ * Assign SIGUSR1 signal handler to toggle progress status information
+ */
+ pqsignal(SIGUSR1, toggle_progress_report);
+#endif
+
+ /*
+ * As progress status information may be requested even after start of
+ * operation, we need to scan the directory tree(s) twice, once to get
+ * the idea how much data we need to scan and finally to do the real
+ * legwork.
+ */
+ total_size = scan_directory(DataDir, "global", true);
+ total_size += scan_directory(DataDir, "base", true);
+ total_size += scan_directory(DataDir, "pg_tblspc", true);
+
+ /*
+ * Remember start time. Required to calculate the current speed in
+ * report_progress()
+ */
+ scan_started = time(NULL);
+
/* Scan all files */
- scan_directory(DataDir, "global");
- scan_directory(DataDir, "base");
- scan_directory(DataDir, "pg_tblspc");
+ scan_directory(DataDir, "global", false);
+ scan_directory(DataDir, "base", false);
+ scan_directory(DataDir, "pg_tblspc", false);
+
+ /*
+ * Done. Move to next line in case progress information was shown.
+ * Otherwise we clutter the summary output.
+ */
+ if (show_progress)
+ {
+ report_progress(true);
+ if (isatty(fileno(stderr)))
+ fprintf(stderr, "\n");
+ }
printf(_("Checksum scan completed\n"));
printf(_("Data checksum version: %d\n"), ControlFile->data_checksum_version);
Hallo Michael,
New patch attached.
Patch applies cleanly. Compiles, "make check" ok. doc build is also ok.
There are no tests, which is probably fine for such an interactive
feature.
Docs look okay to me. Clear and to the point.
About :
total_percent = total_size ? (int64) ((current_size / MEGABYTES) * 100 / (total_size / MEGABYTES)) : 0;
MEGABYTES can be simplified and will enhance precision. ISTM that the
percent could be a double:
total_percent = total_size ? 100.0 * current_size / total_size : 0.0 ;
and then just let fprintf do the rounding.
I would bother rounding down < 100% to 100, because then you would get
1560/1492 MB (100\%, X MB/s)
which is kind of silly. If it goes over it just mean that the initial
estimate was wrong, which might happen if the db is running (other patch
in the queue). I'm fine with over 100 if it is consistent with the MB
ratio next to it.
fprintf could be called once instead of twice by merging the eol and the
previous message.
Maybe I'd consider inverting the sizeonly boolean so that more is done
when true.
I'd still would use more precise than one second precision time so that
the speed displayed at the beginning is more realistic. eg I got on a
short test with probably in system cache files:
188/188 MB (100%, 188 MB/s)
but the reality is that it took 0.126 seconds, and the speed was really
1492 MB/s.
--
Fabien.
Hi,
Am Dienstag, den 19.02.2019, 16:37 +0100 schrieb Fabien COELHO
About :
total_percent = total_size ? (int64) ((current_size / MEGABYTES) * 100 / (total_size / MEGABYTES)) : 0;
MEGABYTES can be simplified and will enhance precision. ISTM that the
percent could be a double:total_percent = total_size ? 100.0 * current_size / total_size : 0.0 ;
and then just let fprintf do the rounding.
Ok, done so.
I would bother rounding down < 100% to 100, because then you would get
1560/1492 MB (100\%, X MB/s)
which is kind of silly.
No, we cap the total_size to current_size so you won't see that (but
total_size will potentially gradually increase). pg_basebackup has the
same behaviour.
I'd still would use more precise than one second precision time so that
the speed displayed at the beginning is more realistic. eg I got on a
short test with probably in system cache files:188/188 MB (100%, 188 MB/s)
but the reality is that it took 0.126 seconds, and the speed was really
1492 MB/s.
Because I implemented I/O throttling for pg_checksums and needed it
there anyway, I've switched it to the instr_time API now. It now updates
the progress 10 times every second.
So now I get
|20/20 MB (100%, 1382 MB/s)
for an empty cluster vs
|20/20 MB (100%, 20 MB/s)
with the previous version of the patch.
New patch attached.
Michael
--
Michael Banck
Projektleiter / Senior Berater
Tel.: +49 2166 9901-171
Fax: +49 2166 9901-100
Email: michael.banck@credativ.de
credativ GmbH, HRB Mönchengladbach 12080
USt-ID-Nummer: DE204566209
Trompeterallee 108, 41189 Mönchengladbach
Geschäftsführung: Dr. Michael Meskes, Jörg Folz, Sascha Heuer
Unser Umgang mit personenbezogenen Daten unterliegt
folgenden Bestimmungen: https://www.credativ.de/datenschutz
Attachments:
pg_verify_checksums_progress_V10.patchtext/x-patch; charset=UTF-8; name=pg_verify_checksums_progress_V10.patchDownload
diff --git a/doc/src/sgml/ref/pg_verify_checksums.sgml b/doc/src/sgml/ref/pg_verify_checksums.sgml
index 905b8f1222..b470c19c08 100644
--- a/doc/src/sgml/ref/pg_verify_checksums.sgml
+++ b/doc/src/sgml/ref/pg_verify_checksums.sgml
@@ -80,6 +80,19 @@ PostgreSQL documentation
</varlistentry>
<varlistentry>
+ <term><option>-P</option></term>
+ <term><option>--progress</option></term>
+ <listitem>
+ <para>
+ Enable progress reporting. Turning this on will deliver an approximate
+ progress report during the checksum verification. Sending the
+ <systemitem>SIGUSR1</systemitem> signal will toggle progress reporting
+ on or off during the verification run (not available on Windows).
+ </para>
+ </listitem>
+ </varlistentry>
+
+ <varlistentry>
<term><option>-V</option></term>
<term><option>--version</option></term>
<listitem>
diff --git a/src/bin/pg_verify_checksums/pg_verify_checksums.c b/src/bin/pg_verify_checksums/pg_verify_checksums.c
index 511262ab5f..ca41a3eae7 100644
--- a/src/bin/pg_verify_checksums/pg_verify_checksums.c
+++ b/src/bin/pg_verify_checksums/pg_verify_checksums.c
@@ -10,6 +10,8 @@
#include "postgres_fe.h"
#include <dirent.h>
+#include <signal.h>
+#include <time.h>
#include <sys/stat.h>
#include <unistd.h>
@@ -17,6 +19,7 @@
#include "common/controldata_utils.h"
#include "getopt_long.h"
#include "pg_getopt.h"
+#include "portability/instr_time.h"
#include "storage/bufpage.h"
#include "storage/checksum.h"
#include "storage/checksum_impl.h"
@@ -30,9 +33,18 @@ static ControlFileData *ControlFile;
static char *only_relfilenode = NULL;
static bool verbose = false;
+static bool show_progress = false;
static const char *progname;
+/*
+ * Progress status information.
+ */
+int64 total_size = 0;
+int64 current_size = 0;
+instr_time last_progress_update;
+instr_time scan_started;
+
static void
usage(void)
{
@@ -43,6 +55,7 @@ usage(void)
printf(_(" [-D, --pgdata=]DATADIR data directory\n"));
printf(_(" -v, --verbose output verbose messages\n"));
printf(_(" -r RELFILENODE check only relation with specified relfilenode\n"));
+ printf(_(" -P, --progress show progress information\n"));
printf(_(" -V, --version output version information, then exit\n"));
printf(_(" -?, --help show this help, then exit\n"));
printf(_("\nIf no data directory (DATADIR) is specified, "
@@ -67,6 +80,76 @@ static const char *const skip[] = {
NULL,
};
+static void
+toggle_progress_report(int signum)
+{
+
+ /* we handle SIGUSR1 only, and toggle the value of show_progress */
+ if (signum == SIGUSR1)
+ show_progress = !show_progress;
+
+}
+
+/*
+ * Report current progress status. Parts borrowed from
+ * PostgreSQLs' src/bin/pg_basebackup.c
+ */
+static void
+report_progress(bool force)
+{
+ instr_time now;
+ double elapsed;
+ double total_percent;
+ int64 current_speed;
+
+ char totalstr[32];
+ char currentstr[32];
+ char currspeedstr[32];
+
+ INSTR_TIME_SET_CURRENT(now);
+
+ /* Make sure we report at most once every tenth of a second */
+ if ((INSTR_TIME_GET_MILLISEC(now) -
+ INSTR_TIME_GET_MILLISEC(last_progress_update) < 100) && !force)
+ return;
+
+ /* Save current time */
+ last_progress_update = now;
+
+ /* Elapsed time in milliseconds since start of scan */
+ elapsed = (INSTR_TIME_GET_MILLISEC(now) -
+ INSTR_TIME_GET_MILLISEC(scan_started));
+
+ /* Calculate current percent done */
+ total_percent = total_size ? 100.0 * current_size / total_size : 0.0;
+
+ /* Don't display larger than 100% */
+ if (total_percent > 100)
+ total_percent = 100;
+
+#define MEGABYTES 1048576
+
+ /* The same for total size */
+ if (current_size > total_size)
+ total_size = current_size / MEGABYTES;
+
+ /* Calculate current speed */
+ current_speed = (int64)(current_size / MEGABYTES) / (elapsed / 1000);
+
+ snprintf(totalstr, sizeof(totalstr), INT64_FORMAT,
+ total_size / MEGABYTES);
+ snprintf(currentstr, sizeof(currentstr), INT64_FORMAT,
+ current_size / MEGABYTES);
+ snprintf(currspeedstr, sizeof(currspeedstr), INT64_FORMAT,
+ current_speed);
+
+ fprintf(stderr, "%s/%s MB (%d%%, %s MB/s)",
+ currentstr, totalstr, (int)total_percent, currspeedstr);
+
+ /* Stay on the same line if reporting to a terminal */
+ fprintf(stderr, isatty(fileno(stderr)) ? "\r" : "\n");
+}
+
static bool
skipfile(const char *fn)
{
@@ -117,6 +200,7 @@ scan_file(const char *fn, BlockNumber segmentno)
continue;
csum = pg_checksum_page(buf.data, blockno + segmentno * RELSEG_SIZE);
+ current_size += r;
if (csum != header->pd_checksum)
{
if (ControlFile->data_checksum_version == PG_DATA_CHECKSUM_VERSION)
@@ -124,6 +208,9 @@ scan_file(const char *fn, BlockNumber segmentno)
progname, fn, blockno, csum, header->pd_checksum);
badblocks++;
}
+
+ if (show_progress)
+ report_progress(false);
}
if (verbose)
@@ -133,9 +220,10 @@ scan_file(const char *fn, BlockNumber segmentno)
close(f);
}
-static void
-scan_directory(const char *basedir, const char *subdir)
+static int64
+scan_directory(const char *basedir, const char *subdir, bool sizeonly)
{
+ int64 dirsize = 0;
char path[MAXPGPATH];
DIR *dir;
struct dirent *de;
@@ -167,7 +255,7 @@ scan_directory(const char *basedir, const char *subdir)
if (strncmp(de->d_name,
PG_TEMP_FILES_DIR,
strlen(PG_TEMP_FILES_DIR)) == 0)
- return;
+ continue;
snprintf(fn, sizeof(fn), "%s/%s", path, de->d_name);
if (lstat(fn, &st) < 0)
@@ -214,16 +302,20 @@ scan_directory(const char *basedir, const char *subdir)
/* Relfilenode not to be included */
continue;
- scan_file(fn, segmentno);
+ dirsize += st.st_size;
+
+ if (!sizeonly)
+ scan_file(fn, segmentno);
}
#ifndef WIN32
else if (S_ISDIR(st.st_mode) || S_ISLNK(st.st_mode))
#else
else if (S_ISDIR(st.st_mode) || pgwin32_is_junction(fn))
#endif
- scan_directory(path, de->d_name);
+ dirsize += scan_directory(path, de->d_name, sizeonly);
}
closedir(dir);
+ return dirsize;
}
int
@@ -231,6 +323,7 @@ main(int argc, char *argv[])
{
static struct option long_options[] = {
{"pgdata", required_argument, NULL, 'D'},
+ {"progress", no_argument, NULL, 'P'},
{"verbose", no_argument, NULL, 'v'},
{NULL, 0, NULL, 0}
};
@@ -258,7 +351,7 @@ main(int argc, char *argv[])
}
}
- while ((c = getopt_long(argc, argv, "D:r:v", long_options, &option_index)) != -1)
+ while ((c = getopt_long(argc, argv, "D:r:vP", long_options, &option_index)) != -1)
{
switch (c)
{
@@ -276,6 +369,9 @@ main(int argc, char *argv[])
}
only_relfilenode = pstrdup(optarg);
break;
+ case 'P':
+ show_progress = true;
+ break;
default:
fprintf(stderr, _("Try \"%s --help\" for more information.\n"), progname);
exit(1);
@@ -329,10 +425,45 @@ main(int argc, char *argv[])
exit(1);
}
+
+#ifndef WIN32
+ /*
+ * Assign SIGUSR1 signal handler to toggle progress status information
+ */
+ pqsignal(SIGUSR1, toggle_progress_report);
+#endif
+
+ /*
+ * As progress status information may be requested even after start of
+ * operation, we need to scan the directory tree(s) twice, once to get
+ * the idea how much data we need to scan and finally to do the real
+ * legwork.
+ */
+ total_size = scan_directory(DataDir, "global", true);
+ total_size += scan_directory(DataDir, "base", true);
+ total_size += scan_directory(DataDir, "pg_tblspc", true);
+
+ /*
+ * Remember start time. Required to calculate the current speed in
+ * report_progress()
+ */
+ INSTR_TIME_SET_CURRENT(scan_started);
+
/* Scan all files */
- scan_directory(DataDir, "global");
- scan_directory(DataDir, "base");
- scan_directory(DataDir, "pg_tblspc");
+ scan_directory(DataDir, "global", false);
+ scan_directory(DataDir, "base", false);
+ scan_directory(DataDir, "pg_tblspc", false);
+
+ /*
+ * Done. Move to next line in case progress information was shown.
+ * Otherwise we clutter the summary output.
+ */
+ if (show_progress)
+ {
+ report_progress(true);
+ if (isatty(fileno(stderr)))
+ fprintf(stderr, "\n");
+ }
printf(_("Checksum scan completed\n"));
printf(_("Data checksum version: %d\n"), ControlFile->data_checksum_version);
Hallo Michael,
I would bother rounding down < 100% to 100, because then you would get
1560/1492 MB (100\%, X MB/s)
which is kind of silly.
No, we cap the total_size to current_size so you won't see that (but
total_size will potentially gradually increase). pg_basebackup has the
same behaviour.
Ok.
Because I implemented I/O throttling for pg_checksums
Great!
New patch attached.
Does not apply because of the renaming committed by Michaᅵl.
Could you rebase?
--
Fabien.
On Wed, Mar 13, 2019 at 07:22:28AM +0100, Fabien COELHO wrote:
Does not apply because of the renaming committed by Michaël.
Could you rebase?
This stuff touches pg_checksums.c, so you may want to wait one day or
two to avoid extra work... I think that I'll be able to finish the
addition of the --enable and --disable switches by then.
--
Michael
Hello.
At Wed, 13 Mar 2019 16:25:15 +0900, Michael Paquier <michael@paquier.xyz> wrote in <20190313072515.GB2988@paquier.xyz>
On Wed, Mar 13, 2019 at 07:22:28AM +0100, Fabien COELHO wrote:
Does not apply because of the renaming committed by Michaël.
Could you rebase?
This stuff touches pg_checksums.c, so you may want to wait one day or
two to avoid extra work... I think that I'll be able to finish the
addition of the --enable and --disable switches by then.
+ /* Make sure we report at most once every tenth of a second */ + if ((INSTR_TIME_GET_MILLISEC(now) - + INSTR_TIME_GET_MILLISEC(last_progress_update) < 100) && !force)
I'm not a skilled gamer and 10Hz update is a bit hard for my
eyes:p The second hand of old clocks ticks at 4Hz. I think it is
enough frequently.
841/1516 MB (55%, 700 MB/s)
Differently from other prgress reporting project, estimating ETC
(estimated time to completion), which is in the most important,
is quite easy. (pgbench does that.) And I'd like to see a
progress bar there but it might be overdoing. I'm not sure let
the current size occupy a part of screen width is needed. I
don't think total size is needed to be fixed in MB and I think at
most four or five digits are enough. (That is the same for
speed.)
If the all of aboves are involved, the line would look as the
follows.
[======================= ] ( 63% of 12.53 GB, 179 MB/s, ETC 26s)
# Note that this is just an opinion.
(pg_checksum runs fast at the beginning so ETC behaves somewhat
strange in the meanwhile.)
+#define MEGABYTES 1048576
1024 * 1024 is preferable than that many digits.
+ /* we handle SIGUSR1 only, and toggle the value of show_progress */ + if (signum == SIGUSR1) + show_progress = !show_progress;
SIGUSR1 *toggles* progress. A garbage line is left alone after
turning it off. It would be erasable. I'm not sure which is
better, though.
@@ -167,7 +255,7 @@ scan_directory(const char *basedir, const char *subdir) if (strncmp(de->d_name, PG_TEMP_FILES_DIR, strlen(PG_TEMP_FILES_DIR)) == 0) - return; + continue;
Why this patch changes the behavior for temprary directories? It
seems like a bug fix of pg_checksums.
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
On Thu, Mar 14, 2019 at 11:54:17AM +0900, Kyotaro HORIGUCHI wrote:
Why this patch changes the behavior for temprary directories? It
seems like a bug fix of pg_checksums.
Oops, that's a thinko from 5c995139, so fixed. Note this has no
actual consequence though as PG_TEMP_FILE_PREFIX and PG_TEMP_FILES_DIR
have the same value. However a bug is a bug.
--
Michael
Hi,
thanks for the additional review!
Am Donnerstag, den 14.03.2019, 11:54 +0900 schrieb Kyotaro HORIGUCHI:
At Wed, 13 Mar 2019 16:25:15 +0900, Michael Paquier <michael@paquier.xyz> wrote in <20190313072515.GB2988@paquier.xyz>
On Wed, Mar 13, 2019 at 07:22:28AM +0100, Fabien COELHO wrote:
Does not apply because of the renaming committed by Michaël.
Could you rebase?
This stuff touches pg_checksums.c, so you may want to wait one day or
two to avoid extra work... I think that I'll be able to finish the
addition of the --enable and --disable switches by then.
I have rebased it now.
+ /* Make sure we report at most once every tenth of a second */ + if ((INSTR_TIME_GET_MILLISEC(now) - + INSTR_TIME_GET_MILLISEC(last_progress_update) < 100) && !force)I'm not a skilled gamer and 10Hz update is a bit hard for my
eyes:p The second hand of old clocks ticks at 4Hz. I think it is
enough frequently.
Ok.
841/1516 MB (55%, 700 MB/s)
Differently from other prgress reporting project, estimating ETC
(estimated time to completion), which is in the most important,
is quite easy. (pgbench does that.) And I'd like to see a
progress bar there but it might be overdoing. I'm not sure let
the current size occupy a part of screen width is needed. I
don't think total size is needed to be fixed in MB and I think at
most four or five digits are enough. (That is the same for
speed.)If the all of aboves are involved, the line would look as the
follows.[======================= ] ( 63% of 12.53 GB, 179 MB/s, ETC 26s)
# Note that this is just an opinion.
(pg_checksum runs fast at the beginning so ETC behaves somewhat
strange in the meanwhile.)
I haven't changed that for now as it seems to be a bit more involved.
I'd like to hear other opinions on whether that is worthwhile?
+#define MEGABYTES 1048576
1024 * 1024 is preferable than that many digits.
Good point, changed that way.
+ /* we handle SIGUSR1 only, and toggle the value of show_progress */ + if (signum == SIGUSR1) + show_progress = !show_progress;SIGUSR1 *toggles* progress.
Not sure what you mean here, are you saying it should be called
toggle_progress and not show_progress? I think it was like that
originally but got changed due to review comments.
A garbage line is left alone after turning it off. It would be
erasable. I'm not sure which is better, though.
How about just printing a "\n" in the signal handler if we are in a
terminal? I think erasing the line might get too clever and will be a
portability hazard? I have done that now in the attached patch, let me
know what you think.
Michael
--
Michael Banck
Projektleiter / Senior Berater
Tel.: +49 2166 9901-171
Fax: +49 2166 9901-100
Email: michael.banck@credativ.de
credativ GmbH, HRB Mönchengladbach 12080
USt-ID-Nummer: DE204566209
Trompeterallee 108, 41189 Mönchengladbach
Geschäftsführung: Dr. Michael Meskes, Jörg Folz, Sascha Heuer
Unser Umgang mit personenbezogenen Daten unterliegt
folgenden Bestimmungen: https://www.credativ.de/datenschutz
Attachments:
pg_verify_checksums_progress_V11.patchtext/x-patch; charset=UTF-8; name=pg_verify_checksums_progress_V11.patchDownload
diff --git a/doc/src/sgml/ref/pg_checksums.sgml b/doc/src/sgml/ref/pg_checksums.sgml
index 6a47dda683..c790170953 100644
--- a/doc/src/sgml/ref/pg_checksums.sgml
+++ b/doc/src/sgml/ref/pg_checksums.sgml
@@ -80,6 +80,19 @@ PostgreSQL documentation
</varlistentry>
<varlistentry>
+ <term><option>-P</option></term>
+ <term><option>--progress</option></term>
+ <listitem>
+ <para>
+ Enable progress reporting. Turning this on will deliver an approximate
+ progress report during the checksum verification. Sending the
+ <systemitem>SIGUSR1</systemitem> signal will toggle progress reporting
+ on or off during the verification run (not available on Windows).
+ </para>
+ </listitem>
+ </varlistentry>
+
+ <varlistentry>
<term><option>-V</option></term>
<term><option>--version</option></term>
<listitem>
diff --git a/src/bin/pg_checksums/pg_checksums.c b/src/bin/pg_checksums/pg_checksums.c
index b7ebc11017..a771f67c18 100644
--- a/src/bin/pg_checksums/pg_checksums.c
+++ b/src/bin/pg_checksums/pg_checksums.c
@@ -14,6 +14,8 @@
#include "postgres_fe.h"
#include <dirent.h>
+#include <signal.h>
+#include <time.h>
#include <sys/stat.h>
#include <unistd.h>
@@ -21,6 +23,7 @@
#include "common/controldata_utils.h"
#include "getopt_long.h"
#include "pg_getopt.h"
+#include "portability/instr_time.h"
#include "storage/bufpage.h"
#include "storage/checksum.h"
#include "storage/checksum_impl.h"
@@ -34,9 +37,18 @@ static ControlFileData *ControlFile;
static char *only_relfilenode = NULL;
static bool verbose = false;
+static bool show_progress = false;
static const char *progname;
+/*
+ * Progress status information.
+ */
+int64 total_size = 0;
+int64 current_size = 0;
+instr_time last_progress_update;
+instr_time scan_started;
+
static void
usage(void)
{
@@ -47,6 +59,7 @@ usage(void)
printf(_(" [-D, --pgdata=]DATADIR data directory\n"));
printf(_(" -v, --verbose output verbose messages\n"));
printf(_(" -r RELFILENODE check only relation with specified relfilenode\n"));
+ printf(_(" -P, --progress show progress information\n"));
printf(_(" -V, --version output version information, then exit\n"));
printf(_(" -?, --help show this help, then exit\n"));
printf(_("\nIf no data directory (DATADIR) is specified, "
@@ -71,6 +84,83 @@ static const char *const skip[] = {
NULL,
};
+static void
+toggle_progress_report(int signum)
+{
+
+ /* we handle SIGUSR1 only, and toggle the value of show_progress */
+ if (signum == SIGUSR1)
+ {
+ /*
+ * Skip to the next line in order to avoid overwriting half of
+ * the progress report line with the final output.
+ */
+ if (show_progress && isatty(fileno(stderr)))
+ fprintf(stderr, "\n");
+ show_progress = !show_progress;
+ }
+}
+
+/*
+ * Report current progress status. Parts borrowed from
+ * PostgreSQLs' src/bin/pg_basebackup.c
+ */
+static void
+report_progress(bool force)
+{
+ instr_time now;
+ double elapsed;
+ double total_percent;
+ int64 current_speed;
+
+ char totalstr[32];
+ char currentstr[32];
+ char currspeedstr[32];
+
+ INSTR_TIME_SET_CURRENT(now);
+
+ /* Make sure we report at most once every 400 milliseconds */
+ if ((INSTR_TIME_GET_MILLISEC(now) -
+ INSTR_TIME_GET_MILLISEC(last_progress_update) < 400) && !force)
+ return;
+
+ /* Save current time */
+ last_progress_update = now;
+
+ /* Elapsed time in milliseconds since start of scan */
+ elapsed = (INSTR_TIME_GET_MILLISEC(now) -
+ INSTR_TIME_GET_MILLISEC(scan_started));
+
+ /* Calculate current percent done */
+ total_percent = total_size ? 100.0 * current_size / total_size : 0.0;
+
+ /* Don't display larger than 100% */
+ if (total_percent > 100)
+ total_percent = 100;
+
+#define MEGABYTES (1024 * 1024)
+
+ /* The same for total size */
+ if (current_size > total_size)
+ total_size = current_size / MEGABYTES;
+
+ /* Calculate current speed */
+ current_speed = (int64)(current_size / MEGABYTES) / (elapsed / 1000);
+
+ snprintf(totalstr, sizeof(totalstr), INT64_FORMAT,
+ total_size / MEGABYTES);
+ snprintf(currentstr, sizeof(currentstr), INT64_FORMAT,
+ current_size / MEGABYTES);
+ snprintf(currspeedstr, sizeof(currspeedstr), INT64_FORMAT,
+ current_speed);
+
+ fprintf(stderr, "%s/%s MB (%d%%, %s MB/s)",
+ currentstr, totalstr, (int)total_percent, currspeedstr);
+
+ /* Stay on the same line if reporting to a terminal */
+ fprintf(stderr, isatty(fileno(stderr)) ? "\r" : "\n");
+}
+
static bool
skipfile(const char *fn)
{
@@ -121,6 +211,7 @@ scan_file(const char *fn, BlockNumber segmentno)
continue;
csum = pg_checksum_page(buf.data, blockno + segmentno * RELSEG_SIZE);
+ current_size += r;
if (csum != header->pd_checksum)
{
if (ControlFile->data_checksum_version == PG_DATA_CHECKSUM_VERSION)
@@ -128,6 +219,9 @@ scan_file(const char *fn, BlockNumber segmentno)
progname, fn, blockno, csum, header->pd_checksum);
badblocks++;
}
+
+ if (show_progress)
+ report_progress(false);
}
if (verbose)
@@ -137,9 +231,10 @@ scan_file(const char *fn, BlockNumber segmentno)
close(f);
}
-static void
-scan_directory(const char *basedir, const char *subdir)
+static int64
+scan_directory(const char *basedir, const char *subdir, bool sizeonly)
{
+ int64 dirsize = 0;
char path[MAXPGPATH];
DIR *dir;
struct dirent *de;
@@ -218,16 +313,20 @@ scan_directory(const char *basedir, const char *subdir)
/* Relfilenode not to be included */
continue;
- scan_file(fn, segmentno);
+ dirsize += st.st_size;
+
+ if (!sizeonly)
+ scan_file(fn, segmentno);
}
#ifndef WIN32
else if (S_ISDIR(st.st_mode) || S_ISLNK(st.st_mode))
#else
else if (S_ISDIR(st.st_mode) || pgwin32_is_junction(fn))
#endif
- scan_directory(path, de->d_name);
+ dirsize += scan_directory(path, de->d_name, sizeonly);
}
closedir(dir);
+ return dirsize;
}
int
@@ -235,6 +334,7 @@ main(int argc, char *argv[])
{
static struct option long_options[] = {
{"pgdata", required_argument, NULL, 'D'},
+ {"progress", no_argument, NULL, 'P'},
{"verbose", no_argument, NULL, 'v'},
{NULL, 0, NULL, 0}
};
@@ -262,7 +362,7 @@ main(int argc, char *argv[])
}
}
- while ((c = getopt_long(argc, argv, "D:r:v", long_options, &option_index)) != -1)
+ while ((c = getopt_long(argc, argv, "D:r:vP", long_options, &option_index)) != -1)
{
switch (c)
{
@@ -280,6 +380,9 @@ main(int argc, char *argv[])
}
only_relfilenode = pstrdup(optarg);
break;
+ case 'P':
+ show_progress = true;
+ break;
default:
fprintf(stderr, _("Try \"%s --help\" for more information.\n"), progname);
exit(1);
@@ -349,10 +452,45 @@ main(int argc, char *argv[])
exit(1);
}
+
+#ifndef WIN32
+ /*
+ * Assign SIGUSR1 signal handler to toggle progress status information
+ */
+ pqsignal(SIGUSR1, toggle_progress_report);
+#endif
+
+ /*
+ * As progress status information may be requested even after start of
+ * operation, we need to scan the directory tree(s) twice, once to get
+ * the idea how much data we need to scan and finally to do the real
+ * legwork.
+ */
+ total_size = scan_directory(DataDir, "global", true);
+ total_size += scan_directory(DataDir, "base", true);
+ total_size += scan_directory(DataDir, "pg_tblspc", true);
+
+ /*
+ * Remember start time. Required to calculate the current speed in
+ * report_progress()
+ */
+ INSTR_TIME_SET_CURRENT(scan_started);
+
/* Scan all files */
- scan_directory(DataDir, "global");
- scan_directory(DataDir, "base");
- scan_directory(DataDir, "pg_tblspc");
+ scan_directory(DataDir, "global", false);
+ scan_directory(DataDir, "base", false);
+ scan_directory(DataDir, "pg_tblspc", false);
+
+ /*
+ * Done. Move to next line in case progress information was shown.
+ * Otherwise we clutter the summary output.
+ */
+ if (show_progress)
+ {
+ report_progress(true);
+ if (isatty(fileno(stderr)))
+ fprintf(stderr, "\n");
+ }
printf(_("Checksum scan completed\n"));
printf(_("Data checksum version: %d\n"), ControlFile->data_checksum_version);
I have rebased it now.
Thanks. Will look at it.
If the all of aboves are involved, the line would look as the
follows.[======================= ] ( 63% of 12.53 GB, 179 MB/s, ETC 26s)
# Note that this is just an opinion.
(pg_checksum runs fast at the beginning so ETC behaves somewhat
strange in the meanwhile.)I haven't changed that for now as it seems to be a bit more involved.
I'd like to hear other opinions on whether that is worthwhile?
I think that the bar is overkill, but ETC is easy and nice.
+ /* we handle SIGUSR1 only, and toggle the value of show_progress */ + if (signum == SIGUSR1) + show_progress = !show_progress;SIGUSR1 *toggles* progress.
Not sure what you mean here,
Probably it is meant to simplify the comment?
--
Fabien.
Hello.
At Mon, 18 Mar 2019 23:14:01 +0100 (CET), Fabien COELHO <coelho@cri.ensmp.fr> wrote in <alpine.DEB.2.21.1903182311130.23282@lancre>
I have rebased it now.
Thanks. Will look at it.
If the all of aboves are involved, the line would look as the
follows.[======================= ] ( 63% of 12.53 GB, 179 MB/s,
ETC 26s)# Note that this is just an opinion.
(pg_checksum runs fast at the beginning so ETC behaves somewhat
strange in the meanwhile.)I haven't changed that for now as it seems to be a bit more involved.
I'd like to hear other opinions on whether that is worthwhile?I think that the bar is overkill, but ETC is easy and nice.
I'm fine with that.
+ /* we handle SIGUSR1 only, and toggle the value of show_progress */ + if (signum == SIGUSR1) + show_progress = !show_progress;SIGUSR1 *toggles* progress.
Not sure what you mean here,
Probably it is meant to simplify the comment?
Sorry. I meant that "it can be turned off and a perhaps-garbage
line is left alone after turning off. Don't we need to erase it?".
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
Hi,
Am Dienstag, den 19.03.2019, 09:04 +0900 schrieb Kyotaro HORIGUCHI:
At Mon, 18 Mar 2019 23:14:01 +0100 (CET), Fabien COELHO <coelho@cri.ensmp.fr> wrote in <alpine.DEB.2.21.1903182311130.23282@lancre>
+ /* we handle SIGUSR1 only, and toggle the value of show_progress */ + if (signum == SIGUSR1) + show_progress = !show_progress;SIGUSR1 *toggles* progress.
Not sure what you mean here,
Probably it is meant to simplify the comment?
Sorry. I meant that "it can be turned off and a perhaps-garbage
line is left alone after turning off. Don't we need to erase it?".
The current version prints a newline when it progress reporting is
toggled off. Do you mean there is a hazard that this happens right when
we are printing the progress, so end up with a partly garbage line? I
don't think that'd be so bad to warrant further code complexitiy, after
all, the user explicitly wanted the progress to stop.
But maybe I am misunderstanding?
Michael
--
Michael Banck
Projektleiter / Senior Berater
Tel.: +49 2166 9901-171
Fax: +49 2166 9901-100
Email: michael.banck@credativ.de
credativ GmbH, HRB Mönchengladbach 12080
USt-ID-Nummer: DE204566209
Trompeterallee 108, 41189 Mönchengladbach
Geschäftsführung: Dr. Michael Meskes, Jörg Folz, Sascha Heuer
Unser Umgang mit personenbezogenen Daten unterliegt
folgenden Bestimmungen: https://www.credativ.de/datenschutz
On Fri, Mar 22, 2019 at 02:23:17PM +0100, Michael Banck wrote:
The current version prints a newline when it progress reporting is
toggled off. Do you mean there is a hazard that this happens right when
we are printing the progress, so end up with a partly garbage line? I
don't think that'd be so bad to warrant further code complexitiy, after
all, the user explicitly wanted the progress to stop.But maybe I am misunderstanding?
The latest patch does not apply because of mainly ed308d7. Could you
send a rebase?
FWIW, I would remove the signal toggling stuff from the patch and keep
the logic simple at this stage. Not having a solution which works on
Windows is perhaps not a strong reason to not include it, but it's a
sign that we could perhaps design something better, and that's
annoying. Personally I think that I would just use --progress all the
time and not use the signaling part at all, my 2c.
--
Michael
Hi,
Am Samstag, den 23.03.2019, 10:49 +0900 schrieb Michael Paquier:
On Fri, Mar 22, 2019 at 02:23:17PM +0100, Michael Banck wrote:
The current version prints a newline when it progress reporting is
toggled off. Do you mean there is a hazard that this happens right when
we are printing the progress, so end up with a partly garbage line? I
don't think that'd be so bad to warrant further code complexitiy, after
all, the user explicitly wanted the progress to stop.But maybe I am misunderstanding?
The latest patch does not apply because of mainly ed308d7. Could you
send a rebase?
I've rebased it now, see attached.
FWIW, I would remove the signal toggling stuff from the patch and keep
the logic simple at this stage. Not having a solution which works on
Windows is perhaps not a strong reason to not include it, but it's a
sign that we could perhaps design something better, and that's
annoying. Personally I think that I would just use --progress all the
time and not use the signaling part at all, my 2c.
We had this discussion already, and back then I was against dropping it
as well.
Do you see --progress being the default? I don't think this is in-line
with other project binaries, so I guess not?
If not, I guess your main point is that we might come up with a better
implementation/UI further down the road. Which is fair enough, but I
don't think this is such an in-your-face feature that changing the way
it works in future releases would be a terrible breakage of backwards
compatibility. Even more so as it is solely an interactive feature and
cannot be used in scripts etc.
Michael
--
Michael Banck
Projektleiter / Senior Berater
Tel.: +49 2166 9901-171
Fax: +49 2166 9901-100
Email: michael.banck@credativ.de
credativ GmbH, HRB Mönchengladbach 12080
USt-ID-Nummer: DE204566209
Trompeterallee 108, 41189 Mönchengladbach
Geschäftsführung: Dr. Michael Meskes, Jörg Folz, Sascha Heuer
Unser Umgang mit personenbezogenen Daten unterliegt
folgenden Bestimmungen: https://www.credativ.de/datenschutz
Attachments:
pg_verify_checksums_progress_V12.patchtext/x-patch; charset=UTF-8; name=pg_verify_checksums_progress_V12.patchDownload
diff --git a/doc/src/sgml/ref/pg_checksums.sgml b/doc/src/sgml/ref/pg_checksums.sgml
index 1f4d4ab8b4..d18072a067 100644
--- a/doc/src/sgml/ref/pg_checksums.sgml
+++ b/doc/src/sgml/ref/pg_checksums.sgml
@@ -136,6 +136,19 @@ PostgreSQL documentation
</varlistentry>
<varlistentry>
+ <term><option>-P</option></term>
+ <term><option>--progress</option></term>
+ <listitem>
+ <para>
+ Enable progress reporting. Turning this on will deliver an approximate
+ progress report during the checksum verification. Sending the
+ <systemitem>SIGUSR1</systemitem> signal will toggle progress reporting
+ on or off during the verification run (not available on Windows).
+ </para>
+ </listitem>
+ </varlistentry>
+
+ <varlistentry>
<term><option>-V</option></term>
<term><option>--version</option></term>
<listitem>
diff --git a/src/bin/pg_checksums/pg_checksums.c b/src/bin/pg_checksums/pg_checksums.c
index 5c185ebed8..4bcafb02dd 100644
--- a/src/bin/pg_checksums/pg_checksums.c
+++ b/src/bin/pg_checksums/pg_checksums.c
@@ -15,6 +15,8 @@
#include "postgres_fe.h"
#include <dirent.h>
+#include <signal.h>
+#include <time.h>
#include <sys/stat.h>
#include <unistd.h>
@@ -24,6 +26,7 @@
#include "common/file_utils.h"
#include "getopt_long.h"
#include "pg_getopt.h"
+#include "portability/instr_time.h"
#include "storage/bufpage.h"
#include "storage/checksum.h"
#include "storage/checksum_impl.h"
@@ -37,6 +40,7 @@ static ControlFileData *ControlFile;
static char *only_relfilenode = NULL;
static bool do_sync = true;
static bool verbose = false;
+static bool show_progress = false;
typedef enum
{
@@ -59,6 +63,14 @@ static PgChecksumMode mode = PG_MODE_CHECK;
static const char *progname;
+/*
+ * Progress status information.
+ */
+int64 total_size = 0;
+int64 current_size = 0;
+instr_time last_progress_update;
+instr_time scan_started;
+
static void
usage(void)
{
@@ -73,6 +85,7 @@ usage(void)
printf(_(" -N, --no-sync do not wait for changes to be written safely to disk\n"));
printf(_(" -v, --verbose output verbose messages\n"));
printf(_(" -r RELFILENODE check only relation with specified relfilenode\n"));
+ printf(_(" -P, --progress show progress information\n"));
printf(_(" -V, --version output version information, then exit\n"));
printf(_(" -?, --help show this help, then exit\n"));
printf(_("\nIf no data directory (DATADIR) is specified, "
@@ -97,6 +110,83 @@ static const char *const skip[] = {
NULL,
};
+static void
+toggle_progress_report(int signum)
+{
+
+ /* we handle SIGUSR1 only, and toggle the value of show_progress */
+ if (signum == SIGUSR1)
+ {
+ /*
+ * Skip to the next line in order to avoid overwriting half of
+ * the progress report line with the final output.
+ */
+ if (show_progress && isatty(fileno(stderr)))
+ fprintf(stderr, "\n");
+ show_progress = !show_progress;
+ }
+}
+
+/*
+ * Report current progress status. Parts borrowed from
+ * PostgreSQLs' src/bin/pg_basebackup.c
+ */
+static void
+report_progress(bool force)
+{
+ instr_time now;
+ double elapsed;
+ double total_percent;
+ int64 current_speed;
+
+ char totalstr[32];
+ char currentstr[32];
+ char currspeedstr[32];
+
+ INSTR_TIME_SET_CURRENT(now);
+
+ /* Make sure we report at most once every 400 milliseconds */
+ if ((INSTR_TIME_GET_MILLISEC(now) -
+ INSTR_TIME_GET_MILLISEC(last_progress_update) < 400) && !force)
+ return;
+
+ /* Save current time */
+ last_progress_update = now;
+
+ /* Elapsed time in milliseconds since start of scan */
+ elapsed = (INSTR_TIME_GET_MILLISEC(now) -
+ INSTR_TIME_GET_MILLISEC(scan_started));
+
+ /* Calculate current percent done */
+ total_percent = total_size ? 100.0 * current_size / total_size : 0.0;
+
+ /* Don't display larger than 100% */
+ if (total_percent > 100)
+ total_percent = 100;
+
+#define MEGABYTES (1024 * 1024)
+
+ /* The same for total size */
+ if (current_size > total_size)
+ total_size = current_size / MEGABYTES;
+
+ /* Calculate current speed */
+ current_speed = (int64)(current_size / MEGABYTES) / (elapsed / 1000);
+
+ snprintf(totalstr, sizeof(totalstr), INT64_FORMAT,
+ total_size / MEGABYTES);
+ snprintf(currentstr, sizeof(currentstr), INT64_FORMAT,
+ current_size / MEGABYTES);
+ snprintf(currspeedstr, sizeof(currspeedstr), INT64_FORMAT,
+ current_speed);
+
+ fprintf(stderr, "%s/%s MB (%d%%, %s MB/s)",
+ currentstr, totalstr, (int)total_percent, currspeedstr);
+
+ /* Stay on the same line if reporting to a terminal */
+ fprintf(stderr, isatty(fileno(stderr)) ? "\r" : "\n");
+}
+
static bool
skipfile(const char *fn)
{
@@ -153,6 +243,7 @@ scan_file(const char *fn, BlockNumber segmentno)
continue;
csum = pg_checksum_page(buf.data, blockno + segmentno * RELSEG_SIZE);
+ current_size += r;
if (mode == PG_MODE_CHECK)
{
if (csum != header->pd_checksum)
@@ -183,6 +274,9 @@ scan_file(const char *fn, BlockNumber segmentno)
exit(1);
}
}
+
+ if (show_progress)
+ report_progress(false);
}
if (verbose)
@@ -198,9 +292,10 @@ scan_file(const char *fn, BlockNumber segmentno)
close(f);
}
-static void
-scan_directory(const char *basedir, const char *subdir)
+static int64
+scan_directory(const char *basedir, const char *subdir, bool sizeonly)
{
+ int64 dirsize = 0;
char path[MAXPGPATH];
DIR *dir;
struct dirent *de;
@@ -279,16 +374,20 @@ scan_directory(const char *basedir, const char *subdir)
/* Relfilenode not to be included */
continue;
- scan_file(fn, segmentno);
+ dirsize += st.st_size;
+
+ if (!sizeonly)
+ scan_file(fn, segmentno);
}
#ifndef WIN32
else if (S_ISDIR(st.st_mode) || S_ISLNK(st.st_mode))
#else
else if (S_ISDIR(st.st_mode) || pgwin32_is_junction(fn))
#endif
- scan_directory(path, de->d_name);
+ dirsize += scan_directory(path, de->d_name, sizeonly);
}
closedir(dir);
+ return dirsize;
}
int
@@ -300,6 +399,7 @@ main(int argc, char *argv[])
{"disable", no_argument, NULL, 'd'},
{"enable", no_argument, NULL, 'e'},
{"no-sync", no_argument, NULL, 'N'},
+ {"progress", no_argument, NULL, 'P'},
{"verbose", no_argument, NULL, 'v'},
{NULL, 0, NULL, 0}
};
@@ -327,7 +427,7 @@ main(int argc, char *argv[])
}
}
- while ((c = getopt_long(argc, argv, "cD:deNr:v", long_options, &option_index)) != -1)
+ while ((c = getopt_long(argc, argv, "cD:deNPr:v", long_options, &option_index)) != -1)
{
switch (c)
{
@@ -357,6 +457,9 @@ main(int argc, char *argv[])
}
only_relfilenode = pstrdup(optarg);
break;
+ case 'P':
+ show_progress = true;
+ break;
default:
fprintf(stderr, _("Try \"%s --help\" for more information.\n"), progname);
exit(1);
@@ -436,6 +539,29 @@ main(int argc, char *argv[])
exit(1);
}
+#ifndef WIN32
+ /*
+ * Assign SIGUSR1 signal handler to toggle progress status information
+ */
+ pqsignal(SIGUSR1, toggle_progress_report);
+#endif
+
+ /*
+ * As progress status information may be requested even after start of
+ * operation, we need to scan the directory tree(s) twice, once to get
+ * the idea how much data we need to scan and finally to do the real
+ * legwork.
+ */
+ total_size = scan_directory(DataDir, "global", true);
+ total_size += scan_directory(DataDir, "base", true);
+ total_size += scan_directory(DataDir, "pg_tblspc", true);
+
+ /*
+ * Remember start time. Required to calculate the current speed in
+ * report_progress()
+ */
+ INSTR_TIME_SET_CURRENT(scan_started);
+
if (ControlFile->data_checksum_version == 0 &&
mode == PG_MODE_DISABLE)
{
@@ -453,9 +579,20 @@ main(int argc, char *argv[])
/* Operate on all files if checking or enabling checksums */
if (mode == PG_MODE_CHECK || mode == PG_MODE_ENABLE)
{
- scan_directory(DataDir, "global");
- scan_directory(DataDir, "base");
- scan_directory(DataDir, "pg_tblspc");
+ scan_directory(DataDir, "global", false);
+ scan_directory(DataDir, "base", false);
+ scan_directory(DataDir, "pg_tblspc", false);
+
+ /*
+ * Done. Move to next line in case progress information was shown.
+ * Otherwise we clutter the summary output.
+ */
+ if (show_progress)
+ {
+ report_progress(true);
+ if (isatty(fileno(stderr)))
+ fprintf(stderr, "\n");
+ }
printf(_("Checksum operation completed\n"));
printf(_("Files scanned: %s\n"), psprintf(INT64_FORMAT, files));
Hallo Michael,
About patch v12:
Patch applies cleanly, compiles. make check ok, but feature is not tested.
Doc build ok.
Although I'm in favor of it, I'm not sure that the signal think will make
it for 12. Maybe it is worth compromising, doing a simple version for now,
and resubmitting the signal feature later?
ISTM that someone suggested 4 Hz was the good eye speed, but you wait for
400 ms, which is 2.5 Hz. What about it?
I seems that current_size and total_size are not in the same unit:
+ if (current_size > total_size)
+ total_size = current_size / MEGABYTES;
But they should both really be bytes, always.
I think that the computations should be inverted:
- first adjust total_size to current_size if needed
- then compute percent
- remove the percent adjustement as it is <= 100
since current_size <= total_size
I still think that the speed should compute a double to avoid integer
rounding errors within the computation. ISTM that rounding should only be
done for display in the end.
I'm okay with calling the report on each file even if this means every few
GB...
Someone suggested ETA, and it seems rather simple to do. What about
adding it?
--
Fabien.
Hi,
thanks again for your review!
Am Mittwoch, den 27.03.2019, 15:34 +0100 schrieb Fabien COELHO:
Hallo Michael,
About patch v12:
Patch applies cleanly, compiles. make check ok, but feature is not tested.
Doc build ok.Although I'm in favor of it, I'm not sure that the signal think will make
it for 12. Maybe it is worth compromising, doing a simple version for now,
and resubmitting the signal feature later?
Let's wait one more round. I think that feature is quite useful and
isolated enough that it could stay, but I'll remove it for the next
patch version if needed.
ISTM that someone suggested 4 Hz was the good eye speed, but you wait for
400 ms, which is 2.5 Hz. What about it?
Uh right, I messed that up, fixed.
I seems that current_size and total_size are not in the same unit:
+ if (current_size > total_size) + total_size = current_size / MEGABYTES;But they should both really be bytes, always.
Yeah, they are both bytes and the "/ MEGABYTES" is wrong, removed.
I think that the computations should be inverted:
- first adjust total_size to current_size if needed
- then compute percent
- remove the percent adjustement as it is <= 100
since current_size <= total_size
Ok, done so.
I still think that the speed should compute a double to avoid integer
rounding errors within the computation. ISTM that rounding should only be
done for display in the end.
Ok, also done this way. I decided to print only full MB and not any
further digits as those don't seem very relevant.
I'm okay with calling the report on each file even if this means every few
GB...
For my I/O throttling branch I've backed off to only call the report
every 100 blocks (and at the end of the file). I've added that logic to
this patch as well.
Someone suggested ETA, and it seems rather simple to do. What about
adding it?
I don't know, just computing the ETA is easy of course. But you'd have
to fiddle with seconds/minutes/hours conversions cause the runtime could
be hours. Then, you don't want to have it bumping around weirdly when
your I/O rate changes, cf. https://xkcd.com/612/ That doesn't sounds
simpler than the signal trigger we might get rid of.
I have attached a POC which just prints the remaining seconds. If
somebody wants to contribute a full implementation as a co-author, I'm
happy to include it. Otherwise, I might take a stab at it over the next
days, but it is not on the top of my TODO list.
New patch attached. I've also changed the reporting line so that it
prints a couple of blanks at the end cause I noticed that if the
previously reported line was longer than the current line, then the end
of it is still visible.
Michael
--
Michael Banck
Projektleiter / Senior Berater
Tel.: +49 2166 9901-171
Fax: +49 2166 9901-100
Email: michael.banck@credativ.de
credativ GmbH, HRB Mönchengladbach 12080
USt-ID-Nummer: DE204566209
Trompeterallee 108, 41189 Mönchengladbach
Geschäftsführung: Dr. Michael Meskes, Jörg Folz, Sascha Heuer
Unser Umgang mit personenbezogenen Daten unterliegt
folgenden Bestimmungen: https://www.credativ.de/datenschutz
Attachments:
pg_verify_checksums_progress_V13.patchtext/x-patch; charset=UTF-8; name=pg_verify_checksums_progress_V13.patchDownload
diff --git a/doc/src/sgml/ref/pg_checksums.sgml b/doc/src/sgml/ref/pg_checksums.sgml
index 1f4d4ab8b4..d18072a067 100644
--- a/doc/src/sgml/ref/pg_checksums.sgml
+++ b/doc/src/sgml/ref/pg_checksums.sgml
@@ -136,6 +136,19 @@ PostgreSQL documentation
</varlistentry>
<varlistentry>
+ <term><option>-P</option></term>
+ <term><option>--progress</option></term>
+ <listitem>
+ <para>
+ Enable progress reporting. Turning this on will deliver an approximate
+ progress report during the checksum verification. Sending the
+ <systemitem>SIGUSR1</systemitem> signal will toggle progress reporting
+ on or off during the verification run (not available on Windows).
+ </para>
+ </listitem>
+ </varlistentry>
+
+ <varlistentry>
<term><option>-V</option></term>
<term><option>--version</option></term>
<listitem>
diff --git a/src/bin/pg_checksums/pg_checksums.c b/src/bin/pg_checksums/pg_checksums.c
index 5c185ebed8..e9a47e96f6 100644
--- a/src/bin/pg_checksums/pg_checksums.c
+++ b/src/bin/pg_checksums/pg_checksums.c
@@ -15,6 +15,8 @@
#include "postgres_fe.h"
#include <dirent.h>
+#include <signal.h>
+#include <time.h>
#include <sys/stat.h>
#include <unistd.h>
@@ -24,6 +26,7 @@
#include "common/file_utils.h"
#include "getopt_long.h"
#include "pg_getopt.h"
+#include "portability/instr_time.h"
#include "storage/bufpage.h"
#include "storage/checksum.h"
#include "storage/checksum_impl.h"
@@ -37,6 +40,7 @@ static ControlFileData *ControlFile;
static char *only_relfilenode = NULL;
static bool do_sync = true;
static bool verbose = false;
+static bool show_progress = false;
typedef enum
{
@@ -59,6 +63,14 @@ static PgChecksumMode mode = PG_MODE_CHECK;
static const char *progname;
+/*
+ * Progress status information.
+ */
+int64 total_size = 0;
+int64 current_size = 0;
+instr_time last_progress_update;
+instr_time scan_started;
+
static void
usage(void)
{
@@ -73,6 +85,7 @@ usage(void)
printf(_(" -N, --no-sync do not wait for changes to be written safely to disk\n"));
printf(_(" -v, --verbose output verbose messages\n"));
printf(_(" -r RELFILENODE check only relation with specified relfilenode\n"));
+ printf(_(" -P, --progress show progress information\n"));
printf(_(" -V, --version output version information, then exit\n"));
printf(_(" -?, --help show this help, then exit\n"));
printf(_("\nIf no data directory (DATADIR) is specified, "
@@ -97,6 +110,80 @@ static const char *const skip[] = {
NULL,
};
+static void
+toggle_progress_report(int signum)
+{
+
+ /* we handle SIGUSR1 only, and toggle the value of show_progress */
+ if (signum == SIGUSR1)
+ {
+ /*
+ * Skip to the next line in order to avoid overwriting half of
+ * the progress report line with the final output.
+ */
+ if (show_progress && isatty(fileno(stderr)))
+ fprintf(stderr, "\n");
+ show_progress = !show_progress;
+ }
+}
+
+/*
+ * Report current progress status. Parts borrowed from
+ * PostgreSQLs' src/bin/pg_basebackup.c
+ */
+static void
+report_progress(bool force)
+{
+ instr_time now;
+ double elapsed;
+ double total_percent;
+ double current_speed;
+
+ char totalstr[32];
+ char currentstr[32];
+
+ INSTR_TIME_SET_CURRENT(now);
+
+ /* Make sure we report at most once every 250 milliseconds */
+ if ((INSTR_TIME_GET_MILLISEC(now) -
+ INSTR_TIME_GET_MILLISEC(last_progress_update) < 250) && !force)
+ return;
+
+ /* Save current time */
+ last_progress_update = now;
+
+ /* Elapsed time in milliseconds since start of scan */
+ elapsed = (INSTR_TIME_GET_MILLISEC(now) -
+ INSTR_TIME_GET_MILLISEC(scan_started));
+
+ /* Adjust total size if current_size is larger */
+ if (current_size > total_size)
+ total_size = current_size;
+
+ /* Calculate current percent done */
+ total_percent = total_size ? 100.0 * current_size / total_size : 0.0;
+
+#define MEGABYTES (1024 * 1024)
+
+ /* Calculate current speed */
+ current_speed = (current_size / MEGABYTES) / (elapsed / 1000);
+
+ snprintf(totalstr, sizeof(totalstr), INT64_FORMAT,
+ total_size / MEGABYTES);
+ snprintf(currentstr, sizeof(currentstr), INT64_FORMAT,
+ current_size / MEGABYTES);
+
+ /*
+ * Print five blanks at the end so the end of previous lines which were
+ * longer don't remain partly visible.
+ */
+ fprintf(stderr, "%s/%s MB (%d%%, %.0f MB/s)%5s",
+ currentstr, totalstr, (int)total_percent, current_speed, "");
+
+ /* Stay on the same line if reporting to a terminal */
+ fprintf(stderr, isatty(fileno(stderr)) ? "\r" : "\n");
+}
+
static bool
skipfile(const char *fn)
{
@@ -153,6 +240,7 @@ scan_file(const char *fn, BlockNumber segmentno)
continue;
csum = pg_checksum_page(buf.data, blockno + segmentno * RELSEG_SIZE);
+ current_size += r;
if (mode == PG_MODE_CHECK)
{
if (csum != header->pd_checksum)
@@ -183,6 +271,10 @@ scan_file(const char *fn, BlockNumber segmentno)
exit(1);
}
}
+
+ /* Call report_progress() every 100 blocks */
+ if ((blockno % 100 == 0) && show_progress)
+ report_progress(false);
}
if (verbose)
@@ -195,12 +287,17 @@ scan_file(const char *fn, BlockNumber segmentno)
_("%s: checksums enabled in file \"%s\"\n"), progname, fn);
}
+ /* Make sure progress is reported at least once per file */
+ if (show_progress)
+ report_progress(false);
+
close(f);
}
-static void
-scan_directory(const char *basedir, const char *subdir)
+static int64
+scan_directory(const char *basedir, const char *subdir, bool sizeonly)
{
+ int64 dirsize = 0;
char path[MAXPGPATH];
DIR *dir;
struct dirent *de;
@@ -279,16 +376,20 @@ scan_directory(const char *basedir, const char *subdir)
/* Relfilenode not to be included */
continue;
- scan_file(fn, segmentno);
+ dirsize += st.st_size;
+
+ if (!sizeonly)
+ scan_file(fn, segmentno);
}
#ifndef WIN32
else if (S_ISDIR(st.st_mode) || S_ISLNK(st.st_mode))
#else
else if (S_ISDIR(st.st_mode) || pgwin32_is_junction(fn))
#endif
- scan_directory(path, de->d_name);
+ dirsize += scan_directory(path, de->d_name, sizeonly);
}
closedir(dir);
+ return dirsize;
}
int
@@ -300,6 +401,7 @@ main(int argc, char *argv[])
{"disable", no_argument, NULL, 'd'},
{"enable", no_argument, NULL, 'e'},
{"no-sync", no_argument, NULL, 'N'},
+ {"progress", no_argument, NULL, 'P'},
{"verbose", no_argument, NULL, 'v'},
{NULL, 0, NULL, 0}
};
@@ -327,7 +429,7 @@ main(int argc, char *argv[])
}
}
- while ((c = getopt_long(argc, argv, "cD:deNr:v", long_options, &option_index)) != -1)
+ while ((c = getopt_long(argc, argv, "cD:deNPr:v", long_options, &option_index)) != -1)
{
switch (c)
{
@@ -357,6 +459,9 @@ main(int argc, char *argv[])
}
only_relfilenode = pstrdup(optarg);
break;
+ case 'P':
+ show_progress = true;
+ break;
default:
fprintf(stderr, _("Try \"%s --help\" for more information.\n"), progname);
exit(1);
@@ -436,6 +541,29 @@ main(int argc, char *argv[])
exit(1);
}
+#ifndef WIN32
+ /*
+ * Assign SIGUSR1 signal handler to toggle progress status information
+ */
+ pqsignal(SIGUSR1, toggle_progress_report);
+#endif
+
+ /*
+ * As progress status information may be requested even after start of
+ * operation, we need to scan the directory tree(s) twice, once to get
+ * the idea how much data we need to scan and finally to do the real
+ * legwork.
+ */
+ total_size = scan_directory(DataDir, "global", true);
+ total_size += scan_directory(DataDir, "base", true);
+ total_size += scan_directory(DataDir, "pg_tblspc", true);
+
+ /*
+ * Remember start time. Required to calculate the current speed in
+ * report_progress()
+ */
+ INSTR_TIME_SET_CURRENT(scan_started);
+
if (ControlFile->data_checksum_version == 0 &&
mode == PG_MODE_DISABLE)
{
@@ -453,9 +581,20 @@ main(int argc, char *argv[])
/* Operate on all files if checking or enabling checksums */
if (mode == PG_MODE_CHECK || mode == PG_MODE_ENABLE)
{
- scan_directory(DataDir, "global");
- scan_directory(DataDir, "base");
- scan_directory(DataDir, "pg_tblspc");
+ scan_directory(DataDir, "global", false);
+ scan_directory(DataDir, "base", false);
+ scan_directory(DataDir, "pg_tblspc", false);
+
+ /*
+ * Done. Move to next line in case progress information was shown.
+ * Otherwise we clutter the summary output.
+ */
+ if (show_progress)
+ {
+ report_progress(true);
+ if (isatty(fileno(stderr)))
+ fprintf(stderr, "\n");
+ }
printf(_("Checksum operation completed\n"));
printf(_("Files scanned: %s\n"), psprintf(INT64_FORMAT, files));
pg_checksums_progress_eta_V0.patchtext/x-patch; charset=UTF-8; name=pg_checksums_progress_eta_V0.patchDownload
diff --git a/src/bin/pg_checksums/pg_checksums.c b/src/bin/pg_checksums/pg_checksums.c
index a439da231f..1d82b92bf0 100644
--- a/src/bin/pg_checksums/pg_checksums.c
+++ b/src/bin/pg_checksums/pg_checksums.c
@@ -136,6 +136,7 @@ report_progress(bool force)
{
instr_time now;
double elapsed;
+ double remaining;
double total_percent;
double current_speed;
@@ -163,6 +164,9 @@ report_progress(bool force)
/* Calculate current percent done */
total_percent = total_size ? 100.0 * current_size / total_size : 0.0;
+ /* Calculate remaining time in milliseconds */
+ remaining = elapsed * (100 / total_percent) - elapsed;
+
#define MEGABYTES (1024 * 1024)
/* Calculate current speed */
@@ -173,8 +177,13 @@ report_progress(bool force)
snprintf(currentstr, sizeof(currentstr), INT64_FORMAT,
current_size / MEGABYTES);
- fprintf(stderr, "%s/%s MB (%d%%, %.0f MB/s)",
- currentstr, totalstr, (int)total_percent, current_speed);
+ /* Only print remainging time after five seconds */
+ if (elapsed / 1000 < 5)
+ fprintf(stderr, "%s/%s MB (%d%%, %.0f MB/s)%20s",
+ currentstr, totalstr, (int)total_percent, current_speed, "");
+ else
+ fprintf(stderr, "%s/%s MB (%d%%, %.0f MB/s, %.0f s remaining)%20s",
+ currentstr, totalstr, (int)total_percent, current_speed, remaining / 1000, "");
/* Stay on the same line if reporting to a terminal */
fprintf(stderr, isatty(fileno(stderr)) ? "\r" : "\n");
I still think that the speed should compute a double to avoid integer
rounding errors within the computation. ISTM that rounding should only be
done for display in the end.Ok, also done this way. I decided to print only full MB and not any
further digits as those don't seem very relevant.
For the computation to be in double, you must convert to double somewhere
in the formula, otherwise it is computed as ints and only cast as a double
afterwards. Maybe:
current_speed = (1000.0 / MEGABYTES) * current_size / elapsed;
Or anything which converts to double early.
Instead of casting percent to int, maybe use "%.0f" as well, just like
current_speed?
+ if ((blockno % 100 == 0) && show_progress)
I'd invert the condition to avoid a modulo when not needed.
I'm not very found of global variables, but it does not matter here and
doing it differently would involve more code. It would matter though if
the progress report would be shared between commands.
I'm okay with calling the report on each file even if this means every few
GB...For my I/O throttling branch I've backed off to only call the report
every 100 blocks (and at the end of the file). I've added that logic to
this patch as well.
The 100 blocks = 800 KiB looks arbitrary. What is the rational? ISTM that
postgres will tend to store a power of two number of pages on full
segments, so maybe there could be a rational for 1024. Not sure.
Someone suggested ETA, and it seems rather simple to do. What about
adding it?I don't know, just computing the ETA is easy of course. But you'd have
to fiddle with seconds/minutes/hours conversions cause the runtime could
be hours. Then, you don't want to have it bumping around weirdly when
your I/O rate changes, cf. https://xkcd.com/612/ That doesn't sounds
simpler than the signal trigger we might get rid of.
Ok, it is more complicated that it looks for large sizes if second is not
the right display unit.
--
Fabien.
Hi,
Am Mittwoch, den 27.03.2019, 21:31 +0100 schrieb Fabien COELHO:
I still think that the speed should compute a double to avoid integer
rounding errors within the computation. ISTM that rounding should only be
done for display in the end.Ok, also done this way. I decided to print only full MB and not any
further digits as those don't seem very relevant.For the computation to be in double, you must convert to double somewhere
in the formula, otherwise it is computed as ints and only cast as a double
afterwards. Maybe:current_speed = (1000.0 / MEGABYTES) * current_size / elapsed;
Or anything which converts to double early.
Are you sure, seeing elapsed is a double already? If I print out two
additional fractional digits for current_speed, I get two non-zero
numbers, are those garbage?
Anyway, I've done that now as it doesn't hurt.
Instead of casting percent to int, maybe use "%.0f" as well, just like
current_speed?
Ok.
+ if ((blockno % 100 == 0) && show_progress)
I'd invert the condition to avoid a modulo when not needed.
Ok.
I'm okay with calling the report on each file even if this means every few
GB...For my I/O throttling branch I've backed off to only call the report
every 100 blocks (and at the end of the file). I've added that logic to
this patch as well.The 100 blocks = 800 KiB looks arbitrary. What is the rational? ISTM that
postgres will tend to store a power of two number of pages on full
segments, so maybe there could be a rational for 1024. Not sure.
It was arbitrary. I've changed it to 1024 now which looks a bit better.
Someone suggested ETA, and it seems rather simple to do. What about
adding it?I don't know, just computing the ETA is easy of course. But you'd have
to fiddle with seconds/minutes/hours conversions cause the runtime could
be hours. Then, you don't want to have it bumping around weirdly when
your I/O rate changes, cf. https://xkcd.com/612/ That doesn't sounds
simpler than the signal trigger we might get rid of.Ok, it is more complicated that it looks for large sizes if second is not
the right display unit.
Right, new version attached.
Michael
--
Michael Banck
Projektleiter / Senior Berater
Tel.: +49 2166 9901-171
Fax: +49 2166 9901-100
Email: michael.banck@credativ.de
credativ GmbH, HRB Mönchengladbach 12080
USt-ID-Nummer: DE204566209
Trompeterallee 108, 41189 Mönchengladbach
Geschäftsführung: Dr. Michael Meskes, Jörg Folz, Sascha Heuer
Unser Umgang mit personenbezogenen Daten unterliegt
folgenden Bestimmungen: https://www.credativ.de/datenschutz
Attachments:
pg_verify_checksums_progress_V14.patchtext/x-patch; charset=UTF-8; name=pg_verify_checksums_progress_V14.patchDownload
diff --git a/doc/src/sgml/ref/pg_checksums.sgml b/doc/src/sgml/ref/pg_checksums.sgml
index 1f4d4ab8b4..d18072a067 100644
--- a/doc/src/sgml/ref/pg_checksums.sgml
+++ b/doc/src/sgml/ref/pg_checksums.sgml
@@ -136,6 +136,19 @@ PostgreSQL documentation
</varlistentry>
<varlistentry>
+ <term><option>-P</option></term>
+ <term><option>--progress</option></term>
+ <listitem>
+ <para>
+ Enable progress reporting. Turning this on will deliver an approximate
+ progress report during the checksum verification. Sending the
+ <systemitem>SIGUSR1</systemitem> signal will toggle progress reporting
+ on or off during the verification run (not available on Windows).
+ </para>
+ </listitem>
+ </varlistentry>
+
+ <varlistentry>
<term><option>-V</option></term>
<term><option>--version</option></term>
<listitem>
diff --git a/src/bin/pg_checksums/pg_checksums.c b/src/bin/pg_checksums/pg_checksums.c
index 5c185ebed8..6fb7bc0b64 100644
--- a/src/bin/pg_checksums/pg_checksums.c
+++ b/src/bin/pg_checksums/pg_checksums.c
@@ -15,6 +15,8 @@
#include "postgres_fe.h"
#include <dirent.h>
+#include <signal.h>
+#include <time.h>
#include <sys/stat.h>
#include <unistd.h>
@@ -24,6 +26,7 @@
#include "common/file_utils.h"
#include "getopt_long.h"
#include "pg_getopt.h"
+#include "portability/instr_time.h"
#include "storage/bufpage.h"
#include "storage/checksum.h"
#include "storage/checksum_impl.h"
@@ -37,6 +40,7 @@ static ControlFileData *ControlFile;
static char *only_relfilenode = NULL;
static bool do_sync = true;
static bool verbose = false;
+static bool show_progress = false;
typedef enum
{
@@ -59,6 +63,14 @@ static PgChecksumMode mode = PG_MODE_CHECK;
static const char *progname;
+/*
+ * Progress status information.
+ */
+int64 total_size = 0;
+int64 current_size = 0;
+instr_time last_progress_update;
+instr_time scan_started;
+
static void
usage(void)
{
@@ -73,6 +85,7 @@ usage(void)
printf(_(" -N, --no-sync do not wait for changes to be written safely to disk\n"));
printf(_(" -v, --verbose output verbose messages\n"));
printf(_(" -r RELFILENODE check only relation with specified relfilenode\n"));
+ printf(_(" -P, --progress show progress information\n"));
printf(_(" -V, --version output version information, then exit\n"));
printf(_(" -?, --help show this help, then exit\n"));
printf(_("\nIf no data directory (DATADIR) is specified, "
@@ -97,6 +110,83 @@ static const char *const skip[] = {
NULL,
};
+static void
+toggle_progress_report(int signum)
+{
+
+ /* we handle SIGUSR1 only, and toggle the value of show_progress */
+ if (signum == SIGUSR1)
+ {
+ /*
+ * Skip to the next line in order to avoid overwriting half of
+ * the progress report line with the final output.
+ */
+ if (show_progress && isatty(fileno(stderr)))
+ fprintf(stderr, "\n");
+ show_progress = !show_progress;
+ }
+}
+
+/*
+ * Report current progress status. Parts borrowed from
+ * PostgreSQLs' src/bin/pg_basebackup.c
+ */
+static void
+report_progress(bool force)
+{
+ instr_time now;
+ double elapsed;
+ double total_percent;
+ double current_speed;
+
+ char totalstr[32];
+ char currentstr[32];
+
+ INSTR_TIME_SET_CURRENT(now);
+
+ /* Make sure we report at most once every 250 milliseconds */
+ if ((INSTR_TIME_GET_MILLISEC(now) -
+ INSTR_TIME_GET_MILLISEC(last_progress_update) < 250) && !force)
+ return;
+
+ /* Save current time */
+ last_progress_update = now;
+
+ /* Elapsed time in milliseconds since start of scan */
+ elapsed = (INSTR_TIME_GET_MILLISEC(now) -
+ INSTR_TIME_GET_MILLISEC(scan_started));
+
+ /* Adjust total size if current_size is larger */
+ if (current_size > total_size)
+ total_size = current_size;
+
+ /* Calculate current percent done */
+ total_percent = total_size ? 100.0 * current_size / total_size : 0.0;
+
+#define MEGABYTES (1024 * 1024)
+
+ /*
+ * Calculate current speed, converting current_size from bytes to
+ * megabytes and elapsed from milliseconds to seconds.
+ */
+ current_speed = (current_size / MEGABYTES) / (elapsed / 1000.0);
+
+ snprintf(totalstr, sizeof(totalstr), INT64_FORMAT,
+ total_size / MEGABYTES);
+ snprintf(currentstr, sizeof(currentstr), INT64_FORMAT,
+ current_size / MEGABYTES);
+
+ /*
+ * Print five blanks at the end so the end of previous lines which were
+ * longer don't remain partly visible.
+ */
+ fprintf(stderr, "%s/%s MB (%.0f%%, %.0f MB/s)%5s",
+ currentstr, totalstr, total_percent, current_speed, "");
+
+ /* Stay on the same line if reporting to a terminal */
+ fprintf(stderr, isatty(fileno(stderr)) ? "\r" : "\n");
+}
+
static bool
skipfile(const char *fn)
{
@@ -153,6 +243,7 @@ scan_file(const char *fn, BlockNumber segmentno)
continue;
csum = pg_checksum_page(buf.data, blockno + segmentno * RELSEG_SIZE);
+ current_size += r;
if (mode == PG_MODE_CHECK)
{
if (csum != header->pd_checksum)
@@ -183,6 +274,13 @@ scan_file(const char *fn, BlockNumber segmentno)
exit(1);
}
}
+
+ /*
+ * Call report_progress() every 1024 blocks if progress
+ * reporting is requested.
+ */
+ if (show_progress && (blockno % 1024 == 0))
+ report_progress(false);
}
if (verbose)
@@ -195,12 +293,17 @@ scan_file(const char *fn, BlockNumber segmentno)
_("%s: checksums enabled in file \"%s\"\n"), progname, fn);
}
+ /* Make sure progress is reported at least once per file */
+ if (show_progress)
+ report_progress(false);
+
close(f);
}
-static void
-scan_directory(const char *basedir, const char *subdir)
+static int64
+scan_directory(const char *basedir, const char *subdir, bool sizeonly)
{
+ int64 dirsize = 0;
char path[MAXPGPATH];
DIR *dir;
struct dirent *de;
@@ -279,16 +382,20 @@ scan_directory(const char *basedir, const char *subdir)
/* Relfilenode not to be included */
continue;
- scan_file(fn, segmentno);
+ dirsize += st.st_size;
+
+ if (!sizeonly)
+ scan_file(fn, segmentno);
}
#ifndef WIN32
else if (S_ISDIR(st.st_mode) || S_ISLNK(st.st_mode))
#else
else if (S_ISDIR(st.st_mode) || pgwin32_is_junction(fn))
#endif
- scan_directory(path, de->d_name);
+ dirsize += scan_directory(path, de->d_name, sizeonly);
}
closedir(dir);
+ return dirsize;
}
int
@@ -300,6 +407,7 @@ main(int argc, char *argv[])
{"disable", no_argument, NULL, 'd'},
{"enable", no_argument, NULL, 'e'},
{"no-sync", no_argument, NULL, 'N'},
+ {"progress", no_argument, NULL, 'P'},
{"verbose", no_argument, NULL, 'v'},
{NULL, 0, NULL, 0}
};
@@ -327,7 +435,7 @@ main(int argc, char *argv[])
}
}
- while ((c = getopt_long(argc, argv, "cD:deNr:v", long_options, &option_index)) != -1)
+ while ((c = getopt_long(argc, argv, "cD:deNPr:v", long_options, &option_index)) != -1)
{
switch (c)
{
@@ -357,6 +465,9 @@ main(int argc, char *argv[])
}
only_relfilenode = pstrdup(optarg);
break;
+ case 'P':
+ show_progress = true;
+ break;
default:
fprintf(stderr, _("Try \"%s --help\" for more information.\n"), progname);
exit(1);
@@ -436,6 +547,29 @@ main(int argc, char *argv[])
exit(1);
}
+#ifndef WIN32
+ /*
+ * Assign SIGUSR1 signal handler to toggle progress status information
+ */
+ pqsignal(SIGUSR1, toggle_progress_report);
+#endif
+
+ /*
+ * As progress status information may be requested even after start of
+ * operation, we need to scan the directory tree(s) twice, once to get
+ * the idea how much data we need to scan and finally to do the real
+ * legwork.
+ */
+ total_size = scan_directory(DataDir, "global", true);
+ total_size += scan_directory(DataDir, "base", true);
+ total_size += scan_directory(DataDir, "pg_tblspc", true);
+
+ /*
+ * Remember start time. Required to calculate the current speed in
+ * report_progress()
+ */
+ INSTR_TIME_SET_CURRENT(scan_started);
+
if (ControlFile->data_checksum_version == 0 &&
mode == PG_MODE_DISABLE)
{
@@ -453,9 +587,20 @@ main(int argc, char *argv[])
/* Operate on all files if checking or enabling checksums */
if (mode == PG_MODE_CHECK || mode == PG_MODE_ENABLE)
{
- scan_directory(DataDir, "global");
- scan_directory(DataDir, "base");
- scan_directory(DataDir, "pg_tblspc");
+ scan_directory(DataDir, "global", false);
+ scan_directory(DataDir, "base", false);
+ scan_directory(DataDir, "pg_tblspc", false);
+
+ /*
+ * Done. Move to next line in case progress information was shown.
+ * Otherwise we clutter the summary output.
+ */
+ if (show_progress)
+ {
+ report_progress(true);
+ if (isatty(fileno(stderr)))
+ fprintf(stderr, "\n");
+ }
printf(_("Checksum operation completed\n"));
printf(_("Files scanned: %s\n"), psprintf(INT64_FORMAT, files));
Hallo Michael,
Or anything which converts to double early.
Are you sure, seeing elapsed is a double already?
Argh, I missed that. You are right that a double elapsed is enough for the
second part. However, with
+ current_speed = (current_size / MEGABYTES) / (elapsed / 1000.0);
the first division is an integer one because both operands are ints, so
megabytes conversion is rounded down. I'd suggest:
+ current_speed = ((double) current_size / MEGABYTES) / (elapsed / 1000);
If I print out two additional fractional digits for current_speed, I get
two non-zero numbers, are those garbage?
Nope, they come from the other divisions, but the fist division will have
wipped out 0.5 MiB on average.
Ok, it is more complicated that it looks for large sizes if second is not
the right display unit.Right, new version attached.
Applies, compiles, global & local "make check" (although not tested) ok,
doc build ok, manual tests ok.
Otherwise a very minor comment: I'd invert !force and the computations in
the return condition to avoid the computations when not needed.
--
Fabien.
Hi,
Am Donnerstag, den 28.03.2019, 09:41 +0100 schrieb Fabien COELHO:
Hallo Michael,
Or anything which converts to double early.
Are you sure, seeing elapsed is a double already?
Argh, I missed that. You are right that a double elapsed is enough for the
second part. However, with+ current_speed = (current_size / MEGABYTES) / (elapsed / 1000.0);
the first division is an integer one because both operands are ints, so
megabytes conversion is rounded down. I'd suggest:+ current_speed = ((double) current_size / MEGABYTES) / (elapsed / 1000);
Ok.
Ok, it is more complicated that it looks for large sizes if second is not
the right display unit.Right, new version attached.
Applies, compiles, global & local "make check" (although not tested) ok,
doc build ok, manual tests ok.
Thanks.
Otherwise a very minor comment: I'd invert !force and the computations in
the return condition to avoid the computations when not needed.
The force is only ever true right at the end of the program so it would
not save anything really and detract from the main gist of that
statement, so I left it as-is.
Michael
--
Michael Banck
Projektleiter / Senior Berater
Tel.: +49 2166 9901-171
Fax: +49 2166 9901-100
Email: michael.banck@credativ.de
credativ GmbH, HRB Mönchengladbach 12080
USt-ID-Nummer: DE204566209
Trompeterallee 108, 41189 Mönchengladbach
Geschäftsführung: Dr. Michael Meskes, Jörg Folz, Sascha Heuer
Unser Umgang mit personenbezogenen Daten unterliegt
folgenden Bestimmungen: https://www.credativ.de/datenschutz
Attachments:
pg_verify_checksums_progress_V15.patchtext/x-patch; charset=UTF-8; name=pg_verify_checksums_progress_V15.patchDownload
diff --git a/doc/src/sgml/ref/pg_checksums.sgml b/doc/src/sgml/ref/pg_checksums.sgml
index 1f4d4ab8b4..d18072a067 100644
--- a/doc/src/sgml/ref/pg_checksums.sgml
+++ b/doc/src/sgml/ref/pg_checksums.sgml
@@ -136,6 +136,19 @@ PostgreSQL documentation
</varlistentry>
<varlistentry>
+ <term><option>-P</option></term>
+ <term><option>--progress</option></term>
+ <listitem>
+ <para>
+ Enable progress reporting. Turning this on will deliver an approximate
+ progress report during the checksum verification. Sending the
+ <systemitem>SIGUSR1</systemitem> signal will toggle progress reporting
+ on or off during the verification run (not available on Windows).
+ </para>
+ </listitem>
+ </varlistentry>
+
+ <varlistentry>
<term><option>-V</option></term>
<term><option>--version</option></term>
<listitem>
diff --git a/src/bin/pg_checksums/pg_checksums.c b/src/bin/pg_checksums/pg_checksums.c
index 5c185ebed8..bc7f29c3c4 100644
--- a/src/bin/pg_checksums/pg_checksums.c
+++ b/src/bin/pg_checksums/pg_checksums.c
@@ -15,6 +15,8 @@
#include "postgres_fe.h"
#include <dirent.h>
+#include <signal.h>
+#include <time.h>
#include <sys/stat.h>
#include <unistd.h>
@@ -24,6 +26,7 @@
#include "common/file_utils.h"
#include "getopt_long.h"
#include "pg_getopt.h"
+#include "portability/instr_time.h"
#include "storage/bufpage.h"
#include "storage/checksum.h"
#include "storage/checksum_impl.h"
@@ -37,6 +40,7 @@ static ControlFileData *ControlFile;
static char *only_relfilenode = NULL;
static bool do_sync = true;
static bool verbose = false;
+static bool show_progress = false;
typedef enum
{
@@ -59,6 +63,14 @@ static PgChecksumMode mode = PG_MODE_CHECK;
static const char *progname;
+/*
+ * Progress status information.
+ */
+int64 total_size = 0;
+int64 current_size = 0;
+instr_time last_progress_update;
+instr_time scan_started;
+
static void
usage(void)
{
@@ -73,6 +85,7 @@ usage(void)
printf(_(" -N, --no-sync do not wait for changes to be written safely to disk\n"));
printf(_(" -v, --verbose output verbose messages\n"));
printf(_(" -r RELFILENODE check only relation with specified relfilenode\n"));
+ printf(_(" -P, --progress show progress information\n"));
printf(_(" -V, --version output version information, then exit\n"));
printf(_(" -?, --help show this help, then exit\n"));
printf(_("\nIf no data directory (DATADIR) is specified, "
@@ -97,6 +110,83 @@ static const char *const skip[] = {
NULL,
};
+static void
+toggle_progress_report(int signum)
+{
+
+ /* we handle SIGUSR1 only, and toggle the value of show_progress */
+ if (signum == SIGUSR1)
+ {
+ /*
+ * Skip to the next line in order to avoid overwriting half of
+ * the progress report line with the final output.
+ */
+ if (show_progress && isatty(fileno(stderr)))
+ fprintf(stderr, "\n");
+ show_progress = !show_progress;
+ }
+}
+
+/*
+ * Report current progress status. Parts borrowed from
+ * PostgreSQLs' src/bin/pg_basebackup.c
+ */
+static void
+report_progress(bool force)
+{
+ instr_time now;
+ double elapsed;
+ double total_percent;
+ double current_speed;
+
+ char totalstr[32];
+ char currentstr[32];
+
+ INSTR_TIME_SET_CURRENT(now);
+
+ /* Make sure we report at most once every 250 milliseconds */
+ if ((INSTR_TIME_GET_MILLISEC(now) -
+ INSTR_TIME_GET_MILLISEC(last_progress_update) < 250) && !force)
+ return;
+
+ /* Save current time */
+ last_progress_update = now;
+
+ /* Elapsed time in milliseconds since start of scan */
+ elapsed = (INSTR_TIME_GET_MILLISEC(now) -
+ INSTR_TIME_GET_MILLISEC(scan_started));
+
+ /* Adjust total size if current_size is larger */
+ if (current_size > total_size)
+ total_size = current_size;
+
+ /* Calculate current percent done */
+ total_percent = total_size ? 100.0 * current_size / total_size : 0.0;
+
+#define MEGABYTES (1024 * 1024)
+
+ /*
+ * Calculate current speed, converting current_size from bytes to
+ * megabytes and elapsed from milliseconds to seconds.
+ */
+ current_speed = ((double) current_size / MEGABYTES) / (elapsed / 1000);
+
+ snprintf(totalstr, sizeof(totalstr), INT64_FORMAT,
+ total_size / MEGABYTES);
+ snprintf(currentstr, sizeof(currentstr), INT64_FORMAT,
+ current_size / MEGABYTES);
+
+ /*
+ * Print five blanks at the end so the end of previous lines which were
+ * longer don't remain partly visible.
+ */
+ fprintf(stderr, "%s/%s MB (%.0f%%, %.0f MB/s)%5s",
+ currentstr, totalstr, total_percent, current_speed, "");
+
+ /* Stay on the same line if reporting to a terminal */
+ fprintf(stderr, isatty(fileno(stderr)) ? "\r" : "\n");
+}
+
static bool
skipfile(const char *fn)
{
@@ -153,6 +243,7 @@ scan_file(const char *fn, BlockNumber segmentno)
continue;
csum = pg_checksum_page(buf.data, blockno + segmentno * RELSEG_SIZE);
+ current_size += r;
if (mode == PG_MODE_CHECK)
{
if (csum != header->pd_checksum)
@@ -183,6 +274,13 @@ scan_file(const char *fn, BlockNumber segmentno)
exit(1);
}
}
+
+ /*
+ * Call report_progress() every 1024 blocks if progress
+ * reporting is requested.
+ */
+ if (show_progress && (blockno % 1024 == 0))
+ report_progress(false);
}
if (verbose)
@@ -195,12 +293,17 @@ scan_file(const char *fn, BlockNumber segmentno)
_("%s: checksums enabled in file \"%s\"\n"), progname, fn);
}
+ /* Make sure progress is reported at least once per file */
+ if (show_progress)
+ report_progress(false);
+
close(f);
}
-static void
-scan_directory(const char *basedir, const char *subdir)
+static int64
+scan_directory(const char *basedir, const char *subdir, bool sizeonly)
{
+ int64 dirsize = 0;
char path[MAXPGPATH];
DIR *dir;
struct dirent *de;
@@ -279,16 +382,20 @@ scan_directory(const char *basedir, const char *subdir)
/* Relfilenode not to be included */
continue;
- scan_file(fn, segmentno);
+ dirsize += st.st_size;
+
+ if (!sizeonly)
+ scan_file(fn, segmentno);
}
#ifndef WIN32
else if (S_ISDIR(st.st_mode) || S_ISLNK(st.st_mode))
#else
else if (S_ISDIR(st.st_mode) || pgwin32_is_junction(fn))
#endif
- scan_directory(path, de->d_name);
+ dirsize += scan_directory(path, de->d_name, sizeonly);
}
closedir(dir);
+ return dirsize;
}
int
@@ -300,6 +407,7 @@ main(int argc, char *argv[])
{"disable", no_argument, NULL, 'd'},
{"enable", no_argument, NULL, 'e'},
{"no-sync", no_argument, NULL, 'N'},
+ {"progress", no_argument, NULL, 'P'},
{"verbose", no_argument, NULL, 'v'},
{NULL, 0, NULL, 0}
};
@@ -327,7 +435,7 @@ main(int argc, char *argv[])
}
}
- while ((c = getopt_long(argc, argv, "cD:deNr:v", long_options, &option_index)) != -1)
+ while ((c = getopt_long(argc, argv, "cD:deNPr:v", long_options, &option_index)) != -1)
{
switch (c)
{
@@ -357,6 +465,9 @@ main(int argc, char *argv[])
}
only_relfilenode = pstrdup(optarg);
break;
+ case 'P':
+ show_progress = true;
+ break;
default:
fprintf(stderr, _("Try \"%s --help\" for more information.\n"), progname);
exit(1);
@@ -436,6 +547,29 @@ main(int argc, char *argv[])
exit(1);
}
+#ifndef WIN32
+ /*
+ * Assign SIGUSR1 signal handler to toggle progress status information
+ */
+ pqsignal(SIGUSR1, toggle_progress_report);
+#endif
+
+ /*
+ * As progress status information may be requested even after start of
+ * operation, we need to scan the directory tree(s) twice, once to get
+ * the idea how much data we need to scan and finally to do the real
+ * legwork.
+ */
+ total_size = scan_directory(DataDir, "global", true);
+ total_size += scan_directory(DataDir, "base", true);
+ total_size += scan_directory(DataDir, "pg_tblspc", true);
+
+ /*
+ * Remember start time. Required to calculate the current speed in
+ * report_progress()
+ */
+ INSTR_TIME_SET_CURRENT(scan_started);
+
if (ControlFile->data_checksum_version == 0 &&
mode == PG_MODE_DISABLE)
{
@@ -453,9 +587,20 @@ main(int argc, char *argv[])
/* Operate on all files if checking or enabling checksums */
if (mode == PG_MODE_CHECK || mode == PG_MODE_ENABLE)
{
- scan_directory(DataDir, "global");
- scan_directory(DataDir, "base");
- scan_directory(DataDir, "pg_tblspc");
+ scan_directory(DataDir, "global", false);
+ scan_directory(DataDir, "base", false);
+ scan_directory(DataDir, "pg_tblspc", false);
+
+ /*
+ * Done. Move to next line in case progress information was shown.
+ * Otherwise we clutter the summary output.
+ */
+ if (show_progress)
+ {
+ report_progress(true);
+ if (isatty(fileno(stderr)))
+ fprintf(stderr, "\n");
+ }
printf(_("Checksum operation completed\n"));
printf(_("Files scanned: %s\n"), psprintf(INT64_FORMAT, files));
Otherwise a very minor comment: I'd invert !force and the computations in
the return condition to avoid the computations when not needed.The force is only ever true right at the end of the program so it would
not save anything really and detract from the main gist of that
statement, so I left it as-is.
Ok.
I marked it as ready, but I'd advise that you split it in (1) progress and
(2) signal toggling so that the first part is more likely to make it
before 12 freeze.
--
Fabien.
Hi,
Am Donnerstag, den 28.03.2019, 10:08 +0100 schrieb Fabien COELHO:
I marked it as ready,
Thanks!
but I'd advise that you split it in (1) progress and (2) signal
toggling so that the first part is more likely to make it before 12
freeze.
Ok, done so in the attached.
Michael
--
Michael Banck
Projektleiter / Senior Berater
Tel.: +49 2166 9901-171
Fax: +49 2166 9901-100
Email: michael.banck@credativ.de
credativ GmbH, HRB Mönchengladbach 12080
USt-ID-Nummer: DE204566209
Trompeterallee 108, 41189 Mönchengladbach
Geschäftsführung: Dr. Michael Meskes, Jörg Folz, Sascha Heuer
Unser Umgang mit personenbezogenen Daten unterliegt
folgenden Bestimmungen: https://www.credativ.de/datenschutz
Attachments:
0001-Add-progress-reporting-to-pg_checksums.patchtext/x-patch; charset=UTF-8; name=0001-Add-progress-reporting-to-pg_checksums.patchDownload
From 0fe29a3c36e1f2b03e8a890996788539c637a5f0 Mon Sep 17 00:00:00 2001
From: Michael Banck <mbanck@debian.org>
Date: Thu, 28 Mar 2019 10:47:54 +0100
Subject: [PATCH 1/2] Add progress reporting to pg_checksums.
This adds a new option -P/--progress that continuously reports the progress of
the operation, including percent done and rate (in MB/s).
Author: Bernd Helmle, Michael Banck
Reviewed-by: Fabien Coelho, Alvaro Herrera, Michael Paquier, Kyotaro HORIGUCHI
---
doc/src/sgml/ref/pg_checksums.sgml | 11 +++
src/bin/pg_checksums/pg_checksums.c | 138 +++++++++++++++++++++++++++++++++---
2 files changed, 141 insertions(+), 8 deletions(-)
diff --git a/doc/src/sgml/ref/pg_checksums.sgml b/doc/src/sgml/ref/pg_checksums.sgml
index 1f4d4ab8b4..0934426e58 100644
--- a/doc/src/sgml/ref/pg_checksums.sgml
+++ b/doc/src/sgml/ref/pg_checksums.sgml
@@ -136,6 +136,17 @@ PostgreSQL documentation
</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 checking or enabling checksums.
+ </para>
+ </listitem>
+ </varlistentry>
+
+ <varlistentry>
<term><option>-V</option></term>
<term><option>--version</option></term>
<listitem>
diff --git a/src/bin/pg_checksums/pg_checksums.c b/src/bin/pg_checksums/pg_checksums.c
index 5c185ebed8..d09cf8068d 100644
--- a/src/bin/pg_checksums/pg_checksums.c
+++ b/src/bin/pg_checksums/pg_checksums.c
@@ -15,6 +15,7 @@
#include "postgres_fe.h"
#include <dirent.h>
+#include <time.h>
#include <sys/stat.h>
#include <unistd.h>
@@ -24,6 +25,7 @@
#include "common/file_utils.h"
#include "getopt_long.h"
#include "pg_getopt.h"
+#include "portability/instr_time.h"
#include "storage/bufpage.h"
#include "storage/checksum.h"
#include "storage/checksum_impl.h"
@@ -37,6 +39,7 @@ static ControlFileData *ControlFile;
static char *only_relfilenode = NULL;
static bool do_sync = true;
static bool verbose = false;
+static bool show_progress = false;
typedef enum
{
@@ -59,6 +62,14 @@ static PgChecksumMode mode = PG_MODE_CHECK;
static const char *progname;
+/*
+ * Progress status information.
+ */
+int64 total_size = 0;
+int64 current_size = 0;
+instr_time last_progress_update;
+instr_time scan_started;
+
static void
usage(void)
{
@@ -73,6 +84,7 @@ usage(void)
printf(_(" -N, --no-sync do not wait for changes to be written safely to disk\n"));
printf(_(" -v, --verbose output verbose messages\n"));
printf(_(" -r RELFILENODE check only relation with specified relfilenode\n"));
+ printf(_(" -P, --progress show progress information\n"));
printf(_(" -V, --version output version information, then exit\n"));
printf(_(" -?, --help show this help, then exit\n"));
printf(_("\nIf no data directory (DATADIR) is specified, "
@@ -97,6 +109,66 @@ static const char *const skip[] = {
NULL,
};
+/*
+ * Report current progress status. Parts borrowed from
+ * PostgreSQLs' src/bin/pg_basebackup.c
+ */
+static void
+report_progress(bool force)
+{
+ instr_time now;
+ double elapsed;
+ double total_percent;
+ double current_speed;
+
+ char totalstr[32];
+ char currentstr[32];
+
+ INSTR_TIME_SET_CURRENT(now);
+
+ /* Make sure we report at most once every 250 milliseconds */
+ if ((INSTR_TIME_GET_MILLISEC(now) -
+ INSTR_TIME_GET_MILLISEC(last_progress_update) < 250) && !force)
+ return;
+
+ /* Save current time */
+ last_progress_update = now;
+
+ /* Elapsed time in milliseconds since start of scan */
+ elapsed = (INSTR_TIME_GET_MILLISEC(now) -
+ INSTR_TIME_GET_MILLISEC(scan_started));
+
+ /* Adjust total size if current_size is larger */
+ if (current_size > total_size)
+ total_size = current_size;
+
+ /* Calculate current percent done */
+ total_percent = total_size ? 100.0 * current_size / total_size : 0.0;
+
+#define MEGABYTES (1024 * 1024)
+
+ /*
+ * Calculate current speed, converting current_size from bytes to
+ * megabytes and elapsed from milliseconds to seconds.
+ */
+ current_speed = ((double) current_size / MEGABYTES) / (elapsed / 1000);
+
+ snprintf(totalstr, sizeof(totalstr), INT64_FORMAT,
+ total_size / MEGABYTES);
+ snprintf(currentstr, sizeof(currentstr), INT64_FORMAT,
+ current_size / MEGABYTES);
+
+ /*
+ * Print five blanks at the end so the end of previous lines which were
+ * longer don't remain partly visible.
+ */
+ fprintf(stderr, "%s/%s MB (%.0f%%, %.0f MB/s)%5s",
+ currentstr, totalstr, total_percent, current_speed, "");
+
+ /* Stay on the same line if reporting to a terminal */
+ fprintf(stderr, isatty(fileno(stderr)) ? "\r" : "\n");
+}
+
static bool
skipfile(const char *fn)
{
@@ -153,6 +225,7 @@ scan_file(const char *fn, BlockNumber segmentno)
continue;
csum = pg_checksum_page(buf.data, blockno + segmentno * RELSEG_SIZE);
+ current_size += r;
if (mode == PG_MODE_CHECK)
{
if (csum != header->pd_checksum)
@@ -183,6 +256,13 @@ scan_file(const char *fn, BlockNumber segmentno)
exit(1);
}
}
+
+ /*
+ * Call report_progress() every 1024 blocks if progress
+ * reporting is requested.
+ */
+ if (show_progress && (blockno % 1024 == 0))
+ report_progress(false);
}
if (verbose)
@@ -195,12 +275,17 @@ scan_file(const char *fn, BlockNumber segmentno)
_("%s: checksums enabled in file \"%s\"\n"), progname, fn);
}
+ /* Make sure progress is reported at least once per file */
+ if (show_progress)
+ report_progress(false);
+
close(f);
}
-static void
-scan_directory(const char *basedir, const char *subdir)
+static int64
+scan_directory(const char *basedir, const char *subdir, bool sizeonly)
{
+ int64 dirsize = 0;
char path[MAXPGPATH];
DIR *dir;
struct dirent *de;
@@ -279,16 +364,20 @@ scan_directory(const char *basedir, const char *subdir)
/* Relfilenode not to be included */
continue;
- scan_file(fn, segmentno);
+ dirsize += st.st_size;
+
+ if (!sizeonly)
+ scan_file(fn, segmentno);
}
#ifndef WIN32
else if (S_ISDIR(st.st_mode) || S_ISLNK(st.st_mode))
#else
else if (S_ISDIR(st.st_mode) || pgwin32_is_junction(fn))
#endif
- scan_directory(path, de->d_name);
+ dirsize += scan_directory(path, de->d_name, sizeonly);
}
closedir(dir);
+ return dirsize;
}
int
@@ -300,6 +389,7 @@ main(int argc, char *argv[])
{"disable", no_argument, NULL, 'd'},
{"enable", no_argument, NULL, 'e'},
{"no-sync", no_argument, NULL, 'N'},
+ {"progress", no_argument, NULL, 'P'},
{"verbose", no_argument, NULL, 'v'},
{NULL, 0, NULL, 0}
};
@@ -327,7 +417,7 @@ main(int argc, char *argv[])
}
}
- while ((c = getopt_long(argc, argv, "cD:deNr:v", long_options, &option_index)) != -1)
+ while ((c = getopt_long(argc, argv, "cD:deNPr:v", long_options, &option_index)) != -1)
{
switch (c)
{
@@ -357,6 +447,9 @@ main(int argc, char *argv[])
}
only_relfilenode = pstrdup(optarg);
break;
+ case 'P':
+ show_progress = true;
+ break;
default:
fprintf(stderr, _("Try \"%s --help\" for more information.\n"), progname);
exit(1);
@@ -436,6 +529,24 @@ main(int argc, char *argv[])
exit(1);
}
+ /*
+ * If progress status information is requested, we need to scan the
+ * directory tree(s) twice, once to get the idea how much data we need to
+ * scan and finally to do the real legwork.
+ */
+ if (show_progress)
+ {
+ total_size = scan_directory(DataDir, "global", true);
+ total_size += scan_directory(DataDir, "base", true);
+ total_size += scan_directory(DataDir, "pg_tblspc", true);
+
+ /*
+ * Remember start time. Required to calculate the current speed in
+ * report_progress().
+ */
+ INSTR_TIME_SET_CURRENT(scan_started);
+ }
+
if (ControlFile->data_checksum_version == 0 &&
mode == PG_MODE_DISABLE)
{
@@ -453,9 +564,20 @@ main(int argc, char *argv[])
/* Operate on all files if checking or enabling checksums */
if (mode == PG_MODE_CHECK || mode == PG_MODE_ENABLE)
{
- scan_directory(DataDir, "global");
- scan_directory(DataDir, "base");
- scan_directory(DataDir, "pg_tblspc");
+ scan_directory(DataDir, "global", false);
+ scan_directory(DataDir, "base", false);
+ scan_directory(DataDir, "pg_tblspc", false);
+
+ /*
+ * Done. Move to next line in case progress information was shown.
+ * Otherwise we clutter the summary output.
+ */
+ if (show_progress)
+ {
+ report_progress(true);
+ if (isatty(fileno(stderr)))
+ fprintf(stderr, "\n");
+ }
printf(_("Checksum operation completed\n"));
printf(_("Files scanned: %s\n"), psprintf(INT64_FORMAT, files));
--
2.11.0
0002-Support-toggling-of-pg_checksums-progress-reporting-.patchtext/x-patch; charset=UTF-8; name=0002-Support-toggling-of-pg_checksums-progress-reporting-.patchDownload
From 78d551ec7e5151dceee2d182a8f258fdc4b59554 Mon Sep 17 00:00:00 2001
From: Michael Banck <mbanck@debian.org>
Date: Thu, 28 Mar 2019 10:57:11 +0100
Subject: [PATCH 2/2] Support toggling of pg_checksums progress reporting via
SIGUSR1.
This feature does not work on Windows.
Author: Bernd Helmle, Michael Banck
Reviewed-by: Fabien Coelho, Alvaro Herrera, Michael Paquier, Kyotaro HORIGUCHI
---
doc/src/sgml/ref/pg_checksums.sgml | 4 ++-
src/bin/pg_checksums/pg_checksums.c | 51 +++++++++++++++++++++++++++----------
2 files changed, 40 insertions(+), 15 deletions(-)
diff --git a/doc/src/sgml/ref/pg_checksums.sgml b/doc/src/sgml/ref/pg_checksums.sgml
index 0934426e58..9f5cbc79ff 100644
--- a/doc/src/sgml/ref/pg_checksums.sgml
+++ b/doc/src/sgml/ref/pg_checksums.sgml
@@ -141,7 +141,9 @@ PostgreSQL documentation
<listitem>
<para>
Enable progress reporting. Turning this on will deliver a progress
- report while checking or enabling checksums.
+ report while checking or enabling checksums. Sending the
+ <systemitem>SIGUSR1</systemitem> signal will toggle progress reporting
+ on or off during the verification run (not available on Windows).
</para>
</listitem>
</varlistentry>
diff --git a/src/bin/pg_checksums/pg_checksums.c b/src/bin/pg_checksums/pg_checksums.c
index d09cf8068d..bc7f29c3c4 100644
--- a/src/bin/pg_checksums/pg_checksums.c
+++ b/src/bin/pg_checksums/pg_checksums.c
@@ -15,6 +15,7 @@
#include "postgres_fe.h"
#include <dirent.h>
+#include <signal.h>
#include <time.h>
#include <sys/stat.h>
#include <unistd.h>
@@ -109,6 +110,23 @@ static const char *const skip[] = {
NULL,
};
+static void
+toggle_progress_report(int signum)
+{
+
+ /* we handle SIGUSR1 only, and toggle the value of show_progress */
+ if (signum == SIGUSR1)
+ {
+ /*
+ * Skip to the next line in order to avoid overwriting half of
+ * the progress report line with the final output.
+ */
+ if (show_progress && isatty(fileno(stderr)))
+ fprintf(stderr, "\n");
+ show_progress = !show_progress;
+ }
+}
+
/*
* Report current progress status. Parts borrowed from
* PostgreSQLs' src/bin/pg_basebackup.c
@@ -529,23 +547,28 @@ main(int argc, char *argv[])
exit(1);
}
+#ifndef WIN32
/*
- * If progress status information is requested, we need to scan the
- * directory tree(s) twice, once to get the idea how much data we need to
- * scan and finally to do the real legwork.
+ * Assign SIGUSR1 signal handler to toggle progress status information
*/
- if (show_progress)
- {
- total_size = scan_directory(DataDir, "global", true);
- total_size += scan_directory(DataDir, "base", true);
- total_size += scan_directory(DataDir, "pg_tblspc", true);
+ pqsignal(SIGUSR1, toggle_progress_report);
+#endif
- /*
- * Remember start time. Required to calculate the current speed in
- * report_progress().
- */
- INSTR_TIME_SET_CURRENT(scan_started);
- }
+ /*
+ * As progress status information may be requested even after start of
+ * operation, we need to scan the directory tree(s) twice, once to get
+ * the idea how much data we need to scan and finally to do the real
+ * legwork.
+ */
+ total_size = scan_directory(DataDir, "global", true);
+ total_size += scan_directory(DataDir, "base", true);
+ total_size += scan_directory(DataDir, "pg_tblspc", true);
+
+ /*
+ * Remember start time. Required to calculate the current speed in
+ * report_progress()
+ */
+ INSTR_TIME_SET_CURRENT(scan_started);
if (ControlFile->data_checksum_version == 0 &&
mode == PG_MODE_DISABLE)
--
2.11.0
Hallo Michael,
but I'd advise that you split it in (1) progress and (2) signal
toggling so that the first part is more likely to make it before 12
freeze.Ok, done so in the attached.
Fine.
I think that it is good to show the overall impact of the signal stuff, in
particular the fact that the size must always be computed if the progress
may be activated.
Also, I'd suggest to add "volatile" on the show_progress variable in the
second patch.
--
Fabien.
On Thu, Mar 28, 2019 at 03:53:59PM +0100, Fabien COELHO wrote:
I think that it is good to show the overall impact of the signal stuff, in
particular the fact that the size must always be computed if the progress
may be activated.
Getting to know the total size and the current size are the two
important factors that matter when it comes to do progress reporting
in my opinion. I have read the patch, and I am not really convinced
by the need to show the progress report based on an interval of 250ms
as we talk about an operation which could take dozens of minutes. So
I have simplified the patch to only show a progress report every
second. This also removes the include for the time-related APIs from
portability/. A second thing is that I don't think that the speed is
much useful. I would expect the speed to be steady, still there is a
risk to show incorrect information if the speed of the operation is
spiky or irregular leading to an incorrect estimation of the remaining
time.
In short, I would like to commit the first patch as attached, which is
much more simple than what has been sent previously, still it provides
the progress information which is useful.
--
Michael
Attachments:
checksums-progress-report.patchtext/x-diff; charset=us-asciiDownload
diff --git a/doc/src/sgml/ref/pg_checksums.sgml b/doc/src/sgml/ref/pg_checksums.sgml
index 70339eaec9..29507ff98e 100644
--- a/doc/src/sgml/ref/pg_checksums.sgml
+++ b/doc/src/sgml/ref/pg_checksums.sgml
@@ -135,6 +135,17 @@ 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 checking or enabling checksums.
+ </para>
+ </listitem>
+ </varlistentry>
+
<varlistentry>
<term><option>-V</option></term>
<term><option>--version</option></term>
diff --git a/src/bin/pg_checksums/pg_checksums.c b/src/bin/pg_checksums/pg_checksums.c
index 5c185ebed8..f9903a441c 100644
--- a/src/bin/pg_checksums/pg_checksums.c
+++ b/src/bin/pg_checksums/pg_checksums.c
@@ -15,6 +15,7 @@
#include "postgres_fe.h"
#include <dirent.h>
+#include <time.h>
#include <sys/stat.h>
#include <unistd.h>
@@ -37,6 +38,7 @@ static ControlFileData *ControlFile;
static char *only_relfilenode = NULL;
static bool do_sync = true;
static bool verbose = false;
+static bool showprogress = false;
typedef enum
{
@@ -59,6 +61,13 @@ static PgChecksumMode mode = PG_MODE_CHECK;
static const char *progname;
+/*
+ * Progress status information.
+ */
+int64 total_size = 0;
+int64 current_size = 0;
+static pg_time_t last_progress_report = 0;
+
static void
usage(void)
{
@@ -71,6 +80,7 @@ usage(void)
printf(_(" -d, --disable disable data checksums\n"));
printf(_(" -e, --enable enable data checksums\n"));
printf(_(" -N, --no-sync do not wait for changes to be written safely to disk\n"));
+ printf(_(" -P, --progress show progress information\n"));
printf(_(" -v, --verbose output verbose messages\n"));
printf(_(" -r RELFILENODE check only relation with specified relfilenode\n"));
printf(_(" -V, --version output version information, then exit\n"));
@@ -97,6 +107,52 @@ static const char *const skip[] = {
NULL,
};
+/*
+ * Report current progress status. Parts borrowed from
+ * PostgreSQLs' src/bin/pg_basebackup.c
+ */
+static void
+progress_report(bool force)
+{
+ int percent;
+ char total_size_str[32];
+ char current_size_str[32];
+ pg_time_t now;
+
+ if (!showprogress)
+ return;
+
+ now = time(NULL);
+ if (now == last_progress_report && !force)
+ return; /* Max once per second */
+
+ /* Save current time */
+ last_progress_report = now;
+
+ /* Adjust total size if current_size is larger */
+ if (current_size > total_size)
+ total_size = current_size;
+
+ /* Calculate current percentage of size done */
+ percent = total_size ? (int) ((current_size) * 100 / total_size) : 0;
+
+ snprintf(total_size_str, sizeof(total_size_str), INT64_FORMAT,
+ total_size / (1024 * 1024));
+ snprintf(current_size_str, sizeof(current_size_str), INT64_FORMAT,
+ current_size / (1024 * 1024));
+
+ /*
+ * Separate step to keep platform-dependent format code out of
+ * translatable strings. And we only test for INT64_FORMAT availability
+ * in snprintf, not fprintf.
+ */
+ fprintf(stderr, "%*s/%s MB (%d%%) done",
+ (int) strlen(current_size_str), current_size_str, total_size_str,
+ percent);
+
+ fprintf(stderr, "\r");
+}
+
static bool
skipfile(const char *fn)
{
@@ -153,6 +209,7 @@ scan_file(const char *fn, BlockNumber segmentno)
continue;
csum = pg_checksum_page(buf.data, blockno + segmentno * RELSEG_SIZE);
+ current_size += r;
if (mode == PG_MODE_CHECK)
{
if (csum != header->pd_checksum)
@@ -183,6 +240,8 @@ scan_file(const char *fn, BlockNumber segmentno)
exit(1);
}
}
+
+ progress_report(false);
}
if (verbose)
@@ -195,12 +254,23 @@ scan_file(const char *fn, BlockNumber segmentno)
_("%s: checksums enabled in file \"%s\"\n"), progname, fn);
}
+ /* Make sure progress is reported at least once per file */
+ progress_report(false);
+
close(f);
}
-static void
-scan_directory(const char *basedir, const char *subdir)
+/*
+ * Scan the given directory for items which can be checksummed and
+ * operate on each one of them. If "sizeonly" is true, the size of
+ * all the items which have checksums is computed and returned back
+ * to the caller without operating on the files. This is used to compile
+ * the total size of the data directory for progress reports.
+ */
+static int64
+scan_directory(const char *basedir, const char *subdir, bool sizeonly)
{
+ int64 dirsize = 0;
char path[MAXPGPATH];
DIR *dir;
struct dirent *de;
@@ -279,16 +349,24 @@ scan_directory(const char *basedir, const char *subdir)
/* Relfilenode not to be included */
continue;
- scan_file(fn, segmentno);
+ dirsize += st.st_size;
+
+ /*
+ * No need to work on the file when calculating only the size
+ * of the items in the data folder.
+ */
+ if (!sizeonly)
+ scan_file(fn, segmentno);
}
#ifndef WIN32
else if (S_ISDIR(st.st_mode) || S_ISLNK(st.st_mode))
#else
else if (S_ISDIR(st.st_mode) || pgwin32_is_junction(fn))
#endif
- scan_directory(path, de->d_name);
+ dirsize += scan_directory(path, de->d_name, sizeonly);
}
closedir(dir);
+ return dirsize;
}
int
@@ -300,6 +378,7 @@ main(int argc, char *argv[])
{"disable", no_argument, NULL, 'd'},
{"enable", no_argument, NULL, 'e'},
{"no-sync", no_argument, NULL, 'N'},
+ {"progress", no_argument, NULL, 'P'},
{"verbose", no_argument, NULL, 'v'},
{NULL, 0, NULL, 0}
};
@@ -327,7 +406,7 @@ main(int argc, char *argv[])
}
}
- while ((c = getopt_long(argc, argv, "cD:deNr:v", long_options, &option_index)) != -1)
+ while ((c = getopt_long(argc, argv, "cD:deNPr:v", long_options, &option_index)) != -1)
{
switch (c)
{
@@ -357,6 +436,9 @@ main(int argc, char *argv[])
}
only_relfilenode = pstrdup(optarg);
break;
+ case 'P':
+ showprogress = true;
+ break;
default:
fprintf(stderr, _("Try \"%s --help\" for more information.\n"), progname);
exit(1);
@@ -436,6 +518,18 @@ main(int argc, char *argv[])
exit(1);
}
+ /*
+ * If progress status information is requested, we need to scan the
+ * directory tree twice: once to know how much total data needs to
+ * be processed and once to do the real work.
+ */
+ if (showprogress)
+ {
+ total_size = scan_directory(DataDir, "global", true);
+ total_size += scan_directory(DataDir, "base", true);
+ total_size += scan_directory(DataDir, "pg_tblspc", true);
+ }
+
if (ControlFile->data_checksum_version == 0 &&
mode == PG_MODE_DISABLE)
{
@@ -453,9 +547,15 @@ main(int argc, char *argv[])
/* Operate on all files if checking or enabling checksums */
if (mode == PG_MODE_CHECK || mode == PG_MODE_ENABLE)
{
- scan_directory(DataDir, "global");
- scan_directory(DataDir, "base");
- scan_directory(DataDir, "pg_tblspc");
+ (void) scan_directory(DataDir, "global", false);
+ (void) scan_directory(DataDir, "base", false);
+ (void) scan_directory(DataDir, "pg_tblspc", false);
+
+ if (showprogress)
+ {
+ progress_report(true);
+ fprintf(stderr, "\n"); /* Need to move to next line */
+ }
printf(_("Checksum operation completed\n"));
printf(_("Files scanned: %s\n"), psprintf(INT64_FORMAT, files));
Bonjour Michaᅵl,
Getting to know the total size and the current size are the two
important factors that matter when it comes to do progress reporting
in my opinion. I have read the patch, and I am not really convinced
by the need to show the progress report based on an interval of 250ms
as we talk about an operation which could take dozens of minutes.
I do not think that it matters. I like to see things moving, and the
performance impact is null.
So I have simplified the patch to only show a progress report every
second. This also removes the include for the time-related APIs from
portability/.
I do not think that it is a good idea, because Michael is thinking of
adding some throttling capability, which would be a very good thing, but
which will need something precise, so better use the precise stuff from
the start. Also, the per second stuff induces rounding effects at the
beginning.
A second thing is that I don't think that the speed is much useful.
Hmmm. I like this information because I this is where I have expectations,
whereas I'm not sure whether 1234 seconds for 12.3 GB is good or bad, but
I know that 10 MB/s on my SSD is not very good.
I would expect the speed to be steady, still there is a risk to show
incorrect information if the speed of the operation is spiky or
irregular leading to an incorrect estimation of the remaining time.
Hmmm. That is life, I'd say I'm used to it.
In short, I would like to commit the first patch as attached, which is
much more simple than what has been sent previously, still it provides
the progress information which is useful.
I would prefer that you would keep the patch with the initial precision &
features for the reasons outlined above, but you are the committer and I'm
only a reviewer.
--
Fabien.
Hi,
so we are basically back at the original patch as written by Bernd :)
+/* + * Report current progress status. Parts borrowed from + * PostgreSQLs' src/bin/pg_basebackup.c + */ +static void +progress_report(bool force) +{ + int percent; + char total_size_str[32]; + char current_size_str[32]; + pg_time_t now; + + if (!showprogress) + return;
The way you've now changed this is that there's a function call into
progress_report() for every block that's being read, even if there is no
progress reporting requested. That looks like a pretty severe
performance problem so I suggest to at least stick with checking
showprogress before calling progress_report() and not the other way
round.
I guess there is still some noticeable performance penalty to pay when
reporting progress, even more so now that you reverted to only doing it
once per second - if we report progress, we presumably call
report_progress() thousands of times and return early 99,9% of the time.
So my vote is in favour of only calling progress_report() every once in
a while - I saw quite a speedup (or removal of slowdown) due to this in
my tests, this was not just some unwarranted microoptimization.
+ fprintf(stderr, "\r");
I think the isatty() check that was in our versions of the patch is
useful here, did you check how this looks when piping the output to a
file?
@@ -195,12 +254,23 @@ scan_file(const char *fn, BlockNumber segmentno)
_("%s: checksums enabled in file
\"%s\"\n"), progname, fn);
} + /* Make sure progress is reported at least once per file */ + progress_report(false); + close(f); }
This hunk is from the performance optimization of calling
progress_report only every 1024 blocks and could be removed if we stick
with calling it for every block anyway (but see above).
-static void -scan_directory(const char *basedir, const char *subdir) +/* + * Scan the given directory for items which can be checksummed and + * operate on each one of them. If "sizeonly" is true, the size of + * all the items which have checksums is computed and returned back
"computed" is maybe a bit strong a word here, how about "added up"?
Michael
--
Michael Banck
Projektleiter / Senior Berater
Tel.: +49 2166 9901-171
Fax: +49 2166 9901-100
Email: michael.banck@credativ.de
credativ GmbH, HRB Mönchengladbach 12080
USt-ID-Nummer: DE204566209
Trompeterallee 108, 41189 Mönchengladbach
Geschäftsführung: Dr. Michael Meskes, Jörg Folz, Sascha Heuer
Unser Umgang mit personenbezogenen Daten unterliegt
folgenden Bestimmungen: https://www.credativ.de/datenschutz
On Sat, Mar 30, 2019 at 09:07:41PM +0100, Michael Banck wrote:
The way you've now changed this is that there's a function call into
progress_report() for every block that's being read, even if there is no
progress reporting requested. That looks like a pretty severe
performance problem so I suggest to at least stick with checking
showprogress before calling progress_report() and not the other way
round.So my vote is in favour of only calling progress_report() every once in
a while - I saw quite a speedup (or removal of slowdown) due to this in
my tests, this was not just some unwarranted microoptimization.
Do you have some runtime numbers? If all the pages are in the OS
cache you should be able to see more impact. I have been testing with
a pgbench database at scale 300 and --check, and perf is showing me
that progress_report counts for 0.10% with or without the option of
the profile while pg_checksum_page() counts for 36%. Switching the
switch in or out of it makes the performance change lost in the noise.
I'd rather keep the check for showprogress within progress_report() so
as extra callers don't miss bypassing the report if --progress is not
enabled, still we are talking about only one caller now so the
attached does it the other way around with an assertion.
+ fprintf(stderr, "\r");
I think the isatty() check that was in our versions of the patch is
useful here, did you check how this looks when piping the output to a
file?
Oops, fixed. The output was strange.
This hunk is from the performance optimization of calling
progress_report only every 1024 blocks and could be removed if we stick
with calling it for every block anyway (but see above).
Indeed, removed this one.
-static void -scan_directory(const char *basedir, const char *subdir) +/* + * Scan the given directory for items which can be checksummed and + * operate on each one of them. If "sizeonly" is true, the size of + * all the items which have checksums is computed and returned back"computed" is maybe a bit strong a word here, how about "added up"?
"computed" sounds fine to me.
--
Michael
Attachments:
checksums-progress-report-v2.patchtext/x-diff; charset=us-asciiDownload
diff --git a/doc/src/sgml/ref/pg_checksums.sgml b/doc/src/sgml/ref/pg_checksums.sgml
index 70339eaec9..29507ff98e 100644
--- a/doc/src/sgml/ref/pg_checksums.sgml
+++ b/doc/src/sgml/ref/pg_checksums.sgml
@@ -135,6 +135,17 @@ 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 checking or enabling checksums.
+ </para>
+ </listitem>
+ </varlistentry>
+
<varlistentry>
<term><option>-V</option></term>
<term><option>--version</option></term>
diff --git a/src/bin/pg_checksums/pg_checksums.c b/src/bin/pg_checksums/pg_checksums.c
index 5c185ebed8..c85e7febed 100644
--- a/src/bin/pg_checksums/pg_checksums.c
+++ b/src/bin/pg_checksums/pg_checksums.c
@@ -15,6 +15,7 @@
#include "postgres_fe.h"
#include <dirent.h>
+#include <time.h>
#include <sys/stat.h>
#include <unistd.h>
@@ -37,6 +38,7 @@ static ControlFileData *ControlFile;
static char *only_relfilenode = NULL;
static bool do_sync = true;
static bool verbose = false;
+static bool showprogress = false;
typedef enum
{
@@ -59,6 +61,13 @@ static PgChecksumMode mode = PG_MODE_CHECK;
static const char *progname;
+/*
+ * Progress status information.
+ */
+int64 total_size = 0;
+int64 current_size = 0;
+static pg_time_t last_progress_report = 0;
+
static void
usage(void)
{
@@ -71,6 +80,7 @@ usage(void)
printf(_(" -d, --disable disable data checksums\n"));
printf(_(" -e, --enable enable data checksums\n"));
printf(_(" -N, --no-sync do not wait for changes to be written safely to disk\n"));
+ printf(_(" -P, --progress show progress information\n"));
printf(_(" -v, --verbose output verbose messages\n"));
printf(_(" -r RELFILENODE check only relation with specified relfilenode\n"));
printf(_(" -V, --version output version information, then exit\n"));
@@ -97,6 +107,52 @@ static const char *const skip[] = {
NULL,
};
+/*
+ * Report current progress status. Parts borrowed from
+ * src/bin/pg_basebackup.c.
+ */
+static void
+progress_report(bool force)
+{
+ int percent;
+ char total_size_str[32];
+ char current_size_str[32];
+ pg_time_t now;
+
+ Assert(showprogress);
+
+ now = time(NULL);
+ if (now == last_progress_report && !force)
+ return; /* Max once per second */
+
+ /* Save current time */
+ last_progress_report = now;
+
+ /* Adjust total size if current_size is larger */
+ if (current_size > total_size)
+ total_size = current_size;
+
+ /* Calculate current percentage of size done */
+ percent = total_size ? (int) ((current_size) * 100 / total_size) : 0;
+
+ snprintf(total_size_str, sizeof(total_size_str), INT64_FORMAT,
+ total_size / (1024 * 1024));
+ snprintf(current_size_str, sizeof(current_size_str), INT64_FORMAT,
+ current_size / (1024 * 1024));
+
+ /*
+ * Separate step to keep platform-dependent format code out of
+ * translatable strings. And we only test for INT64_FORMAT availability
+ * in snprintf, not fprintf.
+ */
+ fprintf(stderr, "%*s/%s MB (%d%%) computed",
+ (int) strlen(current_size_str), current_size_str, total_size_str,
+ percent);
+
+ /* Stay on the same line if reporting to a terminal */
+ fprintf(stderr, isatty(fileno(stderr)) ? "\r" : "\n");
+}
+
static bool
skipfile(const char *fn)
{
@@ -153,6 +209,7 @@ scan_file(const char *fn, BlockNumber segmentno)
continue;
csum = pg_checksum_page(buf.data, blockno + segmentno * RELSEG_SIZE);
+ current_size += r;
if (mode == PG_MODE_CHECK)
{
if (csum != header->pd_checksum)
@@ -183,6 +240,9 @@ scan_file(const char *fn, BlockNumber segmentno)
exit(1);
}
}
+
+ if (showprogress)
+ progress_report(false);
}
if (verbose)
@@ -198,9 +258,17 @@ scan_file(const char *fn, BlockNumber segmentno)
close(f);
}
-static void
-scan_directory(const char *basedir, const char *subdir)
+/*
+ * Scan the given directory for items which can be checksummed and
+ * operate on each one of them. If "sizeonly" is true, the size of
+ * all the items which have checksums is computed and returned back
+ * to the caller without operating on the files. This is used to compile
+ * the total size of the data directory for progress reports.
+ */
+static int64
+scan_directory(const char *basedir, const char *subdir, bool sizeonly)
{
+ int64 dirsize = 0;
char path[MAXPGPATH];
DIR *dir;
struct dirent *de;
@@ -279,16 +347,24 @@ scan_directory(const char *basedir, const char *subdir)
/* Relfilenode not to be included */
continue;
- scan_file(fn, segmentno);
+ dirsize += st.st_size;
+
+ /*
+ * No need to work on the file when calculating only the size
+ * of the items in the data folder.
+ */
+ if (!sizeonly)
+ scan_file(fn, segmentno);
}
#ifndef WIN32
else if (S_ISDIR(st.st_mode) || S_ISLNK(st.st_mode))
#else
else if (S_ISDIR(st.st_mode) || pgwin32_is_junction(fn))
#endif
- scan_directory(path, de->d_name);
+ dirsize += scan_directory(path, de->d_name, sizeonly);
}
closedir(dir);
+ return dirsize;
}
int
@@ -300,6 +376,7 @@ main(int argc, char *argv[])
{"disable", no_argument, NULL, 'd'},
{"enable", no_argument, NULL, 'e'},
{"no-sync", no_argument, NULL, 'N'},
+ {"progress", no_argument, NULL, 'P'},
{"verbose", no_argument, NULL, 'v'},
{NULL, 0, NULL, 0}
};
@@ -327,7 +404,7 @@ main(int argc, char *argv[])
}
}
- while ((c = getopt_long(argc, argv, "cD:deNr:v", long_options, &option_index)) != -1)
+ while ((c = getopt_long(argc, argv, "cD:deNPr:v", long_options, &option_index)) != -1)
{
switch (c)
{
@@ -357,6 +434,9 @@ main(int argc, char *argv[])
}
only_relfilenode = pstrdup(optarg);
break;
+ case 'P':
+ showprogress = true;
+ break;
default:
fprintf(stderr, _("Try \"%s --help\" for more information.\n"), progname);
exit(1);
@@ -436,6 +516,18 @@ main(int argc, char *argv[])
exit(1);
}
+ /*
+ * If progress status information is requested, we need to scan the
+ * directory tree twice: once to know how much total data needs to
+ * be processed and once to do the real work.
+ */
+ if (showprogress)
+ {
+ total_size = scan_directory(DataDir, "global", true);
+ total_size += scan_directory(DataDir, "base", true);
+ total_size += scan_directory(DataDir, "pg_tblspc", true);
+ }
+
if (ControlFile->data_checksum_version == 0 &&
mode == PG_MODE_DISABLE)
{
@@ -453,9 +545,15 @@ main(int argc, char *argv[])
/* Operate on all files if checking or enabling checksums */
if (mode == PG_MODE_CHECK || mode == PG_MODE_ENABLE)
{
- scan_directory(DataDir, "global");
- scan_directory(DataDir, "base");
- scan_directory(DataDir, "pg_tblspc");
+ (void) scan_directory(DataDir, "global", false);
+ (void) scan_directory(DataDir, "base", false);
+ (void) scan_directory(DataDir, "pg_tblspc", false);
+
+ if (showprogress)
+ {
+ progress_report(true);
+ fprintf(stderr, "\n"); /* Need to move to next line */
+ }
printf(_("Checksum operation completed\n"));
printf(_("Files scanned: %s\n"), psprintf(INT64_FORMAT, files));
On Sat, Mar 30, 2019 at 06:52:56PM +0100, Fabien COELHO wrote:
I do not think that it matters. I like to see things moving, and the
performance impact is null.
Another point is that this bloats the logs redirected to a file by 4
compared to the initial proposal. I am not sure that this helps
much for anybody.
I do not think that it is a good idea, because Michael is thinking of adding
some throttling capability, which would be a very good thing, but which will
need something precise, so better use the precise stuff from the start.
Also, the per second stuff induces rounding effects at the beginning.
Let's revisit that when the need shows up then. I'd rather have us
start with a basic set of metrics which can be extended later on.
Hmmm. I like this information because I this is where I have expectations,
whereas I'm not sure whether 1234 seconds for 12.3 GB is good or bad, but I
know that 10 MB/s on my SSD is not very good.
Well, with some progress generated once per second you are one
substraction away to guess how much has been computed in the last N
second...
--
Michael
Bonjour Michaᅵl,
I do not think that it matters. I like to see things moving, and the
performance impact is null.Another point is that this bloats the logs redirected to a file by 4
compared to the initial proposal. I am not sure that this helps
much for anybody.
Hmmm. Progress is more an interactive feature where the previous result is
overriden thanks to the \r. Maybe it should be -P X where X is the
expected delay in seconds. Pgbench progress reporting on initialization
basically outputs 10 rows per second, probably it is too much.
I do not think that it is a good idea, because Michael is thinking of adding
some throttling capability, which would be a very good thing, but which will
need something precise, so better use the precise stuff from the start.
Also, the per second stuff induces rounding effects at the beginning.Let's revisit that when the need shows up then. I'd rather have us
start with a basic set of metrics which can be extended later on.
I do not see why it would be better to do it roughly if it is already
implemented precisely and nicely.
Hmmm. I like this information because I this is where I have expectations,
whereas I'm not sure whether 1234 seconds for 12.3 GB is good or bad, but I
know that 10 MB/s on my SSD is not very good.Well, with some progress generated once per second you are one
substraction away to guess how much has been computed in the last N
second...
I would prefer to have the speed simply printed out.
The per second or more thing is debatable, but for the other changes I do
not think that they improve the feature much.
As I said, I'm only a reviewer, you do as you feel.
--
Fabien.
Hi,
Am Montag, den 01.04.2019, 15:10 +0900 schrieb Michael Paquier:
On Sat, Mar 30, 2019 at 09:07:41PM +0100, Michael Banck wrote:
The way you've now changed this is that there's a function call into
progress_report() for every block that's being read, even if there is no
progress reporting requested. That looks like a pretty severe
performance problem so I suggest to at least stick with checking
showprogress before calling progress_report() and not the other way
round.So my vote is in favour of only calling progress_report() every once in
a while - I saw quite a speedup (or removal of slowdown) due to this in
my tests, this was not just some unwarranted microoptimization.Do you have some runtime numbers?
I had another look and I don't see any slowdown with your patch.
Michael
--
Michael Banck
Projektleiter / Senior Berater
Tel.: +49 2166 9901-171
Fax: +49 2166 9901-100
Email: michael.banck@credativ.de
credativ GmbH, HRB Mönchengladbach 12080
USt-ID-Nummer: DE204566209
Trompeterallee 108, 41189 Mönchengladbach
Geschäftsführung: Dr. Michael Meskes, Jörg Folz, Sascha Heuer
Unser Umgang mit personenbezogenen Daten unterliegt
folgenden Bestimmungen: https://www.credativ.de/datenschutz
On Mon, Apr 01, 2019 at 08:51:03PM +0200, Michael Banck wrote:
I had another look and I don't see any slowdown with your patch.
I noticed one slowdown when using --disable --progress as this was
scanning the data directory for the total size but we don't need it in
this case. Fixed that and committed.
--
Michael
On Mon, Apr 01, 2019 at 07:26:00PM +0200, Fabien COELHO wrote:
Hmmm. Progress is more an interactive feature where the previous result is
overriden thanks to the \r.
Well, many people also redirect the output for such things.
Maybe it should be -P X where X is the expected
delay in seconds. Pgbench progress reporting on initialization basically
outputs 10 rows per second, probably it is too much.
I cannot say for pgbench. I personally think that's a lot but you are
the one who wrote it as such I guess.
I do not see why it would be better to do it roughly if it is already
implemented precisely and nicely.
Simple things can be extended later on, while complicated things
cannot, and we don't have similar metrics for other tools which may
make sense for them to have (not pg_rewind, but pg_basebackup).
Please note that progress reports on the backend also include total
amount of data to process vs current amount of data processed, which
is reliable output. The speed may be nice, but it is easy enough to
see in an output file where things get stuck even if there is no
speed showing up (or maybe just the difference with the last progress
makes more sense to have?).
--
Michael
Bonjour Michaᅵl,
Maybe it should be -P X where X is the expected
delay in seconds. Pgbench progress reporting on initialization basically
outputs 10 rows per second, probably it is too much.I cannot say for pgbench. I personally think that's a lot but you are
the one who wrote it as such I guess.
Nope, this was way before I intervened. ISTM that a patch was submitted to
get per second or slower progress reporting on the initalization, and it
was rejected. Now that there are many SSD, maybe I could bring it back. An
issue with pbbench is that the reported time and progress is for the
insertion phase only, but PK and other FK declaration take as much time
and are not included, so I'm not sure it can be much better.
For pg_checksums, probably some improvement patch will be submitted later,
if someone feels like it.
--
Fabien.
On Tue, Apr 02, 2019 at 08:11:45AM +0200, Fabien COELHO wrote:
Nope, this was way before I intervened. ISTM that a patch was submitted to
get per second or slower progress reporting on the initalization, and it was
rejected. Now that there are many SSD, maybe I could bring it back. An issue
with pbbench is that the reported time and progress is for the insertion
phase only, but PK and other FK declaration take as much time and are not
included, so I'm not sure it can be much better.
Bonus idea: integrate automatically with pg_stat_progress_vacuum() for
the last vacuum at the end of initialization. This step can also take
a very long time and one can only know the progress of the VACUUM with
a second session/terminal.
For pg_checksums, probably some improvement patch will be submitted later,
if someone feels like it.
Let's see. I think that what we have now in v12 is good enough for
checksum operations on an offline cluster. And my take is that we
should focus more on online checksum verification for v13 and future
versions.
Regarding all this tooling around checksums. With v12, enabling
checksums with no actual downtime is doable with a primary-standby
deployment using physical replication and one planned failover
(possible as well with v10 and newer versions and logical replication
but that takes longer), so I think that there is not much value in
being able to enable checksums on a single node while it is online.
--
Michael
For pg_checksums, probably some improvement patch will be submitted later,
if someone feels like it.Let's see. I think that what we have now in v12 is good enough for
checksum operations on an offline cluster. And my take is that we
should focus more on online checksum verification for v13 and future
versions.
I agree.
For online, we should want throttling so that the check can have a reduced
performance impact when scrubbing.
--
Fabien.
Hi,
Am Dienstag, den 02.04.2019, 15:56 +0900 schrieb Michael Paquier:
Regarding all this tooling around checksums. With v12, enabling
checksums with no actual downtime is doable with a primary-standby
deployment using physical replication and one planned failover
Can you explain in more detail how this would work? I thought we came to
the conclusion (and the documentation seems to indicate so), that you
should stop all participating instances of a cluster and then enable
checksums on all of them, which would impose a downtime.
Michael
--
Michael Banck
Projektleiter / Senior Berater
Tel.: +49 2166 9901-171
Fax: +49 2166 9901-100
Email: michael.banck@credativ.de
credativ GmbH, HRB Mönchengladbach 12080
USt-ID-Nummer: DE204566209
Trompeterallee 108, 41189 Mönchengladbach
Geschäftsführung: Dr. Michael Meskes, Jörg Folz, Sascha Heuer
Unser Umgang mit personenbezogenen Daten unterliegt
folgenden Bestimmungen: https://www.credativ.de/datenschutz
On Tue, Apr 02, 2019 at 04:02:59PM +0200, Michael Banck wrote:
Can you explain in more detail how this would work? I thought we came to
the conclusion (and the documentation seems to indicate so), that you
should stop all participating instances of a cluster and then enable
checksums on all of them, which would impose a downtime.
That's what I was pointing out here:
/messages/by-id/20190320225924.GC20192@paquier.xyz
I still agree about keeping in the docs safer recommendations, in the
shape of the ones currently present, than what I mentioned on the
other thread.
--
Michael
On Tue, Apr 02, 2019 at 10:10:34AM +0200, Fabien COELHO wrote:
For online, we should want throttling so that the check can have a reduced
performance impact when scrubbing.
Yes. Throttling is a necessary property in my opinion, perhaps in
combination with some autovacuum-like options to only trigger the
checks for a relation after a certain amout of activity has happened
on it.
--
Michael