Date: Tue, 22 Dec 2020 14:02:46 +0900
From: Michael Paquier <michael@paquier.xyz>
To: exclusion@gmail.com, pgsql-bugs@lists.postgresql.org
Subject: Re: BUG #16780: Inconsistent recovery_target_xid handling across
 platforms
Message-ID: <X+F99vxLRK8MXAni@paquier.xyz>
References: <16780-107fd0c0385b1035@postgresql.org>
 <X+BLDQOGr4V5oeBz@paquier.xyz>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="iRh/4sSWgNkS+Frl"
Content-Disposition: inline
In-Reply-To: <X+BLDQOGr4V5oeBz@paquier.xyz>
Precedence: bulk


--iRh/4sSWgNkS+Frl
Content-Type: multipart/mixed; boundary="3mE+8QWrd/d8kQv6"
Content-Disposition: inline


--3mE+8QWrd/d8kQv6
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

On Mon, Dec 21, 2020 at 04:13:17PM +0900, Michael Paquier wrote:
> In order to make the experience consistent across all platforms, we
> can use pg_strtouint64() for the GUC parsing.  I agree that ignoring
> the high bits can be confusing as recovery would stop once a XID
> matches with a modulo of UINT32_MAX, but beginning to issue an error
> on platforms where unsigned long is 8 bytes could also break existing
> tools.  At the same time, WAL record headers and 2PC state data only
> use 32-bit XIDs, so ignoring the high bits changes nothing at
> recovery (XIDs don't go across more than one epoch in the WAL stream
> AFAIK).  In short, I agree that there is room for improvement here and
> that we should just allow the case to work by replacing the use of
> strtoul() to pg_strtouint64().  I am ready to bet that a lot of users
> setting a target XID rely on the return result of txid_current() or
> pg_current_xact_id() similarly to the test failing here.  And once the
> epoch increments, the problems begin.

So, attached is a patch to make the logic more consistent that I would
like to apply down to 12.  Please let me know if there are any
objections or comments.
--
Michael

--3mE+8QWrd/d8kQv6
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment; filename="recovery-target-xid-parse.patch"
Content-Transfer-Encoding: quoted-printable

diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index dabcbb0736..878fcc2236 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -11938,7 +11938,7 @@ check_recovery_target_xid(char **newval, void **ext=
ra, GucSource source)
 		TransactionId *myextra;
=20
 		errno =3D 0;
-		xid =3D (TransactionId) strtoul(*newval, NULL, 0);
+		xid =3D (TransactionId) pg_strtouint64(*newval, NULL, 0);
 		if (errno =3D=3D EINVAL || errno =3D=3D ERANGE)
 			return false;
=20
diff --git a/src/test/recovery/t/003_recovery_targets.pl b/src/test/recover=
y/t/003_recovery_targets.pl
index cc701c5539..dd8e1cc47c 100644
--- a/src/test/recovery/t/003_recovery_targets.pl
+++ b/src/test/recovery/t/003_recovery_targets.pl
@@ -50,6 +50,10 @@ sub test_recovery_standby
 my $node_primary =3D get_new_node('primary');
 $node_primary->init(has_archiving =3D> 1, allows_streaming =3D> 1);
=20
+# Bump the transaction ID epoch.  This is useful to stress the portability
+# of recovery_target_xid parsing.
+system_or_bail('pg_resetwal', '--epoch', '1', $node_primary->data_dir);
+
 # Start it
 $node_primary->start;
=20

--3mE+8QWrd/d8kQv6--

--iRh/4sSWgNkS+Frl
Content-Type: application/pgp-signature; name="signature.asc"

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

iQIzBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAl/hffYACgkQnvQgOdby
QH19IA/+KXYtTLrVMjbygSQAZWTxaNyeF7bXuFX4Da/kgORSSg4zONv5VvSvLw1L
CGxe3fr4TrZrM8kZ0g+jwInVd19AHQp4qm9sdA/dNUDXPBrSG3eEP3u8HQ9ozQTB
HBY/fSvdkqbc2I6lEb5Bv3VJfVf0EtkP5slgXaFFrSk1lIEsxqcMRM9//Mh6rdBv
a9oY6kIOUY8RlybxrgKEVEMrkQ0dXG869wQ7Odlo7sOfMxsvMKvTTB4LXb63yDV+
M7thLiTpZ5M6tRrNMUUIpZ+d4lw/j8eVbwGLjrlZ6JWIl3JkwfpeKDEcPlheVRVg
rGMNFIqiV4InWk8P7M2nVomQadzg9QGHdRkd00z3f44oX92gaoQtyqavCb2xbkgC
cyjyz7AVLdQo2nzjUUJwdHPvecVFV0Aug/84i/kFxxQIMnyhb3ZuTjF/huA8YlLW
HujFEOaGo7DSzxbwF5xhE14ifXK8fByx4AW/4fN2W6IJ49Ex9rTABB4HRnrccduW
/6Ug991qppyW+H1hWfDpOVhg7yfMpWUG3QbIzb1q7xEvGKQ8sVt4zIIF7W8py/7W
9HflWLsHPbHisOd6/9QXnTzJHutHzBfrRBBgQYNimkN2nBcxB2hFf4ZfPMFpOXvW
jIxDHkSsxSXC+nDiJ0Banuks1aKpmEwCAtWPYJ2J1nf/vDLkJFo=
=Fn0A
-----END PGP SIGNATURE-----

--iRh/4sSWgNkS+Frl--