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