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