8000 Revert bogus fixes of HOT-freezing bug · postgrespro/postgres@7a95966 · GitHub
[go: up one dir, main page]
Skip to content

Commit 7a95966

Browse files
alvherrealvherre
committed
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
1 parent 769756f commit 7a95966

File tree

6 files changed

+38
-105
lines changed

6 files changed

+38
-105
lines changed

‎src/backend/access/heap/heapam.c

Lines changed: 22 additions & 87 deletions
Original file line numberDiff line numberDiff line change
@@ -2060,7 +2060,8 @@ heap_hot_search_buffer(ItemPointer tid, Relation relation, Buffer buffer,
20602060
* broken.
20612061
*/
20622062
if (TransactionIdIsValid(prev_xmax) &&
2063-
!HeapTupleUpdateXmaxMatchesXmin(prev_xmax, heapTuple->t_data))
2063+
!TransactionIdEquals(prev_xmax,
2064+
HeapTupleHeaderGetXmin(heapTuple->t_data)))
20642065
break;
20652066

20662067
/*
@@ -2243,7 +2244,7 @@ heap_get_latest_tid(Relation relation,
22432244
* tuple. Check for XMIN match.
22442245
*/
22452246
if (TransactionIdIsValid(priorXmax) &&
2246-
!HeapTupleUpdateXmaxMatchesXmin(priorXmax, tp.t_data))
2247+
!TransactionIdEquals(priorXmax, HeapTupleHeaderGetXmin(tp.t_data)))
22472248
{
22482249
UnlockReleaseBuffer(buffer);
22492250
break;
@@ -2275,50 +2276,6 @@ heap_get_latest_tid(Relation relation,
22752276
} /* end of loop */
22762277
}
22772278

2278-
/*
2279-
* HeapTupleUpdateXmaxMatchesXmin - verify update chain xmax/xmin lineage
2280-
*
2281-
* Given the new version of a tuple after some update, verify whether the
2282-
* given Xmax (corresponding to the previous version) matches the tuple's
2283-
* Xmin, taking into account that the Xmin might have been frozen after the
2284-
* update.
2285-
*/
2286-
bool
2287-
HeapTupleUpdateXmaxMatchesXmin(TransactionId xmax, HeapTupleHeader htup)
2288-
{
2289-
TransactionId xmin = HeapTupleHeaderGetXmin(htup);
2290-
2291-
/*
2292-
* If the xmax of the old tuple is identical to the xmin of the new one,
2293-
* it's a match.
2294-
*/
2295-
if (TransactionIdEquals(xmax, xmin))
2296-
return true;
2297-
2298-
/*
2299-
* If the Xmin that was in effect prior to a freeze matches the Xmax,
2300-
* it's good too.
2301-
*/
2302-
if (HeapTupleHeaderXminFrozen(htup) &&
2303-
TransactionIdEquals(HeapTupleHeaderGetRawXmin(htup), xmax))
2304-
return true;
2305-
2306-
/*
2307-
* When a tuple is frozen, the original Xmin is lost, but we know it's a
2308-
* committed transaction. So unless the Xmax is InvalidXid, we don't know
2309-
* for certain that there is a match, but there may be one; and we must
2310-
* return true so that a HOT chain that is half-frozen can be walked
2311-
* correctly.
2312-
*
2313-
* We no longer freeze tuples this way, but we must keep this in order to
2314-
* interpret pre-pg_upgrade pages correctly.
2315-
*/
2316-
if (TransactionIdEquals(xmin, FrozenTransactionId) &&
2317-
TransactionIdIsValid(xmax))
2318-
return true;
2319-
2320-
return false;
2321-
}
23222279

23232280
/*
23242281
* UpdateXmaxHintBits - update tuple hint bits after xmax transaction ends
@@ -5736,7 +5693,8 @@ heap_lock_updated_tuple_rec(Relation rel, ItemPointer tid, TransactionId xid,
57365693
* end of the chain, we're done, so return success.
57375694
*/
57385695
if (TransactionIdIsValid(priorXmax) &&
5739-
!HeapTupleUpdateXmaxMatchesXmin(priorXmax, mytup.t_data))
5696+
!TransactionIdEquals(HeapTupleHeaderGetXmin(mytup.t_data),
5697+
priorXmax))
57405698
{
57415699
result = HeapTupleMayBeUpdated;
57425700
goto out_locked;
@@ -6430,23 +6388,14 @@ FreezeMultiXactId(MultiXactId multi, uint16 t_infomask,
64306388
Assert(TransactionIdIsValid(xid));
64316389

64326390
/*
6433-
* The updating transaction cannot possibly be still running, but
6434-
* verify whether it has committed, and request to set the
6435-
* COMMITTED flag if so. (We normally don't see any tuples in
6436-
* this state, because they are removed by page pruning before we
6437-
* try to freeze the page; but this can happen if the updating
6438-
* transaction commits after the page is pruned but before
6439-
* HeapTupleSatisfiesVacuum).
6391+
* If the xid is older than the cutoff, it has to have aborted,
6392+
* otherwise the tuple would have gotten pruned away.
64406393
*/
64416394
if (TransactionIdPrecedes(xid, cutoff_xid))
64426395
{
6443-
if (TransactionIdDidCommit(xid))
6444-
*flags = FRM_MARK_COMMITTED | FRM_RETURN_IS_XID;
6445-
else
6446-
{
6447-
*flags |= FRM_INVALIDATE_XMAX;
6448-
xid = InvalidTransactionId; /* not strictly necessary */
6449-
}
6396+
Assert(!TransactionIdDidCommit(xid));
6397+
*flags |= FRM_INVALIDATE_XMAX;
6398+
xid = InvalidTransactionId; /* not strictly necessary */
64506399
}
64516400
else
64526401
{
@@ -6519,16 +6468,13 @@ FreezeMultiXactId(MultiXactId multi, uint16 t_infomask,
65196468
/*
65206469
* It's an update; should we keep it? If the transaction is known
65216470
* aborted or crashed then it's okay to ignore it, otherwise not.
6471+
* Note that an updater older than cutoff_xid cannot possibly be
6472+
* committed, because HeapTupleSatisfiesVacuum would have returned
6473+
* HEAPTUPLE_DEAD and we would not be trying to freeze the tuple.
65226474
*
65236475
* As with all tuple visibility routines, it's critical to test
65246476
* TransactionIdIsInProgress before TransactionIdDidCommit,
65256477
* because of race conditions explained in detail in tqual.c.
6526-
*
6527-
* We normally don't see committed updating transactions earlier
6528-
* than the cutoff xid, because they are removed by page pruning
6529-
* before we try to freeze the page; but it can happen if the
6530-
* updating transaction commits after the page is pruned but
6531-
* before HeapTupleSatisfiesVacuum.
65326478
*/
65336479
if (TransactionIdIsCurrentTransactionId(xid) ||
65346480
TransactionIdIsInProgress(xid))
@@ -6553,6 +6499,13 @@ FreezeMultiXactId(MultiXactId multi, uint16 t_infomask,
65536499
* we can ignore it.
65546500
*/
65556501

6502+
/*
6503+
* Since the tuple wasn't marked HEAPTUPLE_DEAD by vacuum, the
6504+
* update Xid cannot possibly be older than the xid cutoff.
6505+
*/
6506+
Assert(!TransactionIdIsValid(update_xid) ||
6507+
!TransactionIdPrecedes(update_xid, cutoff_xid));
6508+
65566509
/*
65576510
* If we determined that it's an Xid corresponding to an update
65586511
* that must be retained, additionally add it to the list of
@@ -6631,10 +6584,7 @@ FreezeMultiXactId(MultiXactId multi, uint16 t_infomask,
66316584
*
66326585
* It is assumed that the caller has checked the tuple with
66336586
* HeapTupleSatisfiesVacuum() and determined that it is not HEAPTUPLE_DEAD
6634-
* (else we should be removing the tuple, not freezing it). However, note
6635-
* that we don't remove HOT tuples even if they are dead, and it'd be incorrect
6636-
* to freeze them (because that would make them visible), so we mark them as
6637-
* update-committed, and needing further freezing later on.
6587+
* (else we should be removing the tuple, not freezing it).
66386588
*
66396589
* NB: cutoff_xid *must* be <= the current global xmin, to ensure that any
66406590
* XID older than it could neither be running nor seen as running by any
@@ -6745,22 +6695,7 @@ heap_prepare_freeze_tuple(HeapTupleHeader tuple, TransactionId cutoff_xid,
67456695
else if (TransactionIdIsNormal(xid))
67466696
{
67476697
if (TransactionIdPrecedes(xid, cutoff_xid))
6748-
{
6749-
/*
6750-
* Must freeze regular XIDs older than the cutoff. We must not
6751-
* freeze a HOT-updated tuple, though; doing so would bring it
6752-
* back to life.
6753-
*/
6754-
if (HeapTupleHeaderIsHotUpdated(tuple))
6755-
{
6756-
frz->t_infomask |= HEAP_XMAX_COMMITTED;
6757-
totally_frozen = false;
6758-
changed = true;
6759-
/* must not freeze */
6760-
}
6761-
else
6762-
freeze_xmax = true;
6763-
}
6698+
freeze_xmax = true;
67646699
else
67656700
totally_frozen = false;
67666701
}

‎src/backend/access/heap/pruneheap.c

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -473,7 +473,7 @@ heap_prune_chain(Relation relation, Buffer buffer, OffsetNumber rootoffnum,
473473
* Check the tuple XMIN against prior XMAX, if any
474474
*/
475475
if (TransactionIdIsValid(priorXmax) &&
476-
!HeapTupleUpdateXmaxMatchesXmin(priorXmax, htup))
476+
!TransactionIdEquals(HeapTupleHeaderGetXmin(htup), priorXmax))
477477
break;
478478

479479
/*
@@ -813,7 +813,7 @@ heap_get_root_tuples(Page page, OffsetNumber *root_offsets)
813813
htup = (HeapTupleHeader) PageGetItem(page, lp);
814814

815815
if (TransactionIdIsValid(priorXmax) &&
816-
!HeapTupleUpdateXmaxMatchesXmin(priorXmax, htup))
816+
!TransactionIdEquals(priorXmax, HeapTupleHeaderGetXmin(htup)))
817817
break;
818818

819819
/* Remember the root line pointer for this item */

‎src/backend/commands/vacuumlazy.c

Lines changed: 10 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -2018,17 +2018,17 @@ lazy_record_dead_tuple(LVRelStats *vacrelstats,
20182018
ItemPointer itemptr)
20192019
{
20202020
/*
2021-
* The array must never overflow, since we rely on all deletable tuples
2022-
* being removed; inability to remove a tuple might cause an old XID to
2023-
* persist beyond the freeze limit, which could be disastrous later on.
2021+
* The array shouldn't overflow under normal behavior, but perhaps it
2022+
* could if we are given a really small maintenance_work_mem. In that
2023+
* case, just forget the last few tuples (we'll get 'em next time).
20242024
*/
2025-
if (vacrelstats->num_dead_tuples >= vacrelstats->max_dead_tuples)
2026-
elog(ERROR, "dead tuple array overflow");
2027-
2028-
vacrelstats->dead_tuples[vacrelstats->num_dead_tuples] = *itemptr;
2029-
vacrelstats->num_dead_tuples++;
2030-
pgstat_progress_update_param(PROGRESS_VACUUM_NUM_DEAD_TUPLES,
2031-
vacrelstats->num_dead_tuples);
2025+
if (vacrelstats->num_dead_tuples < vacrelstats->max_dead_tuples)
2026+
{
2027+
vacrelstats->dead_tuples[vacrelstats->num_dead_tuples] = *itemptr;
2028+
vacrelstats->num_dead_tuples++;
2029+
pgstat_progress_update_param(PROGRESS_VACUUM_NUM_DEAD_TUPLES,
2030+
vacrelstats->num_dead_tuples);
2031+
}
20322032
}
20332033

20342034
/*

‎src/backend/executor/execMain.c

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2595,7 +2595,8 @@ EvalPlanQualFetch(EState *estate, Relation relation, int lockmode,
25952595
* atomic, and Xmin never changes in an existing tuple, except to
25962596
* invalid or frozen, and neither of those can match priorXmax.)
25972597
*/
2598-
if (!HeapTupleUpdateXmaxMatchesXmin(priorXmax, tuple.t_data))
2598+
if (!TransactionIdEquals(HeapTupleHeaderGetXmin(tuple.t_data),
2599+
priorXmax))
25992600
{
26002601
ReleaseBuffer(buffer);
26012602
return NULL;
@@ -2742,7 +2743,8 @@ EvalPlanQualFetch(EState *estate, Relation relation, int lockmode,
27422743
/*
27432744
* As above, if xmin isn't what we're expecting, do nothing.
27442745
*/
2745-
if (!HeapTupleUpdateXmaxMatchesXmin(priorXmax, tuple.t_data))
2746+
if (!TransactionIdEquals(HeapTupleHeaderGetXmin(tuple.t_data),
2747+
priorXmax))
27462748
{
27472749
ReleaseBuffer(buffer);
27482750
return NULL;

‎src/include/access/heapam.h

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -146,9 +146,6 @@ extern void heap_get_latest_tid(Relation relation, Snapshot snapshot,
146146
ItemPointer tid);
147147
extern void setLastTid(const ItemPointer tid);
148148

149-
extern bool HeapTupleUpdateXmaxMatchesXmin(TransactionId xmax,
150-
HeapTupleHeader htup);
151-
152149
extern BulkInsertState GetBulkInsertState(void);
153150
extern void FreeBulkInsertState(BulkInsertState);
154151
extern void ReleaseBulkInsertStatePin(BulkInsertState bistate);

‎src/test/isolation/isolation_schedule

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -44,7 +44,6 @@ test: update-locked-tuple
4444
test: propagate-lock-delete
4545
test: tuplelock-conflict
4646
test: tuplelock-update
47-
test: freeze-the-dead
4847
test: nowait
4948
test: nowait-2
5049
test: nowait-3

0 commit comments

Comments
 (0)
0