Hi,
On 2023-01-10 21:32:54 +0100, Matthias van de Meent wrote:
> On Tue, 10 Jan 2023 at 20:14, Andres Freund <and...@anarazel.de> wrote:
> > On 2023-01-10 15:03:42 +0100, Matthias van de Meent wrote:
> > What precisely do you mean with "skew" here? Do you just mean that it'd
> > take a
> > long time until vacuum_defer_cleanup_age takes effect? Somehow it sounds
> > like
> > you might mean more than that?
>
> h->oldest_considered_running can be extremely old due to the global
> nature of the value and the potential existence of a snapshot in
> another database that started in parallel to a very old running
> transaction.
Here's a version that, I think, does not have that issue.
In an earlier, not posted, version I had an vacuum_defer_cleanup_age specific
helper function for this, but it seems likely we'll need it in other places
too. So I named it TransactionIdRetreatSafely(). I made it accept the xid by
pointer, as the line lengths / repetition otherwise end up making it hard to
read the code. For now I have TransactionIdRetreatSafely() be private to
procarray.c, but I expect we'll have to change that eventually.
Not sure I like TransactionIdRetreatSafely() as a name. Maybe
TransactionIdRetreatClamped() is better?
I've been working on a test for vacuum_defer_cleanup_age. It does catch the
corruption at hand, but not much more. It's quite painful to write, right
now. Some of the reasons:
https://postgr.es/m/20230130194350.zj5v467x4jgqt3d6%40awork3.anarazel.de
> > I'm tempted to go with reinterpreting 64bit xids as signed. Except that it
> > seems like a mighty invasive change to backpatch.
>
> I'm not sure either. Protecting against underflow by halving the
> effective valid value space is quite the intervention, but if it is
> necessary to make this work in a performant manner, it would be worth
> it. Maybe someone else with more experience can provide their opinion
> here.
The attached assertions just removes 1/2**32'ths of the space, by reserving
the xid range with the upper 32bit set as something that shouldn't be
reachable.
Still requires us to change the input routines to reject that range, but I
think that's a worthy tradeoff. I didn't find the existing limits for the
type to be documented anywhere.
Obviously something like that could only go into HEAD.
Greetings,
Andres Freund
>From 8be634900796630a772c0131925f38f136fed599 Mon Sep 17 00:00:00 2001
From: Andres Freund <and...@anarazel.de>
Date: Mon, 9 Jan 2023 10:23:10 -0800
Subject: [PATCH v3 1/6] WIP: Fix corruption due to vacuum_defer_cleanup_age
underflowing 64bit xids
Author:
Reviewed-by:
Discussion: https://postgr.es/m/20230108002923.cyoser3ttmt63...@awork3.anarazel.de
Backpatch:
---
src/backend/storage/ipc/procarray.c | 85 +++++++++++++++++++++++++----
1 file changed, 73 insertions(+), 12 deletions(-)
diff --git a/src/backend/storage/ipc/procarray.c b/src/backend/storage/ipc/procarray.c
index 4340bf96416..64d0896b23b 100644
--- a/src/backend/storage/ipc/procarray.c
+++ b/src/backend/storage/ipc/procarray.c
@@ -367,6 +367,9 @@ static inline void ProcArrayEndTransactionInternal(PGPROC *proc, TransactionId l
static void ProcArrayGroupClearXid(PGPROC *proc, TransactionId latestXid);
static void MaintainLatestCompletedXid(TransactionId latestXid);
static void MaintainLatestCompletedXidRecovery(TransactionId latestXid);
+static void TransactionIdRetreatSafely(TransactionId *xid,
+ int retreat_by,
+ FullTransactionId rel);
static inline FullTransactionId FullXidRelativeTo(FullTransactionId rel,
TransactionId xid);
@@ -1888,17 +1891,35 @@ ComputeXidHorizons(ComputeXidHorizonsResult *h)
* so guc.c should limit it to no more than the xidStopLimit threshold
* in varsup.c. Also note that we intentionally don't apply
* vacuum_defer_cleanup_age on standby servers.
+ *
+ * Need to use TransactionIdRetreatSafely() instead of open-coding the
+ * subtraction, to prevent creating an xid before
+ * FirstNormalTransactionId.
*/
- h->oldest_considered_running =
- TransactionIdRetreatedBy(h->oldest_considered_running,
- vacuum_defer_cleanup_age);
- h->shared_oldest_nonremovable =
- TransactionIdRetreatedBy(h->shared_oldest_nonremovable,
- vacuum_defer_cleanup_age);
- h->data_oldest_nonremovable =
- TransactionIdRetreatedBy(h->data_oldest_nonremovable,
- vacuum_defer_cleanup_age);
- /* defer doesn't apply to temp relations */
+ Assert(TransactionIdPrecedesOrEquals(h->oldest_considered_running,
+ h->shared_oldest_nonremovable));
+ Assert(TransactionIdPrecedesOrEquals(h->shared_oldest_nonremovable,
+ h->data_oldest_nonremovable));
+
+ if (vacuum_defer_cleanup_age > 0)
+ {
+ TransactionIdRetreatSafely(&h->oldest_considered_running,
+ vacuum_defer_cleanup_age,
+ h->latest_completed);
+ TransactionIdRetreatSafely(&h->shared_oldest_nonremovable,
+ vacuum_defer_cleanup_age,
+ h->latest_completed);
+ TransactionIdRetreatSafely(&h->data_oldest_nonremovable,
+ vacuum_defer_cleanup_age,
+ h->latest_completed);
+ /* defer doesn't apply to temp relations */
+
+
+ Assert(TransactionIdPrecedesOrEquals(h->oldest_considered_running,
+ h->shared_oldest_nonremovable));
+ Assert(TransactionIdPrecedesOrEquals(h->shared_oldest_nonremovable,
+ h->data_oldest_nonremovable));
+ }
}
/*
@@ -2470,8 +2491,10 @@ GetSnapshotData(Snapshot snapshot)
oldestfxid = FullXidRelativeTo(latest_completed, oldestxid);
/* apply vacuum_defer_cleanup_age */
- def_vis_xid_data =
- TransactionIdRetreatedBy(xmin, vacuum_defer_cleanup_age);
+ def_vis_xid_data = xmin;
+ TransactionIdRetreatSafely(&def_vis_xid_data,
+ vacuum_defer_cleanup_age,
+ oldestfxid);
/* Check whether there's a replication slot requiring an older xmin. */
def_vis_xid_data =
@@ -4295,6 +4318,44 @@ GlobalVisCheckRemovableXid(Relation rel, TransactionId xid)
return GlobalVisTestIsRemovableXid(state, xid);
}
+/*
+ * Safely retract *xid by retreat_by, store the result in *xid.
+ *
+ * Need to be careful to prevent *xid from retreating below
+ * FirstNormalTransactionId during epoch 0. This is important to prevent
+ * generating xids that cannot be converted to a FullTransactionId without
+ * wrapping around.
+ *
+ * If retreat_by would lead to a too old xid, FirstNormalTransactionId is
+ * returned instead.
+ */
+static void
+TransactionIdRetreatSafely(TransactionId *xid, int retreat_by, FullTransactionId rel)
+{
+ TransactionId original_xid = *xid;
+ FullTransactionId fxid;
+ uint64 fxid_i;
+
+ Assert(TransactionIdIsNormal(original_xid));
+ Assert(retreat_by >= 0); /* relevant GUCs are stored as ints */
+ AssertTransactionIdInAllowableRange(original_xid);
+
+ if (retreat_by == 0)
+ return;
+
+ fxid = FullXidRelativeTo(rel, original_xid);
+ fxid_i = U64FromFullTransactionId(fxid);
+
+ if ((fxid_i - FirstNormalTransactionId) <= retreat_by)
+ *xid = FirstNormalTransactionId;
+ else
+ {
+ *xid = TransactionIdRetreatedBy(original_xid, retreat_by);
+ Assert(TransactionIdIsNormal(*xid));
+ Assert(NormalTransactionIdPrecedes(*xid, original_xid));
+ }
+}
+
/*
* Convert a 32 bit transaction id into 64 bit transaction id, by assuming it
* is within MaxTransactionId / 2 of XidFromFullTransactionId(rel).
--
2.38.0
>From 356f709f3173da84c464b7aaedac5a8595619610 Mon Sep 17 00:00:00 2001
From: Andres Freund <and...@anarazel.de>
Date: Mon, 30 Jan 2023 11:50:44 -0800
Subject: [PATCH v3 2/6] WIP: very incomplete test for vacuum_defer_cleanup_age
But it's enough to notice the corruption without the bugfix
---
src/test/recovery/meson.build | 1 +
src/test/recovery/t/034_defer_cleanup.pl | 173 +++++++++++++++++++++++
2 files changed, 174 insertions(+)
create mode 100644 src/test/recovery/t/034_defer_cleanup.pl
diff --git a/src/test/recovery/meson.build b/src/test/recovery/meson.build
index edaaa1a3ce5..da355782b02 100644
--- a/src/test/recovery/meson.build
+++ b/src/test/recovery/meson.build
@@ -40,6 +40,7 @@ tests += {
't/031_recovery_conflict.pl',
't/032_relfilenode_reuse.pl',
't/033_replay_tsp_drops.pl',
+ 't/034_defer_cleanup.pl',
],
},
}
diff --git a/src/test/recovery/t/034_defer_cleanup.pl b/src/test/recovery/t/034_defer_cleanup.pl
new file mode 100644
index 00000000000..77ce6037db8
--- /dev/null
+++ b/src/test/recovery/t/034_defer_cleanup.pl
@@ -0,0 +1,173 @@
+# Copyright (c) 2023, PostgreSQL Global Development Group
+
+# Check that vacuum_defer_cleanup_age works.
+
+use strict;
+use warnings;
+
+use PostgreSQL::Test::Cluster;
+use PostgreSQL::Test::Utils;
+use Test::More;
+
+my $primary = PostgreSQL::Test::Cluster->new('primary');
+$primary->init(allows_streaming => 1);
+$primary->start;
+
+# to make things more predictable, we schedule all vacuums ourselves
+$primary->append_conf('postgresql.conf', qq[
+autovacuum = 0
+log_line_prefix='%m [%p][%b][%v:%x][%a] '
+]);
+
+$primary->backup('backup');
+my $standby = PostgreSQL::Test::Cluster->new('standby');
+$standby->init_from_backup($primary, 'backup',
+ has_streaming => 1);
+
+# vacuum_defer_cleanup_age only makes sense without feedback
+# We don't want to ever wait for the standby to apply, otherwise the test will
+# take forever.
+$standby->append_conf('postgresql.conf', qq[
+hot_standby_feedback = 0
+max_standby_streaming_delay = 0
+]);
+$standby->start;
+
+# to avoid constantly needing to wait manually, use syncrep with remote_apply
+$primary->append_conf('postgresql.conf', qq[
+synchronous_standby_names = '*'
+synchronous_commit = remote_apply
+]);
+
+$standby->reload;
+
+
+# Find original txid at start, we want to use a cleanup age bigger than that,
+# because we had bugs with horizons wrapping around to before the big bang
+my $xid_at_start = $primary->safe_psql('postgres', 'SELECT txid_current()');
+my $defer_age = $xid_at_start + 100;
+
+note "Initial xid is $xid_at_start, will set defer to $defer_age";
+
+$primary->safe_psql('postgres', qq[
+ ALTER SYSTEM SET vacuum_defer_cleanup_age = $defer_age;
+ SELECT pg_reload_conf();
+]);
+
+$primary->safe_psql('postgres', qq[
+ CREATE TABLE testdata(id int not null unique, data text);
+ INSERT INTO testdata(id, data) VALUES(1, '1');
+ INSERT INTO testdata(id, data) VALUES(2, '2');
+ INSERT INTO testdata(id, data) VALUES(3, '3');
+ INSERT INTO testdata(id, data) VALUES(4, '4');
+ INSERT INTO testdata(id, data) VALUES(5, '5');
+]);
+
+
+# start a background psql on both nodes, to test effects on running sessions
+my $psql_timeout = IPC::Run::timer($PostgreSQL::Test::Utils::timeout_default);
+my %psql_primary = ('stdin' => '', 'stdout' => '');
+$psql_primary{run} =
+ $primary->background_psql('postgres', \$psql_primary{stdin},
+ \$psql_primary{stdout},
+ $psql_timeout);
+
+my %psql_standby = ('stdin' => '', 'stdout' => '');
+$psql_standby{run} =
+ $standby->background_psql('postgres', \$psql_standby{stdin},
+ \$psql_standby{stdout},
+ $psql_timeout);
+
+$psql_primary{stdin} .= '\t\set QUIET off';
+$psql_standby{stdin} .= '\t\set QUIET off';
+
+# start transaction on the standby
+my $q = qq[
+SET application_name = 'background_psql';
+BEGIN TRANSACTION ISOLATION LEVEL REPEATABLE READ;
+SELECT 1;
+];
+$psql_primary{stdin} .= $q;
+$psql_standby{stdin} .= $q;
+ok( pump_until_primary(qr/\n\(1 row\)\n$/s), 'started transaction');
+ok( pump_until_standby(qr/\n\(1 row\)\n$/s), 'started transaction');
+
+# update a row on the primary, select from the table to remove old row version
+my $res = $primary->safe_psql('postgres', qq[
+ SELECT txid_current();
+ UPDATE testdata SET data = data || '-updated' WHERE id = 1;
+ SELECT txid_current();
+ VACUUM testdata;
+ SELECT count(*) FROM testdata;
+ SELECT count(*) FROM testdata;
+ SELECT txid_current();
+ SET enable_seqscan = 0;
+ EXPLAIN SELECT * FROM testdata WHERE id = 1;
+ SELECT * FROM testdata WHERE id = 1;
+]);
+
+
+# check query result is as expected on primary and on a new snapshot on the standby
+$q = "SELECT data FROM testdata WHERE id = 1";
+is($primary->safe_psql('postgres', $q), '1-updated', "query result on primary as expected");
+is($standby->safe_psql('postgres', $q), '1-updated', "query result on standby as expected");
+
+$q = "SELECT data FROM testdata WHERE id = 1;\n";
+$psql_primary{stdin} .= $q;
+$psql_standby{stdin} .= $q;
+
+$res = pump_until_primary(qr/^.*\n\(1 row\)\n$/s);
+my $expect = "data\n1\n(1 row)\n";
+is( $res, $expect, 'version 1 still visible on primary after update on primary');
+$res = pump_until_standby(qr/.*\n\(1 row\)\n$/s);
+is( $res, $expect, 'row 1, version 1 still visible on standby after update on primary');
+
+$psql_primary{stdin} .= "COMMIT;\n";
+is(pump_until_primary(qr/^COMMIT\n/m), "COMMIT\n", "release on primary");
+
+
+done_testing();
+
+sub pump_until_node
+{
+ my $psql = shift;
+ my $match = shift;
+ my $ret;
+
+ pump_until($$psql{run}, $psql_timeout,
+ \$$psql{stdout}, $match);
+ $ret = $$psql{stdout};
+ $$psql{stdout} = '';
+ return $ret;
+}
+
+sub pump_until_primary
+{
+ my $match = shift;
+
+ return pump_until_node(\%psql_primary, $match);
+}
+
+sub pump_until_standby
+{
+ my $match = shift;
+
+ return pump_until_node(\%psql_standby, $match);
+}
+
+sub burn_xids
+{
+ my $cnt = shift;
+
+ my $out = $primary->safe_psql('postgres', qq[
+SELECT txid_current();
+DO $$
+ BEGIN FOR i IN 1..100 LOOP
+ PERFORM txid_current();
+ COMMIT AND CHAIN;
+ END LOOP;
+END; $$;
+SELECT txid_current();
+]);
+ note $out;
+}
--
2.38.0
>From a3032b89ce8e8e325035e2858a2b58a92d6ede7c Mon Sep 17 00:00:00 2001
From: Andres Freund <and...@anarazel.de>
Date: Mon, 9 Jan 2023 11:24:37 -0800
Subject: [PATCH v3 3/6] wip: amcheck: Fix bugs relating to 64bit xids
Discussion: https://postgr.es/m/20230108002923.cyoser3ttmt63...@awork3.anarazel.de
---
contrib/amcheck/verify_heapam.c | 16 +++++++++++++++-
1 file changed, 15 insertions(+), 1 deletion(-)
diff --git a/contrib/amcheck/verify_heapam.c b/contrib/amcheck/verify_heapam.c
index 4fcfd6df72d..56a3b3e9974 100644
--- a/contrib/amcheck/verify_heapam.c
+++ b/contrib/amcheck/verify_heapam.c
@@ -1576,11 +1576,25 @@ FullTransactionIdFromXidAndCtx(TransactionId xid, const HeapCheckContext *ctx)
{
uint32 epoch;
+ Assert(TransactionIdIsValid(ctx->next_xid));
+ Assert(FullTransactionIdIsValid(ctx->next_fxid));
+
if (!TransactionIdIsNormal(xid))
return FullTransactionIdFromEpochAndXid(0, xid);
epoch = EpochFromFullTransactionId(ctx->next_fxid);
if (xid > ctx->next_xid)
+ {
+ /*
+ * FIXME, doubtful this is the best fix.
+ *
+ * Can't represent the 32bit xid as a 64bit xid, as it's before fxid
+ * 0. Represent it as an xid from the future instead.
+ */
+ if (epoch == 0)
+ return FullTransactionIdFromEpochAndXid(0, xid);
+
epoch--;
+ }
return FullTransactionIdFromEpochAndXid(epoch, xid);
}
@@ -1597,8 +1611,8 @@ update_cached_xid_range(HeapCheckContext *ctx)
LWLockRelease(XidGenLock);
/* And compute alternate versions of the same */
- ctx->oldest_fxid = FullTransactionIdFromXidAndCtx(ctx->oldest_xid, ctx);
ctx->next_xid = XidFromFullTransactionId(ctx->next_fxid);
+ ctx->oldest_fxid = FullTransactionIdFromXidAndCtx(ctx->oldest_xid, ctx);
}
/*
--
2.38.0
>From 5f995d8f3893adf263942606f2e0511e9d21f8f4 Mon Sep 17 00:00:00 2001
From: Andres Freund <and...@anarazel.de>
Date: Mon, 9 Jan 2023 11:25:43 -0800
Subject: [PATCH v3 4/6] wip: stricter validation for 64bit xids
64bit xids can't represent xids before xid 0, but 32bit xids can. This has
lead to a number of bugs, some data corrupting. To make it easier to catch
such bugs, disallow 64bit where the upper 32bit are all set, which is what one
gets when naively trying to convert such 32bit xids. Additionally explicitly
disallow 64bit xids that are already aren't allowed because they'd map to
special 32bit xids.
This changes the input routines for 64bit xids to error out in more cases.
Discussion: https://postgr.es/m/20230108002923.cyoser3ttmt63...@awork3.anarazel.de
---
src/include/access/transam.h | 136 ++++++++++++++++++++++++++++--
src/backend/utils/adt/xid.c | 7 ++
src/backend/utils/adt/xid8funcs.c | 17 ++--
src/test/regress/expected/xid.out | 44 ++++++++--
src/test/regress/sql/xid.sql | 12 ++-
5 files changed, 192 insertions(+), 24 deletions(-)
diff --git a/src/include/access/transam.h b/src/include/access/transam.h
index f5af6d30556..0d4bf57e63a 100644
--- a/src/include/access/transam.h
+++ b/src/include/access/transam.h
@@ -44,15 +44,6 @@
#define TransactionIdStore(xid, dest) (*(dest) = (xid))
#define StoreInvalidTransactionId(dest) (*(dest) = InvalidTransactionId)
-#define EpochFromFullTransactionId(x) ((uint32) ((x).value >> 32))
-#define XidFromFullTransactionId(x) ((uint32) (x).value)
-#define U64FromFullTransactionId(x) ((x).value)
-#define FullTransactionIdEquals(a, b) ((a).value == (b).value)
-#define FullTransactionIdPrecedes(a, b) ((a).value < (b).value)
-#define FullTransactionIdPrecedesOrEquals(a, b) ((a).value <= (b).value)
-#define FullTransactionIdFollows(a, b) ((a).value > (b).value)
-#define FullTransactionIdFollowsOrEquals(a, b) ((a).value >= (b).value)
-#define FullTransactionIdIsValid(x) TransactionIdIsValid(XidFromFullTransactionId(x))
#define InvalidFullTransactionId FullTransactionIdFromEpochAndXid(0, InvalidTransactionId)
#define FirstNormalFullTransactionId FullTransactionIdFromEpochAndXid(0, FirstNormalTransactionId)
#define FullTransactionIdIsNormal(x) FullTransactionIdFollowsOrEquals(x, FirstNormalFullTransactionId)
@@ -61,12 +52,127 @@
* A 64 bit value that contains an epoch and a TransactionId. This is
* wrapped in a struct to prevent implicit conversion to/from TransactionId.
* Not all values represent valid normal XIDs.
+ *
+ * 64bit xids that would map to a non-normal 32bit xid are not allowed, for
+ * obvious reasons.
+
+ * In addition, no xids with all the upper 32bit sets are allowed to exist,
+ * this is to make it easier to catch conversion errors from 32bit xids (which
+ * can point to "before" xid 0).
*/
typedef struct FullTransactionId
{
uint64 value;
} FullTransactionId;
+#define MAX_FULL_TRANSACTION_ID ((((uint64)~(uint32)0)<<32) - 1)
+
+/*
+ * This is separate from FullTransactionIdValidRange() so that input routines
+ * can check for invalid values without triggering an assert.
+ */
+static inline bool
+InFullTransactionIdRange(uint64 val)
+{
+ /* 64bit xid above the upper bound */
+ if (val > MAX_FULL_TRANSACTION_ID)
+ return false;
+
+ /*
+ * normal 64bit xids that'd map to special 32 xids aren't allowed.
+ */
+ if (val >= (uint64) FirstNormalTransactionId &&
+ (uint32) val < FirstNormalTransactionId)
+ return false;
+ return true;
+}
+
+/*
+ * Validation routine, typically used in asserts.
+ */
+static inline bool
+FullTransactionIdValidRange(FullTransactionId x)
+{
+ return InFullTransactionIdRange(x.value);
+}
+
+static inline uint64
+U64FromFullTransactionId(FullTransactionId x)
+{
+ Assert(FullTransactionIdValidRange(x));
+
+ return x.value;
+}
+
+static inline TransactionId
+XidFromFullTransactionId(FullTransactionId x)
+{
+ Assert(FullTransactionIdValidRange(x));
+
+ return (TransactionId) x.value;
+}
+
+static inline uint32
+EpochFromFullTransactionId(FullTransactionId x)
+{
+ Assert(FullTransactionIdValidRange(x));
+
+ return (uint32) (x.value >> 32);
+}
+
+static inline bool
+FullTransactionIdEquals(FullTransactionId a, FullTransactionId b)
+{
+ Assert(FullTransactionIdValidRange(a));
+ Assert(FullTransactionIdValidRange(b));
+
+ return a.value == b.value;
+}
+
+static inline bool
+FullTransactionIdPrecedes(FullTransactionId a, FullTransactionId b)
+{
+ Assert(FullTransactionIdValidRange(a));
+ Assert(FullTransactionIdValidRange(b));
+
+ return a.value < b.value;
+}
+
+static inline bool
+FullTransactionIdPrecedesOrEquals(FullTransactionId a, FullTransactionId b)
+{
+ Assert(FullTransactionIdValidRange(a));
+ Assert(FullTransactionIdValidRange(b));
+
+ return a.value <= b.value;
+}
+
+static inline bool
+FullTransactionIdFollows(FullTransactionId a, FullTransactionId b)
+{
+ Assert(FullTransactionIdValidRange(a));
+ Assert(FullTransactionIdValidRange(b));
+
+ return a.value > b.value;
+}
+
+static inline bool
+FullTransactionIdFollowsOrEquals(FullTransactionId a, FullTransactionId b)
+{
+ Assert(FullTransactionIdValidRange(a));
+ Assert(FullTransactionIdValidRange(b));
+
+ return a.value >= b.value;
+}
+
+static inline bool
+FullTransactionIdIsValid(FullTransactionId x)
+{
+ Assert(FullTransactionIdValidRange(x));
+
+ return x.value != (uint64) InvalidTransactionId;
+}
+
static inline FullTransactionId
FullTransactionIdFromEpochAndXid(uint32 epoch, TransactionId xid)
{
@@ -74,6 +180,8 @@ FullTransactionIdFromEpochAndXid(uint32 epoch, TransactionId xid)
result.value = ((uint64) epoch) << 32 | xid;
+ Assert(FullTransactionIdValidRange(result));
+
return result;
}
@@ -84,6 +192,8 @@ FullTransactionIdFromU64(uint64 value)
result.value = value;
+ Assert(FullTransactionIdValidRange(result));
+
return result;
}
@@ -102,6 +212,8 @@ FullTransactionIdFromU64(uint64 value)
static inline void
FullTransactionIdRetreat(FullTransactionId *dest)
{
+ Assert(FullTransactionIdValidRange(*dest));
+
dest->value--;
/*
@@ -118,6 +230,8 @@ FullTransactionIdRetreat(FullTransactionId *dest)
*/
while (XidFromFullTransactionId(*dest) < FirstNormalTransactionId)
dest->value--;
+
+ Assert(FullTransactionIdValidRange(*dest));
}
/*
@@ -127,6 +241,8 @@ FullTransactionIdRetreat(FullTransactionId *dest)
static inline void
FullTransactionIdAdvance(FullTransactionId *dest)
{
+ Assert(FullTransactionIdValidRange(*dest));
+
dest->value++;
/* see FullTransactionIdAdvance() */
@@ -135,6 +251,8 @@ FullTransactionIdAdvance(FullTransactionId *dest)
while (XidFromFullTransactionId(*dest) < FirstNormalTransactionId)
dest->value++;
+
+ Assert(FullTransactionIdValidRange(*dest));
}
/* back up a transaction ID variable, handling wraparound correctly */
diff --git a/src/backend/utils/adt/xid.c b/src/backend/utils/adt/xid.c
index 8ac1679c381..21c0f4c67df 100644
--- a/src/backend/utils/adt/xid.c
+++ b/src/backend/utils/adt/xid.c
@@ -188,6 +188,13 @@ xid8in(PG_FUNCTION_ARGS)
uint64 result;
result = uint64in_subr(str, NULL, "xid8", fcinfo->context);
+
+ if (!InFullTransactionIdRange(result))
+ ereturn(fcinfo->context, 0,
+ (errcode(ERRCODE_NUMERIC_VALUE_OUT_OF_RANGE),
+ errmsg("value \"%s\" is out of range for type %s",
+ str, "xid8")));
+
PG_RETURN_FULLTRANSACTIONID(FullTransactionIdFromU64(result));
}
diff --git a/src/backend/utils/adt/xid8funcs.c b/src/backend/utils/adt/xid8funcs.c
index e616303a292..8b33d145a36 100644
--- a/src/backend/utils/adt/xid8funcs.c
+++ b/src/backend/utils/adt/xid8funcs.c
@@ -302,15 +302,18 @@ parse_snapshot(const char *str, Node *escontext)
const char *str_start = str;
char *endp;
StringInfo buf;
+ uint64 raw_fxid;
- xmin = FullTransactionIdFromU64(strtou64(str, &endp, 10));
- if (*endp != ':')
+ raw_fxid = strtou64(str, &endp, 10);
+ if (*endp != ':' || !InFullTransactionIdRange(raw_fxid))
goto bad_format;
+ xmin = FullTransactionIdFromU64(raw_fxid);
str = endp + 1;
- xmax = FullTransactionIdFromU64(strtou64(str, &endp, 10));
- if (*endp != ':')
+ raw_fxid = strtou64(str, &endp, 10);
+ if (*endp != ':' || !InFullTransactionIdRange(raw_fxid))
goto bad_format;
+ xmax = FullTransactionIdFromU64(raw_fxid);
str = endp + 1;
/* it should look sane */
@@ -326,7 +329,11 @@ parse_snapshot(const char *str, Node *escontext)
while (*str != '\0')
{
/* read next value */
- val = FullTransactionIdFromU64(strtou64(str, &endp, 10));
+ raw_fxid = strtou64(str, &endp, 10);
+
+ val = FullTransactionIdFromU64(raw_fxid);
+ if (!InFullTransactionIdRange(raw_fxid))
+ goto bad_format;
str = endp;
/* require the input to be in order */
diff --git a/src/test/regress/expected/xid.out b/src/test/regress/expected/xid.out
index e62f7019434..4348eb66bd5 100644
--- a/src/test/regress/expected/xid.out
+++ b/src/test/regress/expected/xid.out
@@ -6,11 +6,11 @@ select '010'::xid,
'-1'::xid,
'010'::xid8,
'42'::xid8,
- '0xffffffffffffffff'::xid8,
- '-1'::xid8;
- xid | xid | xid | xid | xid8 | xid8 | xid8 | xid8
------+-----+------------+------------+------+------+----------------------+----------------------
- 8 | 42 | 4294967295 | 4294967295 | 8 | 42 | 18446744073709551615 | 18446744073709551615
+ '0xefffffffffffffff'::xid8,
+ '0'::xid8;
+ xid | xid | xid | xid | xid8 | xid8 | xid8 | xid8
+-----+-----+------------+------------+------+------+----------------------+------
+ 8 | 42 | 4294967295 | 4294967295 | 8 | 42 | 17293822569102704639 | 0
(1 row)
-- garbage values
@@ -30,6 +30,18 @@ select 'asdf'::xid8;
ERROR: invalid input syntax for type xid8: "asdf"
LINE 1: select 'asdf'::xid8;
^
+select '-1'::xid8;
+ERROR: value "-1" is out of range for type xid8
+LINE 1: select '-1'::xid8;
+ ^
+select '0xffffffffffffffff'::xid8;
+ERROR: value "0xffffffffffffffff" is out of range for type xid8
+LINE 1: select '0xffffffffffffffff'::xid8;
+ ^
+select '0x0000300000000001'::xid8;
+ERROR: value "0x0000300000000001" is out of range for type xid8
+LINE 1: select '0x0000300000000001'::xid8;
+ ^
-- Also try it with non-error-throwing API
SELECT pg_input_is_valid('42', 'xid');
pg_input_is_valid
@@ -67,6 +79,24 @@ SELECT pg_input_error_message('0xffffffffffffffffffff', 'xid8');
value "0xffffffffffffffffffff" is out of range for type xid8
(1 row)
+SELECT pg_input_is_valid('-1', 'xid8');
+ pg_input_is_valid
+-------------------
+ f
+(1 row)
+
+SELECT pg_input_is_valid('0xffffffffffffffff', 'xid8');
+ pg_input_is_valid
+-------------------
+ f
+(1 row)
+
+SELECT pg_input_is_valid('0x0000300000000001', 'xid8');
+ pg_input_is_valid
+-------------------
+ f
+(1 row)
+
-- equality
select '1'::xid = '1'::xid;
?column?
@@ -160,11 +190,11 @@ select xid8cmp('1', '2'), xid8cmp('2', '2'), xid8cmp('2', '1');
-- min() and max() for xid8
create table xid8_t1 (x xid8);
-insert into xid8_t1 values ('0'), ('010'), ('42'), ('0xffffffffffffffff'), ('-1');
+insert into xid8_t1 values ('0'), ('010'), ('42'), ('0xefffffffffffffff');
select min(x), max(x) from xid8_t1;
min | max
-----+----------------------
- 0 | 18446744073709551615
+ 0 | 17293822569102704639
(1 row)
-- xid8 has btree and hash opclasses
diff --git a/src/test/regress/sql/xid.sql b/src/test/regress/sql/xid.sql
index b6996588ef6..b2104e4ce20 100644
--- a/src/test/regress/sql/xid.sql
+++ b/src/test/regress/sql/xid.sql
@@ -7,14 +7,17 @@ select '010'::xid,
'-1'::xid,
'010'::xid8,
'42'::xid8,
- '0xffffffffffffffff'::xid8,
- '-1'::xid8;
+ '0xefffffffffffffff'::xid8,
+ '0'::xid8;
-- garbage values
select ''::xid;
select 'asdf'::xid;
select ''::xid8;
select 'asdf'::xid8;
+select '-1'::xid8;
+select '0xffffffffffffffff'::xid8;
+select '0x0000300000000001'::xid8;
-- Also try it with non-error-throwing API
SELECT pg_input_is_valid('42', 'xid');
@@ -23,6 +26,9 @@ SELECT pg_input_error_message('0xffffffffff', 'xid');
SELECT pg_input_is_valid('42', 'xid8');
SELECT pg_input_is_valid('asdf', 'xid8');
SELECT pg_input_error_message('0xffffffffffffffffffff', 'xid8');
+SELECT pg_input_is_valid('-1', 'xid8');
+SELECT pg_input_is_valid('0xffffffffffffffff', 'xid8');
+SELECT pg_input_is_valid('0x0000300000000001', 'xid8');
-- equality
select '1'::xid = '1'::xid;
@@ -51,7 +57,7 @@ select xid8cmp('1', '2'), xid8cmp('2', '2'), xid8cmp('2', '1');
-- min() and max() for xid8
create table xid8_t1 (x xid8);
-insert into xid8_t1 values ('0'), ('010'), ('42'), ('0xffffffffffffffff'), ('-1');
+insert into xid8_t1 values ('0'), ('010'), ('42'), ('0xefffffffffffffff');
select min(x), max(x) from xid8_t1;
-- xid8 has btree and hash opclasses
--
2.38.0
