From 08ba67d596a1443c32d0b5300aaad8042729cd41 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 | 109 ++++++-------------------- src/backend/access/heap/pruneheap.c | 4 +- src/backend/commands/vacuumlazy.c | 20 ++--- src/backend/executor/execMain.c | 6 +- src/include/access/heapam.h | 3 - src/test/isolation/isolation_schedule | 1 - 6 files changed, 38 insertions(+), 105 deletions(-) diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c index 6d98e6cd7b..4327be8610 100644 --- a/src/backend/access/heap/heapam.c +++ b/src/backend/access/heap/heapam.c @@ -2046,7 +2046,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; /* @@ -2229,7 +2230,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; @@ -2261,50 +2262,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; - - /* - * If the Xmin that was in effect prior to a freeze matches the Xmax, - * it's good too. - */ - if (HeapTupleHeaderXminFrozen(htup) && - TransactionIdEquals(HeapTupleHeaderGetRawXmin(htup), xmax)) - 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. - * - * We no longer freeze tuples this way, but we must keep this in order to - * interpret pre-pg_upgrade pages correctly. - */ - if (TransactionIdEquals(xmin, FrozenTransactionId) && - TransactionIdIsValid(xmax)) - return true; - - return false; -} /* * UpdateXmaxHintBits - update tuple hint bits after xmax transaction ends @@ -5762,7 +5719,8 @@ l4: * end of the chain, we're done, so return success. */ if (TransactionIdIsValid(priorXmax) && - !HeapTupleUpdateXmaxMatchesXmin(priorXmax, mytup.t_data)) + !TransactionIdEquals(HeapTupleHeaderGetXmin(mytup.t_data), + priorXmax)) { result = HeapTupleMayBeUpdated; goto out_locked; @@ -6456,23 +6414,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)) - *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 { @@ -6545,16 +6494,13 @@ 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. + * Note that an updater older than cutoff_xid cannot possibly be + * committed, because HeapTupleSatisfiesVacuum would have returned + * HEAPTUPLE_DEAD and we would not be trying to freeze the tuple. * * As with all tuple visibility routines, it's critical to test * TransactionIdIsInProgress before TransactionIdDidCommit, * 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)) @@ -6579,6 +6525,13 @@ FreezeMultiXactId(MultiXactId multi, uint16 t_infomask, * we can ignore it. */ + /* + * Since the tuple wasn't marked HEAPTUPLE_DEAD by vacuum, the + * update Xid cannot possibly be older than the xid cutoff. + */ + Assert(!TransactionIdIsValid(update_xid) || + !TransactionIdPrecedes(update_xid, cutoff_xid)); + /* * If we determined that it's an Xid corresponding to an update * that must be retained, additionally add it to the list of @@ -6657,10 +6610,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 @@ -6771,22 +6721,7 @@ heap_prepare_freeze_tuple(HeapTupleHeader tuple, TransactionId cutoff_xid, else if (TransactionIdIsNormal(xid)) { if (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; - totally_frozen = false; - changed = true; - /* must not freeze */ - } - else - freeze_xmax = true; - } + freeze_xmax = true; else totally_frozen = false; } diff --git a/src/backend/access/heap/pruneheap.c b/src/backend/access/heap/pruneheap.c index 3bb6e19cf7..6ff92516ed 100644 --- a/src/backend/access/heap/pruneheap.c +++ b/src/backend/access/heap/pruneheap.c @@ -474,7 +474,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; /* @@ -814,7 +814,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 284b09d9d1..8f59732442 100644 --- a/src/backend/commands/vacuumlazy.c +++ b/src/backend/commands/vacuumlazy.c @@ -1984,17 +1984,17 @@ 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++; - pgstat_progress_update_param(PROGRESS_VACUUM_NUM_DEAD_TUPLES, - 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++; + pgstat_progress_update_param(PROGRESS_VACUUM_NUM_DEAD_TUPLES, + vacrelstats->num_dead_tuples); + } } /* diff --git a/src/backend/executor/execMain.c b/src/backend/executor/execMain.c index d8b46a11f4..10ccf59b79 100644 --- a/src/backend/executor/execMain.c +++ b/src/backend/executor/execMain.c @@ -2275,7 +2275,8 @@ EvalPlanQualFetch(EState *estate, Relation relation, int lockmode, * atomic, and Xmin never changes in an existing tuple, except to * invalid or frozen, and neither of those can match priorXmax.) */ - if (!HeapTupleUpdateXmaxMatchesXmin(priorXmax, tuple.t_data)) + if (!TransactionIdEquals(HeapTupleHeaderGetXmin(tuple.t_data), + priorXmax)) { ReleaseBuffer(buffer); return NULL; @@ -2422,7 +2423,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 53839f5270..b3a595c67e 100644 --- a/src/include/access/heapam.h +++ b/src/include/access/heapam.h @@ -145,9 +145,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 f1af282853..8e404b7a35 100644 --- a/src/test/isolation/isolation_schedule +++ b/src/test/isolation/isolation_schedule @@ -41,7 +41,6 @@ test: update-locked-tuple test: propagate-lock-delete test: tuplelock-conflict test: tuplelock-update -test: freeze-the-dead test: nowait test: nowait-2 test: nowait-3 -- 2.40.0