From 088e68b7afc7c8b601ac0cef24d962b13a71ec3b Mon Sep 17 00:00:00 2001 From: Melanie Plageman Date: Tue, 18 Jun 2024 16:06:48 -0400 Subject: [PATCH v1 2/2] Fix infinite loop in vacuum caused by visibility mismatch lazy_scan_prune() may loop infinitely if a tuple deemed non-removable by heap_prune_satisfies_vacuum() in heap_page_prune() is later deemed HEAPTUPLE_DEAD in lazy_scan_prune() by HeapTupleSatisfiesVacuum(). Both determine what transaction ID a tuple would be dead_after using HeapTupleSatisfiesVacuumHorizon() and then compare that transaction ID to a horizon. HeapTupleSatisfiesVacuum() uses OldestXmin from VacuumCutoffs as that horizon and heap_prune_satisfies_vacuum() uses GlobalVisTestIsRemovableXid() -- which checks the GlobalVisState that was passed in to heap_page_prune(). OldestXmin is determined at the start of vacuuming the relation, but GlobalVisState's maybe_needed value can change if a tuple's xmax falls within GlobalVisState's fuzzy window and vacuum has had to take a new snapshot since it started vacuuming the relation. In rare cases, maybe_needed may move backward -- for example if a lagging standby which was not connected at the start of the vacuum later connects. When this happened, a tuple with an xmax older than OldestXmin but newer than the new value of maybe_needed after GlobalVisState was updated could cause lazy_scan_prune() to loop infinitely. By only computing the tuple's HTSV_Result once, in heap_page_prune(), and saving it to be passed back to lazy_scan_prune(), we can avoid the problem altogether. This also happens to be cheaper, since HeapTupleSatisfiesVacuumHorizon() accesses the clog. As a consequence, OldestXmin is now only used for determining whether or not to freeze tuples and whether or not the whole page is visible -- not for determining whether or not tuples are removable. --- src/backend/access/heap/pruneheap.c | 47 ++++++++++++++++------------ src/backend/access/heap/vacuumlazy.c | 46 ++++++++++----------------- src/include/access/heapam.h | 15 +++++++++ 3 files changed, 58 insertions(+), 50 deletions(-) diff --git a/src/backend/access/heap/pruneheap.c b/src/backend/access/heap/pruneheap.c index 9f43bbe25f5..801cba51bc6 100644 --- a/src/backend/access/heap/pruneheap.c +++ b/src/backend/access/heap/pruneheap.c @@ -65,16 +65,6 @@ typedef struct * 1. Otherwise every access would need to subtract 1. */ bool marked[MaxHeapTuplesPerPage + 1]; - - /* - * Tuple visibility is only computed once for each tuple, for correctness - * and efficiency reasons; see comment in heap_page_prune() for details. - * This is of type int8[], instead of HTSV_Result[], so we can use -1 to - * indicate no visibility has been computed, e.g. for LP_DEAD items. - * - * Same indexing as ->marked. - */ - int8 htsv[MaxHeapTuplesPerPage + 1]; } PruneState; /* Local functions */ @@ -83,7 +73,7 @@ static HTSV_Result heap_prune_satisfies_vacuum(PruneState *prstate, Buffer buffer); static int heap_prune_chain(Buffer buffer, OffsetNumber rootoffnum, - PruneState *prstate); + PruneState *prstate, int8 *htsv); static void heap_prune_record_prunable(PruneState *prstate, TransactionId xid); static void heap_prune_record_redirect(PruneState *prstate, OffsetNumber offnum, OffsetNumber rdoffnum); @@ -205,9 +195,10 @@ heap_page_prune_opt(Relation relation, Buffer buffer) { int ndeleted, nnewlpdead; + int8 htsv[MaxHeapTuplesPerPage + 1]; ndeleted = heap_page_prune(relation, buffer, vistest, limited_xmin, - limited_ts, &nnewlpdead, NULL); + limited_ts, htsv, &nnewlpdead, NULL); /* * Report the number of tuples reclaimed to pgstats. This is @@ -254,6 +245,18 @@ heap_page_prune_opt(Relation relation, Buffer buffer) * either have been set by TransactionIdLimitedForOldSnapshots, or * InvalidTransactionId/0 respectively. * + * htsv is an in/out parameter filled in by heap_page_prune() with tuple + * visibility, computed once for each tuple for correctness and efficiency + * reasons. Some callers want access to htsv for later decisions and metrics. + * htsv is of type int8[], instead of HTSV_Result[], so we can use -1 to + * indicate no visibility has been computed, e.g. for LP_DEAD items. It can be + * passed in uninitialized because all spots will be set either to a valid + * HTSV_Result value or -1. + * + * It is the caller's responsibility to ensure that htsv is + * MaxHeapTuplesPerPage + 1 in size. By making it MaxHeapTuplesPerPage + 1, we + * can avoid subtracting 1 everywhere even though FirstOffsetNumber is 1. + * * Sets *nnewlpdead for caller, indicating the number of items that were * newly set LP_DEAD during prune operation. * @@ -267,6 +270,7 @@ heap_page_prune(Relation relation, Buffer buffer, GlobalVisState *vistest, TransactionId old_snap_xmin, TimestampTz old_snap_ts, + int8 htsv[MaxHeapTuplesPerPage + 1], int *nnewlpdead, OffsetNumber *off_loc) { @@ -332,7 +336,7 @@ heap_page_prune(Relation relation, Buffer buffer, /* Nothing to do if slot doesn't contain a tuple */ if (!ItemIdIsNormal(itemid)) { - prstate.htsv[offnum] = -1; + htsv[offnum] = -1; continue; } @@ -348,7 +352,7 @@ heap_page_prune(Relation relation, Buffer buffer, if (off_loc) *off_loc = offnum; - prstate.htsv[offnum] = heap_prune_satisfies_vacuum(&prstate, &tup, + htsv[offnum] = heap_prune_satisfies_vacuum(&prstate, &tup, buffer); } @@ -373,7 +377,7 @@ heap_page_prune(Relation relation, Buffer buffer, continue; /* Process this item or chain of items */ - ndeleted += heap_prune_chain(buffer, offnum, &prstate); + ndeleted += heap_prune_chain(buffer, offnum, &prstate, htsv); } /* Clear the offset information once we have processed the given page. */ @@ -561,6 +565,8 @@ heap_prune_satisfies_vacuum(PruneState *prstate, HeapTuple tup, Buffer buffer) /* * Prune specified line pointer or a HOT chain originating at line pointer. * + * Tuple visibility information is provided in htsv. + * * If the item is an index-referenced tuple (i.e. not a heap-only tuple), * the HOT chain is pruned by removing all DEAD tuples at the start of the HOT * chain. We also prune any RECENTLY_DEAD tuples preceding a DEAD tuple. @@ -588,7 +594,8 @@ heap_prune_satisfies_vacuum(PruneState *prstate, HeapTuple tup, Buffer buffer) * Returns the number of tuples (to be) deleted from the page. */ static int -heap_prune_chain(Buffer buffer, OffsetNumber rootoffnum, PruneState *prstate) +heap_prune_chain(Buffer buffer, OffsetNumber rootoffnum, PruneState *prstate, + int8 *htsv) { int ndeleted = 0; Page dp = (Page) BufferGetPage(buffer); @@ -609,7 +616,7 @@ heap_prune_chain(Buffer buffer, OffsetNumber rootoffnum, PruneState *prstate) */ if (ItemIdIsNormal(rootlp)) { - Assert(prstate->htsv[rootoffnum] != -1); + Assert(htsv[rootoffnum] != -1); htup = (HeapTupleHeader) PageGetItem(dp, rootlp); if (HeapTupleHeaderIsHeapOnly(htup)) @@ -632,7 +639,7 @@ heap_prune_chain(Buffer buffer, OffsetNumber rootoffnum, PruneState *prstate) * either here or while following a chain below. Whichever path * gets there first will mark the tuple unused. */ - if (prstate->htsv[rootoffnum] == HEAPTUPLE_DEAD && + if (htsv[rootoffnum] == HEAPTUPLE_DEAD && !HeapTupleHeaderIsHotUpdated(htup)) { heap_prune_record_unused(prstate, rootoffnum); @@ -700,7 +707,7 @@ heap_prune_chain(Buffer buffer, OffsetNumber rootoffnum, PruneState *prstate) break; Assert(ItemIdIsNormal(lp)); - Assert(prstate->htsv[offnum] != -1); + Assert(htsv[offnum] != -1); htup = (HeapTupleHeader) PageGetItem(dp, lp); /* @@ -720,7 +727,7 @@ heap_prune_chain(Buffer buffer, OffsetNumber rootoffnum, PruneState *prstate) */ tupdead = recent_dead = false; - switch ((HTSV_Result) prstate->htsv[offnum]) + switch ((HTSV_Result) htsv[offnum]) { case HEAPTUPLE_DEAD: tupdead = true; diff --git a/src/backend/access/heap/vacuumlazy.c b/src/backend/access/heap/vacuumlazy.c index 2e61e2f20b4..8ba0da063db 100644 --- a/src/backend/access/heap/vacuumlazy.c +++ b/src/backend/access/heap/vacuumlazy.c @@ -1526,12 +1526,14 @@ lazy_scan_new_or_empty(LVRelState *vacrel, Buffer buf, BlockNumber blkno, * of complexity just so we could deal with tuples that were DEAD to VACUUM, * but nevertheless were left with storage after pruning. * - * The approach we take now is to restart pruning when the race condition is - * detected. This allows heap_page_prune() to prune the tuples inserted by - * the now-aborted transaction. This is a little crude, but it guarantees - * that any items that make it into the dead_items array are simple LP_DEAD - * line pointers, and that every remaining item with tuple storage is - * considered as a candidate for freezing. + * As of minor release blah, we circumvent this problem altogether by reusing + * the result of heap_page_prune()'s visibility check. Without the second call + * to HeapTupleSatisfiesVacuum(), there is no new HTSV_Result and there can be + * no disagreement. We'll just handle such tuples as if they had become fully + * dead right after this operation completes instead of in the middle of it. + * Note that any tuple that becomes dead after the call to heap_page_prune() + * can't need to be frozen, because it was visible to another session when + * vacuum started. */ static void lazy_scan_prune(LVRelState *vacrel, @@ -1544,8 +1546,6 @@ lazy_scan_prune(LVRelState *vacrel, OffsetNumber offnum, maxoff; ItemId itemid; - HeapTupleData tuple; - HTSV_Result res; int tuples_deleted, lpdead_items, live_tuples, @@ -1556,6 +1556,7 @@ lazy_scan_prune(LVRelState *vacrel, MultiXactId NewRelminMxid; OffsetNumber deadoffsets[MaxHeapTuplesPerPage]; xl_heap_freeze_tuple frozen[MaxHeapTuplesPerPage]; + int8 htsv[MaxHeapTuplesPerPage + 1]; Assert(BufferGetBlockNumber(buf) == blkno); @@ -1566,8 +1567,6 @@ lazy_scan_prune(LVRelState *vacrel, */ maxoff = PageGetMaxOffsetNumber(page); -retry: - /* Initialize (or reset) page-level state */ NewRelfrozenXid = vacrel->NewRelfrozenXid; NewRelminMxid = vacrel->NewRelminMxid; @@ -1586,7 +1585,7 @@ retry: * that were deleted from indexes. */ tuples_deleted = heap_page_prune(rel, buf, vacrel->vistest, - InvalidTransactionId, 0, &nnewlpdead, + InvalidTransactionId, 0, htsv, &nnewlpdead, &vacrel->offnum); /* @@ -1604,6 +1603,7 @@ retry: offnum <= maxoff; offnum = OffsetNumberNext(offnum)) { + HeapTupleHeader htup; bool tuple_totally_frozen; /* @@ -1649,21 +1649,7 @@ retry: Assert(ItemIdIsNormal(itemid)); - ItemPointerSet(&(tuple.t_self), blkno, offnum); - tuple.t_data = (HeapTupleHeader) PageGetItem(page, itemid); - tuple.t_len = ItemIdGetLength(itemid); - tuple.t_tableOid = RelationGetRelid(rel); - - /* - * DEAD tuples are almost always pruned into LP_DEAD line pointers by - * heap_page_prune(), but it's possible that the tuple state changed - * since heap_page_prune() looked. Handle that here by restarting. - * (See comments at the top of function for a full explanation.) - */ - res = HeapTupleSatisfiesVacuum(&tuple, vacrel->OldestXmin, buf); - - if (unlikely(res == HEAPTUPLE_DEAD)) - goto retry; + htup = (HeapTupleHeader) PageGetItem(page, itemid); /* * The criteria for counting a tuple as live in this block need to @@ -1684,7 +1670,7 @@ retry: * (Cases where we bypass index vacuuming will violate this optimistic * assumption, but the overall impact of that should be negligible.) */ - switch (res) + switch (htsv_get_valid_status(htsv[offnum])) { case HEAPTUPLE_LIVE: @@ -1706,7 +1692,7 @@ retry: { TransactionId xmin; - if (!HeapTupleHeaderXminCommitted(tuple.t_data)) + if (!HeapTupleHeaderXminCommitted(htup)) { prunestate->all_visible = false; break; @@ -1716,7 +1702,7 @@ retry: * The inserter definitely committed. But is it old enough * that everyone sees it as committed? */ - xmin = HeapTupleHeaderGetXmin(tuple.t_data); + xmin = HeapTupleHeaderGetXmin(htup); if (!TransactionIdPrecedes(xmin, vacrel->OldestXmin)) { prunestate->all_visible = false; @@ -1772,7 +1758,7 @@ retry: * now. */ prunestate->hastup = true; /* page makes rel truncation unsafe */ - if (heap_prepare_freeze_tuple(tuple.t_data, + if (heap_prepare_freeze_tuple(htup, vacrel->relfrozenxid, vacrel->relminmxid, vacrel->FreezeLimit, diff --git a/src/include/access/heapam.h b/src/include/access/heapam.h index abf62d9df79..0b349427982 100644 --- a/src/include/access/heapam.h +++ b/src/include/access/heapam.h @@ -188,6 +188,7 @@ extern int heap_page_prune(Relation relation, Buffer buffer, struct GlobalVisState *vistest, TransactionId old_snap_xmin, TimestampTz old_snap_ts_ts, + int8 htsv[MaxHeapTuplesPerPage + 1], int *nnewlpdead, OffsetNumber *off_loc); extern void heap_page_prune_execute(Buffer buffer, @@ -230,4 +231,18 @@ extern bool ResolveCminCmaxDuringDecoding(struct HTAB *tuplecid_data, extern void HeapCheckForSerializableConflictOut(bool valid, Relation relation, HeapTuple tuple, Buffer buffer, Snapshot snapshot); +/* + * Pruning calculates tuple visibility once and saves the results in an array + * of int8. See PruneResult.htsv for details. This helper function is meant to + * guard against examining visibility status array members which have not yet + * been computed. + */ +static inline HTSV_Result +htsv_get_valid_status(int status) +{ + Assert(status >= HEAPTUPLE_DEAD && + status <= HEAPTUPLE_DELETE_IN_PROGRESS); + return (HTSV_Result) status; +} + #endif /* HEAPAM_H */ -- 2.34.1