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

Commit f05ae2f

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 e89867c commit f05ae2f

File tree

6 files changed

+60
-105
lines changed

6 files changed

+60
-105
lines changed

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

Lines changed: 46 additions & 89 deletions
Original file line numberDiff line numberDiff line change
@@ -1718,7 +1718,8 @@ heap_hot_search_buffer(ItemPointer tid, Relation relation, Buffer buffer,
17181718
* broken.
17191719
*/
17201720
if (TransactionIdIsValid(prev_xmax) &&
1721-
!HeapTupleUpdateXmaxMatchesXmin(prev_xmax, heapTuple->t_data))
1721+
!TransactionIdEquals(prev_xmax,
1722+
HeapTupleHeaderGetXmin(heapTuple->t_data)))
17221723
break;
17231724

17241725
/*
@@ -1887,7 +1888,7 @@ heap_get_latest_tid(Relation relation,
18871888
* tuple. Check for XMIN match.
18881889
*/
18891890
if (TransactionIdIsValid(priorXmax) &&
1890-
!HeapTupleUpdateXmaxMatchesXmin(priorXmax, tp.t_data))
1891+
!TransactionIdEquals(priorXmax, HeapTupleHeaderGetXmin(tp.t_data)))
18911892
{
18921893
UnlockReleaseBuffer(buffer);
18931894
break;
@@ -1919,39 +1920,6 @@ heap_get_latest_tid(Relation relation,
19191920
} /* end of loop */
19201921
}
19211922

1922-
/*
1923-
* HeapTupleUpdateXmaxMatchesXmin - verify update chain xmax/xmin lineage
1924-
*
1925-
* Given the new version of a tuple after some update, verify whether the
1926-
* given Xmax (corresponding to the previous version) matches the tuple's
1927-
* Xmin, taking into account that the Xmin might have been frozen after the
1928-
* update.
1929-
*/
1930-
bool
1931-
HeapTupleUpdateXmaxMatchesXmin(TransactionId xmax, HeapTupleHeader htup)
1932-
{
1933-
TransactionId xmin = HeapTupleHeaderGetXmin(htup);
1934-
1935-
/*
1936-
* If the xmax of the old tuple is identical to the xmin of the new one,
1937-
* it's a match.
1938-
*/
1939-
if (TransactionIdEquals(xmax, xmin))
1940-
return true;
1941-
1942-
/*
1943-
* When a tuple is frozen, the original Xmin is lost, but we know it's a
1944-
* committed transaction. So unless the Xmax is InvalidXid, we don't know
1945-
* for certain that there is a match, but there may be one; and we must
1946-
* return true so that a HOT chain that is half-frozen can be walked
1947-
* correctly.
1948-
*/
1949-
if (TransactionIdEquals(xmin, FrozenTransactionId) &&
1950-
TransactionIdIsValid(xmax))
1951-
return true;
1952-
1953-
return false;
1954-
}
19551923

19561924
/*
19571925
* UpdateXmaxHintBits - update tuple hint bits after xmax transaction ends
@@ -5077,7 +5045,8 @@ heap_lock_updated_tuple_rec(Relation rel, ItemPointer tid, TransactionId xid,
50775045
* the end of the chain, we're done, so return success.
50785046
*/
50795047
if (TransactionIdIsValid(priorXmax) &&
5080-
!HeapTupleUpdateXmaxMatchesXmin(priorXmax, mytup.t_data))
5048+
!TransactionIdEquals(HeapTupleHeaderGetXmin(mytup.t_data),
5049+
priorXmax))
50815050
{
50825051
UnlockReleaseBuffer(buf);
50835052
return HeapTupleMayBeUpdated;
@@ -5520,26 +5489,14 @@ FreezeMultiXactId(MultiXactId multi, uint16 t_infomask,
55205489
Assert(TransactionIdIsValid(xid));
55215490

55225491
/*
5523-
* The updating transaction cannot possibly be still running, but
5524-
* verify whether it has committed, and request to set the
5525-
* COMMITTED flag if so. (We normally don't see any tuples in
5526-
* this state, because they are removed by page pruning before we
5527-
* try to freeze the page; but this can happen if the updating
5528-
* transaction commits after the page is pruned but before
5529-
* HeapTupleSatisfiesVacuum).
5492+
* If the xid is older than the cutoff, it has to have aborted,
5493+
* otherwise the tuple would have gotten pruned away.
55305494
*/
55315495
if (TransactionIdPrecedes(xid, cutoff_xid))
55325496
{
5533-
if (TransactionIdDidCommit(xid))
5534-
{
5535-
xid = FrozenTransactionId;
5536-
*flags = FRM_MARK_COMMITTED | FRM_RETURN_IS_XID;
5537-
}
5538-
else
5539-
{
5540-
*flags |= FRM_INVALIDATE_XMAX;
5541-
xid = InvalidTransactionId; /* not strictly necessary */
5542-
}
5497+
Assert(!TransactionIdDidCommit(xid));
5498+
*flags |= FRM_INVALIDATE_XMAX;
5499+
xid = InvalidTransactionId; /* not strictly necessary */
55435500
}
55445501
else
55455502
{
@@ -5610,51 +5567,65 @@ FreezeMultiXactId(MultiXactId multi, uint16 t_infomask,
56105567

56115568
/*
56125569
* It's an update; should we keep it? If the transaction is known
5613-
* aborted or crashed then it's okay to ignore it, otherwise not.
5570+
* aborted then it's okay to ignore it, otherwise not. However,
5571+
* if the Xid is older than the cutoff_xid, we must remove it.
5572+
* Note that such an old updater cannot possibly be committed,
5573+
* because HeapTupleSatisfiesVacuum would have returned
5574+
* HEAPTUPLE_DEAD and we would not be trying to freeze the tuple.
5575+
*
5576+
* Note the TransactionIdDidAbort() test is just an optimization
5577+
* and not strictly necessary for correctness.
56145578
*
56155579
* As with all tuple visibility routines, it's critical to test
5616-
* TransactionIdIsInProgress before TransactionIdDidCommit,
5580+
* TransactionIdIsInProgress before the transam.c routines,
56175581
* because of race conditions explained in detail in tqual.c.
5618-
*
5619-
* We normally don't see committed updating transactions earlier
5620-
* than the cutoff xid, because they are removed by page pruning
5621-
* before we try to freeze the page; but it can happen if the
5622-
* updating transaction commits after the page is pruned but
5623-
* before HeapTupleSatisfiesVacuum.
56245582
*/
56255583
if (TransactionIdIsCurrentTransactionId(xid) ||
56265584
TransactionIdIsInProgress(xid))
56275585
{
56285586
Assert(!TransactionIdIsValid(update_xid));
56295587
update_xid = xid;
56305588
}
5631-
else if (TransactionIdDidCommit(xid))
5589+
else if (!TransactionIdDidAbort(xid))
56325590
{
56335591
/*
5634-
* The transaction committed, so we can tell caller to set
5635-
* HEAP_XMAX_COMMITTED. (We can only do this because we know
5636-
* the transaction is not running.)
5592+
* Test whether to tell caller to set HEAP_XMAX_COMMITTED
5593+
* while we have the Xid still in cache. Note this can only
5594+
* be done if the transaction is known not running.
56375595
*/
5596+
if (TransactionIdDidCommit(xid))
5597+
update_committed = true;
56385598
Assert(!TransactionIdIsValid(update_xid));
5639-
update_committed = true;
56405599
update_xid = xid;
56415600
}
56425601

5643-
/*
5644-
* Not in progress, not committed -- must be aborted or crashed;
5645-
* we can ignore it.
5646-
*/
5647-
56485602
/*
56495603
* If we determined that it's an Xid corresponding to an update
56505604
* that must be retained, additionally add it to the list of
5651-
* members of the new Multi, in case we end up using that. (We
5605+
* members of the new Multis, in case we end up using that. (We
56525606
* might still decide to use only an update Xid and not a multi,
56535607
* but it's easier to maintain the list as we walk the old members
56545608
* list.)
5609+
*
5610+
* It is possible to end up with a very old updater Xid that
5611+
* crashed and thus did not mark itself as aborted in pg_clog.
5612+
* That would manifest as a pre-cutoff Xid. Make sure to ignore
5613+
* it.
56555614
*/
56565615
if (TransactionIdIsValid(update_xid))
5657-
newmembers[nnewmembers++] = members[i];
5616+
{
5617+
if (!TransactionIdPrecedes(update_xid, cutoff_xid))
5618+
{
5619+
newmembers[nnewmembers++] = members[i];
5620+
}
5621+
else
5622+
{
5623+
/* cannot have committed: would be HEAPTUPLE_DEAD */
5624+
Assert(!TransactionIdDidCommit(update_xid));
5625+
update_xid = InvalidTransactionId;
5626+
update_committed = false;
5627+
}
5628+
}
56585629
}
56595630
else
56605631
{
@@ -5721,10 +5692,7 @@ FreezeMultiXactId(MultiXactId multi, uint16 t_infomask,
57215692
*
57225693
* It is assumed that the caller has checked the tuple with
57235694
* HeapTupleSatisfiesVacuum() and determined that it is not HEAPTUPLE_DEAD
5724-
* (else we should be removing the tuple, not freezing it). However, note
5725-
* that we don't remove HOT tuples even if they are dead, and it'd be incorrect
5726-
* to freeze them (because that would make them visible), so we mark them as
5727-
* update-committed, and needing further freezing later on.
5695+
* (else we should be removing the tuple, not freezing it).
57285696
*
57295697
* NB: cutoff_xid *must* be <= the current global xmin, to ensure that any
57305698
* 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,
58355803
else if (TransactionIdIsNormal(xid) &&
58365804
TransactionIdPrecedes(xid, cutoff_xid))
58375805
{
5838-
/*
5839-
* Must freeze regular XIDs older than the cutoff. We must not freeze
5840-
* a HOT-updated tuple, though; doing so would bring it back to life.
5841-
*/
5842-
if (HeapTupleHeaderIsHotUpdated(tuple))
5843-
{
5844-
frz->t_infomask |= HEAP_XMAX_COMMITTED;
5845-
changed = true;
5846-
/* must not freeze */
5847-
}
5848-
else
5849-
freeze_xmax = true;
5806+
freeze_xmax = true;
58505807
}
58515808

58525809
if (freeze_xmax)

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

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -435,7 +435,7 @@ heap_prune_chain(Relation relation, Buffer buffer, OffsetNumber rootoffnum,
435435
* Check the tuple XMIN against prior XMAX, if any
436436
*/
437437
if (TransactionIdIsValid(priorXmax) &&
438-
!HeapTupleUpdateXmaxMatchesXmin(priorXmax, htup))
438+
!TransactionIdEquals(HeapTupleHeaderGetXmin(htup), priorXmax))
439439
break;
440440

441441
/*
@@ -774,7 +774,7 @@ heap_get_root_tuples(Page page, OffsetNumber *root_offsets)
774774
htup = (HeapTupleHeader) PageGetItem(page, lp);
775775

776776
if (TransactionIdIsValid(priorXmax) &&
777-
!HeapTupleUpdateXmaxMatchesXmin(priorXmax, htup))
777+
!TransactionIdEquals(priorXmax, HeapTupleHeaderGetXmin(htup)))
778778
break;
779779

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

‎src/backend/commands/vacuumlazy.c

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1678,15 +1678,15 @@ lazy_record_dead_tuple(LVRelStats *vacrelstats,
16781678
ItemPointer itemptr)
16791679
{
16801680
/*
1681-
* The array must never overflow, since we rely on all deletable tuples
1682-
* being removed; inability to remove a tuple might cause an old XID to
1683-
* persist beyond the freeze limit, which could be disastrous later on.
1681+
* The array shouldn't overflow under normal behavior, but perhaps it
1682+
* could if we are given a really small maintenance_work_mem. In that
1683+
* case, just forget the last few tuples (we'll get 'em next time).
16841684
*/
1685-
if (vacrelstats->num_dead_tuples >= vacrelstats->max_dead_tuples)
1686-
elog(ERROR, "dead tuple array overflow");
1687-
1688-
vacrelstats->dead_tuples[vacrelstats->num_dead_tuples] = *itemptr;
1689-
vacrelstats->num_dead_tuples++;
1685+
if (vacrelstats->num_dead_tuples < vacrelstats->max_dead_tuples)
1686+
{
1687+
vacrelstats->dead_tuples[vacrelstats->num_dead_tuples] = *itemptr;
1688+
vacrelstats->num_dead_tuples++;
1689+
}
16901690
}
16911691

16921692
/*

‎src/backend/executor/execMain.c

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2019,7 +2019,8 @@ EvalPlanQualFetch(EState *estate, Relation relation, int lockmode,
20192019
* buffer's content lock, since xmin never changes in an existing
20202020
* tuple.)
20212021
*/
2022-
if (!HeapTupleUpdateXmaxMatchesXmin(priorXmax, tuple.t_data))
2022+
if (!TransactionIdEquals(HeapTupleHeaderGetXmin(tuple.t_data),
2023+
priorXmax))
20232024
{
20242025
ReleaseBuffer(buffer);
20252026
return NULL;
@@ -2137,7 +2138,8 @@ EvalPlanQualFetch(EState *estate, Relation relation, int lockmode,
21372138
/*
21382139
* As above, if xmin isn't what we're expecting, do nothing.
21392140
*/
2140-
if (!HeapTupleUpdateXmaxMatchesXmin(priorXmax, tuple.t_data))
2141+
if (!TransactionIdEquals(HeapTupleHeaderGetXmin(tuple.t_data),
2142+
priorXmax))
21412143
{
21422144
ReleaseBuffer(buffer);
21432145
return NULL;

‎src/include/access/heapam.h

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

130-
extern bool HeapTupleUpdateXmaxMatchesXmin(TransactionId xmax,
131-
HeapTupleHeader htup);
132-
133130
extern BulkInsertState GetBulkInsertState(void);
134131
extern void FreeBulkInsertState(BulkInsertState);
135132

‎src/test/isolation/isolation_schedule

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,5 @@ test: lock-committed-keyupdate
3030
test: update-locked-tuple
3131
test: propagate-lock-delete
3232
test: tuplelock-conflict
33-
test: freeze-the-dead
3433
test: drop-index-concurrently-1
3534
test: timeouts

0 commit comments

Comments
 (0)
0