From 557434b7fd0397cc33cd87b66a002c833d79986d Mon Sep 17 00:00:00 2001 From: Peter Geoghegan Date: Sat, 30 Oct 2021 09:33:50 -0700 Subject: [PATCH v3] Add hardening to catch invalid TIDs from indexes. In passing, add some similar assertions to heap pruning code, that document our assumptions. --- src/include/access/tableam.h | 2 + src/backend/access/heap/heapam.c | 69 ++++++++++++++++++++++ src/backend/access/heap/pruneheap.c | 84 +++++++++++++++++++++++++-- src/backend/access/index/genam.c | 4 +- src/backend/access/nbtree/nbtdedup.c | 2 + src/backend/access/nbtree/nbtinsert.c | 2 + 6 files changed, 158 insertions(+), 5 deletions(-) diff --git a/src/include/access/tableam.h b/src/include/access/tableam.h index 4aff18215..ed1856c42 100644 --- a/src/include/access/tableam.h +++ b/src/include/access/tableam.h @@ -220,6 +220,8 @@ typedef struct TM_IndexStatus */ typedef struct TM_IndexDeleteOp { + Relation irel; /* Target index relation */ + BlockNumber iblknum; /* Can be used for error reporting */ bool bottomup; /* Bottom-up (not simple) deletion? */ int bottomupfreespace; /* Bottom-up space target */ diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c index fecb97286..bf6d0ce18 100644 --- a/src/backend/access/heap/heapam.c +++ b/src/backend/access/heap/heapam.c @@ -7252,6 +7252,67 @@ index_delete_prefetch_buffer(Relation rel, } #endif +/* + * Helper function for heap_index_delete_tuples. Checks for index corruption + * involving an invalid TID in index AM caller's index. + * + * This is an ideal place for these checks. The index AM caller must hold a + * buffer lock on the index page containing the TIDs we examine here, so we + * don't have to worry about concurrent VACUUMs at all. We can be sure that + * the index is corrupt when htid points directly to an LP_UNUSED item or + * heap-only tuple, which is not generally the case when performing standard + * index scans. + * + * It's not certain that the index is corrupt (it might actually be the heap + * page). We blame the index here all the same, since the true underlying + * problem at least has a decent chance of being in index vacuuming. + */ +static inline void +index_delete_check_htid(TM_IndexDeleteOp *delstate, + Page page, OffsetNumber maxoff, + ItemPointer htid, TM_IndexStatus *istatus) +{ + OffsetNumber indexpagehoffnum = ItemPointerGetOffsetNumber(htid); + ItemId iid; + HeapTupleHeader htup; + + Assert(OffsetNumberIsValid(istatus->idxoffnum)); + + if (indexpagehoffnum > maxoff) + ereport(ERROR, + (errcode(ERRCODE_INDEX_CORRUPTED), + errmsg_internal("heap tid from index tuple (%u,%u) points past end of heap page line pointer array at offset %u of block %u in index \"%s\"", + ItemPointerGetBlockNumber(htid), + indexpagehoffnum, + istatus->idxoffnum, delstate->iblknum, + RelationGetRelationName(delstate->irel)))); + + iid = PageGetItemId(page, indexpagehoffnum); + if (!ItemIdIsUsed(iid)) + ereport(ERROR, + (errcode(ERRCODE_INDEX_CORRUPTED), + errmsg_internal("heap tid from index tuple (%u,%u) points to unused heap page item at offset %u of block %u in index \"%s\"", + ItemPointerGetBlockNumber(htid), + indexpagehoffnum, + istatus->idxoffnum, delstate->iblknum, + RelationGetRelationName(delstate->irel)))); + + if (ItemIdHasStorage(iid)) + { + Assert(ItemIdIsNormal(iid)); + htup = (HeapTupleHeader) PageGetItem(page, iid); + + if (HeapTupleHeaderIsHeapOnly(htup)) + ereport(ERROR, + (errcode(ERRCODE_INDEX_CORRUPTED), + errmsg_internal("heap tid from index tuple (%u,%u) points to heap-only tuple at offset %u of block %u in index \"%s\"", + ItemPointerGetBlockNumber(htid), + indexpagehoffnum, + istatus->idxoffnum, delstate->iblknum, + RelationGetRelationName(delstate->irel)))); + } +} + /* * heapam implementation of tableam's index_delete_tuples interface. * @@ -7446,6 +7507,14 @@ heap_index_delete_tuples(Relation rel, TM_IndexDeleteOp *delstate) maxoff = PageGetMaxOffsetNumber(page); } + /* + * In passing, detect index corruption involving an index page with a + * TID that point to a location in the heap that couldn't possibly be + * correct. We only do this with actual TIDs from caller's index page + * (not items reached by traversing through a HOT chain). + */ + index_delete_check_htid(delstate, page, maxoff, htid, istatus); + if (istatus->knowndeletable) Assert(!delstate->bottomup && !istatus->promising); else diff --git a/src/backend/access/heap/pruneheap.c b/src/backend/access/heap/pruneheap.c index db6912e9f..43549be04 100644 --- a/src/backend/access/heap/pruneheap.c +++ b/src/backend/access/heap/pruneheap.c @@ -844,39 +844,115 @@ heap_page_prune_execute(Buffer buffer, { Page page = (Page) BufferGetPage(buffer); OffsetNumber *offnum; - int i; + HeapTupleHeader htup PG_USED_FOR_ASSERTS_ONLY; /* Shouldn't be called unless there's something to do */ Assert(nredirected > 0 || ndead > 0 || nunused > 0); /* Update all redirected line pointers */ offnum = redirected; - for (i = 0; i < nredirected; i++) + for (int i = 0; i < nredirected; i++) { OffsetNumber fromoff = *offnum++; OffsetNumber tooff = *offnum++; ItemId fromlp = PageGetItemId(page, fromoff); + ItemId tolp PG_USED_FOR_ASSERTS_ONLY; + +#ifdef USE_ASSERT_CHECKING + + /* + * The existing items that we set as redirects must be the first tuple + * of a HOT chain that has not be pruned before now (can't be a + * heap-only tuple) when target item has tuple storage. When the item + * has no storage, then we must just be maintaining the LP_REDIRECT of + * a HOT chain that has been pruned at least once before now. + */ + if (!ItemIdIsRedirected(fromlp)) + { + Assert(ItemIdHasStorage(fromlp) && ItemIdIsNormal(fromlp)); + + htup = (HeapTupleHeader) PageGetItem(page, fromlp); + Assert(!HeapTupleHeaderIsHeapOnly(htup)); + } + else + { + /* We shouldn't need to redundantly set the redirect */ + Assert(ItemIdGetRedirect(fromlp) != tooff); + } + + /* + * Redirect links must point to heap-only tuples. There can be at + * most one LP_REDIRECT item per HOT chain. + * + * We need to keep around an LP_REDIRECT item (after original + * non-heap-only root tuple gets pruned away) so that it's always + * possible for VACUUM to easily figure out what TID to delete from + * indexes when an entire HOT chain becomes dead. A heap-only tuple + * can never become LP_DEAD; an LP_REDIRECT item or a regular heap + * tuple can. + */ + tolp = PageGetItemId(page, tooff); + Assert(ItemIdHasStorage(tolp) && ItemIdIsNormal(tolp)); + htup = (HeapTupleHeader) PageGetItem(page, tolp); + Assert(HeapTupleHeaderIsHeapOnly(htup)); +#endif ItemIdSetRedirect(fromlp, tooff); } /* Update all now-dead line pointers */ offnum = nowdead; - for (i = 0; i < ndead; i++) + for (int i = 0; i < ndead; i++) { OffsetNumber off = *offnum++; ItemId lp = PageGetItemId(page, off); +#ifdef USE_ASSERT_CHECKING + + /* + * LP_DEAD line pointers must be left behind when they could be + * pointed to by TIDs from indexes. Index scans expect TIDs to always + * work as stable identifiers of a tuple version (or HOT chain). + */ + if (ItemIdHasStorage(lp)) + { + Assert(ItemIdIsNormal(lp)); + htup = (HeapTupleHeader) PageGetItem(page, lp); + Assert(!HeapTupleHeaderIsHeapOnly(htup)); + } + else + { + /* + * Whole HOT chain becomes dead. (We shouldn't need to redundantly + * mark existing LP_DEAD items LP_DEAD.) + */ + Assert(ItemIdIsRedirected(lp)); + } +#endif + ItemIdSetDead(lp); } /* Update all now-unused line pointers */ offnum = nowunused; - for (i = 0; i < nunused; i++) + for (int i = 0; i < nunused; i++) { OffsetNumber off = *offnum++; ItemId lp = PageGetItemId(page, off); +#ifdef USE_ASSERT_CHECKING + + /* + * Only heap-only tuples can become LP_UNUSED during pruning. They + * don't need to be left in place as LP_DEAD items until VACUUM gets + * around to doing index vacuuming. We are already sure that there + * cannot be a TID in an index that points to the items. + */ + Assert(ItemIdHasStorage(lp) && ItemIdIsNormal(lp)); + htup = (HeapTupleHeader) PageGetItem(page, lp); + Assert(HeapTupleHeaderIsHeapOnly(htup)); +#endif + ItemIdSetUnused(lp); } diff --git a/src/backend/access/index/genam.c b/src/backend/access/index/genam.c index b93288a6f..4fe4514fb 100644 --- a/src/backend/access/index/genam.c +++ b/src/backend/access/index/genam.c @@ -303,6 +303,8 @@ index_compute_xid_horizon_for_tuples(Relation irel, Assert(nitems > 0); + delstate.irel = irel; + delstate.iblknum = BufferGetBlockNumber(ibuf); delstate.bottomup = false; delstate.bottomupfreespace = 0; delstate.ndeltids = 0; @@ -321,7 +323,7 @@ index_compute_xid_horizon_for_tuples(Relation irel, ItemPointerCopy(&itup->t_tid, &delstate.deltids[i].tid); delstate.deltids[i].id = delstate.ndeltids; - delstate.status[i].idxoffnum = InvalidOffsetNumber; /* unused */ + delstate.status[i].idxoffnum = itemnos[i]; delstate.status[i].knowndeletable = true; /* LP_DEAD-marked */ delstate.status[i].promising = false; /* unused */ delstate.status[i].freespace = 0; /* unused */ diff --git a/src/backend/access/nbtree/nbtdedup.c b/src/backend/access/nbtree/nbtdedup.c index 6401fce57..c88dc6eed 100644 --- a/src/backend/access/nbtree/nbtdedup.c +++ b/src/backend/access/nbtree/nbtdedup.c @@ -348,6 +348,8 @@ _bt_bottomupdel_pass(Relation rel, Buffer buf, Relation heapRel, * concerning ourselves with avoiding work during the tableam call. Our * role in costing the bottom-up deletion process is strictly advisory. */ + delstate.irel = rel; + delstate.iblknum = BufferGetBlockNumber(buf); delstate.bottomup = true; delstate.bottomupfreespace = Max(BLCKSZ / 16, newitemsz); delstate.ndeltids = 0; diff --git a/src/backend/access/nbtree/nbtinsert.c b/src/backend/access/nbtree/nbtinsert.c index ccddb0378..0fe8c7093 100644 --- a/src/backend/access/nbtree/nbtinsert.c +++ b/src/backend/access/nbtree/nbtinsert.c @@ -2810,6 +2810,8 @@ _bt_simpledel_pass(Relation rel, Buffer buffer, Relation heapRel, &ndeadblocks); /* Initialize tableam state that describes index deletion operation */ + delstate.irel = rel; + delstate.iblknum = BufferGetBlockNumber(buffer); delstate.bottomup = false; delstate.bottomupfreespace = 0; delstate.ndeltids = 0; -- 2.30.2