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
>From 2d6b372cf61782e0fd52590b57b1c914b0ed7a4c Mon Sep 17 00:00:00 2001
From: Nathan Bossart <nathandboss...@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
