Re: XMAX_LOCK_ONLY and XMAX_COMMITTED (fk/multixact code)
On 8/26/20, 2:13 PM, "Bossart, Nathan" <bossartn@amazon.com> wrote:
On 8/26/20, 12:16 PM, "Alvaro Herrera" <alvherre@2ndquadrant.com> wrote:
On 2020-Aug-20, Jeremy Schneider wrote:
Alternatively, if we don't want to take this approach, then I'd argue
that we should update README.tuplock to explicitly state that
XMAX_LOCK_ONLY and XMAX_COMMITTED are incompatible (just as it already
states for HEAP_XMAX_IS_MULTI and HEAP_XMAX_COMMITTED) and clean up the
code in heapam_visibility.c for consistency.Yeah, I like this approach better for the master branch; not just clean
up as in remove the cases that handle it, but also actively elog(ERROR)
if the condition ever occurs (hopefully with some known way to fix the
problem; maybe by "WITH tup AS (DELETE FROM tab WHERE .. RETURNING *)
INSERT * INTO tab FROM tup" or similar.)+1. I wouldn't mind picking this up, but it might be some time before
I can get to it.
I've finally gotten started on this, and I've attached a work-in-
progress patch to gather some early feedback. I'm specifically
wondering if there are other places it'd be good to check for these
unsupported combinations and whether we should use the
HEAP_XMAX_IS_LOCKED_ONLY macro or just look for the
HEAP_XMAX_LOCK_ONLY bit. IIUC HEAP_XMAX_IS_LOCKED_ONLY is intended to
handle some edge cases after pg_upgrade, but AFAICT
HEAP_XMAX_COMMITTED was not used for those previous bit combinations,
either. Therefore, I've used the HEAP_XMAX_IS_LOCKED_ONLY macro in
the attached patch, but I would not be surprised to learn that this is
wrong for some reason.
Nathan
Attachments:
v1-0001-Disallow-HEAP_XMAX_COMMITTED-and-HEAP_XMAX_IS_LOC.patchapplication/octet-stream; name=v1-0001-Disallow-HEAP_XMAX_COMMITTED-and-HEAP_XMAX_IS_LOC.patchDownload
From 35dcf427767399395a0e52118984cc97670e6853 Mon Sep 17 00:00:00 2001
From: Nathan Bossart <bossartn@amazon.com>
Date: Wed, 24 Nov 2021 00:02:41 +0000
Subject: [PATCH v1 1/1] Disallow HEAP_XMAX_COMMITTED and
HEAP_XMAX_IS_LOCKED_ONLY.
---
contrib/amcheck/verify_heapam.c | 8 +++
src/backend/access/heap/README.tuplock | 2 +-
src/backend/access/heap/heapam.c | 6 ++
src/backend/access/heap/heapam_visibility.c | 97 ++++++++++++++++++++---------
src/backend/access/heap/hio.c | 2 +
5 files changed, 85 insertions(+), 30 deletions(-)
diff --git a/contrib/amcheck/verify_heapam.c b/contrib/amcheck/verify_heapam.c
index bae5340111..4eeb5c967c 100644
--- a/contrib/amcheck/verify_heapam.c
+++ b/contrib/amcheck/verify_heapam.c
@@ -705,6 +705,14 @@ check_tuple_header(HeapCheckContext *ctx)
*/
}
+ if ((ctx->tuphdr->t_infomask & HEAP_XMAX_COMMITTED) &&
+ HEAP_XMAX_IS_LOCKED_ONLY(ctx->tuphdr->t_infomask))
+ {
+ report_corruption(ctx,
+ psprintf("locked-only should not be marked committed"));
+ result = false;
+ }
+
if (infomask & HEAP_HASNULL)
expected_hoff = MAXALIGN(SizeofHeapTupleHeader + BITMAPLEN(ctx->natts));
else
diff --git a/src/backend/access/heap/README.tuplock b/src/backend/access/heap/README.tuplock
index 6441e8baf0..4e565e60ab 100644
--- a/src/backend/access/heap/README.tuplock
+++ b/src/backend/access/heap/README.tuplock
@@ -152,4 +152,4 @@ The following infomask bits are applicable:
whether the XMAX is a TransactionId or a MultiXactId.
We currently never set the HEAP_XMAX_COMMITTED when the HEAP_XMAX_IS_MULTI bit
-is set.
+or the HEAP_XMAX_LOCK_ONLY bit is set.
diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c
index ec234a5e59..31b3da5b49 100644
--- a/src/backend/access/heap/heapam.c
+++ b/src/backend/access/heap/heapam.c
@@ -5108,6 +5108,12 @@ l5:
MultiXactStatus status;
MultiXactStatus new_status;
+ /*
+ * Currently we don't allow XMAX_LOCK_ONLY and XMAX_COMMITTED to both be
+ * set in a tuple header, so cross-check.
+ */
+ Assert(!HEAP_XMAX_IS_LOCKED_ONLY(old_infomask));
+
if (old_infomask2 & HEAP_KEYS_UPDATED)
status = MultiXactStatusUpdate;
else
diff --git a/src/backend/access/heap/heapam_visibility.c b/src/backend/access/heap/heapam_visibility.c
index b72b03ea25..68c051abbd 100644
--- a/src/backend/access/heap/heapam_visibility.c
+++ b/src/backend/access/heap/heapam_visibility.c
@@ -127,6 +127,12 @@ SetHintBits(HeapTupleHeader tuple, Buffer buffer,
}
tuple->t_infomask |= infomask;
+
+ Assert(!((tuple->t_infomask & HEAP_XMAX_COMMITTED) &&
+ HEAP_XMAX_IS_LOCKED_ONLY(tuple->t_infomask)));
+ Assert(!((tuple->t_infomask & HEAP_XMAX_COMMITTED) &&
+ (tuple->t_infomask & HEAP_XMAX_IS_MULTI)));
+
MarkBufferDirtyHint(buffer, true);
}
@@ -272,8 +278,18 @@ HeapTupleSatisfiesSelf(HeapTuple htup, Snapshot snapshot, Buffer buffer)
if (tuple->t_infomask & HEAP_XMAX_COMMITTED)
{
- if (HEAP_XMAX_IS_LOCKED_ONLY(tuple->t_infomask))
- return true;
+ if (unlikely(HEAP_XMAX_IS_LOCKED_ONLY(tuple->t_infomask)))
+ ereport(ERROR,
+ (errcode(ERRCODE_DATA_CORRUPTED),
+ errmsg_internal("found tuple with HEAP_XMAX_COMMITTED and "
+ "HEAP_XMAX_IS_LOCKED_ONLY")));
+
+ if (unlikely(tuple->t_infomask & HEAP_XMAX_IS_MULTI))
+ ereport(ERROR,
+ (errcode(ERRCODE_DATA_CORRUPTED),
+ errmsg_internal("found tuple with HEAP_XMAX_COMMITTED and "
+ "HEAP_XMAX_IS_MULTI")));
+
return false; /* updated by other */
}
@@ -605,8 +621,18 @@ HeapTupleSatisfiesUpdate(HeapTuple htup, CommandId curcid,
if (tuple->t_infomask & HEAP_XMAX_COMMITTED)
{
- if (HEAP_XMAX_IS_LOCKED_ONLY(tuple->t_infomask))
- return TM_Ok;
+ if (unlikely(HEAP_XMAX_IS_LOCKED_ONLY(tuple->t_infomask)))
+ ereport(ERROR,
+ (errcode(ERRCODE_DATA_CORRUPTED),
+ errmsg_internal("found tuple with HEAP_XMAX_COMMITTED and "
+ "HEAP_XMAX_IS_LOCKED_ONLY")));
+
+ if (unlikely(tuple->t_infomask & HEAP_XMAX_IS_MULTI))
+ ereport(ERROR,
+ (errcode(ERRCODE_DATA_CORRUPTED),
+ errmsg_internal("found tuple with HEAP_XMAX_COMMITTED and "
+ "HEAP_XMAX_IS_MULTI")));
+
if (!ItemPointerEquals(&htup->t_self, &tuple->t_ctid))
return TM_Updated; /* updated by other */
else
@@ -867,8 +893,18 @@ HeapTupleSatisfiesDirty(HeapTuple htup, Snapshot snapshot,
if (tuple->t_infomask & HEAP_XMAX_COMMITTED)
{
- if (HEAP_XMAX_IS_LOCKED_ONLY(tuple->t_infomask))
- return true;
+ if (unlikely(HEAP_XMAX_IS_LOCKED_ONLY(tuple->t_infomask)))
+ ereport(ERROR,
+ (errcode(ERRCODE_DATA_CORRUPTED),
+ errmsg_internal("found tuple with HEAP_XMAX_COMMITTED and "
+ "HEAP_XMAX_IS_LOCKED_ONLY")));
+
+ if (unlikely(tuple->t_infomask & HEAP_XMAX_IS_MULTI))
+ ereport(ERROR,
+ (errcode(ERRCODE_DATA_CORRUPTED),
+ errmsg_internal("found tuple with HEAP_XMAX_COMMITTED and "
+ "HEAP_XMAX_IS_MULTI")));
+
return false; /* updated by other */
}
@@ -1304,33 +1340,36 @@ HeapTupleSatisfiesVacuumHorizon(HeapTuple htup, Buffer buffer, TransactionId *de
if (HEAP_XMAX_IS_LOCKED_ONLY(tuple->t_infomask))
{
+ if (unlikely(tuple->t_infomask & HEAP_XMAX_COMMITTED))
+ ereport(ERROR,
+ (errcode(ERRCODE_DATA_CORRUPTED),
+ errmsg_internal("found tuple with HEAP_XMAX_COMMITTED and "
+ "HEAP_XMAX_IS_LOCKED_ONLY")));
+
/*
* "Deleting" xact really only locked it, so the tuple is live in any
- * case. However, we should make sure that either XMAX_COMMITTED or
- * XMAX_INVALID gets set once the xact is gone, to reduce the costs of
- * examining the tuple for future xacts.
+ * case. However, we should make sure that either XMAX_INVALID gets set
+ * once the xact is gone, to reduce the costs of examining the tuple for
+ * future xacts.
*/
- if (!(tuple->t_infomask & HEAP_XMAX_COMMITTED))
+ if (tuple->t_infomask & HEAP_XMAX_IS_MULTI)
{
- if (tuple->t_infomask & HEAP_XMAX_IS_MULTI)
- {
- /*
- * If it's a pre-pg_upgrade tuple, the multixact cannot
- * possibly be running; otherwise have to check.
- */
- if (!HEAP_LOCKED_UPGRADED(tuple->t_infomask) &&
- MultiXactIdIsRunning(HeapTupleHeaderGetRawXmax(tuple),
- true))
- return HEAPTUPLE_LIVE;
- SetHintBits(tuple, buffer, HEAP_XMAX_INVALID, InvalidTransactionId);
- }
- else
- {
- if (TransactionIdIsInProgress(HeapTupleHeaderGetRawXmax(tuple)))
- return HEAPTUPLE_LIVE;
- SetHintBits(tuple, buffer, HEAP_XMAX_INVALID,
- InvalidTransactionId);
- }
+ /*
+ * If it's a pre-pg_upgrade tuple, the multixact cannot
+ * possibly be running; otherwise have to check.
+ */
+ if (!HEAP_LOCKED_UPGRADED(tuple->t_infomask) &&
+ MultiXactIdIsRunning(HeapTupleHeaderGetRawXmax(tuple),
+ true))
+ return HEAPTUPLE_LIVE;
+ SetHintBits(tuple, buffer, HEAP_XMAX_INVALID, InvalidTransactionId);
+ }
+ else
+ {
+ if (TransactionIdIsInProgress(HeapTupleHeaderGetRawXmax(tuple)))
+ return HEAPTUPLE_LIVE;
+ SetHintBits(tuple, buffer, HEAP_XMAX_INVALID,
+ InvalidTransactionId);
}
/*
diff --git a/src/backend/access/heap/hio.c b/src/backend/access/heap/hio.c
index d34edb4190..990c336575 100644
--- a/src/backend/access/heap/hio.c
+++ b/src/backend/access/heap/hio.c
@@ -55,6 +55,8 @@ RelationPutHeapTuple(Relation relation,
*/
Assert(!((tuple->t_data->t_infomask & HEAP_XMAX_COMMITTED) &&
(tuple->t_data->t_infomask & HEAP_XMAX_IS_MULTI)));
+ Assert(!((tuple->t_data->t_infomask & HEAP_XMAX_COMMITTED) &&
+ HEAP_XMAX_IS_LOCKED_ONLY(tuple->t_data->t_infomask)));
/* Add the tuple to the page */
pageHeader = BufferGetPage(buffer);
--
2.16.6
The archives seem unhappy with my attempt to revive this old thread,
so here is a link to it in case anyone is looking for more context:
/messages/by-id/3476708e-7919-20cb-ca45-6603470565f7@amazon.com
Nathan
On Nov 23, 2021, at 4:26 PM, Bossart, Nathan <bossartn@amazon.com> wrote:
I've finally gotten started on this, and I've attached a work-in-
progress patch to gather some early feedback. I'm specifically
wondering if there are other places it'd be good to check for these
unsupported combinations and whether we should use the
HEAP_XMAX_IS_LOCKED_ONLY macro or just look for the
HEAP_XMAX_LOCK_ONLY bit.
I have to wonder if, when corruption is reported for conditions like this:
+ if ((ctx->tuphdr->t_infomask & HEAP_XMAX_COMMITTED) &&
+ HEAP_XMAX_IS_LOCKED_ONLY(ctx->tuphdr->t_infomask))if the first thing we're going to want to know is which branch of the HEAP_XMAX_IS_LOCKED_ONLY macro evaluated true? Should we split this check to do each branch of the macro separately, such as:
if (ctx->tuphdr->t_infomask & HEAP_XMAX_COMMITTED)
{
if (ctx->tuphdr->t_infomask & HEAP_XMAX_LOCK_ONLY)
... report something ...
else if ((ctx->tuphdr->t_infomask & (HEAP_XMAX_IS_MULTI | HEAP_LOCK_MASK)) == HEAP_XMAX_EXCL_LOCK)
... report a different thing ...
}
—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
On 11/23/21, 4:36 PM, "Mark Dilger" <mark.dilger@enterprisedb.com> wrote:
I have to wonder if, when corruption is reported for conditions like this:
+ if ((ctx->tuphdr->t_infomask & HEAP_XMAX_COMMITTED) && + HEAP_XMAX_IS_LOCKED_ONLY(ctx->tuphdr->t_infomask))if the first thing we're going to want to know is which branch of the HEAP_XMAX_IS_LOCKED_ONLY macro evaluated true? Should we split this check to do each branch of the macro separately, such as:
if (ctx->tuphdr->t_infomask & HEAP_XMAX_COMMITTED)
{
if (ctx->tuphdr->t_infomask & HEAP_XMAX_LOCK_ONLY)
... report something ...
else if ((ctx->tuphdr->t_infomask & (HEAP_XMAX_IS_MULTI | HEAP_LOCK_MASK)) == HEAP_XMAX_EXCL_LOCK)
... report a different thing ...
}
This is a good point. Right now, you'd have to manually inspect the
infomask field to determine the exact form of the invalid combination.
My only worry with this is that we'd want to make sure these checks
stayed consistent with the definition of HEAP_XMAX_IS_LOCKED_ONLY in
htup_details.h. I'm guessing HEAP_XMAX_IS_LOCKED_ONLY is unlikely to
change all that often, though.
Nathan
On Nov 23, 2021, at 4:51 PM, Bossart, Nathan <bossartn@amazon.com> wrote:
This is a good point. Right now, you'd have to manually inspect the
infomask field to determine the exact form of the invalid combination.
My only worry with this is that we'd want to make sure these checks
stayed consistent with the definition of HEAP_XMAX_IS_LOCKED_ONLY in
htup_details.h. I'm guessing HEAP_XMAX_IS_LOCKED_ONLY is unlikely to
change all that often, though.
I expect that your patch will contain some addition to the amcheck (or pg_amcheck) tests, so if we ever allow some currently disallowed bit combination, we'd be reminded of the need to update amcheck. So I'm not too worried about that aspect of this.
I prefer not to have to get a page (or full file) from a customer when the check reports corruption. Even assuming they are comfortable giving that, which they may not be, it is an extra round trip to the customer asking for stuff.
—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
On 11/23/21, 4:59 PM, "Mark Dilger" <mark.dilger@enterprisedb.com> wrote:
On Nov 23, 2021, at 4:51 PM, Bossart, Nathan <bossartn@amazon.com> wrote:
This is a good point. Right now, you'd have to manually inspect the
infomask field to determine the exact form of the invalid combination.
My only worry with this is that we'd want to make sure these checks
stayed consistent with the definition of HEAP_XMAX_IS_LOCKED_ONLY in
htup_details.h. I'm guessing HEAP_XMAX_IS_LOCKED_ONLY is unlikely to
change all that often, though.I expect that your patch will contain some addition to the amcheck (or pg_amcheck) tests, so if we ever allow some currently disallowed bit combination, we'd be reminded of the need to update amcheck. So I'm not too worried about that aspect of this.
I prefer not to have to get a page (or full file) from a customer when the check reports corruption. Even assuming they are comfortable giving that, which they may not be, it is an extra round trip to the customer asking for stuff.
Another option we might consider is only checking for the
HEAP_XMAX_LOCK_ONLY bit instead of everything in
HEAP_XMAX_IS_LOCKED_ONLY. IIUC everything else is only expected to
happen for upgrades from v9.2 or earlier, so it might be pretty rare
at this point. Otherwise, I'll extract the exact bit pattern for the
error message as you suggest.
Nathan
On Nov 24, 2021, at 12:53 PM, Bossart, Nathan <bossartn@amazon.com> wrote:
Another option we might consider is only checking for the
HEAP_XMAX_LOCK_ONLY bit instead of everything in
HEAP_XMAX_IS_LOCKED_ONLY. IIUC everything else is only expected to
happen for upgrades from v9.2 or earlier, so it might be pretty rare
at this point. Otherwise, I'll extract the exact bit pattern for the
error message as you suggest.
I would prefer to detect and report any "can't happen" bit patterns without regard for how likely the pattern may be. The difficulty is in proving that a bit pattern is disallowed. Just because you can't find a code path in the current code base that would create a pattern doesn't mean it won't have legitimately been created by some past release or upgrade path. As such, any prohibitions explicitly in the backend, such as Asserts around a condition, are really valuable. You can know that the pattern is disallowed, since the server would Assert on it if encountered.
Aside from that, I don't really buy the argument that databases upgraded from v9.2 or earlier are rare. Even if servers *running* v9.2 or earlier are (or become) rare, servers initialized that far back which have been upgraded one or more times since then may be common.
—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
On 11/25/21, 9:16 AM, "Mark Dilger" <mark.dilger@enterprisedb.com> wrote:
On Nov 24, 2021, at 12:53 PM, Bossart, Nathan <bossartn@amazon.com> wrote:
Another option we might consider is only checking for the
HEAP_XMAX_LOCK_ONLY bit instead of everything in
HEAP_XMAX_IS_LOCKED_ONLY. IIUC everything else is only expected to
happen for upgrades from v9.2 or earlier, so it might be pretty rare
at this point. Otherwise, I'll extract the exact bit pattern for the
error message as you suggest.I would prefer to detect and report any "can't happen" bit patterns without regard for how likely the pattern may be. The difficulty is in proving that a bit pattern is disallowed. Just because you can't find a code path in the current code base that would create a pattern doesn't mean it won't have legitimately been created by some past release or upgrade path. As such, any prohibitions explicitly in the backend, such as Asserts around a condition, are really valuable. You can know that the pattern is disallowed, since the server would Assert on it if encountered.
Aside from that, I don't really buy the argument that databases upgraded from v9.2 or earlier are rare. Even if servers *running* v9.2 or earlier are (or become) rare, servers initialized that far back which have been upgraded one or more times since then may be common.
Okay, I'll do it that way in the next revision.
Nathan
On 11/29/21, 10:10 AM, "Bossart, Nathan" <bossartn@amazon.com> wrote:
Okay, I'll do it that way in the next revision.
v2 attached.
Nathan
Attachments:
v2-0001-Disallow-HEAP_XMAX_COMMITTED-and-HEAP_XMAX_IS_LOC.patchapplication/octet-stream; name=v2-0001-Disallow-HEAP_XMAX_COMMITTED-and-HEAP_XMAX_IS_LOC.patchDownload
From 5b5bcc7422203e8b62a6cab2c3e72e3476f5dfb0 Mon Sep 17 00:00:00 2001
From: Nathan Bossart <bossartn@amazon.com>
Date: Tue, 30 Nov 2021 16:33:39 -0800
Subject: [PATCH v2 1/1] Disallow HEAP_XMAX_COMMITTED and
HEAP_XMAX_IS_LOCKED_ONLY.
---
contrib/amcheck/verify_heapam.c | 19 +++
src/backend/access/heap/README.tuplock | 2 +-
src/backend/access/heap/heapam.c | 6 +
src/backend/access/heap/heapam_visibility.c | 149 ++++++++++++++++----
src/backend/access/heap/hio.c | 2 +
5 files changed, 148 insertions(+), 30 deletions(-)
diff --git a/contrib/amcheck/verify_heapam.c b/contrib/amcheck/verify_heapam.c
index bae5340111..63759516a6 100644
--- a/contrib/amcheck/verify_heapam.c
+++ b/contrib/amcheck/verify_heapam.c
@@ -705,6 +705,25 @@ check_tuple_header(HeapCheckContext *ctx)
*/
}
+ if ((ctx->tuphdr->t_infomask & HEAP_XMAX_COMMITTED) &&
+ (ctx->tuphdr->t_infomask & HEAP_XMAX_LOCK_ONLY))
+ {
+ report_corruption(ctx,
+ pstrdup("locked-only should not be marked committed"));
+
+ /* As above, do not skip further checks. */
+ }
+
+ /* also check for pre-v9.3 lock-only bit pattern */
+ if ((ctx->tuphdr->t_infomask & HEAP_XMAX_COMMITTED) &&
+ HEAP_XMAX_IS_LOCKED_ONLY(ctx->tuphdr->t_infomask))
+ {
+ report_corruption(ctx,
+ pstrdup("tuple with HEAP_XMAX_EXCL_LOCK set and HEAP_XMAX_IS_MULTI unset should not be marked committed"));
+
+ /* As above, do not skip further checks. */
+ }
+
if (infomask & HEAP_HASNULL)
expected_hoff = MAXALIGN(SizeofHeapTupleHeader + BITMAPLEN(ctx->natts));
else
diff --git a/src/backend/access/heap/README.tuplock b/src/backend/access/heap/README.tuplock
index 6441e8baf0..4e565e60ab 100644
--- a/src/backend/access/heap/README.tuplock
+++ b/src/backend/access/heap/README.tuplock
@@ -152,4 +152,4 @@ The following infomask bits are applicable:
whether the XMAX is a TransactionId or a MultiXactId.
We currently never set the HEAP_XMAX_COMMITTED when the HEAP_XMAX_IS_MULTI bit
-is set.
+or the HEAP_XMAX_LOCK_ONLY bit is set.
diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c
index 17afe1ea4c..e02a788060 100644
--- a/src/backend/access/heap/heapam.c
+++ b/src/backend/access/heap/heapam.c
@@ -5089,6 +5089,12 @@ l5:
MultiXactStatus status;
MultiXactStatus new_status;
+ /*
+ * Currently we don't allow XMAX_LOCK_ONLY and XMAX_COMMITTED to both be
+ * set in a tuple header, so cross-check.
+ */
+ Assert(!HEAP_XMAX_IS_LOCKED_ONLY(old_infomask));
+
if (old_infomask2 & HEAP_KEYS_UPDATED)
status = MultiXactStatusUpdate;
else
diff --git a/src/backend/access/heap/heapam_visibility.c b/src/backend/access/heap/heapam_visibility.c
index b72b03ea25..e5eaf74f85 100644
--- a/src/backend/access/heap/heapam_visibility.c
+++ b/src/backend/access/heap/heapam_visibility.c
@@ -127,6 +127,12 @@ SetHintBits(HeapTupleHeader tuple, Buffer buffer,
}
tuple->t_infomask |= infomask;
+
+ Assert(!((tuple->t_infomask & HEAP_XMAX_COMMITTED) &&
+ HEAP_XMAX_IS_LOCKED_ONLY(tuple->t_infomask)));
+ Assert(!((tuple->t_infomask & HEAP_XMAX_COMMITTED) &&
+ (tuple->t_infomask & HEAP_XMAX_IS_MULTI)));
+
MarkBufferDirtyHint(buffer, true);
}
@@ -272,8 +278,28 @@ HeapTupleSatisfiesSelf(HeapTuple htup, Snapshot snapshot, Buffer buffer)
if (tuple->t_infomask & HEAP_XMAX_COMMITTED)
{
- if (HEAP_XMAX_IS_LOCKED_ONLY(tuple->t_infomask))
- return true;
+ if (unlikely(HEAP_XMAX_IS_LOCKED_ONLY(tuple->t_infomask)))
+ {
+ if (tuple->t_infomask & HEAP_XMAX_LOCK_ONLY)
+ ereport(ERROR,
+ (errcode(ERRCODE_DATA_CORRUPTED),
+ errmsg_internal("found tuple with HEAP_XMAX_COMMITTED "
+ "and HEAP_XMAX_LOCK_ONLY")));
+
+ /* pre-v9.3 lock-only bit pattern */
+ ereport(ERROR,
+ (errcode(ERRCODE_DATA_CORRUPTED),
+ errmsg_internal("found tuple with HEAP_XMAX_COMMITTED and"
+ "HEAP_XMAX_EXCL_LOCK set and"
+ "HEAP_XMAX_IS_MULTI unset")));
+ }
+
+ if (unlikely(tuple->t_infomask & HEAP_XMAX_IS_MULTI))
+ ereport(ERROR,
+ (errcode(ERRCODE_DATA_CORRUPTED),
+ errmsg_internal("found tuple with HEAP_XMAX_COMMITTED and "
+ "HEAP_XMAX_IS_MULTI")));
+
return false; /* updated by other */
}
@@ -605,8 +631,28 @@ HeapTupleSatisfiesUpdate(HeapTuple htup, CommandId curcid,
if (tuple->t_infomask & HEAP_XMAX_COMMITTED)
{
- if (HEAP_XMAX_IS_LOCKED_ONLY(tuple->t_infomask))
- return TM_Ok;
+ if (unlikely(HEAP_XMAX_IS_LOCKED_ONLY(tuple->t_infomask)))
+ {
+ if (tuple->t_infomask & HEAP_XMAX_LOCK_ONLY)
+ ereport(ERROR,
+ (errcode(ERRCODE_DATA_CORRUPTED),
+ errmsg_internal("found tuple with HEAP_XMAX_COMMITTED "
+ "and HEAP_XMAX_LOCK_ONLY")));
+
+ /* pre-v9.3 lock-only bit pattern */
+ ereport(ERROR,
+ (errcode(ERRCODE_DATA_CORRUPTED),
+ errmsg_internal("found tuple with HEAP_XMAX_COMMITTED and"
+ "HEAP_XMAX_EXCL_LOCK set and "
+ "HEAP_XMAX_IS_MULTI unset")));
+ }
+
+ if (unlikely(tuple->t_infomask & HEAP_XMAX_IS_MULTI))
+ ereport(ERROR,
+ (errcode(ERRCODE_DATA_CORRUPTED),
+ errmsg_internal("found tuple with HEAP_XMAX_COMMITTED and "
+ "HEAP_XMAX_IS_MULTI")));
+
if (!ItemPointerEquals(&htup->t_self, &tuple->t_ctid))
return TM_Updated; /* updated by other */
else
@@ -867,8 +913,28 @@ HeapTupleSatisfiesDirty(HeapTuple htup, Snapshot snapshot,
if (tuple->t_infomask & HEAP_XMAX_COMMITTED)
{
- if (HEAP_XMAX_IS_LOCKED_ONLY(tuple->t_infomask))
- return true;
+ if (unlikely(HEAP_XMAX_IS_LOCKED_ONLY(tuple->t_infomask)))
+ {
+ if (tuple->t_infomask & HEAP_XMAX_LOCK_ONLY)
+ ereport(ERROR,
+ (errcode(ERRCODE_DATA_CORRUPTED),
+ errmsg_internal("found tuple with HEAP_XMAX_COMMITTED "
+ "and HEAP_XMAX_LOCK_ONLY")));
+
+ /* pre-v9.3 lock-only bit pattern */
+ ereport(ERROR,
+ (errcode(ERRCODE_DATA_CORRUPTED),
+ errmsg_internal("found tuple with HEAP_XMAX_COMMITTED and"
+ "HEAP_XMAX_EXCL_LOCK set and "
+ "HEAP_XMAX_IS_MULTI unset")));
+ }
+
+ if (unlikely(tuple->t_infomask & HEAP_XMAX_IS_MULTI))
+ ereport(ERROR,
+ (errcode(ERRCODE_DATA_CORRUPTED),
+ errmsg_internal("found tuple with HEAP_XMAX_COMMITTED and "
+ "HEAP_XMAX_IS_MULTI")));
+
return false; /* updated by other */
}
@@ -1304,33 +1370,52 @@ HeapTupleSatisfiesVacuumHorizon(HeapTuple htup, Buffer buffer, TransactionId *de
if (HEAP_XMAX_IS_LOCKED_ONLY(tuple->t_infomask))
{
+ if (unlikely(tuple->t_infomask & HEAP_XMAX_COMMITTED))
+ {
+ if (tuple->t_infomask & HEAP_XMAX_LOCK_ONLY)
+ ereport(ERROR,
+ (errcode(ERRCODE_DATA_CORRUPTED),
+ errmsg_internal("found tuple with HEAP_XMAX_COMMITTED "
+ "and HEAP_XMAX_LOCK_ONLY")));
+
+ /* pre-v9.3 lock-only bit pattern */
+ ereport(ERROR,
+ (errcode(ERRCODE_DATA_CORRUPTED),
+ errmsg_internal("found tuple with HEAP_XMAX_COMMITTED and"
+ "HEAP_XMAX_EXCL_LOCK set and "
+ "HEAP_XMAX_IS_MULTI unset")));
+ }
+
/*
* "Deleting" xact really only locked it, so the tuple is live in any
- * case. However, we should make sure that either XMAX_COMMITTED or
- * XMAX_INVALID gets set once the xact is gone, to reduce the costs of
- * examining the tuple for future xacts.
+ * case. However, we should make sure that XMAX_INVALID gets set once
+ * the xact is gone, to reduce the costs of examining the tuple for
+ * future xacts.
*/
- if (!(tuple->t_infomask & HEAP_XMAX_COMMITTED))
+ if (tuple->t_infomask & HEAP_XMAX_IS_MULTI)
{
- if (tuple->t_infomask & HEAP_XMAX_IS_MULTI)
- {
- /*
- * If it's a pre-pg_upgrade tuple, the multixact cannot
- * possibly be running; otherwise have to check.
- */
- if (!HEAP_LOCKED_UPGRADED(tuple->t_infomask) &&
- MultiXactIdIsRunning(HeapTupleHeaderGetRawXmax(tuple),
- true))
- return HEAPTUPLE_LIVE;
- SetHintBits(tuple, buffer, HEAP_XMAX_INVALID, InvalidTransactionId);
- }
- else
- {
- if (TransactionIdIsInProgress(HeapTupleHeaderGetRawXmax(tuple)))
- return HEAPTUPLE_LIVE;
- SetHintBits(tuple, buffer, HEAP_XMAX_INVALID,
- InvalidTransactionId);
- }
+ if (unlikely(tuple->t_infomask & HEAP_XMAX_COMMITTED))
+ ereport(ERROR,
+ (errcode(ERRCODE_DATA_CORRUPTED),
+ errmsg_internal("found tuple with HEAP_XMAX_COMMITTED "
+ "and HEAP_XMAX_IS_MULTI")));
+
+ /*
+ * If it's a pre-pg_upgrade tuple, the multixact cannot
+ * possibly be running; otherwise have to check.
+ */
+ if (!HEAP_LOCKED_UPGRADED(tuple->t_infomask) &&
+ MultiXactIdIsRunning(HeapTupleHeaderGetRawXmax(tuple),
+ true))
+ return HEAPTUPLE_LIVE;
+ SetHintBits(tuple, buffer, HEAP_XMAX_INVALID, InvalidTransactionId);
+ }
+ else
+ {
+ if (TransactionIdIsInProgress(HeapTupleHeaderGetRawXmax(tuple)))
+ return HEAPTUPLE_LIVE;
+ SetHintBits(tuple, buffer, HEAP_XMAX_INVALID,
+ InvalidTransactionId);
}
/*
@@ -1352,6 +1437,12 @@ HeapTupleSatisfiesVacuumHorizon(HeapTuple htup, Buffer buffer, TransactionId *de
/* not LOCKED_ONLY, so it has to have an xmax */
Assert(TransactionIdIsValid(xmax));
+ if (unlikely(tuple->t_infomask & HEAP_XMAX_COMMITTED))
+ ereport(ERROR,
+ (errcode(ERRCODE_DATA_CORRUPTED),
+ errmsg_internal("found tuple with HEAP_XMAX_COMMITTED and "
+ "HEAP_XMAX_IS_MULTI")));
+
if (TransactionIdIsInProgress(xmax))
return HEAPTUPLE_DELETE_IN_PROGRESS;
else if (TransactionIdDidCommit(xmax))
diff --git a/src/backend/access/heap/hio.c b/src/backend/access/heap/hio.c
index d34edb4190..990c336575 100644
--- a/src/backend/access/heap/hio.c
+++ b/src/backend/access/heap/hio.c
@@ -55,6 +55,8 @@ RelationPutHeapTuple(Relation relation,
*/
Assert(!((tuple->t_data->t_infomask & HEAP_XMAX_COMMITTED) &&
(tuple->t_data->t_infomask & HEAP_XMAX_IS_MULTI)));
+ Assert(!((tuple->t_data->t_infomask & HEAP_XMAX_COMMITTED) &&
+ HEAP_XMAX_IS_LOCKED_ONLY(tuple->t_data->t_infomask)));
/* Add the tuple to the page */
pageHeader = BufferGetPage(buffer);
--
2.25.1
On 11/30/21, 4:54 PM, "Bossart, Nathan" <bossartn@amazon.com> wrote:
v2 attached.
I accidentally left a redundant check in v2, so here is a v3 without
it.
My proposed patch adds a few checks for the unsupported bit patterns
in the visibility code, but it is far from exhaustive. I'm wondering
if it might be better just to add a function or macro that everything
exported from heapam_visibility.c is expected to call. My guess is
the main argument against that would be the possible performance
impact.
Nathan
Attachments:
v3-0001-Disallow-HEAP_XMAX_COMMITTED-and-HEAP_XMAX_IS_LOC.patchapplication/octet-stream; name=v3-0001-Disallow-HEAP_XMAX_COMMITTED-and-HEAP_XMAX_IS_LOC.patchDownload
From 80fac3954cccf78579d6aed604020447b4004ad9 Mon Sep 17 00:00:00 2001
From: Nathan Bossart <bossartn@amazon.com>
Date: Wed, 1 Dec 2021 18:39:24 +0000
Subject: [PATCH v3 1/1] Disallow HEAP_XMAX_COMMITTED and
HEAP_XMAX_IS_LOCKED_ONLY.
---
contrib/amcheck/verify_heapam.c | 19 ++++
src/backend/access/heap/README.tuplock | 2 +-
src/backend/access/heap/heapam.c | 6 ++
src/backend/access/heap/heapam_visibility.c | 143 ++++++++++++++++++++++------
src/backend/access/heap/hio.c | 2 +
5 files changed, 142 insertions(+), 30 deletions(-)
diff --git a/contrib/amcheck/verify_heapam.c b/contrib/amcheck/verify_heapam.c
index bae5340111..63759516a6 100644
--- a/contrib/amcheck/verify_heapam.c
+++ b/contrib/amcheck/verify_heapam.c
@@ -705,6 +705,25 @@ check_tuple_header(HeapCheckContext *ctx)
*/
}
+ if ((ctx->tuphdr->t_infomask & HEAP_XMAX_COMMITTED) &&
+ (ctx->tuphdr->t_infomask & HEAP_XMAX_LOCK_ONLY))
+ {
+ report_corruption(ctx,
+ pstrdup("locked-only should not be marked committed"));
+
+ /* As above, do not skip further checks. */
+ }
+
+ /* also check for pre-v9.3 lock-only bit pattern */
+ if ((ctx->tuphdr->t_infomask & HEAP_XMAX_COMMITTED) &&
+ HEAP_XMAX_IS_LOCKED_ONLY(ctx->tuphdr->t_infomask))
+ {
+ report_corruption(ctx,
+ pstrdup("tuple with HEAP_XMAX_EXCL_LOCK set and HEAP_XMAX_IS_MULTI unset should not be marked committed"));
+
+ /* As above, do not skip further checks. */
+ }
+
if (infomask & HEAP_HASNULL)
expected_hoff = MAXALIGN(SizeofHeapTupleHeader + BITMAPLEN(ctx->natts));
else
diff --git a/src/backend/access/heap/README.tuplock b/src/backend/access/heap/README.tuplock
index 6441e8baf0..4e565e60ab 100644
--- a/src/backend/access/heap/README.tuplock
+++ b/src/backend/access/heap/README.tuplock
@@ -152,4 +152,4 @@ The following infomask bits are applicable:
whether the XMAX is a TransactionId or a MultiXactId.
We currently never set the HEAP_XMAX_COMMITTED when the HEAP_XMAX_IS_MULTI bit
-is set.
+or the HEAP_XMAX_LOCK_ONLY bit is set.
diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c
index 17afe1ea4c..e02a788060 100644
--- a/src/backend/access/heap/heapam.c
+++ b/src/backend/access/heap/heapam.c
@@ -5089,6 +5089,12 @@ l5:
MultiXactStatus status;
MultiXactStatus new_status;
+ /*
+ * Currently we don't allow XMAX_LOCK_ONLY and XMAX_COMMITTED to both be
+ * set in a tuple header, so cross-check.
+ */
+ Assert(!HEAP_XMAX_IS_LOCKED_ONLY(old_infomask));
+
if (old_infomask2 & HEAP_KEYS_UPDATED)
status = MultiXactStatusUpdate;
else
diff --git a/src/backend/access/heap/heapam_visibility.c b/src/backend/access/heap/heapam_visibility.c
index b72b03ea25..777a07e28e 100644
--- a/src/backend/access/heap/heapam_visibility.c
+++ b/src/backend/access/heap/heapam_visibility.c
@@ -127,6 +127,12 @@ SetHintBits(HeapTupleHeader tuple, Buffer buffer,
}
tuple->t_infomask |= infomask;
+
+ Assert(!((tuple->t_infomask & HEAP_XMAX_COMMITTED) &&
+ HEAP_XMAX_IS_LOCKED_ONLY(tuple->t_infomask)));
+ Assert(!((tuple->t_infomask & HEAP_XMAX_COMMITTED) &&
+ (tuple->t_infomask & HEAP_XMAX_IS_MULTI)));
+
MarkBufferDirtyHint(buffer, true);
}
@@ -272,8 +278,28 @@ HeapTupleSatisfiesSelf(HeapTuple htup, Snapshot snapshot, Buffer buffer)
if (tuple->t_infomask & HEAP_XMAX_COMMITTED)
{
- if (HEAP_XMAX_IS_LOCKED_ONLY(tuple->t_infomask))
- return true;
+ if (unlikely(HEAP_XMAX_IS_LOCKED_ONLY(tuple->t_infomask)))
+ {
+ if (tuple->t_infomask & HEAP_XMAX_LOCK_ONLY)
+ ereport(ERROR,
+ (errcode(ERRCODE_DATA_CORRUPTED),
+ errmsg_internal("found tuple with HEAP_XMAX_COMMITTED "
+ "and HEAP_XMAX_LOCK_ONLY")));
+
+ /* pre-v9.3 lock-only bit pattern */
+ ereport(ERROR,
+ (errcode(ERRCODE_DATA_CORRUPTED),
+ errmsg_internal("found tuple with HEAP_XMAX_COMMITTED and"
+ "HEAP_XMAX_EXCL_LOCK set and"
+ "HEAP_XMAX_IS_MULTI unset")));
+ }
+
+ if (unlikely(tuple->t_infomask & HEAP_XMAX_IS_MULTI))
+ ereport(ERROR,
+ (errcode(ERRCODE_DATA_CORRUPTED),
+ errmsg_internal("found tuple with HEAP_XMAX_COMMITTED and "
+ "HEAP_XMAX_IS_MULTI")));
+
return false; /* updated by other */
}
@@ -605,8 +631,28 @@ HeapTupleSatisfiesUpdate(HeapTuple htup, CommandId curcid,
if (tuple->t_infomask & HEAP_XMAX_COMMITTED)
{
- if (HEAP_XMAX_IS_LOCKED_ONLY(tuple->t_infomask))
- return TM_Ok;
+ if (unlikely(HEAP_XMAX_IS_LOCKED_ONLY(tuple->t_infomask)))
+ {
+ if (tuple->t_infomask & HEAP_XMAX_LOCK_ONLY)
+ ereport(ERROR,
+ (errcode(ERRCODE_DATA_CORRUPTED),
+ errmsg_internal("found tuple with HEAP_XMAX_COMMITTED "
+ "and HEAP_XMAX_LOCK_ONLY")));
+
+ /* pre-v9.3 lock-only bit pattern */
+ ereport(ERROR,
+ (errcode(ERRCODE_DATA_CORRUPTED),
+ errmsg_internal("found tuple with HEAP_XMAX_COMMITTED and"
+ "HEAP_XMAX_EXCL_LOCK set and "
+ "HEAP_XMAX_IS_MULTI unset")));
+ }
+
+ if (unlikely(tuple->t_infomask & HEAP_XMAX_IS_MULTI))
+ ereport(ERROR,
+ (errcode(ERRCODE_DATA_CORRUPTED),
+ errmsg_internal("found tuple with HEAP_XMAX_COMMITTED and "
+ "HEAP_XMAX_IS_MULTI")));
+
if (!ItemPointerEquals(&htup->t_self, &tuple->t_ctid))
return TM_Updated; /* updated by other */
else
@@ -867,8 +913,28 @@ HeapTupleSatisfiesDirty(HeapTuple htup, Snapshot snapshot,
if (tuple->t_infomask & HEAP_XMAX_COMMITTED)
{
- if (HEAP_XMAX_IS_LOCKED_ONLY(tuple->t_infomask))
- return true;
+ if (unlikely(HEAP_XMAX_IS_LOCKED_ONLY(tuple->t_infomask)))
+ {
+ if (tuple->t_infomask & HEAP_XMAX_LOCK_ONLY)
+ ereport(ERROR,
+ (errcode(ERRCODE_DATA_CORRUPTED),
+ errmsg_internal("found tuple with HEAP_XMAX_COMMITTED "
+ "and HEAP_XMAX_LOCK_ONLY")));
+
+ /* pre-v9.3 lock-only bit pattern */
+ ereport(ERROR,
+ (errcode(ERRCODE_DATA_CORRUPTED),
+ errmsg_internal("found tuple with HEAP_XMAX_COMMITTED and"
+ "HEAP_XMAX_EXCL_LOCK set and "
+ "HEAP_XMAX_IS_MULTI unset")));
+ }
+
+ if (unlikely(tuple->t_infomask & HEAP_XMAX_IS_MULTI))
+ ereport(ERROR,
+ (errcode(ERRCODE_DATA_CORRUPTED),
+ errmsg_internal("found tuple with HEAP_XMAX_COMMITTED and "
+ "HEAP_XMAX_IS_MULTI")));
+
return false; /* updated by other */
}
@@ -1304,33 +1370,46 @@ HeapTupleSatisfiesVacuumHorizon(HeapTuple htup, Buffer buffer, TransactionId *de
if (HEAP_XMAX_IS_LOCKED_ONLY(tuple->t_infomask))
{
+ if (unlikely(tuple->t_infomask & HEAP_XMAX_COMMITTED))
+ {
+ if (tuple->t_infomask & HEAP_XMAX_LOCK_ONLY)
+ ereport(ERROR,
+ (errcode(ERRCODE_DATA_CORRUPTED),
+ errmsg_internal("found tuple with HEAP_XMAX_COMMITTED "
+ "and HEAP_XMAX_LOCK_ONLY")));
+
+ /* pre-v9.3 lock-only bit pattern */
+ ereport(ERROR,
+ (errcode(ERRCODE_DATA_CORRUPTED),
+ errmsg_internal("found tuple with HEAP_XMAX_COMMITTED and"
+ "HEAP_XMAX_EXCL_LOCK set and "
+ "HEAP_XMAX_IS_MULTI unset")));
+ }
+
/*
* "Deleting" xact really only locked it, so the tuple is live in any
- * case. However, we should make sure that either XMAX_COMMITTED or
- * XMAX_INVALID gets set once the xact is gone, to reduce the costs of
- * examining the tuple for future xacts.
+ * case. However, we should make sure that XMAX_INVALID gets set once
+ * the xact is gone, to reduce the costs of examining the tuple for
+ * future xacts.
*/
- if (!(tuple->t_infomask & HEAP_XMAX_COMMITTED))
+ if (tuple->t_infomask & HEAP_XMAX_IS_MULTI)
{
- if (tuple->t_infomask & HEAP_XMAX_IS_MULTI)
- {
- /*
- * If it's a pre-pg_upgrade tuple, the multixact cannot
- * possibly be running; otherwise have to check.
- */
- if (!HEAP_LOCKED_UPGRADED(tuple->t_infomask) &&
- MultiXactIdIsRunning(HeapTupleHeaderGetRawXmax(tuple),
- true))
- return HEAPTUPLE_LIVE;
- SetHintBits(tuple, buffer, HEAP_XMAX_INVALID, InvalidTransactionId);
- }
- else
- {
- if (TransactionIdIsInProgress(HeapTupleHeaderGetRawXmax(tuple)))
- return HEAPTUPLE_LIVE;
- SetHintBits(tuple, buffer, HEAP_XMAX_INVALID,
- InvalidTransactionId);
- }
+ /*
+ * If it's a pre-pg_upgrade tuple, the multixact cannot
+ * possibly be running; otherwise have to check.
+ */
+ if (!HEAP_LOCKED_UPGRADED(tuple->t_infomask) &&
+ MultiXactIdIsRunning(HeapTupleHeaderGetRawXmax(tuple),
+ true))
+ return HEAPTUPLE_LIVE;
+ SetHintBits(tuple, buffer, HEAP_XMAX_INVALID, InvalidTransactionId);
+ }
+ else
+ {
+ if (TransactionIdIsInProgress(HeapTupleHeaderGetRawXmax(tuple)))
+ return HEAPTUPLE_LIVE;
+ SetHintBits(tuple, buffer, HEAP_XMAX_INVALID,
+ InvalidTransactionId);
}
/*
@@ -1352,6 +1431,12 @@ HeapTupleSatisfiesVacuumHorizon(HeapTuple htup, Buffer buffer, TransactionId *de
/* not LOCKED_ONLY, so it has to have an xmax */
Assert(TransactionIdIsValid(xmax));
+ if (unlikely(tuple->t_infomask & HEAP_XMAX_COMMITTED))
+ ereport(ERROR,
+ (errcode(ERRCODE_DATA_CORRUPTED),
+ errmsg_internal("found tuple with HEAP_XMAX_COMMITTED and "
+ "HEAP_XMAX_IS_MULTI")));
+
if (TransactionIdIsInProgress(xmax))
return HEAPTUPLE_DELETE_IN_PROGRESS;
else if (TransactionIdDidCommit(xmax))
diff --git a/src/backend/access/heap/hio.c b/src/backend/access/heap/hio.c
index d34edb4190..990c336575 100644
--- a/src/backend/access/heap/hio.c
+++ b/src/backend/access/heap/hio.c
@@ -55,6 +55,8 @@ RelationPutHeapTuple(Relation relation,
*/
Assert(!((tuple->t_data->t_infomask & HEAP_XMAX_COMMITTED) &&
(tuple->t_data->t_infomask & HEAP_XMAX_IS_MULTI)));
+ Assert(!((tuple->t_data->t_infomask & HEAP_XMAX_COMMITTED) &&
+ HEAP_XMAX_IS_LOCKED_ONLY(tuple->t_data->t_infomask)));
/* Add the tuple to the page */
pageHeader = BufferGetPage(buffer);
--
2.16.6
On Dec 1, 2021, at 10:59 AM, Bossart, Nathan <bossartn@amazon.com> wrote:
here is a v3
It took a while for me to get to this....
@@ -1304,33 +1370,46 @@ HeapTupleSatisfiesVacuumHorizon(HeapTuple htup, Buffer buffer, TransactionId *de
if (HEAP_XMAX_IS_LOCKED_ONLY(tuple->t_infomask))
{
+ if (unlikely(tuple->t_infomask & HEAP_XMAX_COMMITTED))
+ {
+ if (tuple->t_infomask & HEAP_XMAX_LOCK_ONLY)
+ ereport(ERROR,
+ (errcode(ERRCODE_DATA_CORRUPTED),
+ errmsg_internal("found tuple with HEAP_XMAX_COMMITTED "
+ "and HEAP_XMAX_LOCK_ONLY")));
+
+ /* pre-v9.3 lock-only bit pattern */
+ ereport(ERROR,
+ (errcode(ERRCODE_DATA_CORRUPTED),
+ errmsg_internal("found tuple with HEAP_XMAX_COMMITTED and"
+ "HEAP_XMAX_EXCL_LOCK set and "
+ "HEAP_XMAX_IS_MULTI unset")));
+ }
+I find this bit hard to understand. Does the comment mean to suggest that the *upgrade* process should have eliminated all pre-v9.3 bit patterns, and therefore any such existing patterns are certainly corruption, or does it mean that data written by pre-v9.3 servers (and not subsequently updated) is defined as corrupt, or .... ?
I am not complaining that the logic is wrong, just trying to wrap my head around what the comment means.
—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
On 12/21/21, 11:42 AM, "Mark Dilger" <mark.dilger@enterprisedb.com> wrote:
+ /* pre-v9.3 lock-only bit pattern */ + ereport(ERROR, + (errcode(ERRCODE_DATA_CORRUPTED), + errmsg_internal("found tuple with HEAP_XMAX_COMMITTED and" + "HEAP_XMAX_EXCL_LOCK set and " + "HEAP_XMAX_IS_MULTI unset"))); + } +I find this bit hard to understand. Does the comment mean to suggest that the *upgrade* process should have eliminated all pre-v9.3 bit patterns, and therefore any such existing patterns are certainly corruption, or does it mean that data written by pre-v9.3 servers (and not subsequently updated) is defined as corrupt, or .... ?
I am not complaining that the logic is wrong, just trying to wrap my head around what the comment means.
This is just another way that a tuple may be marked locked-only, and
we want to explicitly disallow locked-only + xmax-committed. This bit
pattern may be present on servers that were pg_upgraded from pre-v9.3
versions. See commits 0ac5ad5 and 74ebba8 for more detail.
Nathan
I think this one requires some more work, and it needn't be a priority for
v15, so I've adjusted the commitfest entry to v16 and moved it to the next
commitfest.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
On Thu, Mar 17, 2022 at 04:45:28PM -0700, Nathan Bossart wrote:
I think this one requires some more work, and it needn't be a priority for
v15, so I've adjusted the commitfest entry to v16 and moved it to the next
commitfest.
Here is a new patch. The main differences from v3 are in
heapam_visibility.c. Specifically, instead of trying to work the infomask
checks into the visibility logic, I added a new function that does a couple
of assertions. This function is called at the beginning of each visibility
function.
What do folks think? The options I've considered are 1) not adding any
such checks to heapam_visibility.c, 2) only adding assertions like the
attached patch, or 3) actually using elog(ERROR, ...) when the invalid bit
patterns are detected. AFAICT (1) is more in line with existing invalid
bit patterns (e.g., XMAX_COMMITTED + XMAX_IS_MULTI). There are a couple of
scattered assertions, but most code paths don't check for it. (2) adds
additional checks, but only for --enable-cassert builds. (3) would add
checks even for non-assert builds, but there would presumably be some
performance cost involved.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
Attachments:
v4-0001-disallow-XMAX_COMMITTED-XMAX_LOCK_ONLY.patchtext/x-diff; charset=us-asciiDownload
From 2d6b372cf61782e0fd52590b57b1c914b0ed7a4c Mon Sep 17 00:00:00 2001
From: Nathan Bossart <nathandbossart@gmail.com>
Date: Tue, 22 Mar 2022 15:35:34 -0700
Subject: [PATCH v4 1/1] disallow XMAX_COMMITTED + XMAX_LOCK_ONLY
---
contrib/amcheck/verify_heapam.c | 19 ++++
src/backend/access/heap/README.tuplock | 2 +-
src/backend/access/heap/heapam.c | 10 +++
src/backend/access/heap/heapam_visibility.c | 97 ++++++++++++++-------
src/backend/access/heap/hio.c | 2 +
5 files changed, 96 insertions(+), 34 deletions(-)
diff --git a/contrib/amcheck/verify_heapam.c b/contrib/amcheck/verify_heapam.c
index e5f7355dcb..f30b9c234f 100644
--- a/contrib/amcheck/verify_heapam.c
+++ b/contrib/amcheck/verify_heapam.c
@@ -665,6 +665,25 @@ check_tuple_header(HeapCheckContext *ctx)
*/
}
+ if ((ctx->tuphdr->t_infomask & HEAP_XMAX_COMMITTED) &&
+ (ctx->tuphdr->t_infomask & HEAP_XMAX_LOCK_ONLY))
+ {
+ report_corruption(ctx,
+ pstrdup("locked-only should not be marked committed"));
+
+ /* As above, do not skip further checks. */
+ }
+
+ /* also check for pre-v9.3 lock-only bit pattern */
+ if ((ctx->tuphdr->t_infomask & HEAP_XMAX_COMMITTED) &&
+ HEAP_XMAX_IS_LOCKED_ONLY(ctx->tuphdr->t_infomask))
+ {
+ report_corruption(ctx,
+ pstrdup("tuple with HEAP_XMAX_EXCL_LOCK set and HEAP_XMAX_IS_MULTI unset should not be marked committed"));
+
+ /* As above, do not skip further checks. */
+ }
+
if (infomask & HEAP_HASNULL)
expected_hoff = MAXALIGN(SizeofHeapTupleHeader + BITMAPLEN(ctx->natts));
else
diff --git a/src/backend/access/heap/README.tuplock b/src/backend/access/heap/README.tuplock
index 6441e8baf0..4e565e60ab 100644
--- a/src/backend/access/heap/README.tuplock
+++ b/src/backend/access/heap/README.tuplock
@@ -152,4 +152,4 @@ The following infomask bits are applicable:
whether the XMAX is a TransactionId or a MultiXactId.
We currently never set the HEAP_XMAX_COMMITTED when the HEAP_XMAX_IS_MULTI bit
-is set.
+or the HEAP_XMAX_LOCK_ONLY bit is set.
diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c
index 3746336a09..3f0b4cd754 100644
--- a/src/backend/access/heap/heapam.c
+++ b/src/backend/access/heap/heapam.c
@@ -5151,6 +5151,16 @@ l5:
MultiXactStatus status;
MultiXactStatus new_status;
+ /*
+ * Currently we don't allow XMAX_LOCK_ONLY and XMAX_COMMITTED to both be
+ * set in a tuple header, so cross-check.
+ *
+ * Note that this also checks for the special locked-only bit pattern
+ * that may be present on servers that were pg_upgraded from pre-v9.3
+ * versions.
+ */
+ Assert(!HEAP_XMAX_IS_LOCKED_ONLY(old_infomask));
+
if (old_infomask2 & HEAP_KEYS_UPDATED)
status = MultiXactStatusUpdate;
else
diff --git a/src/backend/access/heap/heapam_visibility.c b/src/backend/access/heap/heapam_visibility.c
index ff0b8a688d..7d6fb66b0d 100644
--- a/src/backend/access/heap/heapam_visibility.c
+++ b/src/backend/access/heap/heapam_visibility.c
@@ -78,6 +78,31 @@
#include "utils/snapmgr.h"
+/*
+ * InfomaskAssertions()
+ *
+ * Checks for invalid infomask bit patterns.
+ */
+static inline void
+InfomaskAssertions(HeapTupleHeader tuple)
+{
+ /*
+ * XMAX_COMMITTED and XMAX_LOCK_ONLY cannot both be set in a tuple header.
+ *
+ * Note that this also checks for the special locked-only bit pattern that
+ * may be present on servers that were pg_upgraded from pre-v9.3 versions.
+ */
+ Assert(!((tuple->t_infomask & HEAP_XMAX_COMMITTED) &&
+ HEAP_XMAX_IS_LOCKED_ONLY(tuple->t_infomask)));
+
+ /*
+ * XMAX_COMMITTED and XMAX_IS_MULTI cannot be be set in a tuple header.
+ */
+ Assert(!((tuple->t_infomask & HEAP_XMAX_COMMITTED) &&
+ (tuple->t_infomask & HEAP_XMAX_IS_MULTI)));
+}
+
+
/*
* SetHintBits()
*
@@ -113,6 +138,8 @@ static inline void
SetHintBits(HeapTupleHeader tuple, Buffer buffer,
uint16 infomask, TransactionId xid)
{
+ InfomaskAssertions(tuple);
+
if (TransactionIdIsValid(xid))
{
/* NB: xid must be known committed here! */
@@ -127,6 +154,7 @@ SetHintBits(HeapTupleHeader tuple, Buffer buffer,
}
tuple->t_infomask |= infomask;
+ InfomaskAssertions(tuple); /* we check again after infomask updates */
MarkBufferDirtyHint(buffer, true);
}
@@ -172,6 +200,7 @@ HeapTupleSatisfiesSelf(HeapTuple htup, Snapshot snapshot, Buffer buffer)
Assert(ItemPointerIsValid(&htup->t_self));
Assert(htup->t_tableOid != InvalidOid);
+ InfomaskAssertions(tuple);
if (!HeapTupleHeaderXminCommitted(tuple))
{
@@ -271,11 +300,7 @@ HeapTupleSatisfiesSelf(HeapTuple htup, Snapshot snapshot, Buffer buffer)
return true;
if (tuple->t_infomask & HEAP_XMAX_COMMITTED)
- {
- if (HEAP_XMAX_IS_LOCKED_ONLY(tuple->t_infomask))
- return true;
return false; /* updated by other */
- }
if (tuple->t_infomask & HEAP_XMAX_IS_MULTI)
{
@@ -365,6 +390,7 @@ HeapTupleSatisfiesToast(HeapTuple htup, Snapshot snapshot,
Assert(ItemPointerIsValid(&htup->t_self));
Assert(htup->t_tableOid != InvalidOid);
+ InfomaskAssertions(tuple);
if (!HeapTupleHeaderXminCommitted(tuple))
{
@@ -461,6 +487,7 @@ HeapTupleSatisfiesUpdate(HeapTuple htup, CommandId curcid,
Assert(ItemPointerIsValid(&htup->t_self));
Assert(htup->t_tableOid != InvalidOid);
+ InfomaskAssertions(tuple);
if (!HeapTupleHeaderXminCommitted(tuple))
{
@@ -605,8 +632,6 @@ HeapTupleSatisfiesUpdate(HeapTuple htup, CommandId curcid,
if (tuple->t_infomask & HEAP_XMAX_COMMITTED)
{
- if (HEAP_XMAX_IS_LOCKED_ONLY(tuple->t_infomask))
- return TM_Ok;
if (!ItemPointerEquals(&htup->t_self, &tuple->t_ctid))
return TM_Updated; /* updated by other */
else
@@ -746,6 +771,7 @@ HeapTupleSatisfiesDirty(HeapTuple htup, Snapshot snapshot,
Assert(ItemPointerIsValid(&htup->t_self));
Assert(htup->t_tableOid != InvalidOid);
+ InfomaskAssertions(tuple);
snapshot->xmin = snapshot->xmax = InvalidTransactionId;
snapshot->speculativeToken = 0;
@@ -866,11 +892,7 @@ HeapTupleSatisfiesDirty(HeapTuple htup, Snapshot snapshot,
return true;
if (tuple->t_infomask & HEAP_XMAX_COMMITTED)
- {
- if (HEAP_XMAX_IS_LOCKED_ONLY(tuple->t_infomask))
- return true;
return false; /* updated by other */
- }
if (tuple->t_infomask & HEAP_XMAX_IS_MULTI)
{
@@ -963,6 +985,7 @@ HeapTupleSatisfiesMVCC(HeapTuple htup, Snapshot snapshot,
Assert(ItemPointerIsValid(&htup->t_self));
Assert(htup->t_tableOid != InvalidOid);
+ InfomaskAssertions(tuple);
if (!HeapTupleHeaderXminCommitted(tuple))
{
@@ -1164,6 +1187,8 @@ HeapTupleSatisfiesVacuum(HeapTuple htup, TransactionId OldestXmin,
TransactionId dead_after = InvalidTransactionId;
HTSV_Result res;
+ InfomaskAssertions(htup->t_data);
+
res = HeapTupleSatisfiesVacuumHorizon(htup, buffer, &dead_after);
if (res == HEAPTUPLE_RECENTLY_DEAD)
@@ -1199,6 +1224,7 @@ HeapTupleSatisfiesVacuumHorizon(HeapTuple htup, Buffer buffer, TransactionId *de
Assert(ItemPointerIsValid(&htup->t_self));
Assert(htup->t_tableOid != InvalidOid);
Assert(dead_after != NULL);
+ InfomaskAssertions(tuple);
*dead_after = InvalidTransactionId;
@@ -1306,31 +1332,28 @@ HeapTupleSatisfiesVacuumHorizon(HeapTuple htup, Buffer buffer, TransactionId *de
{
/*
* "Deleting" xact really only locked it, so the tuple is live in any
- * case. However, we should make sure that either XMAX_COMMITTED or
- * XMAX_INVALID gets set once the xact is gone, to reduce the costs of
- * examining the tuple for future xacts.
+ * case. However, we should make sure that XMAX_INVALID gets set once
+ * the xact is gone, to reduce the costs of examining the tuple for
+ * future xacts.
*/
- if (!(tuple->t_infomask & HEAP_XMAX_COMMITTED))
+ if (tuple->t_infomask & HEAP_XMAX_IS_MULTI)
{
- if (tuple->t_infomask & HEAP_XMAX_IS_MULTI)
- {
- /*
- * If it's a pre-pg_upgrade tuple, the multixact cannot
- * possibly be running; otherwise have to check.
- */
- if (!HEAP_LOCKED_UPGRADED(tuple->t_infomask) &&
- MultiXactIdIsRunning(HeapTupleHeaderGetRawXmax(tuple),
- true))
- return HEAPTUPLE_LIVE;
- SetHintBits(tuple, buffer, HEAP_XMAX_INVALID, InvalidTransactionId);
- }
- else
- {
- if (TransactionIdIsInProgress(HeapTupleHeaderGetRawXmax(tuple)))
- return HEAPTUPLE_LIVE;
- SetHintBits(tuple, buffer, HEAP_XMAX_INVALID,
- InvalidTransactionId);
- }
+ /*
+ * If it's a pre-pg_upgrade tuple, the multixact cannot possibly be
+ * running; otherwise have to check.
+ */
+ if (!HEAP_LOCKED_UPGRADED(tuple->t_infomask) &&
+ MultiXactIdIsRunning(HeapTupleHeaderGetRawXmax(tuple),
+ true))
+ return HEAPTUPLE_LIVE;
+ SetHintBits(tuple, buffer, HEAP_XMAX_INVALID, InvalidTransactionId);
+ }
+ else
+ {
+ if (TransactionIdIsInProgress(HeapTupleHeaderGetRawXmax(tuple)))
+ return HEAPTUPLE_LIVE;
+ SetHintBits(tuple, buffer, HEAP_XMAX_INVALID,
+ InvalidTransactionId);
}
/*
@@ -1431,6 +1454,8 @@ HeapTupleSatisfiesNonVacuumable(HeapTuple htup, Snapshot snapshot,
TransactionId dead_after = InvalidTransactionId;
HTSV_Result res;
+ InfomaskAssertions(htup->t_data);
+
res = HeapTupleSatisfiesVacuumHorizon(htup, buffer, &dead_after);
if (res == HEAPTUPLE_RECENTLY_DEAD)
@@ -1467,6 +1492,7 @@ HeapTupleIsSurelyDead(HeapTuple htup, GlobalVisState *vistest)
Assert(ItemPointerIsValid(&htup->t_self));
Assert(htup->t_tableOid != InvalidOid);
+ InfomaskAssertions(tuple);
/*
* If the inserting transaction is marked invalid, then it aborted, and
@@ -1520,6 +1546,8 @@ HeapTupleHeaderIsOnlyLocked(HeapTupleHeader tuple)
{
TransactionId xmax;
+ InfomaskAssertions(tuple);
+
/* if there's no valid Xmax, then there's obviously no update either */
if (tuple->t_infomask & HEAP_XMAX_INVALID)
return true;
@@ -1592,6 +1620,7 @@ HeapTupleSatisfiesHistoricMVCC(HeapTuple htup, Snapshot snapshot,
Assert(ItemPointerIsValid(&htup->t_self));
Assert(htup->t_tableOid != InvalidOid);
+ InfomaskAssertions(tuple);
/* inserting transaction aborted */
if (HeapTupleHeaderXminInvalid(tuple))
@@ -1765,6 +1794,8 @@ HeapTupleSatisfiesHistoricMVCC(HeapTuple htup, Snapshot snapshot,
bool
HeapTupleSatisfiesVisibility(HeapTuple tup, Snapshot snapshot, Buffer buffer)
{
+ InfomaskAssertions(tup->t_data);
+
switch (snapshot->snapshot_type)
{
case SNAPSHOT_MVCC:
diff --git a/src/backend/access/heap/hio.c b/src/backend/access/heap/hio.c
index ae2e2ce37a..d6274617b8 100644
--- a/src/backend/access/heap/hio.c
+++ b/src/backend/access/heap/hio.c
@@ -55,6 +55,8 @@ RelationPutHeapTuple(Relation relation,
*/
Assert(!((tuple->t_data->t_infomask & HEAP_XMAX_COMMITTED) &&
(tuple->t_data->t_infomask & HEAP_XMAX_IS_MULTI)));
+ Assert(!((tuple->t_data->t_infomask & HEAP_XMAX_COMMITTED) &&
+ HEAP_XMAX_IS_LOCKED_ONLY(tuple->t_data->t_infomask)));
/* Add the tuple to the page */
pageHeader = BufferGetPage(buffer);
--
2.25.1
Hi,
On 2022-03-22 16:06:40 -0700, Nathan Bossart wrote:
On Thu, Mar 17, 2022 at 04:45:28PM -0700, Nathan Bossart wrote:
I think this one requires some more work, and it needn't be a priority for
v15, so I've adjusted the commitfest entry to v16 and moved it to the next
commitfest.Here is a new patch. The main differences from v3 are in
heapam_visibility.c. Specifically, instead of trying to work the infomask
checks into the visibility logic, I added a new function that does a couple
of assertions. This function is called at the beginning of each visibility
function.What do folks think? The options I've considered are 1) not adding any
such checks to heapam_visibility.c, 2) only adding assertions like the
attached patch, or 3) actually using elog(ERROR, ...) when the invalid bit
patterns are detected. AFAICT (1) is more in line with existing invalid
bit patterns (e.g., XMAX_COMMITTED + XMAX_IS_MULTI). There are a couple of
scattered assertions, but most code paths don't check for it. (2) adds
additional checks, but only for --enable-cassert builds. (3) would add
checks even for non-assert builds, but there would presumably be some
performance cost involved.
From 2d6b372cf61782e0fd52590b57b1c914b0ed7a4c Mon Sep 17 00:00:00 2001
From: Nathan Bossart <nathandbossart@gmail.com>
Date: Tue, 22 Mar 2022 15:35:34 -0700
Subject: [PATCH v4 1/1] disallow XMAX_COMMITTED + XMAX_LOCK_ONLY
Just skimming this thread quickly, I really have no idea what this is trying
to achieve and the commit message doesn't help either... I didn't read the
referenced thread, but I shouldn't have to, to get a basic idea.
Greetings,
Andres Freund
On Tue, Mar 22, 2022 at 04:13:47PM -0700, Andres Freund wrote:
Just skimming this thread quickly, I really have no idea what this is trying
to achieve and the commit message doesn't help either... I didn't read the
referenced thread, but I shouldn't have to, to get a basic idea.
Ah, my bad. I should've made sure the context was carried over better. I
updated the commit message with some basic information about the intent.
Please let me know if there is anything else that needs to be cleared up.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
Attachments:
v5-0001-Disallow-setting-XMAX_COMMITTED-and-XMAX_LOCK_ONL.patchtext/x-diff; charset=us-asciiDownload
From 739ec6e42183280d57d867157cfe1b37d127ca54 Mon Sep 17 00:00:00 2001
From: Nathan Bossart <nathandbossart@gmail.com>
Date: Tue, 22 Mar 2022 15:35:34 -0700
Subject: [PATCH v5 1/1] Disallow setting XMAX_COMMITTED and XMAX_LOCK_ONLY
together.
Even though Postgres doesn't set both the XMAX_COMMITTED and
XMAX_LOCK_ONLY infomask bits on the same tuple header, the heap
visibility logic still accepts it. However, other functions like
compute_new_xmax_infomask() seem to assume that this bit pattern
is not possible.
This change marks this bit pattern as disallowed, removes the heap
visibility logic that handles it, and adds checks like those for
other disallowed infomask bit combinations (e.g., XMAX_COMMITTED
and XMAX_IS_MULTI). Besides simplifying the heap visibility logic
a bit, this change aims to reduce ambiguity about the legal tuple
header states.
Note that this change also disallows XMAX_COMMITTED together with
the special pre-v9.3 locked-only bit pattern that
HEAP_XMAX_IS_LOCKED_ONLY checks for. This locked-only bit pattern
may still be present on servers pg_upgraded from pre-v9.3 versions.
---
contrib/amcheck/verify_heapam.c | 19 ++++
src/backend/access/heap/README.tuplock | 2 +-
src/backend/access/heap/heapam.c | 10 +++
src/backend/access/heap/heapam_visibility.c | 97 ++++++++++++++-------
src/backend/access/heap/hio.c | 2 +
5 files changed, 96 insertions(+), 34 deletions(-)
diff --git a/contrib/amcheck/verify_heapam.c b/contrib/amcheck/verify_heapam.c
index e5f7355dcb..f30b9c234f 100644
--- a/contrib/amcheck/verify_heapam.c
+++ b/contrib/amcheck/verify_heapam.c
@@ -665,6 +665,25 @@ check_tuple_header(HeapCheckContext *ctx)
*/
}
+ if ((ctx->tuphdr->t_infomask & HEAP_XMAX_COMMITTED) &&
+ (ctx->tuphdr->t_infomask & HEAP_XMAX_LOCK_ONLY))
+ {
+ report_corruption(ctx,
+ pstrdup("locked-only should not be marked committed"));
+
+ /* As above, do not skip further checks. */
+ }
+
+ /* also check for pre-v9.3 lock-only bit pattern */
+ if ((ctx->tuphdr->t_infomask & HEAP_XMAX_COMMITTED) &&
+ HEAP_XMAX_IS_LOCKED_ONLY(ctx->tuphdr->t_infomask))
+ {
+ report_corruption(ctx,
+ pstrdup("tuple with HEAP_XMAX_EXCL_LOCK set and HEAP_XMAX_IS_MULTI unset should not be marked committed"));
+
+ /* As above, do not skip further checks. */
+ }
+
if (infomask & HEAP_HASNULL)
expected_hoff = MAXALIGN(SizeofHeapTupleHeader + BITMAPLEN(ctx->natts));
else
diff --git a/src/backend/access/heap/README.tuplock b/src/backend/access/heap/README.tuplock
index 6441e8baf0..4e565e60ab 100644
--- a/src/backend/access/heap/README.tuplock
+++ b/src/backend/access/heap/README.tuplock
@@ -152,4 +152,4 @@ The following infomask bits are applicable:
whether the XMAX is a TransactionId or a MultiXactId.
We currently never set the HEAP_XMAX_COMMITTED when the HEAP_XMAX_IS_MULTI bit
-is set.
+or the HEAP_XMAX_LOCK_ONLY bit is set.
diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c
index 3746336a09..3f0b4cd754 100644
--- a/src/backend/access/heap/heapam.c
+++ b/src/backend/access/heap/heapam.c
@@ -5151,6 +5151,16 @@ l5:
MultiXactStatus status;
MultiXactStatus new_status;
+ /*
+ * Currently we don't allow XMAX_LOCK_ONLY and XMAX_COMMITTED to both be
+ * set in a tuple header, so cross-check.
+ *
+ * Note that this also checks for the special locked-only bit pattern
+ * that may be present on servers that were pg_upgraded from pre-v9.3
+ * versions.
+ */
+ Assert(!HEAP_XMAX_IS_LOCKED_ONLY(old_infomask));
+
if (old_infomask2 & HEAP_KEYS_UPDATED)
status = MultiXactStatusUpdate;
else
diff --git a/src/backend/access/heap/heapam_visibility.c b/src/backend/access/heap/heapam_visibility.c
index ff0b8a688d..7d6fb66b0d 100644
--- a/src/backend/access/heap/heapam_visibility.c
+++ b/src/backend/access/heap/heapam_visibility.c
@@ -78,6 +78,31 @@
#include "utils/snapmgr.h"
+/*
+ * InfomaskAssertions()
+ *
+ * Checks for invalid infomask bit patterns.
+ */
+static inline void
+InfomaskAssertions(HeapTupleHeader tuple)
+{
+ /*
+ * XMAX_COMMITTED and XMAX_LOCK_ONLY cannot both be set in a tuple header.
+ *
+ * Note that this also checks for the special locked-only bit pattern that
+ * may be present on servers that were pg_upgraded from pre-v9.3 versions.
+ */
+ Assert(!((tuple->t_infomask & HEAP_XMAX_COMMITTED) &&
+ HEAP_XMAX_IS_LOCKED_ONLY(tuple->t_infomask)));
+
+ /*
+ * XMAX_COMMITTED and XMAX_IS_MULTI cannot be be set in a tuple header.
+ */
+ Assert(!((tuple->t_infomask & HEAP_XMAX_COMMITTED) &&
+ (tuple->t_infomask & HEAP_XMAX_IS_MULTI)));
+}
+
+
/*
* SetHintBits()
*
@@ -113,6 +138,8 @@ static inline void
SetHintBits(HeapTupleHeader tuple, Buffer buffer,
uint16 infomask, TransactionId xid)
{
+ InfomaskAssertions(tuple);
+
if (TransactionIdIsValid(xid))
{
/* NB: xid must be known committed here! */
@@ -127,6 +154,7 @@ SetHintBits(HeapTupleHeader tuple, Buffer buffer,
}
tuple->t_infomask |= infomask;
+ InfomaskAssertions(tuple); /* we check again after infomask updates */
MarkBufferDirtyHint(buffer, true);
}
@@ -172,6 +200,7 @@ HeapTupleSatisfiesSelf(HeapTuple htup, Snapshot snapshot, Buffer buffer)
Assert(ItemPointerIsValid(&htup->t_self));
Assert(htup->t_tableOid != InvalidOid);
+ InfomaskAssertions(tuple);
if (!HeapTupleHeaderXminCommitted(tuple))
{
@@ -271,11 +300,7 @@ HeapTupleSatisfiesSelf(HeapTuple htup, Snapshot snapshot, Buffer buffer)
return true;
if (tuple->t_infomask & HEAP_XMAX_COMMITTED)
- {
- if (HEAP_XMAX_IS_LOCKED_ONLY(tuple->t_infomask))
- return true;
return false; /* updated by other */
- }
if (tuple->t_infomask & HEAP_XMAX_IS_MULTI)
{
@@ -365,6 +390,7 @@ HeapTupleSatisfiesToast(HeapTuple htup, Snapshot snapshot,
Assert(ItemPointerIsValid(&htup->t_self));
Assert(htup->t_tableOid != InvalidOid);
+ InfomaskAssertions(tuple);
if (!HeapTupleHeaderXminCommitted(tuple))
{
@@ -461,6 +487,7 @@ HeapTupleSatisfiesUpdate(HeapTuple htup, CommandId curcid,
Assert(ItemPointerIsValid(&htup->t_self));
Assert(htup->t_tableOid != InvalidOid);
+ InfomaskAssertions(tuple);
if (!HeapTupleHeaderXminCommitted(tuple))
{
@@ -605,8 +632,6 @@ HeapTupleSatisfiesUpdate(HeapTuple htup, CommandId curcid,
if (tuple->t_infomask & HEAP_XMAX_COMMITTED)
{
- if (HEAP_XMAX_IS_LOCKED_ONLY(tuple->t_infomask))
- return TM_Ok;
if (!ItemPointerEquals(&htup->t_self, &tuple->t_ctid))
return TM_Updated; /* updated by other */
else
@@ -746,6 +771,7 @@ HeapTupleSatisfiesDirty(HeapTuple htup, Snapshot snapshot,
Assert(ItemPointerIsValid(&htup->t_self));
Assert(htup->t_tableOid != InvalidOid);
+ InfomaskAssertions(tuple);
snapshot->xmin = snapshot->xmax = InvalidTransactionId;
snapshot->speculativeToken = 0;
@@ -866,11 +892,7 @@ HeapTupleSatisfiesDirty(HeapTuple htup, Snapshot snapshot,
return true;
if (tuple->t_infomask & HEAP_XMAX_COMMITTED)
- {
- if (HEAP_XMAX_IS_LOCKED_ONLY(tuple->t_infomask))
- return true;
return false; /* updated by other */
- }
if (tuple->t_infomask & HEAP_XMAX_IS_MULTI)
{
@@ -963,6 +985,7 @@ HeapTupleSatisfiesMVCC(HeapTuple htup, Snapshot snapshot,
Assert(ItemPointerIsValid(&htup->t_self));
Assert(htup->t_tableOid != InvalidOid);
+ InfomaskAssertions(tuple);
if (!HeapTupleHeaderXminCommitted(tuple))
{
@@ -1164,6 +1187,8 @@ HeapTupleSatisfiesVacuum(HeapTuple htup, TransactionId OldestXmin,
TransactionId dead_after = InvalidTransactionId;
HTSV_Result res;
+ InfomaskAssertions(htup->t_data);
+
res = HeapTupleSatisfiesVacuumHorizon(htup, buffer, &dead_after);
if (res == HEAPTUPLE_RECENTLY_DEAD)
@@ -1199,6 +1224,7 @@ HeapTupleSatisfiesVacuumHorizon(HeapTuple htup, Buffer buffer, TransactionId *de
Assert(ItemPointerIsValid(&htup->t_self));
Assert(htup->t_tableOid != InvalidOid);
Assert(dead_after != NULL);
+ InfomaskAssertions(tuple);
*dead_after = InvalidTransactionId;
@@ -1306,31 +1332,28 @@ HeapTupleSatisfiesVacuumHorizon(HeapTuple htup, Buffer buffer, TransactionId *de
{
/*
* "Deleting" xact really only locked it, so the tuple is live in any
- * case. However, we should make sure that either XMAX_COMMITTED or
- * XMAX_INVALID gets set once the xact is gone, to reduce the costs of
- * examining the tuple for future xacts.
+ * case. However, we should make sure that XMAX_INVALID gets set once
+ * the xact is gone, to reduce the costs of examining the tuple for
+ * future xacts.
*/
- if (!(tuple->t_infomask & HEAP_XMAX_COMMITTED))
+ if (tuple->t_infomask & HEAP_XMAX_IS_MULTI)
{
- if (tuple->t_infomask & HEAP_XMAX_IS_MULTI)
- {
- /*
- * If it's a pre-pg_upgrade tuple, the multixact cannot
- * possibly be running; otherwise have to check.
- */
- if (!HEAP_LOCKED_UPGRADED(tuple->t_infomask) &&
- MultiXactIdIsRunning(HeapTupleHeaderGetRawXmax(tuple),
- true))
- return HEAPTUPLE_LIVE;
- SetHintBits(tuple, buffer, HEAP_XMAX_INVALID, InvalidTransactionId);
- }
- else
- {
- if (TransactionIdIsInProgress(HeapTupleHeaderGetRawXmax(tuple)))
- return HEAPTUPLE_LIVE;
- SetHintBits(tuple, buffer, HEAP_XMAX_INVALID,
- InvalidTransactionId);
- }
+ /*
+ * If it's a pre-pg_upgrade tuple, the multixact cannot possibly be
+ * running; otherwise have to check.
+ */
+ if (!HEAP_LOCKED_UPGRADED(tuple->t_infomask) &&
+ MultiXactIdIsRunning(HeapTupleHeaderGetRawXmax(tuple),
+ true))
+ return HEAPTUPLE_LIVE;
+ SetHintBits(tuple, buffer, HEAP_XMAX_INVALID, InvalidTransactionId);
+ }
+ else
+ {
+ if (TransactionIdIsInProgress(HeapTupleHeaderGetRawXmax(tuple)))
+ return HEAPTUPLE_LIVE;
+ SetHintBits(tuple, buffer, HEAP_XMAX_INVALID,
+ InvalidTransactionId);
}
/*
@@ -1431,6 +1454,8 @@ HeapTupleSatisfiesNonVacuumable(HeapTuple htup, Snapshot snapshot,
TransactionId dead_after = InvalidTransactionId;
HTSV_Result res;
+ InfomaskAssertions(htup->t_data);
+
res = HeapTupleSatisfiesVacuumHorizon(htup, buffer, &dead_after);
if (res == HEAPTUPLE_RECENTLY_DEAD)
@@ -1467,6 +1492,7 @@ HeapTupleIsSurelyDead(HeapTuple htup, GlobalVisState *vistest)
Assert(ItemPointerIsValid(&htup->t_self));
Assert(htup->t_tableOid != InvalidOid);
+ InfomaskAssertions(tuple);
/*
* If the inserting transaction is marked invalid, then it aborted, and
@@ -1520,6 +1546,8 @@ HeapTupleHeaderIsOnlyLocked(HeapTupleHeader tuple)
{
TransactionId xmax;
+ InfomaskAssertions(tuple);
+
/* if there's no valid Xmax, then there's obviously no update either */
if (tuple->t_infomask & HEAP_XMAX_INVALID)
return true;
@@ -1592,6 +1620,7 @@ HeapTupleSatisfiesHistoricMVCC(HeapTuple htup, Snapshot snapshot,
Assert(ItemPointerIsValid(&htup->t_self));
Assert(htup->t_tableOid != InvalidOid);
+ InfomaskAssertions(tuple);
/* inserting transaction aborted */
if (HeapTupleHeaderXminInvalid(tuple))
@@ -1765,6 +1794,8 @@ HeapTupleSatisfiesHistoricMVCC(HeapTuple htup, Snapshot snapshot,
bool
HeapTupleSatisfiesVisibility(HeapTuple tup, Snapshot snapshot, Buffer buffer)
{
+ InfomaskAssertions(tup->t_data);
+
switch (snapshot->snapshot_type)
{
case SNAPSHOT_MVCC:
diff --git a/src/backend/access/heap/hio.c b/src/backend/access/heap/hio.c
index ae2e2ce37a..d6274617b8 100644
--- a/src/backend/access/heap/hio.c
+++ b/src/backend/access/heap/hio.c
@@ -55,6 +55,8 @@ RelationPutHeapTuple(Relation relation,
*/
Assert(!((tuple->t_data->t_infomask & HEAP_XMAX_COMMITTED) &&
(tuple->t_data->t_infomask & HEAP_XMAX_IS_MULTI)));
+ Assert(!((tuple->t_data->t_infomask & HEAP_XMAX_COMMITTED) &&
+ HEAP_XMAX_IS_LOCKED_ONLY(tuple->t_data->t_infomask)));
/* Add the tuple to the page */
pageHeader = BufferGetPage(buffer);
--
2.25.1
Here is a rebased patch for cfbot.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
Attachments:
v6-0001-Disallow-setting-XMAX_COMMITTED-and-XMAX_LOCK_ONL.patchtext/x-diff; charset=us-asciiDownload
From 9ae1f5409ddeee17b99a9565247da885cbb86be9 Mon Sep 17 00:00:00 2001
From: Nathan Bossart <nathandbossart@gmail.com>
Date: Tue, 22 Mar 2022 15:35:34 -0700
Subject: [PATCH v6 1/1] Disallow setting XMAX_COMMITTED and XMAX_LOCK_ONLY
together.
Even though Postgres doesn't set both the XMAX_COMMITTED and
XMAX_LOCK_ONLY infomask bits on the same tuple header, the heap
visibility logic still accepts it. However, other functions like
compute_new_xmax_infomask() seem to assume that this bit pattern
is not possible.
This change marks this bit pattern as disallowed, removes the heap
visibility logic that handles it, and adds checks like those for
other disallowed infomask bit combinations (e.g., XMAX_COMMITTED
and XMAX_IS_MULTI). Besides simplifying the heap visibility logic
a bit, this change aims to reduce ambiguity about the legal tuple
header states.
Note that this change also disallows XMAX_COMMITTED together with
the special pre-v9.3 locked-only bit pattern that
HEAP_XMAX_IS_LOCKED_ONLY checks for. This locked-only bit pattern
may still be present on servers pg_upgraded from pre-v9.3 versions.
---
contrib/amcheck/verify_heapam.c | 19 ++++
src/backend/access/heap/README.tuplock | 2 +-
src/backend/access/heap/heapam.c | 10 +++
src/backend/access/heap/heapam_visibility.c | 97 ++++++++++++++-------
src/backend/access/heap/hio.c | 2 +
5 files changed, 96 insertions(+), 34 deletions(-)
diff --git a/contrib/amcheck/verify_heapam.c b/contrib/amcheck/verify_heapam.c
index d33f33f170..f74f88afc5 100644
--- a/contrib/amcheck/verify_heapam.c
+++ b/contrib/amcheck/verify_heapam.c
@@ -663,6 +663,25 @@ check_tuple_header(HeapCheckContext *ctx)
*/
}
+ if ((ctx->tuphdr->t_infomask & HEAP_XMAX_COMMITTED) &&
+ (ctx->tuphdr->t_infomask & HEAP_XMAX_LOCK_ONLY))
+ {
+ report_corruption(ctx,
+ pstrdup("locked-only should not be marked committed"));
+
+ /* As above, do not skip further checks. */
+ }
+
+ /* also check for pre-v9.3 lock-only bit pattern */
+ if ((ctx->tuphdr->t_infomask & HEAP_XMAX_COMMITTED) &&
+ HEAP_XMAX_IS_LOCKED_ONLY(ctx->tuphdr->t_infomask))
+ {
+ report_corruption(ctx,
+ pstrdup("tuple with HEAP_XMAX_EXCL_LOCK set and HEAP_XMAX_IS_MULTI unset should not be marked committed"));
+
+ /* As above, do not skip further checks. */
+ }
+
if (infomask & HEAP_HASNULL)
expected_hoff = MAXALIGN(SizeofHeapTupleHeader + BITMAPLEN(ctx->natts));
else
diff --git a/src/backend/access/heap/README.tuplock b/src/backend/access/heap/README.tuplock
index 6441e8baf0..4e565e60ab 100644
--- a/src/backend/access/heap/README.tuplock
+++ b/src/backend/access/heap/README.tuplock
@@ -152,4 +152,4 @@ The following infomask bits are applicable:
whether the XMAX is a TransactionId or a MultiXactId.
We currently never set the HEAP_XMAX_COMMITTED when the HEAP_XMAX_IS_MULTI bit
-is set.
+or the HEAP_XMAX_LOCK_ONLY bit is set.
diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c
index eb811d751e..616df576c3 100644
--- a/src/backend/access/heap/heapam.c
+++ b/src/backend/access/heap/heapam.c
@@ -5115,6 +5115,16 @@ l5:
MultiXactStatus status;
MultiXactStatus new_status;
+ /*
+ * Currently we don't allow XMAX_LOCK_ONLY and XMAX_COMMITTED to both be
+ * set in a tuple header, so cross-check.
+ *
+ * Note that this also checks for the special locked-only bit pattern
+ * that may be present on servers that were pg_upgraded from pre-v9.3
+ * versions.
+ */
+ Assert(!HEAP_XMAX_IS_LOCKED_ONLY(old_infomask));
+
if (old_infomask2 & HEAP_KEYS_UPDATED)
status = MultiXactStatusUpdate;
else
diff --git a/src/backend/access/heap/heapam_visibility.c b/src/backend/access/heap/heapam_visibility.c
index 6e33d1c881..e6ee8ff1fa 100644
--- a/src/backend/access/heap/heapam_visibility.c
+++ b/src/backend/access/heap/heapam_visibility.c
@@ -78,6 +78,31 @@
#include "utils/snapmgr.h"
+/*
+ * InfomaskAssertions()
+ *
+ * Checks for invalid infomask bit patterns.
+ */
+static inline void
+InfomaskAssertions(HeapTupleHeader tuple)
+{
+ /*
+ * XMAX_COMMITTED and XMAX_LOCK_ONLY cannot both be set in a tuple header.
+ *
+ * Note that this also checks for the special locked-only bit pattern that
+ * may be present on servers that were pg_upgraded from pre-v9.3 versions.
+ */
+ Assert(!((tuple->t_infomask & HEAP_XMAX_COMMITTED) &&
+ HEAP_XMAX_IS_LOCKED_ONLY(tuple->t_infomask)));
+
+ /*
+ * XMAX_COMMITTED and XMAX_IS_MULTI cannot be be set in a tuple header.
+ */
+ Assert(!((tuple->t_infomask & HEAP_XMAX_COMMITTED) &&
+ (tuple->t_infomask & HEAP_XMAX_IS_MULTI)));
+}
+
+
/*
* SetHintBits()
*
@@ -113,6 +138,8 @@ static inline void
SetHintBits(HeapTupleHeader tuple, Buffer buffer,
uint16 infomask, TransactionId xid)
{
+ InfomaskAssertions(tuple);
+
if (TransactionIdIsValid(xid))
{
/* NB: xid must be known committed here! */
@@ -127,6 +154,7 @@ SetHintBits(HeapTupleHeader tuple, Buffer buffer,
}
tuple->t_infomask |= infomask;
+ InfomaskAssertions(tuple); /* we check again after infomask updates */
MarkBufferDirtyHint(buffer, true);
}
@@ -172,6 +200,7 @@ HeapTupleSatisfiesSelf(HeapTuple htup, Snapshot snapshot, Buffer buffer)
Assert(ItemPointerIsValid(&htup->t_self));
Assert(htup->t_tableOid != InvalidOid);
+ InfomaskAssertions(tuple);
if (!HeapTupleHeaderXminCommitted(tuple))
{
@@ -271,11 +300,7 @@ HeapTupleSatisfiesSelf(HeapTuple htup, Snapshot snapshot, Buffer buffer)
return true;
if (tuple->t_infomask & HEAP_XMAX_COMMITTED)
- {
- if (HEAP_XMAX_IS_LOCKED_ONLY(tuple->t_infomask))
- return true;
return false; /* updated by other */
- }
if (tuple->t_infomask & HEAP_XMAX_IS_MULTI)
{
@@ -365,6 +390,7 @@ HeapTupleSatisfiesToast(HeapTuple htup, Snapshot snapshot,
Assert(ItemPointerIsValid(&htup->t_self));
Assert(htup->t_tableOid != InvalidOid);
+ InfomaskAssertions(tuple);
if (!HeapTupleHeaderXminCommitted(tuple))
{
@@ -461,6 +487,7 @@ HeapTupleSatisfiesUpdate(HeapTuple htup, CommandId curcid,
Assert(ItemPointerIsValid(&htup->t_self));
Assert(htup->t_tableOid != InvalidOid);
+ InfomaskAssertions(tuple);
if (!HeapTupleHeaderXminCommitted(tuple))
{
@@ -605,8 +632,6 @@ HeapTupleSatisfiesUpdate(HeapTuple htup, CommandId curcid,
if (tuple->t_infomask & HEAP_XMAX_COMMITTED)
{
- if (HEAP_XMAX_IS_LOCKED_ONLY(tuple->t_infomask))
- return TM_Ok;
if (!ItemPointerEquals(&htup->t_self, &tuple->t_ctid))
return TM_Updated; /* updated by other */
else
@@ -746,6 +771,7 @@ HeapTupleSatisfiesDirty(HeapTuple htup, Snapshot snapshot,
Assert(ItemPointerIsValid(&htup->t_self));
Assert(htup->t_tableOid != InvalidOid);
+ InfomaskAssertions(tuple);
snapshot->xmin = snapshot->xmax = InvalidTransactionId;
snapshot->speculativeToken = 0;
@@ -866,11 +892,7 @@ HeapTupleSatisfiesDirty(HeapTuple htup, Snapshot snapshot,
return true;
if (tuple->t_infomask & HEAP_XMAX_COMMITTED)
- {
- if (HEAP_XMAX_IS_LOCKED_ONLY(tuple->t_infomask))
- return true;
return false; /* updated by other */
- }
if (tuple->t_infomask & HEAP_XMAX_IS_MULTI)
{
@@ -963,6 +985,7 @@ HeapTupleSatisfiesMVCC(HeapTuple htup, Snapshot snapshot,
Assert(ItemPointerIsValid(&htup->t_self));
Assert(htup->t_tableOid != InvalidOid);
+ InfomaskAssertions(tuple);
if (!HeapTupleHeaderXminCommitted(tuple))
{
@@ -1164,6 +1187,8 @@ HeapTupleSatisfiesVacuum(HeapTuple htup, TransactionId OldestXmin,
TransactionId dead_after = InvalidTransactionId;
HTSV_Result res;
+ InfomaskAssertions(htup->t_data);
+
res = HeapTupleSatisfiesVacuumHorizon(htup, buffer, &dead_after);
if (res == HEAPTUPLE_RECENTLY_DEAD)
@@ -1199,6 +1224,7 @@ HeapTupleSatisfiesVacuumHorizon(HeapTuple htup, Buffer buffer, TransactionId *de
Assert(ItemPointerIsValid(&htup->t_self));
Assert(htup->t_tableOid != InvalidOid);
Assert(dead_after != NULL);
+ InfomaskAssertions(tuple);
*dead_after = InvalidTransactionId;
@@ -1306,31 +1332,28 @@ HeapTupleSatisfiesVacuumHorizon(HeapTuple htup, Buffer buffer, TransactionId *de
{
/*
* "Deleting" xact really only locked it, so the tuple is live in any
- * case. However, we should make sure that either XMAX_COMMITTED or
- * XMAX_INVALID gets set once the xact is gone, to reduce the costs of
- * examining the tuple for future xacts.
+ * case. However, we should make sure that XMAX_INVALID gets set once
+ * the xact is gone, to reduce the costs of examining the tuple for
+ * future xacts.
*/
- if (!(tuple->t_infomask & HEAP_XMAX_COMMITTED))
+ if (tuple->t_infomask & HEAP_XMAX_IS_MULTI)
{
- if (tuple->t_infomask & HEAP_XMAX_IS_MULTI)
- {
- /*
- * If it's a pre-pg_upgrade tuple, the multixact cannot
- * possibly be running; otherwise have to check.
- */
- if (!HEAP_LOCKED_UPGRADED(tuple->t_infomask) &&
- MultiXactIdIsRunning(HeapTupleHeaderGetRawXmax(tuple),
- true))
- return HEAPTUPLE_LIVE;
- SetHintBits(tuple, buffer, HEAP_XMAX_INVALID, InvalidTransactionId);
- }
- else
- {
- if (TransactionIdIsInProgress(HeapTupleHeaderGetRawXmax(tuple)))
- return HEAPTUPLE_LIVE;
- SetHintBits(tuple, buffer, HEAP_XMAX_INVALID,
- InvalidTransactionId);
- }
+ /*
+ * If it's a pre-pg_upgrade tuple, the multixact cannot possibly be
+ * running; otherwise have to check.
+ */
+ if (!HEAP_LOCKED_UPGRADED(tuple->t_infomask) &&
+ MultiXactIdIsRunning(HeapTupleHeaderGetRawXmax(tuple),
+ true))
+ return HEAPTUPLE_LIVE;
+ SetHintBits(tuple, buffer, HEAP_XMAX_INVALID, InvalidTransactionId);
+ }
+ else
+ {
+ if (TransactionIdIsInProgress(HeapTupleHeaderGetRawXmax(tuple)))
+ return HEAPTUPLE_LIVE;
+ SetHintBits(tuple, buffer, HEAP_XMAX_INVALID,
+ InvalidTransactionId);
}
/*
@@ -1431,6 +1454,8 @@ HeapTupleSatisfiesNonVacuumable(HeapTuple htup, Snapshot snapshot,
TransactionId dead_after = InvalidTransactionId;
HTSV_Result res;
+ InfomaskAssertions(htup->t_data);
+
res = HeapTupleSatisfiesVacuumHorizon(htup, buffer, &dead_after);
if (res == HEAPTUPLE_RECENTLY_DEAD)
@@ -1467,6 +1492,7 @@ HeapTupleIsSurelyDead(HeapTuple htup, GlobalVisState *vistest)
Assert(ItemPointerIsValid(&htup->t_self));
Assert(htup->t_tableOid != InvalidOid);
+ InfomaskAssertions(tuple);
/*
* If the inserting transaction is marked invalid, then it aborted, and
@@ -1520,6 +1546,8 @@ HeapTupleHeaderIsOnlyLocked(HeapTupleHeader tuple)
{
TransactionId xmax;
+ InfomaskAssertions(tuple);
+
/* if there's no valid Xmax, then there's obviously no update either */
if (tuple->t_infomask & HEAP_XMAX_INVALID)
return true;
@@ -1592,6 +1620,7 @@ HeapTupleSatisfiesHistoricMVCC(HeapTuple htup, Snapshot snapshot,
Assert(ItemPointerIsValid(&htup->t_self));
Assert(htup->t_tableOid != InvalidOid);
+ InfomaskAssertions(tuple);
/* inserting transaction aborted */
if (HeapTupleHeaderXminInvalid(tuple))
@@ -1765,6 +1794,8 @@ HeapTupleSatisfiesHistoricMVCC(HeapTuple htup, Snapshot snapshot,
bool
HeapTupleSatisfiesVisibility(HeapTuple htup, Snapshot snapshot, Buffer buffer)
{
+ InfomaskAssertions(htup->t_data);
+
switch (snapshot->snapshot_type)
{
case SNAPSHOT_MVCC:
diff --git a/src/backend/access/heap/hio.c b/src/backend/access/heap/hio.c
index ae2e2ce37a..d6274617b8 100644
--- a/src/backend/access/heap/hio.c
+++ b/src/backend/access/heap/hio.c
@@ -55,6 +55,8 @@ RelationPutHeapTuple(Relation relation,
*/
Assert(!((tuple->t_data->t_infomask & HEAP_XMAX_COMMITTED) &&
(tuple->t_data->t_infomask & HEAP_XMAX_IS_MULTI)));
+ Assert(!((tuple->t_data->t_infomask & HEAP_XMAX_COMMITTED) &&
+ HEAP_XMAX_IS_LOCKED_ONLY(tuple->t_data->t_infomask)));
/* Add the tuple to the page */
pageHeader = BufferGetPage(buffer);
--
2.25.1
On Tue, Sep 20, 2022 at 2:32 PM Nathan Bossart <nathandbossart@gmail.com>
wrote:
Here is a rebased patch for cfbot.
Applies, passes make check world.
Patch is straightforward, but the previous code is less so. It purported to
set XMAX_COMMITTED _or_ XMAX_INVALID, but never seemed to un-set
XMAX_COMMITTED, was that the source of the double-setting?
Hi,
On 2022-09-20 11:32:02 -0700, Nathan Bossart wrote:
Note that this change also disallows XMAX_COMMITTED together with
the special pre-v9.3 locked-only bit pattern that
HEAP_XMAX_IS_LOCKED_ONLY checks for. This locked-only bit pattern
may still be present on servers pg_upgraded from pre-v9.3 versions.
Given that fact, that aspect at least seems to be not viable?
Greetings,
Andres Freund
On Thu, Feb 02, 2023 at 06:59:51AM -0800, Andres Freund wrote:
On 2022-09-20 11:32:02 -0700, Nathan Bossart wrote:
Note that this change also disallows XMAX_COMMITTED together with
the special pre-v9.3 locked-only bit pattern that
HEAP_XMAX_IS_LOCKED_ONLY checks for. This locked-only bit pattern
may still be present on servers pg_upgraded from pre-v9.3 versions.Given that fact, that aspect at least seems to be not viable?
AFAICT from looking at the v9.2 code, the same idea holds true for this
special bit pattern. I only see HEAP_XMAX_INVALID set when one of the
infomask lock bits is set, and those bits now correspond to
HEAP_XMAX_LOCK_ONLY and HEAP_XMAX_EXCL_LOCK (which are both covered by the
HEAP_XMAX_IS_LOCKED_ONLY macro). Of course, I could be missing something.
Do you think we should limit this to the v9.3+ bit pattern?
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com