Hi,
Am Sonntag, den 03.02.2019, 02:06 -0800 schrieb Andres Freund:
> Hi,
>
> On 2018-12-25 10:25:46 +0100, Fabien COELHO wrote:
> > Hallo Michael,
> >
> > > Yeah, new rebased version attached.
> >
> > Patch v8 applies cleanly, compiles, global & local make check are ok.
> >
> > A few comments:
> >
> > About added tests: the node is left running at the end of the script, which
> > is not very clean. I'd suggest to either move the added checks before
> > stopping, or to stop again at the end of the script, depending on the
> > intention.
>
> Michael?
Uh, I kinda forgot about this, I've made the tests stop the node now.
> > I'm wondering (possibly again) about the existing early exit if one block
> > cannot be read on retry: the command should count this as a kind of bad
> > block, proceed on checking other files, and obviously fail in the end, but
> > having checked everything else and generated a report. I do not think that
> > this condition warrants a full stop. ISTM that under rare race conditions
> > (eg, an unlucky concurrent "drop database" or "drop table") this could
> > happen when online, although I could not trigger one despite heavy testing,
> > so I'm possibly mistaken.
>
> This seems like a defensible judgement call either way.
Right now we have a few tests that explicitly check that
pg_verify_checksums fail on broken data ("foo" in the file). Those
would then just get skipped AFAICT, which I think is the worse behaviour
, but if everybody thinks that should be the way to go, we can
drop/adjust those tests and make pg_verify_checksums skip them.
Thoughts?
In the meanwhile, v9 is attached with the above change and rebased
(without changes) to master.
Michael
--
Michael Banck
Projektleiter / Senior Berater
Tel.: +49 2166 9901-171
Fax: +49 2166 9901-100
Email: michael.ba...@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/datenschutzdiff --git a/doc/src/sgml/ref/pg_verify_checksums.sgml b/doc/src/sgml/ref/pg_verify_checksums.sgml
index 905b8f1222..4ad6edcde6 100644
--- a/doc/src/sgml/ref/pg_verify_checksums.sgml
+++ b/doc/src/sgml/ref/pg_verify_checksums.sgml
@@ -37,9 +37,8 @@ PostgreSQL documentation
<title>Description</title>
<para>
<command>pg_verify_checksums</command> verifies data checksums in a
- <productname>PostgreSQL</productname> cluster. The server must be shut
- down cleanly before running <application>pg_verify_checksums</application>.
- The exit status is zero if there are no checksum errors, otherwise nonzero.
+ <productname>PostgreSQL</productname> cluster. The exit status is zero if
+ there are no checksum errors, otherwise nonzero.
</para>
</refsect1>
diff --git a/src/bin/pg_verify_checksums/pg_verify_checksums.c b/src/bin/pg_verify_checksums/pg_verify_checksums.c
index 511262ab5f..13fb51223a 100644
--- a/src/bin/pg_verify_checksums/pg_verify_checksums.c
+++ b/src/bin/pg_verify_checksums/pg_verify_checksums.c
@@ -1,7 +1,7 @@
/*
* pg_verify_checksums
*
- * Verifies page level checksums in an offline cluster
+ * Verifies page level checksums in a cluster
*
* Copyright (c) 2010-2019, PostgreSQL Global Development Group
*
@@ -26,10 +26,13 @@
static int64 files = 0;
static int64 blocks = 0;
static int64 badblocks = 0;
+static int64 skippedblocks = 0;
static ControlFileData *ControlFile;
+static XLogRecPtr checkpointLSN;
static char *only_relfilenode = NULL;
static bool verbose = false;
+static bool online = false;
static const char *progname;
@@ -86,10 +89,17 @@ scan_file(const char *fn, BlockNumber segmentno)
PageHeader header = (PageHeader) buf.data;
int f;
BlockNumber blockno;
+ bool block_retry = false;
f = open(fn, O_RDONLY | PG_BINARY, 0);
if (f < 0)
{
+ if (online && errno == ENOENT)
+ {
+ /* File was removed in the meantime */
+ return;
+ }
+
fprintf(stderr, _("%s: could not open file \"%s\": %s\n"),
progname, fn, strerror(errno));
exit(1);
@@ -104,26 +114,106 @@ scan_file(const char *fn, BlockNumber segmentno)
if (r == 0)
break;
+ if (r < 0)
+ {
+ fprintf(stderr, _("%s: could not read block %u in file \"%s\": %s\n"),
+ progname, blockno, fn, strerror(errno));
+ return;
+ }
if (r != BLCKSZ)
{
- fprintf(stderr, _("%s: could not read block %u in file \"%s\": read %d of %d\n"),
- progname, blockno, fn, r, BLCKSZ);
- exit(1);
+ if (block_retry)
+ {
+ /* We already tried once to reread the block, bail out */
+ fprintf(stderr, _("%s: could not read block %u in file \"%s\": read %d of %d\n"),
+ progname, blockno, fn, r, BLCKSZ);
+ exit(1);
+ }
+
+ /*
+ * Retry the block. It's possible that we read the block while it
+ * was extended or shrinked, so it it ends up looking torn to us.
+ */
+
+ /*
+ * Seek back by the amount of bytes we read to the beginning of
+ * the failed block.
+ */
+ if (lseek(f, -r, SEEK_CUR) == -1)
+ {
+ fprintf(stderr, _("%s: could not lseek in file \"%s\": %m\n"),
+ progname, fn);
+ exit(1);
+ }
+
+ /* Set flag so we know a retry was attempted */
+ block_retry = true;
+
+ /* Reset loop to validate the block again */
+ blockno--;
+
+ continue;
}
- blocks++;
/* New pages have no checksum yet */
if (PageIsNew(header))
+ {
+ skippedblocks++;
continue;
+ }
+
+ blocks++;
csum = pg_checksum_page(buf.data, blockno + segmentno * RELSEG_SIZE);
if (csum != header->pd_checksum)
{
+ /*
+ * Retry the block on the first failure. If the verification is
+ * done while the instance is online, it is possible that we read
+ * the first 4K page of the block just before postgres updated the
+ * entire block so it ends up looking torn to us. We only need to
+ * retry once because the LSN should be updated to something we can
+ * ignore on the next pass. If the error happens again then it is
+ * a true validation failure.
+ */
+ if (!block_retry)
+ {
+ /* Seek to the beginning of the failed block */
+ if (lseek(f, -BLCKSZ, SEEK_CUR) == -1)
+ {
+ fprintf(stderr, _("%s: could not lseek in file \"%s\": %m\n"),
+ progname, fn);
+ exit(1);
+ }
+
+ /* Set flag so we know a retry was attempted */
+ block_retry = true;
+
+ /* Reset loop to validate the block again */
+ blockno--;
+
+ continue;
+ }
+
+ /*
+ * The checksum verification failed on retry as well. Check if the
+ * page has been modified since the checkpoint and skip it in this
+ * case.
+ */
+ if (PageGetLSN(buf.data) > checkpointLSN)
+ {
+ block_retry = false;
+ blocks--;
+ skippedblocks++;
+ continue;
+ }
+
if (ControlFile->data_checksum_version == PG_DATA_CHECKSUM_VERSION)
fprintf(stderr, _("%s: checksum verification failed in file \"%s\", block %u: calculated checksum %X but block contains %X\n"),
progname, fn, blockno, csum, header->pd_checksum);
badblocks++;
}
+ block_retry = false;
}
if (verbose)
@@ -172,6 +262,12 @@ scan_directory(const char *basedir, const char *subdir)
snprintf(fn, sizeof(fn), "%s/%s", path, de->d_name);
if (lstat(fn, &st) < 0)
{
+ if (online && errno == ENOENT)
+ {
+ /* File was removed in the meantime */
+ continue;
+ }
+
fprintf(stderr, _("%s: could not stat file \"%s\": %s\n"),
progname, fn, strerror(errno));
exit(1);
@@ -308,7 +404,7 @@ main(int argc, char *argv[])
exit(1);
}
- /* Check if cluster is running */
+ /* Check if checksums are enabled */
ControlFile = get_controlfile(DataDir, progname, &crc_ok);
if (!crc_ok)
{
@@ -316,12 +412,10 @@ main(int argc, char *argv[])
exit(1);
}
+ /* Check if cluster is running */
if (ControlFile->state != DB_SHUTDOWNED &&
ControlFile->state != DB_SHUTDOWNED_IN_RECOVERY)
- {
- fprintf(stderr, _("%s: cluster must be shut down to verify checksums\n"), progname);
- exit(1);
- }
+ online = true;
if (ControlFile->data_checksum_version == 0)
{
@@ -329,6 +423,9 @@ main(int argc, char *argv[])
exit(1);
}
+ /* Get checkpoint LSN */
+ checkpointLSN = ControlFile->checkPoint;
+
/* Scan all files */
scan_directory(DataDir, "global");
scan_directory(DataDir, "base");
@@ -338,6 +435,8 @@ main(int argc, char *argv[])
printf(_("Data checksum version: %d\n"), ControlFile->data_checksum_version);
printf(_("Files scanned: %s\n"), psprintf(INT64_FORMAT, files));
printf(_("Blocks scanned: %s\n"), psprintf(INT64_FORMAT, blocks));
+ if (skippedblocks > 0)
+ printf(_("Blocks skipped: %s\n"), psprintf(INT64_FORMAT, skippedblocks));
printf(_("Bad checksums: %s\n"), psprintf(INT64_FORMAT, badblocks));
if (badblocks > 0)
diff --git a/src/bin/pg_verify_checksums/t/002_actions.pl b/src/bin/pg_verify_checksums/t/002_actions.pl
index 5250b5a728..7a9fe82625 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 => 69;
# Utility routine to create and check a table with corrupted checksums
@@ -104,10 +104,10 @@ append_to_file "$pgdata/global/pgsql_tmp/1.1", "foo";
command_ok(['pg_verify_checksums', '-D', $pgdata],
"succeeds with offline cluster");
-# Checks cannot happen with an online cluster
+# Checksums pass on an online cluster
$node->start;
-command_fails(['pg_verify_checksums', '-D', $pgdata],
- "fails with online cluster");
+command_ok(['pg_verify_checksums', '-D', $pgdata],
+ "succeeds with online cluster");
# Check corruption of table on default tablespace.
check_relation_corruption($node, 'corrupt1', 'pg_default');
@@ -158,3 +158,17 @@ fail_corrupt($node, "99990_vm");
fail_corrupt($node, "99990_init.123");
fail_corrupt($node, "99990_fsm.123");
fail_corrupt($node, "99990_vm.123");
+
+# Start node again to test online verification correctly finds failures
+$node->start;
+fail_corrupt($node, "99990");
+fail_corrupt($node, "99990.123");
+fail_corrupt($node, "99990_fsm");
+fail_corrupt($node, "99990_init");
+fail_corrupt($node, "99990_vm");
+fail_corrupt($node, "99990_init.123");
+fail_corrupt($node, "99990_fsm.123");
+fail_corrupt($node, "99990_vm.123");
+
+# Stop node again at the end of tests
+$node->stop;
