Date: Thu, 25 Nov 2021 16:58:28 -0300
From: Alvaro Herrera <alvherre@alvh.no-ip.org>
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: Amul Sul <sulamul@gmail.com>, Andres Freund <andres@anarazel.de>,
	Andrew Dunstan <andrew@dunslane.net>,
	"Bossart, Nathan" <bossartn@amazon.com>,
	Kyotaro Horiguchi <horikyota.ntt@gmail.com>,
	"masao.fujii@oss.nttdata.com" <masao.fujii@oss.nttdata.com>,
	"pgsql-hackers@lists.postgresql.org" <pgsql-hackers@lists.postgresql.org>,
	"mengjuan.cmj@alibaba-inc.com" <mengjuan.cmj@alibaba-inc.com>,
	"Jakub.Wartak@tomtom.com" <Jakub.Wartak@tomtom.com>,
	Ryo Matsumura <matsumura.ryo@fujitsu.com>
Subject: Re: prevent immature WAL streaming
Message-ID: <202111251958.z7xaqbby632w@alvherre.pgsql>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="zwyauspwcig5eb3y"
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <313645.1637867900@sss.pgh.pa.us>
Archived-At: 
 <https://www.postgresql.org/message-id/202111251958.z7xaqbby632w%40alvherre.pgsql>
Precedence: bulk


--zwyauspwcig5eb3y
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit

On 2021-Nov-25, Tom Lane wrote:

> Alvaro Herrera <alvherre@alvh.no-ip.org> writes:
> 
> > The problem is that the bug occurs while writing the WAL record.  Fixed
> > servers won't produce such records, but if you run an unpatched server
> > and it happens to write one, without a mitigation you cannot get away
> > from FATAL during replay.
> 
> Really?  AFAICS the WAL record contains the correct value, or at least
> we should define that one as being correct, for precisely this reason.

I don't know what is the correct value for a record that comes exactly
after the page header.  But here's a patch that fixes the problem; and
if a standby replays WAL written by an unpatched primary, it will be
able to read past instead of dying of FATAL.

I originally wrote this to have a WARNING in VerifyOverwriteContrecord
(in the cases that are new), with the idea that it'd prompt people to
upgrade, but that's probably a waste of time.

-- 
Álvaro Herrera              Valdivia, Chile  —  https://www.EnterpriseDB.com/
"El hombre nunca sabe de lo que es capaz hasta que lo intenta" (C. Dickens)

--zwyauspwcig5eb3y
Content-Type: text/x-diff; charset=utf-8
Content-Disposition: attachment;
	filename="0001-Fix-LSN-in-OVERWRITTEN_CONTRECORD.patch"

From c28d96dfeb0884dff58b989fc0b869ee0f7806e5 Mon Sep 17 00:00:00 2001
From: Alvaro Herrera <alvherre@alvh.no-ip.org>
Date: Thu, 25 Nov 2021 16:19:42 -0300
Subject: [PATCH] Fix LSN in OVERWRITTEN_CONTRECORD

If the record that was broken starts exactly at a page boundary, we
write the byte position at which the record data appears (skipping the
WAL page header), which isn't the actual LSN of the record.  Repair.

To avoid problems on production systems which might use the bogus code,
change the test so that LSNs at page boundaries are also considered
equal to the byte position for the record data.  This means that if an
unpatched server produces a bogus OVERWRITTEN_CONTRECORD, WAL replay can
continue after upgrading to a server containing this patch.
---
 src/backend/access/transam/xlog.c       | 9 ++++++++-
 src/backend/access/transam/xlogreader.c | 2 +-
 2 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 5a8851a602..1f1ae23526 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -10706,7 +10706,14 @@ xlog_redo(XLogReaderState *record)
 static void
 VerifyOverwriteContrecord(xl_overwrite_contrecord *xlrec, XLogReaderState *state)
 {
-	if (xlrec->overwritten_lsn != state->overwrittenRecPtr)
+	/*
+	 * 14.1 and sibling server versions wrote XLOG_OVERWRITE_CONTRECORD with a
+	 * bogus recptr. See https://postgr.es/m/45597.1637694259@sss.pgh.pa.us for
+	 * details.
+	 */
+	if (xlrec->overwritten_lsn != state->overwrittenRecPtr &&
+		xlrec->overwritten_lsn - SizeOfXLogShortPHD != state->overwrittenRecPtr &&
+		xlrec->overwritten_lsn - SizeOfXLogLongPHD != state->overwrittenRecPtr)
 		elog(FATAL, "mismatching overwritten LSN %X/%X -> %X/%X",
 			 LSN_FORMAT_ARGS(xlrec->overwritten_lsn),
 			 LSN_FORMAT_ARGS(state->overwrittenRecPtr));
diff --git a/src/backend/access/transam/xlogreader.c b/src/backend/access/transam/xlogreader.c
index f39f8044a9..4772cd1664 100644
--- a/src/backend/access/transam/xlogreader.c
+++ b/src/backend/access/transam/xlogreader.c
@@ -586,7 +586,7 @@ err:
 		 * in turn signal downstream WAL consumers that the broken WAL record
 		 * is to be ignored.
 		 */
-		state->abortedRecPtr = RecPtr;
+		state->abortedRecPtr = state->currRecPtr;
 		state->missingContrecPtr = targetPagePtr;
 	}
 
-- 
2.30.2


--zwyauspwcig5eb3y--