Hi.
Am Montag, den 17.09.2018, 20:45 -0400 schrieb Stephen Frost:
> > You're right it's not about the fsync, sorry for the confusion. My point
> > is that using the checkpoint LSN gives us a guarantee that write is no
> > longer in progress, and so we can't see a page torn because of it. And
> > if we see a partial write due to a new write, it's guaranteed to update
> > the page LSN (and we'll notice it).
>
> Right, no worries about the confusion, I hadn't been fully thinking
> through the LSN bit either and that what we really need is some external
> confirmation of a write having *completed* (not just started) and that
> makes a definite difference.
>
> > > Right, I'm in agreement with doing that and it's what is done in
> > > pgbasebackup and pgBackRest.
> >
> > OK. All I'm saying is pg_verify_checksums should probably do the same
> > thing, i.e. grab checkpoint LSN and roll with that.
>
> Agreed.
I've attached the patch I added to my branch to swap out the pg_sleep()
with a check against the checkpoint LSN on a recheck verification
failure.
Let me know if there are still issues with it. I'll send a new patch for
the whole online verification feature in a bit.
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/datenschutz
commit 3b7d80fa7431098de4aab19a85f7d7b01c43f8a8
Author: Michael Banck <mba...@debian.org>
Date: Tue Sep 18 11:53:36 2018 +0200
Skip pages with new LSNs on recheck verification failure.
Before, we only skipped new pages and added a pg_sleep during the recheck.
This logic is broken, so replace it with a check against the (startup)
checkpoint LSN on a verification failure on recheck. If the LSN in the page
header is newer than the checkpoint LSN, we assume a torn page and skip it in
this run of pg_verify_checksums.
This now follows the logic during base backup checksum verification, however,
contrary to that we do not skip a page immediately if the LSN is newer than the
checkpoint, but try to (re-)verify it first and only skip it if its LSN is
newer on a recheck verification failure.
diff --git a/src/bin/pg_verify_checksums/pg_verify_checksums.c b/src/bin/pg_verify_checksums/pg_verify_checksums.c
index 7c2a06390c..2f8b5bf167 100644
--- a/src/bin/pg_verify_checksums/pg_verify_checksums.c
+++ b/src/bin/pg_verify_checksums/pg_verify_checksums.c
@@ -26,6 +26,7 @@ static int64 files = 0;
static int64 blocks = 0;
static int64 badblocks = 0;
static ControlFileData *ControlFile;
+static XLogRecPtr checkpointLSN;
static char *only_relfilenode = NULL;
static bool verbose = false;
@@ -125,14 +126,14 @@ scan_file(const char *fn, BlockNumber segmentno)
* 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. If there were less than 10 bad blocks so
- * far, we wait for 500ms before we retry.
+ * 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 == false)
{
- if (badblocks < 10)
- pg_usleep(500);
-
/* Seek to the beginning of the failed block */
if (lseek(f, -BLCKSZ, SEEK_CUR) == -1)
{
@@ -150,6 +151,16 @@ scan_file(const char *fn, BlockNumber segmentno)
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;
+ 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);
@@ -344,6 +355,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");
