Feedback-ID: i0fe9450f:Fastmail
Date: Thu, 26 Dec 2024 14:34:48 +0900
From: Michael Paquier <michael@paquier.xyz>
To: Vitaly Davydov <v.davydov@postgrespro.ru>
Cc: pgsql-hackers <pgsql-hackers@postgresql.org>
Subject: Re: An improvement of ProcessTwoPhaseBuffer logic
Message-ID: <Z2zq-GrOpIDPOHUj@paquier.xyz>
References: <13b5b6-676c3080-4d-531db900@47931709>
 <Z2yKWtB4iB8BdEiI@paquier.xyz>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="l0hP8fKFOi4g3tJg"
Content-Disposition: inline
In-Reply-To: <Z2yKWtB4iB8BdEiI@paquier.xyz>
Archived-At: <https://www.postgresql.org/message-id/Z2zq-GrOpIDPOHUj%40paquier.xyz>
Precedence: bulk


--l0hP8fKFOi4g3tJg
Content-Type: multipart/mixed; boundary="eIUwJeSCLm7Q3E1L"
Content-Disposition: inline


--eIUwJeSCLm7Q3E1L
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

On Thu, Dec 26, 2024 at 07:42:34AM +0900, Michael Paquier wrote:
> I suspect that this can be still dangerous as-is while complicating
> the code with more possible paths for the removal of the 2PC files,
> because we may still build a file name from an XID, and that's what's
> causing this issue..

Here are two patches to address both issues:
- 0001 for the epoch calculation, down to 17, which would take care of
the underflow problem when having a 2PC file that has an XID in the
future at epoch 0.  It is true that it could be smarter when dealing
with files from other epochs, and that this is a problem in v17~.  I
think that this should integrate better with FullTransactionIds moving
forward rather than pass the file names.  That would be something
quite invasive, only for HEAD.  At least that's my impression.
- 0002, to take care of the future file issue, down to 13.  This
includes a TAP test to demonstrate the problem.  The test needs a
tweak for the 2PC file name in v17~.
--
Michael

--eIUwJeSCLm7Q3E1L
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
	filename="v1-0001-Fix-failure-with-incorrect-epoch-for-future-2PC-f.patch"
Content-Transfer-Encoding: quoted-printable

=46rom d45d0c95a0ddee51563b5e94ea046ce4c5cf1baf Mon Sep 17 00:00:00 2001
=46rom: Michael Paquier <michael@paquier.xyz>
Date: Thu, 26 Dec 2024 14:06:01 +0900
Subject: [PATCH v1 1/2] Fix failure with incorrect epoch for future 2PC fil=
es
 at recovery

A two-phase file in the future would be able to trigger an assertion
failure in AdjustToFullTransactionId() as the epoch counter would
underflow if the checkpoint record uses an epoch of 0.

In non-assert builds, this would create a WARNING message referring to a
2PC file with an epoch of "FFFFFFFF" (or UINT32_MAX), as an effect of
the underflow calculation.

Issue introduced by 5a1dfde8334b.

Reported-by: Vitaly Davydov
Discussion: https://postgr.es/m/13b5b6-676c3080-4d-531db900@47931709
Backpatch-through: 17
---
 src/backend/access/transam/twophase.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/src/backend/access/transam/twophase.c b/src/backend/access/tra=
nsam/twophase.c
index 49be1df91c..d2649264f9 100644
--- a/src/backend/access/transam/twophase.c
+++ b/src/backend/access/transam/twophase.c
@@ -947,10 +947,9 @@ AdjustToFullTransactionId(TransactionId xid)
=20
 	nextXid =3D XidFromFullTransactionId(nextFullXid);
 	epoch =3D EpochFromFullTransactionId(nextFullXid);
-	if (unlikely(xid > nextXid))
+	if (unlikely(xid > nextXid) && epoch > 0)
 	{
 		/* Wraparound occurred, must be from a prev epoch. */
-		Assert(epoch > 0);
 		epoch--;
 	}
=20
--=20
2.45.2


--eIUwJeSCLm7Q3E1L
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
	filename="v1-0002-Fix-handling-of-orphaned-2PC-files-in-the-future-.patch"
Content-Transfer-Encoding: quoted-printable

=46rom 31ebbadbca629abf8b7cf982d573ad2c838bf27f Mon Sep 17 00:00:00 2001
=46rom: Michael Paquier <michael@paquier.xyz>
Date: Thu, 26 Dec 2024 14:15:51 +0900
Subject: [PATCH v1 2/2] Fix handling of orphaned 2PC files in the future at
 recovery

Issue introduced by 728bd991c3c4, that has added support for 2PC files
in shared memory at recovery.

Before this feature was introduced, files in the future of the
transaction ID horizon were checked first, followed by a check if a
transaction ID is aborted or committed which could involve a pg_xact
lookup.  After this commit, these checks were done in a reversed order,
but files in the future do not have a state that can be checked in
pg_xact, hence this caused recovery to fail abruptly should an orphaned
2PC file in the future of the transaction horizon exist in pg_twophase
at the beginning of recovery.

A test is added to check this scenario, using an empty 2PC with a
transaction ID large enough to be in the future when running the test.

Author: Vitaly Davydov, Michael Paquier
Discussion: https://postgr.es/m/11e597-676ab680-8d-374f23c0@145466129
Backpatch-through: 13
---
 src/backend/access/transam/twophase.c | 40 ++++++++++-----------
 src/test/recovery/t/009_twophase.pl   | 51 ++++++++++-----------------
 2 files changed, 38 insertions(+), 53 deletions(-)

diff --git a/src/backend/access/transam/twophase.c b/src/backend/access/tra=
nsam/twophase.c
index d2649264f9..4b5a3146f1 100644
--- a/src/backend/access/transam/twophase.c
+++ b/src/backend/access/transam/twophase.c
@@ -2205,26 +2205,6 @@ ProcessTwoPhaseBuffer(TransactionId xid,
 	if (!fromdisk)
 		Assert(prepare_start_lsn !=3D InvalidXLogRecPtr);
=20
-	/* Already processed? */
-	if (TransactionIdDidCommit(xid) || TransactionIdDidAbort(xid))
-	{
-		if (fromdisk)
-		{
-			ereport(WARNING,
-					(errmsg("removing stale two-phase state file for transaction %u",
-							xid)));
-			RemoveTwoPhaseFile(xid, true);
-		}
-		else
-		{
-			ereport(WARNING,
-					(errmsg("removing stale two-phase state from memory for transaction %=
u",
-							xid)));
-			PrepareRedoRemove(xid, true);
-		}
-		return NULL;
-	}
-
 	/* Reject XID if too new */
 	if (TransactionIdFollowsOrEquals(xid, origNextXid))
 	{
@@ -2245,6 +2225,26 @@ ProcessTwoPhaseBuffer(TransactionId xid,
 		return NULL;
 	}
=20
+	/* Already processed? */
+	if (TransactionIdDidCommit(xid) || TransactionIdDidAbort(xid))
+	{
+		if (fromdisk)
+		{
+			ereport(WARNING,
+					(errmsg("removing stale two-phase state file for transaction %u",
+							xid)));
+			RemoveTwoPhaseFile(xid, true);
+		}
+		else
+		{
+			ereport(WARNING,
+					(errmsg("removing stale two-phase state from memory for transaction %=
u",
+							xid)));
+			PrepareRedoRemove(xid, true);
+		}
+		return NULL;
+	}
+
 	if (fromdisk)
 	{
 		/* Read and validate file */
diff --git a/src/test/recovery/t/009_twophase.pl b/src/test/recovery/t/009_=
twophase.pl
index 4b3e0f77dc..854910ea68 100644
--- a/src/test/recovery/t/009_twophase.pl
+++ b/src/test/recovery/t/009_twophase.pl
@@ -534,42 +534,27 @@ is( $psql_out,
 	qq{27|issued to paris},
 	"Check expected t_009_tbl2 data on standby");
=20
+##########################################################################=
#####
+# Check handling of orphaned 2PC files at recovery.
+##########################################################################=
#####
=20
-# Exercise the 2PC recovery code in StartupSUBTRANS, which is concerned wi=
th
-# ensuring that enough pg_subtrans pages exist on disk to cover the range =
of
-# prepared transactions at server start time.  There's not much we can ver=
ify
-# directly, but let's at least get the code to run.
-$cur_standby->stop();
-configure_and_reload($cur_primary, "synchronous_standby_names =3D ''");
+$cur_primary->teardown_node;
=20
-$cur_primary->safe_psql('postgres', "CHECKPOINT");
+# Grab location in logs of primary
+my $log_offset =3D -s $cur_primary->logfile;
+
+# Create a fake file with a transaction ID large enough to be in the futur=
e,
+# then check that the primary is able to start and remove this file at
+# recovery.
+
+my $future_2pc_file =3D $cur_primary->data_dir . '/pg_twophase/0000000000F=
FFFFF';
+append_to_file $future_2pc_file, "";
=20
-my $start_lsn =3D
-  $cur_primary->safe_psql('postgres', 'select pg_current_wal_insert_lsn()'=
);
-$cur_primary->safe_psql('postgres',
-	"CREATE TABLE test(); BEGIN; CREATE TABLE test1(); PREPARE TRANSACTION 'f=
oo';"
-);
-my $osubtrans =3D $cur_primary->safe_psql('postgres',
-	"select 'pg_subtrans/'||f, s.size from pg_ls_dir('pg_subtrans') f, pg_sta=
t_file('pg_subtrans/'||f) s"
-);
-$cur_primary->pgbench(
-	'--no-vacuum --client=3D5 --transactions=3D1000',
-	0,
-	[],
-	[],
-	'pgbench run to cause pg_subtrans traffic',
-	{
-		'009_twophase.pgb' =3D> 'insert into test default values'
-	});
-# StartupSUBTRANS is exercised with a wide range of visible XIDs in this
-# stop/start sequence, because we left a prepared transaction open above.
-# Also, setting subtransaction_buffers to 32 above causes to switch SLRU
-# bank, for additional code coverage.
-$cur_primary->stop;
 $cur_primary->start;
-my $nsubtrans =3D $cur_primary->safe_psql('postgres',
-	"select 'pg_subtrans/'||f, s.size from pg_ls_dir('pg_subtrans') f, pg_sta=
t_file('pg_subtrans/'||f) s"
-);
-isnt($osubtrans, $nsubtrans, "contents of pg_subtrans/ have changed");
+$cur_primary->log_check(
+	"future two-phase file removed at recovery",
+	$log_offset,
+	log_like =3D>
+	  [qr/removing future two-phase state file for transaction 16777215/]);
=20
 done_testing();
--=20
2.45.2


--eIUwJeSCLm7Q3E1L
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
	filename="v1-0002-Fix-handling-of-orphaned-2PC-files-in-the-fut-v16.patch"
Content-Transfer-Encoding: quoted-printable

=46rom 854c87ae2ec2a44457143f24bc457158d2dac3ac Mon Sep 17 00:00:00 2001
=46rom: Michael Paquier <michael@paquier.xyz>
Date: Thu, 26 Dec 2024 13:26:46 +0900
Subject: [PATCH v1] Fix handling of orphaned 2PC files in the future at
 recovery

Issue introduced by 728bd991c3c4, that has added support for 2PC files
in shared memory at recovery.

Before this feature was introduced, files in the future of the
transaction ID horizon were checked first, followed by a check if a
transaction ID is aborted or committed which could involve a pg_xact
lookup.  After this commit, these checks were done in a reversed order,
but files in the future do not have a state that can be checked in
pg_xact, hence this caused recovery to fail abruptly should an orphaned
2PC file in the future of the transaction horizon exist in pg_twophase
at the beginning of recovery.

A test is added to check this scenario, using an empty 2PC with a
transaction ID large enough to be in the future when running the test.

Author: Vitaly Davydov, Michael Paquier
Discussion: https://postgr.es/m/11e597-676ab680-8d-374f23c0@145466129
Backpatch-through: 13
---
 src/backend/access/transam/twophase.c | 40 +++++++++++++--------------
 src/test/recovery/t/009_twophase.pl   | 23 +++++++++++++++
 2 files changed, 43 insertions(+), 20 deletions(-)

diff --git a/src/backend/access/transam/twophase.c b/src/backend/access/tra=
nsam/twophase.c
index bca653e485..5adb98163a 100644
--- a/src/backend/access/transam/twophase.c
+++ b/src/backend/access/transam/twophase.c
@@ -2183,26 +2183,6 @@ ProcessTwoPhaseBuffer(TransactionId xid,
 	if (!fromdisk)
 		Assert(prepare_start_lsn !=3D InvalidXLogRecPtr);
=20
-	/* Already processed? */
-	if (TransactionIdDidCommit(xid) || TransactionIdDidAbort(xid))
-	{
-		if (fromdisk)
-		{
-			ereport(WARNING,
-					(errmsg("removing stale two-phase state file for transaction %u",
-							xid)));
-			RemoveTwoPhaseFile(xid, true);
-		}
-		else
-		{
-			ereport(WARNING,
-					(errmsg("removing stale two-phase state from memory for transaction %=
u",
-							xid)));
-			PrepareRedoRemove(xid, true);
-		}
-		return NULL;
-	}
-
 	/* Reject XID if too new */
 	if (TransactionIdFollowsOrEquals(xid, origNextXid))
 	{
@@ -2223,6 +2203,26 @@ ProcessTwoPhaseBuffer(TransactionId xid,
 		return NULL;
 	}
=20
+	/* Already processed? */
+	if (TransactionIdDidCommit(xid) || TransactionIdDidAbort(xid))
+	{
+		if (fromdisk)
+		{
+			ereport(WARNING,
+					(errmsg("removing stale two-phase state file for transaction %u",
+							xid)));
+			RemoveTwoPhaseFile(xid, true);
+		}
+		else
+		{
+			ereport(WARNING,
+					(errmsg("removing stale two-phase state from memory for transaction %=
u",
+							xid)));
+			PrepareRedoRemove(xid, true);
+		}
+		return NULL;
+	}
+
 	if (fromdisk)
 	{
 		/* Read and validate file */
diff --git a/src/test/recovery/t/009_twophase.pl b/src/test/recovery/t/009_=
twophase.pl
index fe7e8e7980..7642e4f0b3 100644
--- a/src/test/recovery/t/009_twophase.pl
+++ b/src/test/recovery/t/009_twophase.pl
@@ -528,4 +528,27 @@ is( $psql_out,
 	qq{27|issued to paris},
 	"Check expected t_009_tbl2 data on standby");
=20
+##########################################################################=
#####
+# Check handling of orphaned 2PC files at recovery.
+##########################################################################=
#####
+
+$cur_primary->teardown_node;
+
+# Grab location in logs of primary
+my $log_offset =3D -s $cur_primary->logfile;
+
+# Create a fake file with a transaction ID large enough to be in the futur=
e,
+# then check that the primary is able to start and remove this file at
+# recovery.
+
+my $future_2pc_file =3D $cur_primary->data_dir . '/pg_twophase/00FFFFFF';
+append_to_file $future_2pc_file, "";
+
+$cur_primary->start;
+$cur_primary->log_check(
+	"future two-phase file removed at recovery",
+	$log_offset,
+	log_like =3D>
+	  [qr/removing future two-phase state file for transaction 16777215/]);
+
 done_testing();
--=20
2.45.2


--eIUwJeSCLm7Q3E1L--

--l0hP8fKFOi4g3tJg
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQIzBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAmds6vgACgkQnvQgOdby
QH1G7w//Y3faT0X3RL2lmemlJwTTt9KTAcPYAnVtA47AF/undPHoZXa4XatWojFY
sG6eHgJx5eDnJ/buKtMFhPwsvFTRHWHO7X3qrp8AlHB6vhdPgXLQSju+Z2cNrBNX
EyjayqFGURDIERTF15qyGZaVmFlJRPVByZ2ds5W3XR2iemZJQuO+SaBOTB1q+gII
EznvrwB1eQ2iHt0SNPWivoX7utz5eLe6of4WyHYTDEGstPWpoAyyXpv7BO5O/Tsi
/bCoawRwB7w8hdA2T9TChGOdUyV0GY1K8ls1ONInADV4LRcM3a6ZHqXUitZlJI/k
UK0GsnK1BwL9dbbz2ZmugD35ZnJEF4EUnRGincgybTd6uVyUVhPMuvQd2WXE8VWv
kXfMc9JDolfq8PtmBooUO76OltdwbsIrvxgbXXQpxLE77rpb53ofYvB//3z43+3u
6Uy4stIWdzthIYFOVBUxOdVd411k+JJYg/wMYcwkGI+MGpy93pEm02d9xTzhvECO
iz68K5276/nzGzRhGxelcpNtiH4SAcP0uFranp+lJp2yg8f3CvtcqgtYVxnzFqY7
816PSE8wLsemzAifQhf5Dlrd1QqySkz2sR06lwPzeuS22DK7J+V+2NWR0AdkrLV2
lUTNMppMWc4O/EYnrRACwaWvCY3+c7Y1a53tK94x9MS4eU1cCY8=
=cO7F
-----END PGP SIGNATURE-----

--l0hP8fKFOi4g3tJg--