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