Subject: "inconsistent page found" with checksum and wal_consistency_checking enabled

From: Ashwin Agrawal <[email-masked]>
Date: Tue, 19 Sep 2017 13:23:58 -0700

Currently, page checksum is not masked by Page masking routines used
by wal_consistency_checking facility. So, when running `make installcheck`
with data checksum enabled and wal_consistency_checking='all', it easily
and consistently FATALs with "inconsistent page found".

If anything needs to be masked on Page to perform / pass wal consistency
checking, definitely checksum is not going to match and hence must be
masked as well. Attaching patch to fix the same, installcheck passes with
checksums enabled and wal_consistency_checking='all' with the fix.

Clubbed to perform the masking with lsn as it sounds logical to have them
together, as lsn is masked is all the cases so far and such is needed for
checksum as well.

Thank You,
Ashwin Agrawal
diff --git a/src/backend/access/brin/brin_xlog.c b/src/backend/access/brin/brin_xlog.c
index dff7198..60daa54 100644
--- a/src/backend/access/brin/brin_xlog.c
+++ b/src/backend/access/brin/brin_xlog.c
@@ -332,7 +332,7 @@ brin_mask(char *pagedata, BlockNumber blkno)
 {
 	Page		page = (Page) pagedata;
 
-	mask_page_lsn(page);
+	mask_page_lsn_and_checksum(page);
 
 	mask_page_hint_bits(page);
 
diff --git a/src/backend/access/common/bufmask.c b/src/backend/access/common/bufmask.c
index 10253d3..d880aef 100644
--- a/src/backend/access/common/bufmask.c
+++ b/src/backend/access/common/bufmask.c
@@ -23,15 +23,17 @@
  * mask_page_lsn
  *
  * In consistency checks, the LSN of the two pages compared will likely be
- * different because of concurrent operations when the WAL is generated
- * and the state of the page when WAL is applied.
+ * different because of concurrent operations when the WAL is generated and
+ * the state of the page when WAL is applied. Also, mask out checksum as
+ * masking anything else on page means checksum is not going to match as well.
  */
 void
-mask_page_lsn(Page page)
+mask_page_lsn_and_checksum(Page page)
 {
 	PageHeader	phdr = (PageHeader) page;
 
 	PageXLogRecPtrSet(phdr->pd_lsn, (uint64) MASK_MARKER);
+	phdr->pd_checksum = MASK_MARKER;
 }
 
 /*
diff --git a/src/backend/access/gin/ginxlog.c b/src/backend/access/gin/ginxlog.c
index 7ba04e3..92cafe9 100644
--- a/src/backend/access/gin/ginxlog.c
+++ b/src/backend/access/gin/ginxlog.c
@@ -770,7 +770,7 @@ gin_mask(char *pagedata, BlockNumber blkno)
 	Page		page = (Page) pagedata;
 	GinPageOpaque opaque;
 
-	mask_page_lsn(page);
+	mask_page_lsn_and_checksum(page);
 	opaque = GinPageGetOpaque(page);
 
 	mask_page_hint_bits(page);
diff --git a/src/backend/access/gist/gistxlog.c b/src/backend/access/gist/gistxlog.c
index 4f4fe8f..7fd91ce 100644
--- a/src/backend/access/gist/gistxlog.c
+++ b/src/backend/access/gist/gistxlog.c
@@ -352,14 +352,14 @@ gist_mask(char *pagedata, BlockNumber blkno)
 {
 	Page		page = (Page) pagedata;
 
-	mask_page_lsn(page);
+	mask_page_lsn_and_checksum(page);
 
 	mask_page_hint_bits(page);
 	mask_unused_space(page);
 
 	/*
 	 * NSN is nothing but a special purpose LSN. Hence, mask it for the same
-	 * reason as mask_page_lsn.
+	 * reason as mask_page_lsn_and_checksum.
 	 */
 	GistPageSetNSN(page, (uint64) MASK_MARKER);
 
diff --git a/src/backend/access/hash/hash_xlog.c b/src/backend/access/hash/hash_xlog.c
index 67a856c..f19f6fd 100644
--- a/src/backend/access/hash/hash_xlog.c
+++ b/src/backend/access/hash/hash_xlog.c
@@ -1263,7 +1263,7 @@ hash_mask(char *pagedata, BlockNumber blkno)
 	HashPageOpaque opaque;
 	int			pagetype;
 
-	mask_page_lsn(page);
+	mask_page_lsn_and_checksum(page);
 
 	mask_page_hint_bits(page);
 	mask_unused_space(page);
diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c
index d20f038..d03f544 100644
--- a/src/backend/access/heap/heapam.c
+++ b/src/backend/access/heap/heapam.c
@@ -9166,7 +9166,7 @@ heap_mask(char *pagedata, BlockNumber blkno)
 	Page		page = (Page) pagedata;
 	OffsetNumber off;
 
-	mask_page_lsn(page);
+	mask_page_lsn_and_checksum(page);
 
 	mask_page_hint_bits(page);
 	mask_unused_space(page);
diff --git a/src/backend/access/nbtree/nbtxlog.c b/src/backend/access/nbtree/nbtxlog.c
index 4afdf47..82337f8 100644
--- a/src/backend/access/nbtree/nbtxlog.c
+++ b/src/backend/access/nbtree/nbtxlog.c
@@ -1034,7 +1034,7 @@ btree_mask(char *pagedata, BlockNumber blkno)
 	Page		page = (Page) pagedata;
 	BTPageOpaque maskopaq;
 
-	mask_page_lsn(page);
+	mask_page_lsn_and_checksum(page);
 
 	mask_page_hint_bits(page);
 	mask_unused_space(page);
diff --git a/src/backend/access/spgist/spgxlog.c b/src/backend/access/spgist/spgxlog.c
index c440d21..87def79 100644
--- a/src/backend/access/spgist/spgxlog.c
+++ b/src/backend/access/spgist/spgxlog.c
@@ -1034,7 +1034,7 @@ spg_mask(char *pagedata, BlockNumber blkno)
 {
 	Page		page = (Page) pagedata;
 
-	mask_page_lsn(page);
+	mask_page_lsn_and_checksum(page);
 
 	mask_page_hint_bits(page);
 
diff --git a/src/backend/access/transam/generic_xlog.c b/src/backend/access/transam/generic_xlog.c
index fbc6810..3adbf7b 100644
--- a/src/backend/access/transam/generic_xlog.c
+++ b/src/backend/access/transam/generic_xlog.c
@@ -541,7 +541,7 @@ generic_redo(XLogReaderState *record)
 void
 generic_mask(char *page, BlockNumber blkno)
 {
-	mask_page_lsn(page);
+	mask_page_lsn_and_checksum(page);
 
 	mask_unused_space(page);
 }
diff --git a/src/backend/commands/sequence.c b/src/backend/commands/sequence.c
index 6293712..5c2ce78 100644
--- a/src/backend/commands/sequence.c
+++ b/src/backend/commands/sequence.c
@@ -1941,7 +1941,7 @@ ResetSequenceCaches(void)
 void
 seq_mask(char *page, BlockNumber blkno)
 {
-	mask_page_lsn(page);
+	mask_page_lsn_and_checksum(page);
 
 	mask_unused_space(page);
 }
diff --git a/src/include/access/bufmask.h b/src/include/access/bufmask.h
index 95c6c3a..6a24c94 100644
--- a/src/include/access/bufmask.h
+++ b/src/include/access/bufmask.h
@@ -23,7 +23,7 @@
 /* Marker used to mask pages consistently */
 #define MASK_MARKER		0
 
-extern void mask_page_lsn(Page page);
+extern void mask_page_lsn_and_checksum(Page page);
 extern void mask_page_hint_bits(Page page);
 extern void mask_unused_space(Page page);
 extern void mask_lp_flags(Page page);

-- 
Sent via pgsql-hackers mailing list ([email-masked])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

---

From: Michael Paquier <[email-masked]>
Date: Wed, 20 Sep 2017 13:52:15 +0900

On Wed, Sep 20, 2017 at 5:23 AM, Ashwin Agrawal <[email-masked]> wrote:
> Currently, page checksum is not masked by Page masking routines used by
> wal_consistency_checking facility. So, when running `make installcheck` with
> data checksum enabled and wal_consistency_checking='all', it easily and
> consistently FATALs with "inconsistent page found".

Indeed. This had better be fixed before PG10 is out. I am adding an open item.

> checking, definitely checksum is not going to match and hence must be masked
> as well. Attaching patch to fix the same, installcheck passes with checksums
> enabled and wal_consistency_checking='all' with the fix.
>

Agreed.

Nit: Using "the LSN and the checksum" instead of the "the LSN".
-- 
Michael

---

From: Kuntal Ghosh <[email-masked]>
Date: Wed, 20 Sep 2017 10:56:07 +0530

> Indeed. This had better be fixed before PG10 is out. I am adding an open item.
Oops and surprised! How come we missed that previously. If page lsns
are different, checksums will be different as well. Anyhow, nice catch
and thanks for the patch.

-- 
Thanks & Regards,
Kuntal Ghosh
EnterpriseDB: http://www.enterprisedb.com

---

From: Michael Paquier <[email-masked]>
Date: Wed, 20 Sep 2017 14:35:06 +0900

> and thanks for the patch.

That happens. We have really covered maaany points during many rounds
of reviews, still I am not surprised to see one or two things that
fell into the cracks.
-- 
Michael

---

From: Kuntal Ghosh <[email-masked]>
Date: Wed, 20 Sep 2017 11:32:26 +0530

> of reviews, still I am not surprised to see one or two things that
Yup, that's true. :-)

---

From: Noah Misch <[email-masked]>
Date: Fri, 22 Sep 2017 07:42:33 -0700

> Indeed. This had better be fixed before PG10 is out. I am adding an open item.

[Action required within three days.  This is a generic notification.]

The above-described topic is currently a PostgreSQL 10 open item.  Robert,
since you committed the patch believed to have created it, you own this open
item.  If some other commit is more relevant or if this does not belong as a
v10 open item, please let us know.  Otherwise, please observe the policy on
open item ownership[1] and send a status update within three calendar days of
this message.  Include a date for your subsequent status update.  Testers may
discover new open items at any time, and I want to plan to get them all fixed
well in advance of shipping v10.  Consequently, I will appreciate your efforts
toward speedy resolution.  Thanks.

[1] https://www.postgresql.org/message-id/20170404140717.GA2675809%40tornado.leadboat.com

---

From: Robert Haas <[email-masked]>
Date: Fri, 22 Sep 2017 14:36:31 -0400

> Indeed. This had better be fixed before PG10 is out. I am adding an open item.

This seems a little hyperbolic to me.  Sure, it's a new bug in v10,
and sure, it's always better to fix bugs sooner rather than later, but
there's nothing particularly serious or urgent about this bug as
compared to any other one.

I've committed the proposed patch to master and REL_10_STABLE.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company