From fdc9c6ce6bff50b0dfde3960f800a455d835d3d0 Mon Sep 17 00:00:00 2001 From: Matthias van de Meent Date: Fri, 25 Apr 2025 16:14:26 +0200 Subject: [PATCH v01 2/4] GIST: Fix visibility issues in IOS Previously, GIST IOS could buffer tuples from pages while VACUUM came along and cleaned up an ALL_DEAD tuple, marking the tuple's page ALL_VISIBLE again and making IOS mistakenly believe the tuple is indeed visible. With this commit, buffer pins now conflict with GIST vacuum, and we now do visibility checks on heap items returned from index pages while we hold the pin, so that the IOS infrastructure knows to recheck the heap page even if that heap page has become ALL_VISIBLE after we released the index page. Idea from Heikki Linnakangas Backpatch: 17- --- src/include/access/gist_private.h | 5 ++++ src/backend/access/gist/gistget.c | 36 ++++++++++++++++++++++++---- src/backend/access/gist/gistscan.c | 11 +++++++++ src/backend/access/gist/gistvacuum.c | 6 ++--- 4 files changed, 50 insertions(+), 8 deletions(-) diff --git a/src/include/access/gist_private.h b/src/include/access/gist_private.h index 7b8749c8db0..78bee3a93f3 100644 --- a/src/include/access/gist_private.h +++ b/src/include/access/gist_private.h @@ -22,6 +22,7 @@ #include "storage/buffile.h" #include "utils/hsearch.h" #include "access/genam.h" +#include "tableam.h" /* * Maximum number of "halves" a page can be split into in one operation. @@ -124,6 +125,8 @@ typedef struct GISTSearchHeapItem * index-only scans */ OffsetNumber offnum; /* track offset in page to mark tuple as * LP_DEAD */ + uint8 visrecheck; /* Cached visibility check result for this + * heap pointer. */ } GISTSearchHeapItem; /* Unvisited item, either index page or heap tuple */ @@ -176,6 +179,8 @@ typedef struct GISTScanOpaqueData OffsetNumber curPageData; /* next item to return */ MemoryContext pageDataCxt; /* context holding the fetched tuples, for * index-only scans */ + /* info used by Index-Only Scans */ + Buffer vmbuf; /* reusable buffer for IOS' vm lookups */ } GISTScanOpaqueData; typedef GISTScanOpaqueData *GISTScanOpaque; diff --git a/src/backend/access/gist/gistget.c b/src/backend/access/gist/gistget.c index b35b8a97577..8f6a680b010 100644 --- a/src/backend/access/gist/gistget.c +++ b/src/backend/access/gist/gistget.c @@ -462,8 +462,9 @@ gistScanPage(IndexScanDesc scan, GISTSearchItem *pageItem, so->pageData[so->nPageData].offnum = i; /* - * In an index-only scan, also fetch the data from the tuple. The - * reconstructed tuples are stored in pageDataCxt. + * In an index-only scan, also fetch the data from the tuple, + * and its visibility state. The reconstructed tuples are stored + * in pageDataCxt. */ if (scan->xs_want_itup) { @@ -471,6 +472,10 @@ gistScanPage(IndexScanDesc scan, GISTSearchItem *pageItem, so->pageData[so->nPageData].recontup = gistFetchTuple(giststate, r, it); MemoryContextSwitchTo(oldcxt); + + so->pageData[so->nPageData].visrecheck = + table_index_vischeck_tuple(scan->heapRelation, + &so->vmbuf, &it->t_tid); } so->nPageData++; } @@ -498,10 +503,16 @@ gistScanPage(IndexScanDesc scan, GISTSearchItem *pageItem, item->data.heap.recheckDistances = recheck_distances; /* - * In an index-only scan, also fetch the data from the tuple. + * In an index-only scan, also fetch the data from the tuple, + * and its visibility state. */ if (scan->xs_want_itup) + { item->data.heap.recontup = gistFetchTuple(giststate, r, it); + item->data.heap.visrecheck = + table_index_vischeck_tuple(scan->heapRelation, + &so->vmbuf, &it->t_tid); + } } else { @@ -586,9 +597,16 @@ getNextNearest(IndexScanDesc scan) item->distances, item->data.heap.recheckDistances); - /* in an index-only scan, also return the reconstructed tuple. */ + /* + * In an index-only scan, also return the reconstructed tuple + * and the visibility check we did when we still had a pin on + * the index page. + */ if (scan->xs_want_itup) + { scan->xs_hitup = item->data.heap.recontup; + scan->xs_visrecheck = item->data.heap.visrecheck; + } res = true; } else @@ -671,9 +689,17 @@ gistgettuple(IndexScanDesc scan, ScanDirection dir) scan->xs_heaptid = so->pageData[so->curPageData].heapPtr; scan->xs_recheck = so->pageData[so->curPageData].recheck; - /* in an index-only scan, also return the reconstructed tuple */ + /* + * In an index-only scan, also return the reconstructed tuple + * and the visibility check we did when we still had a pin on + * the index page. + */ if (scan->xs_want_itup) + { scan->xs_hitup = so->pageData[so->curPageData].recontup; + scan->xs_visrecheck = + so->pageData[so->curPageData].visrecheck; + } so->curPageData++; diff --git a/src/backend/access/gist/gistscan.c b/src/backend/access/gist/gistscan.c index e05801e2f5b..97fecfcb1b2 100644 --- a/src/backend/access/gist/gistscan.c +++ b/src/backend/access/gist/gistscan.c @@ -341,6 +341,12 @@ gistrescan(IndexScanDesc scan, ScanKey key, int nkeys, pfree(fn_extras); } + if (BufferIsValid(so->vmbuf)) + { + ReleaseBuffer(so->vmbuf); + so->vmbuf = InvalidBuffer; + } + /* any previous xs_hitup will have been pfree'd in context resets above */ scan->xs_hitup = NULL; } @@ -349,6 +355,11 @@ void gistendscan(IndexScanDesc scan) { GISTScanOpaque so = (GISTScanOpaque) scan->opaque; + if (BufferIsValid(so->vmbuf)) + { + ReleaseBuffer(so->vmbuf); + so->vmbuf = InvalidBuffer; + } /* * freeGISTstate is enough to clean up everything made by gistbeginscan, diff --git a/src/backend/access/gist/gistvacuum.c b/src/backend/access/gist/gistvacuum.c index 24fb94f473e..e0da6e37dca 100644 --- a/src/backend/access/gist/gistvacuum.c +++ b/src/backend/access/gist/gistvacuum.c @@ -289,10 +289,10 @@ restart: info->strategy); /* - * We are not going to stay here for a long time, aggressively grab an - * exclusive lock. + * We are not going to stay here for a long time, aggressively grab a + * cleanup lock. */ - LockBuffer(buffer, GIST_EXCLUSIVE); + LockBufferForCleanup(buffer); page = (Page) BufferGetPage(buffer); if (gistPageRecyclable(page)) -- 2.45.2