From: Michael Paquier <michael@paquier.xyz>
To: exclusion@gmail.com
To: pgsql-bugs@lists.postgresql.org
Subject: Re: BUG #16780: Inconsistent recovery_target_xid handling across platforms
Date: Tue, 22 Dec 2020 14:02:46 +0900
Message-ID: <X+F99vxLRK8MXAni@paquier.xyz> (raw)
In-Reply-To: <X+BLDQOGr4V5oeBz@paquier.xyz>
References: <16780-107fd0c0385b1035@postgresql.org>
<X+BLDQOGr4V5oeBz@paquier.xyz>
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
Attachments:recovery-target-xid-parse.patch (text/x-diff, 1.1K) downloaddiff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.cindex 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 **extra, GucSource source)
TransactionId *myextra;
errno = 0;
- xid = (TransactionId) strtoul(*newval, NULL, 0);+ xid = (TransactionId) pg_strtouint64(*newval, NULL, 0);
if (errno == EINVAL || errno == ERANGE)
return false;
diff --git a/src/test/recovery/t/003_recovery_targets.pl b/src/test/recovery/t/003_recovery_targets.plindex 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 = get_new_node('primary');
$node_primary->init(has_archiving => 1, allows_streaming => 1);
+# 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;
signature.asc (application/pgp-signature, 832B) download
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-bugs@postgresql.org
Cc: michael@paquier.xyz, exclusion@gmail.com, pgsql-bugs@lists.postgresql.org
Subject: Re: BUG #16780: Inconsistent recovery_target_xid handling across platforms
In-Reply-To: <X+F99vxLRK8MXAni@paquier.xyz>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox