From 9eec297b177dc3d187d83eb7add03d6367081a1d Mon Sep 17 00:00:00 2001 From: Peter Geoghegan Date: Tue, 18 Aug 2026 15:00:15 -0400 Subject: [PATCH v1 1/3] GiST: Invalidate killed items consistently. GiST neglected to invalidate its killedItems[] array on a rescan. As a result, it was just about possible for the wrong tuples from the wrong index page to be LP_DEAD-marked on a rescan. The scan mistakenly believed that the previous rescan's killedItems[] were for this rescan's curBlkno, causing index corruption (corruption has only been proven on the master branch; see explanation below). To fix, bring GiST in line with nbtree and hash: call gistkillitems() from both gistrescan() and gistendscan() (in addition to the existing gistgettuple caller). That way the scan's pending killedItems[] are passed to gistkillitems while they still describe items from curBlkno. When gistkillitems runs, it'll invalidate the array in passing (and won't needlessly miss out on an opportunity to LP_DEAD-mark eligible index tuples). This only became a serious problem in commit 9c9ddf109, which isn't on any stable branch. While we were just as sloppy about invalidating killedItems[] state before that commit, it probably didn't cause any real harm. The test case that actually proved index corruption on the master branch couldn't do so on any stable branch. The test would clobber curPageLSN (without also invalidating killedItems[] or curBlkno), which accidentally prevented corruption. It's far from obvious that that's reliable, which is why this is being treated as a bug affecting all stable branches. Back branches get minimal hardening that just invalidates killedItems[] in places where the master branch now calls gistkillitems. Reviewed-By: Andrey Borodin Discussion: https://postgr.es/m/CAH2-WzmwEThnQf17Ju+t0N9_KJLsEQSXzYrFnaS2=s4KnGGrqw@mail.gmail.com Backpatch-through: 14 --- src/include/access/gist_private.h | 1 + src/backend/access/gist/gistget.c | 40 ++++++++++++++++-------------- src/backend/access/gist/gistscan.c | 10 ++++++++ 3 files changed, 32 insertions(+), 19 deletions(-) diff --git a/src/include/access/gist_private.h b/src/include/access/gist_private.h index 44514f1cb..39fdd27f5 100644 --- a/src/include/access/gist_private.h +++ b/src/include/access/gist_private.h @@ -458,6 +458,7 @@ extern XLogRecPtr gistXLogSplit(bool page_is_leaf, Buffer leftchildbuf, bool markfollowright); /* gistget.c */ +extern void gistkillitems(IndexScanDesc scan); extern bool gistgettuple(IndexScanDesc scan, ScanDirection dir); extern int64 gistgetbitmap(IndexScanDesc scan, TIDBitmap *tbm); extern bool gistcanreturn(Relation index, int attno); diff --git a/src/backend/access/gist/gistget.c b/src/backend/access/gist/gistget.c index 5df2d4022..613e6d46d 100644 --- a/src/backend/access/gist/gistget.c +++ b/src/backend/access/gist/gistget.c @@ -35,25 +35,27 @@ * flag any entries because it is possible that the old entry was vacuumed * away and the TID was re-used by a completely different heap tuple. */ -static void +void gistkillitems(IndexScanDesc scan) { GISTScanOpaque so = (GISTScanOpaque) scan->opaque; + int numKilled = so->numKilled; Buffer buffer; Page page; - OffsetNumber offnum; - ItemId iid; - int i; bool killedsomething = false; Assert(so->curBlkno != InvalidBlockNumber); Assert(XLogRecPtrIsValid(so->curPageLSN)); Assert(so->killedItems != NULL); + Assert(numKilled > 0); + + /* + * Always reset the scan state, so we don't look for same items on other + * pages + */ + so->numKilled = 0; buffer = ReadBuffer(scan->indexRelation, so->curBlkno); - if (!BufferIsValid(buffer)) - return; - LockBuffer(buffer, GIST_SHARE); gistcheckpage(scan->indexRelation, buffer); page = BufferGetPage(buffer); @@ -64,7 +66,10 @@ gistkillitems(IndexScanDesc scan) * safe. */ if (BufferGetLSNAtomic(buffer) != so->curPageLSN) - goto unlock; + { + UnlockReleaseBuffer(buffer); + return; + } Assert(GistPageIsLeaf(page)); @@ -72,8 +77,11 @@ gistkillitems(IndexScanDesc scan) * Mark all killedItems as dead. We need no additional recheck, because, * if page was modified, curPageLSN must have changed. */ - for (i = 0; i < so->numKilled; i++) + for (int i = 0; i < numKilled; i++) { + OffsetNumber offnum = so->killedItems[i]; + ItemId iid = PageGetItemId(page, offnum); + if (!killedsomething) { /* @@ -82,11 +90,12 @@ gistkillitems(IndexScanDesc scan) * there's no point continuing. */ if (!BufferBeginSetHintBits(buffer)) - goto unlock; + { + UnlockReleaseBuffer(buffer); + return; + } } - offnum = so->killedItems[i]; - iid = PageGetItemId(page, offnum); ItemIdMarkDead(iid); killedsomething = true; } @@ -97,14 +106,7 @@ gistkillitems(IndexScanDesc scan) BufferFinishSetHintBits(buffer, true, true); } -unlock: UnlockReleaseBuffer(buffer); - - /* - * Always reset the scan state, so we don't look for same items on other - * pages. - */ - so->numKilled = 0; } /* diff --git a/src/backend/access/gist/gistscan.c b/src/backend/access/gist/gistscan.c index 5583a5a35..b67ba5a8a 100644 --- a/src/backend/access/gist/gistscan.c +++ b/src/backend/access/gist/gistscan.c @@ -133,7 +133,13 @@ gistrescan(IndexScanDesc scan, ScanKey key, int nkeys, int i; MemoryContext oldCxt; + /* Before leaving current page, deal with any killed items */ + if (so->numKilled > 0) + gistkillitems(scan); + /* rescan an existing indexscan --- reset state */ + so->curBlkno = InvalidBlockNumber; + so->curPageLSN = InvalidXLogRecPtr; /* * The first time through, we create the search queue in the scanCxt. @@ -349,6 +355,10 @@ gistendscan(IndexScanDesc scan) { GISTScanOpaque so = (GISTScanOpaque) scan->opaque; + /* Before leaving current page, deal with any killed items */ + if (so->numKilled > 0) + gistkillitems(scan); + /* * freeGISTstate is enough to clean up everything made by gistbeginscan, * as well as the queueCxt if there is a separate context for it. -- 2.53.0