From f05ae2fa94b4e8c8fae4ccbc8d79cfbaa6a0e7b2 Mon Sep 17 00:00:00 2001 From: Alvaro Herrera Date: Thu, 2 Nov 2017 15:51:05 +0100 Subject: [PATCH] Revert bogus fixes of HOT-freezing bug It turns out we misdiagnosed what the real problem was. Revert the previous changes, because they may have worse consequences going forward. A better fix is forthcoming. The simplistic test case is kept, though disabled. Discussion: https://postgr.es/m/20171102112019.33wb7g5wp4zpjelu@alap3.anarazel.de --- src/backend/access/heap/heapam.c | 135 +++++++++----------------- src/backend/access/heap/pruneheap.c | 4 +- src/backend/commands/vacuumlazy.c | 16 +-- src/backend/executor/execMain.c | 6 +- src/include/access/heapam.h | 3 - src/test/isolation/isolation_schedule | 1 - 6 files changed, 60 insertions(+), 105 deletions(-) diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c index a7dffd151ab..f5db3c775c3 100644 --- a/src/backend/access/heap/heapam.c +++ b/src/backend/access/heap/heapam.c @@ -1718,7 +1718,8 @@ heap_hot_search_buffer(ItemPointer tid, Relation relation, Buffer buffer, * broken. */ if (TransactionIdIsValid(prev_xmax) && - !HeapTupleUpdateXmaxMatchesXmin(prev_xmax, heapTuple->t_data)) + !TransactionIdEquals(prev_xmax, + HeapTupleHeaderGetXmin(heapTuple->t_data))) break; /* @@ -1887,7 +1888,7 @@ heap_get_latest_tid(Relation relation, * tuple. Check for XMIN match. */ if (TransactionIdIsValid(priorXmax) && - !HeapTupleUpdateXmaxMatchesXmin(priorXmax, tp.t_data)) + !TransactionIdEquals(priorXmax, HeapTupleHeaderGetXmin(tp.t_data))) { UnlockReleaseBuffer(buffer); break; @@ -1919,39 +1920,6 @@ heap_get_latest_tid(Relation relation, } /* end of loop */ } -/* - * HeapTupleUpdateXmaxMatchesXmin - verify update chain xmax/xmin lineage - * - * Given the new version of a tuple after some update, verify whether the - * given Xmax (corresponding to the previous version) matches the tuple's - * Xmin, taking into account that the Xmin might have been frozen after the - * update. - */ -bool -HeapTupleUpdateXmaxMatchesXmin(TransactionId xmax, HeapTupleHeader htup) -{ - TransactionId xmin = HeapTupleHeaderGetXmin(htup); - - /* - * If the xmax of the old tuple is identical to the xmin of the new one, - * it's a match. - */ - if (TransactionIdEquals(xmax, xmin)) - return true; - - /* - * When a tuple is frozen, the original Xmin is lost, but we know it's a - * committed transaction. So unless the Xmax is InvalidXid, we don't know - * for certain that there is a match, but there may be one; and we must - * return true so that a HOT chain that is half-frozen can be walked - * correctly. - */ - if (TransactionIdEquals(xmin, FrozenTransactionId) && - TransactionIdIsValid(xmax)) - return true; - - return false; -} /* * UpdateXmaxHintBits - update tuple hint bits after xmax transaction ends @@ -5077,7 +5045,8 @@ l4: * the end of the chain, we're done, so return success. */ if (TransactionIdIsValid(priorXmax) && - !HeapTupleUpdateXmaxMatchesXmin(priorXmax, mytup.t_data)) + !TransactionIdEquals(HeapTupleHeaderGetXmin(mytup.t_data), + priorXmax)) { UnlockReleaseBuffer(buf); return HeapTupleMayBeUpdated; @@ -5520,26 +5489,14 @@ FreezeMultiXactId(MultiXactId multi, uint16 t_infomask, Assert(TransactionIdIsValid(xid)); /* - * The updating transaction cannot possibly be still running, but - * verify whether it has committed, and request to set the - * COMMITTED flag if so. (We normally don't see any tuples in - * this state, because they are removed by page pruning before we - * try to freeze the page; but this can happen if the updating - * transaction commits after the page is pruned but before - * HeapTupleSatisfiesVacuum). + * If the xid is older than the cutoff, it has to have aborted, + * otherwise the tuple would have gotten pruned away. */ if (TransactionIdPrecedes(xid, cutoff_xid)) { - if (TransactionIdDidCommit(xid)) - { - xid = FrozenTransactionId; - *flags = FRM_MARK_COMMITTED | FRM_RETURN_IS_XID; - } - else - { - *flags |= FRM_INVALIDATE_XMAX; - xid = InvalidTransactionId; /* not strictly necessary */ - } + Assert(!TransactionIdDidCommit(xid)); + *flags |= FRM_INVALIDATE_XMAX; + xid = InvalidTransactionId; /* not strictly necessary */ } else { @@ -5610,17 +5567,18 @@ FreezeMultiXactId(MultiXactId multi, uint16 t_infomask, /* * It's an update; should we keep it? If the transaction is known - * aborted or crashed then it's okay to ignore it, otherwise not. + * aborted then it's okay to ignore it, otherwise not. However, + * if the Xid is older than the cutoff_xid, we must remove it. + * Note that such an old updater cannot possibly be committed, + * because HeapTupleSatisfiesVacuum would have returned + * HEAPTUPLE_DEAD and we would not be trying to freeze the tuple. + * + * Note the TransactionIdDidAbort() test is just an optimization + * and not strictly necessary for correctness. * * As with all tuple visibility routines, it's critical to test - * TransactionIdIsInProgress before TransactionIdDidCommit, + * TransactionIdIsInProgress before the transam.c routines, * because of race conditions explained in detail in tqual.c. - * - * We normally don't see committed updating transactions earlier - * than the cutoff xid, because they are removed by page pruning - * before we try to freeze the page; but it can happen if the - * updating transaction commits after the page is pruned but - * before HeapTupleSatisfiesVacuum. */ if (TransactionIdIsCurrentTransactionId(xid) || TransactionIdIsInProgress(xid)) @@ -5628,33 +5586,46 @@ FreezeMultiXactId(MultiXactId multi, uint16 t_infomask, Assert(!TransactionIdIsValid(update_xid)); update_xid = xid; } - else if (TransactionIdDidCommit(xid)) + else if (!TransactionIdDidAbort(xid)) { /* - * The transaction committed, so we can tell caller to set - * HEAP_XMAX_COMMITTED. (We can only do this because we know - * the transaction is not running.) + * Test whether to tell caller to set HEAP_XMAX_COMMITTED + * while we have the Xid still in cache. Note this can only + * be done if the transaction is known not running. */ + if (TransactionIdDidCommit(xid)) + update_committed = true; Assert(!TransactionIdIsValid(update_xid)); - update_committed = true; update_xid = xid; } - /* - * Not in progress, not committed -- must be aborted or crashed; - * we can ignore it. - */ - /* * If we determined that it's an Xid corresponding to an update * that must be retained, additionally add it to the list of - * members of the new Multi, in case we end up using that. (We + * members of the new Multis, in case we end up using that. (We * might still decide to use only an update Xid and not a multi, * but it's easier to maintain the list as we walk the old members * list.) + * + * It is possible to end up with a very old updater Xid that + * crashed and thus did not mark itself as aborted in pg_clog. + * That would manifest as a pre-cutoff Xid. Make sure to ignore + * it. */ if (TransactionIdIsValid(update_xid)) - newmembers[nnewmembers++] = members[i]; + { + if (!TransactionIdPrecedes(update_xid, cutoff_xid)) + { + newmembers[nnewmembers++] = members[i]; + } + else + { + /* cannot have committed: would be HEAPTUPLE_DEAD */ + Assert(!TransactionIdDidCommit(update_xid)); + update_xid = InvalidTransactionId; + update_committed = false; + } + } } else { @@ -5721,10 +5692,7 @@ FreezeMultiXactId(MultiXactId multi, uint16 t_infomask, * * It is assumed that the caller has checked the tuple with * HeapTupleSatisfiesVacuum() and determined that it is not HEAPTUPLE_DEAD - * (else we should be removing the tuple, not freezing it). However, note - * that we don't remove HOT tuples even if they are dead, and it'd be incorrect - * to freeze them (because that would make them visible), so we mark them as - * update-committed, and needing further freezing later on. + * (else we should be removing the tuple, not freezing it). * * NB: cutoff_xid *must* be <= the current global xmin, to ensure that any * XID older than it could neither be running nor seen as running by any @@ -5835,18 +5803,7 @@ heap_prepare_freeze_tuple(HeapTupleHeader tuple, TransactionId cutoff_xid, else if (TransactionIdIsNormal(xid) && TransactionIdPrecedes(xid, cutoff_xid)) { - /* - * Must freeze regular XIDs older than the cutoff. We must not freeze - * a HOT-updated tuple, though; doing so would bring it back to life. - */ - if (HeapTupleHeaderIsHotUpdated(tuple)) - { - frz->t_infomask |= HEAP_XMAX_COMMITTED; - changed = true; - /* must not freeze */ - } - else - freeze_xmax = true; + freeze_xmax = true; } if (freeze_xmax) diff --git a/src/backend/access/heap/pruneheap.c b/src/backend/access/heap/pruneheap.c index a342f57f42f..9a8db74cb95 100644 --- a/src/backend/access/heap/pruneheap.c +++ b/src/backend/access/heap/pruneheap.c @@ -435,7 +435,7 @@ heap_prune_chain(Relation relation, Buffer buffer, OffsetNumber rootoffnum, * Check the tuple XMIN against prior XMAX, if any */ if (TransactionIdIsValid(priorXmax) && - !HeapTupleUpdateXmaxMatchesXmin(priorXmax, htup)) + !TransactionIdEquals(HeapTupleHeaderGetXmin(htup), priorXmax)) break; /* @@ -774,7 +774,7 @@ heap_get_root_tuples(Page page, OffsetNumber *root_offsets) htup = (HeapTupleHeader) PageGetItem(page, lp); if (TransactionIdIsValid(priorXmax) && - !HeapTupleUpdateXmaxMatchesXmin(priorXmax, htup)) + !TransactionIdEquals(priorXmax, HeapTupleHeaderGetXmin(htup))) break; /* Remember the root line pointer for this item */ diff --git a/src/backend/commands/vacuumlazy.c b/src/backend/commands/vacuumlazy.c index abef4181068..6ece3591e4f 100644 --- a/src/backend/commands/vacuumlazy.c +++ b/src/backend/commands/vacuumlazy.c @@ -1678,15 +1678,15 @@ lazy_record_dead_tuple(LVRelStats *vacrelstats, ItemPointer itemptr) { /* - * The array must never overflow, since we rely on all deletable tuples - * being removed; inability to remove a tuple might cause an old XID to - * persist beyond the freeze limit, which could be disastrous later on. + * The array shouldn't overflow under normal behavior, but perhaps it + * could if we are given a really small maintenance_work_mem. In that + * case, just forget the last few tuples (we'll get 'em next time). */ - if (vacrelstats->num_dead_tuples >= vacrelstats->max_dead_tuples) - elog(ERROR, "dead tuple array overflow"); - - vacrelstats->dead_tuples[vacrelstats->num_dead_tuples] = *itemptr; - vacrelstats->num_dead_tuples++; + if (vacrelstats->num_dead_tuples < vacrelstats->max_dead_tuples) + { + vacrelstats->dead_tuples[vacrelstats->num_dead_tuples] = *itemptr; + vacrelstats->num_dead_tuples++; + } } /* diff --git a/src/backend/executor/execMain.c b/src/backend/executor/execMain.c index 79e4701bb8a..315a555dcff 100644 --- a/src/backend/executor/execMain.c +++ b/src/backend/executor/execMain.c @@ -2019,7 +2019,8 @@ EvalPlanQualFetch(EState *estate, Relation relation, int lockmode, * buffer's content lock, since xmin never changes in an existing * tuple.) */ - if (!HeapTupleUpdateXmaxMatchesXmin(priorXmax, tuple.t_data)) + if (!TransactionIdEquals(HeapTupleHeaderGetXmin(tuple.t_data), + priorXmax)) { ReleaseBuffer(buffer); return NULL; @@ -2137,7 +2138,8 @@ EvalPlanQualFetch(EState *estate, Relation relation, int lockmode, /* * As above, if xmin isn't what we're expecting, do nothing. */ - if (!HeapTupleUpdateXmaxMatchesXmin(priorXmax, tuple.t_data)) + if (!TransactionIdEquals(HeapTupleHeaderGetXmin(tuple.t_data), + priorXmax)) { ReleaseBuffer(buffer); return NULL; diff --git a/src/include/access/heapam.h b/src/include/access/heapam.h index 93a481f4831..4a187f899cf 100644 --- a/src/include/access/heapam.h +++ b/src/include/access/heapam.h @@ -127,9 +127,6 @@ extern void heap_get_latest_tid(Relation relation, Snapshot snapshot, ItemPointer tid); extern void setLastTid(const ItemPointer tid); -extern bool HeapTupleUpdateXmaxMatchesXmin(TransactionId xmax, - HeapTupleHeader htup); - extern BulkInsertState GetBulkInsertState(void); extern void FreeBulkInsertState(BulkInsertState); diff --git a/src/test/isolation/isolation_schedule b/src/test/isolation/isolation_schedule index 07cff08b337..46134c4a77e 100644 --- a/src/test/isolation/isolation_schedule +++ b/src/test/isolation/isolation_schedule @@ -30,6 +30,5 @@ test: lock-committed-keyupdate test: update-locked-tuple test: propagate-lock-delete test: tuplelock-conflict -test: freeze-the-dead test: drop-index-concurrently-1 test: timeouts