pg.ddx.io pgsql-bugs@postgresql.org mailing list archive
help / color / mirror / Atom feedBUG #16780: Inconsistent recovery_target_xid handling across platforms
6+ messages / 3 participants
[nested] [flat]
Commits from this thread:
2020-12-23 6db27037b9 1e54664eee 67a9411d2f 35ad5c7c7e f3ee99df72 0b54a80a38 Fix portability issues with parsing of recovery_target_xid
* BUG #16780: Inconsistent recovery_target_xid handling across platforms
@ 2020-12-18 10:00 PG Bug reporting form <noreply@postgresql.org>
0 siblings, 1 reply; 6+ messages in thread
From: PG Bug reporting form @ 2020-12-18 10:00 UTC (permalink / raw)
To: pgsql-bugs@lists.postgresql.org; +Cc: exclusion@gmail.com
The following bug has been logged on the website:
Bug reference: 16780
Logged by: Alexander Lakhin
Email address: exclusion@gmail.com
PostgreSQL version: 13.1
Operating system: Windows
Description:
While testing src/test/recovery with a non-zero epoch the inconsistency was
discovered.
The 003_recovery_targets.pl test with the slight modification passes on
Linux x86_64, but fails on Windows x64.
The modification is to append just after the $node_master->init:
command([ 'pg_resetwal', '--epoch=1', $node_master->data_dir ]);
And on Windows I get:
t/003_recovery_targets.pl ............ 2/9 Bailout called. Further testing
stopped: pg_ctl start failed
FAILED--Further testing stopped: pg_ctl start failed
regress_log_003_recovery_targets contains:
# pg_ctl start failed; logfile:
2020-12-18 12:25:11.597 MSK [1312] LOG: invalid value for parameter
"recovery_target_xid": "4294967782"
2020-12-18 12:25:11.597 MSK [1312] FATAL: configuration file
".../src/test/recovery/tmp_check/t_003_recovery_targets_standby_2_data/pgdata/postgresql.conf"
contains errors
So on Windows the recovery_target_xid parameter doesn't accept 64-bit
value.
As I see in check_recovery_target_xid() the strtoul() function is used to
check/convert the input value, that is later stored into 32-bit variable.
Thus, on platforms where unsigned long is 32-bit wide, you can't specify a
result of pg_current_xact_id() as recovery_target_xid, while on platforms
where unsigned long is 64-bit, you can, but high 32 bits are just ignored.
^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: BUG #16780: Inconsistent recovery_target_xid handling across platforms
@ 2020-12-21 07:13 Michael Paquier <michael@paquier.xyz>
parent: PG Bug reporting form <noreply@postgresql.org>
0 siblings, 1 reply; 6+ messages in thread
From: Michael Paquier @ 2020-12-21 07:13 UTC (permalink / raw)
To: exclusion@gmail.com; pgsql-bugs@lists.postgresql.org
On Fri, Dec 18, 2020 at 10:00:00AM +0000, PG Bug reporting form wrote:
> So on Windows the recovery_target_xid parameter doesn't accept 64-bit
> value.
> As I see in check_recovery_target_xid() the strtoul() function is used to
> check/convert the input value, that is later stored into 32-bit variable.
> Thus, on platforms where unsigned long is 32-bit wide, you can't specify a
> result of pg_current_xact_id() as recovery_target_xid, while on platforms
> where unsigned long is 64-bit, you can, but high 32 bits are just ignored.
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.
Thoughts?
--
Michael
Attachments:
signature.asc (application/pgp-signature, 832B) download
^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: BUG #16780: Inconsistent recovery_target_xid handling across platforms
@ 2020-12-22 05:02 Michael Paquier <michael@paquier.xyz>
parent: Michael Paquier <michael@paquier.xyz>
0 siblings, 1 reply; 6+ messages in thread
From: Michael Paquier @ 2020-12-22 05:02 UTC (permalink / raw)
To: exclusion@gmail.com; pgsql-bugs@lists.postgresql.org
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) download
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 **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.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 = 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;
* Re: BUG #16780: Inconsistent recovery_target_xid handling across platforms
@ 2020-12-23 04:44 Michael Paquier <michael@paquier.xyz>
parent: Michael Paquier <michael@paquier.xyz>
0 siblings, 1 reply; 6+ messages in thread
From: Michael Paquier @ 2020-12-23 04:44 UTC (permalink / raw)
To: exclusion@gmail.com; pgsql-bugs@lists.postgresql.org
On Tue, Dec 22, 2020 at 02:02:46PM +0900, Michael Paquier wrote:
> 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.
I have looked at that again this morning, and applied a fix down to
9.6 with the change in the TAP tests based on pg_resetwal. In 9.5,
pg_strtouint64() is not available, but this branch is old enough that
it did not seem worth the extra effort.
--
Michael
Attachments:
signature.asc (application/pgp-signature, 832B) download
^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: BUG #16780: Inconsistent recovery_target_xid handling across platforms
@ 2020-12-23 06:00 Alexander Lakhin <exclusion@gmail.com>
parent: Michael Paquier <michael@paquier.xyz>
0 siblings, 1 reply; 6+ messages in thread
From: Alexander Lakhin @ 2020-12-23 06:00 UTC (permalink / raw)
To: Michael Paquier <michael@paquier.xyz>; pgsql-bugs@lists.postgresql.org
23.12.2020 07:44, Michael Paquier wrote:
> On Tue, Dec 22, 2020 at 02:02:46PM +0900, Michael Paquier wrote:
>> 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.
> I have looked at that again this morning, and applied a fix down to
> 9.6 with the change in the TAP tests based on pg_resetwal. In 9.5,
> pg_strtouint64() is not available, but this branch is old enough that
> it did not seem worth the extra effort.
Thank you!
I think that with this change, if the check for epoch will be added
someday, it will work consistently too.
Best regards,
Alexander
^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: BUG #16780: Inconsistent recovery_target_xid handling across platforms
@ 2020-12-23 07:42 Michael Paquier <michael@paquier.xyz>
parent: Alexander Lakhin <exclusion@gmail.com>
0 siblings, 0 replies; 6+ messages in thread
From: Michael Paquier @ 2020-12-23 07:42 UTC (permalink / raw)
To: Alexander Lakhin <exclusion@gmail.com>; +Cc: pgsql-bugs@lists.postgresql.org
On Wed, Dec 23, 2020 at 09:00:00AM +0300, Alexander Lakhin wrote:
> I think that with this change, if the check for epoch will be added
> someday, it will work consistently too.
WAL record size has always been a sensitive topic, and this would add
4 extra bytes to each record. So that's hard to say.
--
Michael
Attachments:
signature.asc (application/pgp-signature, 832B) download
^ permalink raw reply [nested|flat] 6+ messages in thread
end of thread, other threads:[~2020-12-23 07:42 UTC | newest]
Thread overview: 6+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2020-12-18 10:00 BUG #16780: Inconsistent recovery_target_xid handling across platforms PG Bug reporting form <noreply@postgresql.org>
2020-12-21 07:13 ` Michael Paquier <michael@paquier.xyz>
2020-12-22 05:02 ` Michael Paquier <michael@paquier.xyz>
2020-12-23 04:44 ` Michael Paquier <michael@paquier.xyz>
2020-12-23 06:00 ` Alexander Lakhin <exclusion@gmail.com>
2020-12-23 07:42 ` Michael Paquier <michael@paquier.xyz>
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