v1-0001-Remove-BitmapScan-s-skip_fetch-optimization.patch

From 43144d7511c93ed153b4ab5b30bf59ea3af20fbd Mon Sep 17 00:00:00 2001
From: Matthias van de Meent <boekewurm+postgres@gmail.com>
Date: Mon, 2 Dec 2024 15:38:14 +0100
Subject: [PATCH v1] Remove BitmapScan's skip_fetch optimization

The optimization doesn't take concurrent vacuum's removal of TIDs into
account, which can remove dead TIDs and make ALL_VISIBLE pages for which
we have pointers to those dead TIDs in the bitmap.

The optimization itself isn't impossible, but would require more work
figuring out that vacuum started removing TIDs and then stop using the
optimization. However, we currently don't have such a system in place,
making the optimization unsound to keep around.

Reported-By: Konstantin Knizhnik <knizhnik@garret.ru>
Backpatch-through: 13
---
 src/include/access/heapam.h               |  1 -
 src/include/access/tableam.h              | 12 +------
 src/backend/access/heap/heapam.c          | 18 ----------
 src/backend/access/heap/heapam_handler.c  | 28 ---------------
 src/backend/executor/nodeBitmapHeapscan.c | 43 ++---------------------
 5 files changed, 4 insertions(+), 98 deletions(-)

diff --git a/src/include/access/heapam.h b/src/include/access/heapam.h
index 96cf82f97b..6dd233dc17 100644
--- a/src/include/access/heapam.h
+++ b/src/include/access/heapam.h
@@ -99,7 +99,6 @@ typedef struct HeapScanDescData
 	 * block reported by the bitmap to determine how many NULL-filled tuples
 	 * to return.
 	 */
-	Buffer		rs_vmbuffer;
 	int			rs_empty_tuples_pending;
 
 	/* these fields only used in page-at-a-time mode and for bitmap scans */
diff --git a/src/include/access/tableam.h b/src/include/access/tableam.h
index adb478a93c..283a19babe 100644
--- a/src/include/access/tableam.h
+++ b/src/include/access/tableam.h
@@ -62,13 +62,6 @@ typedef enum ScanOptions
 
 	/* unregister snapshot at scan end? */
 	SO_TEMP_SNAPSHOT = 1 << 9,
-
-	/*
-	 * At the discretion of the table AM, bitmap table scans may be able to
-	 * skip fetching a block from the table if none of the table data is
-	 * needed. If table data may be needed, set SO_NEED_TUPLES.
-	 */
-	SO_NEED_TUPLES = 1 << 10,
 }			ScanOptions;
 
 /*
@@ -955,14 +948,11 @@ table_beginscan_strat(Relation rel, Snapshot snapshot,
  */
 static inline TableScanDesc
 table_beginscan_bm(Relation rel, Snapshot snapshot,
-				   int nkeys, struct ScanKeyData *key, bool need_tuple)
+				   int nkeys, struct ScanKeyData *key)
 {
 	TableScanDesc result;
 	uint32		flags = SO_TYPE_BITMAPSCAN | SO_ALLOW_PAGEMODE;
 
-	if (need_tuple)
-		flags |= SO_NEED_TUPLES;
-
 	result = rel->rd_tableam->scan_begin(rel, snapshot, nkeys, key,
 										 NULL, flags);
 	result->st.bitmap.rs_shared_iterator = NULL;
diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c
index d00300c5dc..9ea9dec863 100644
--- a/src/backend/access/heap/heapam.c
+++ b/src/backend/access/heap/heapam.c
@@ -1053,8 +1053,6 @@ heap_beginscan(Relation relation, Snapshot snapshot,
 	scan->rs_base.rs_flags = flags;
 	scan->rs_base.rs_parallel = parallel_scan;
 	scan->rs_strategy = NULL;	/* set in initscan */
-	scan->rs_vmbuffer = InvalidBuffer;
-	scan->rs_empty_tuples_pending = 0;
 
 	/*
 	 * Disable page-at-a-time mode if it's not a MVCC-safe snapshot.
@@ -1170,19 +1168,6 @@ heap_rescan(TableScanDesc sscan, ScanKey key, bool set_params,
 	if (BufferIsValid(scan->rs_cbuf))
 		ReleaseBuffer(scan->rs_cbuf);
 
-	if (BufferIsValid(scan->rs_vmbuffer))
-	{
-		ReleaseBuffer(scan->rs_vmbuffer);
-		scan->rs_vmbuffer = InvalidBuffer;
-	}
-
-	/*
-	 * Reset rs_empty_tuples_pending, a field only used by bitmap heap scan,
-	 * to avoid incorrectly emitting NULL-filled tuples from a previous scan
-	 * on rescan.
-	 */
-	scan->rs_empty_tuples_pending = 0;
-
 	/*
 	 * The read stream is reset on rescan. This must be done before
 	 * initscan(), as some state referred to by read_stream_reset() is reset
@@ -1210,9 +1195,6 @@ heap_endscan(TableScanDesc sscan)
 	if (BufferIsValid(scan->rs_cbuf))
 		ReleaseBuffer(scan->rs_cbuf);
 
-	if (BufferIsValid(scan->rs_vmbuffer))
-		ReleaseBuffer(scan->rs_vmbuffer);
-
 	/*
 	 * Must free the read stream before freeing the BufferAccessStrategy.
 	 */
diff --git a/src/backend/access/heap/heapam_handler.c b/src/backend/access/heap/heapam_handler.c
index a8d95e0f1c..b432eb8140 100644
--- a/src/backend/access/heap/heapam_handler.c
+++ b/src/backend/access/heap/heapam_handler.c
@@ -2158,24 +2158,6 @@ heapam_scan_bitmap_next_block(TableScanDesc scan,
 	*blockno = tbmres->blockno;
 	*recheck = tbmres->recheck;
 
-	/*
-	 * We can skip fetching the heap page if we don't need any fields from the
-	 * heap, the bitmap entries don't need rechecking, and all tuples on the
-	 * page are visible to our transaction.
-	 */
-	if (!(scan->rs_flags & SO_NEED_TUPLES) &&
-		!tbmres->recheck &&
-		VM_ALL_VISIBLE(scan->rs_rd, tbmres->blockno, &hscan->rs_vmbuffer))
-	{
-		/* can't be lossy in the skip_fetch case */
-		Assert(tbmres->ntuples >= 0);
-		Assert(hscan->rs_empty_tuples_pending >= 0);
-
-		hscan->rs_empty_tuples_pending += tbmres->ntuples;
-
-		return true;
-	}
-
 	block = tbmres->blockno;
 
 	/*
@@ -2290,16 +2272,6 @@ heapam_scan_bitmap_next_tuple(TableScanDesc scan,
 	Page		page;
 	ItemId		lp;
 
-	if (hscan->rs_empty_tuples_pending > 0)
-	{
-		/*
-		 * If we don't have to fetch the tuple, just return nulls.
-		 */
-		ExecStoreAllNullTuple(slot);
-		hscan->rs_empty_tuples_pending--;
-		return true;
-	}
-
 	/*
 	 * Out of range?  If so, nothing more to look at on this page
 	 */
diff --git a/src/backend/executor/nodeBitmapHeapscan.c b/src/backend/executor/nodeBitmapHeapscan.c
index 689bde16dd..9bf01bb0c3 100644
--- a/src/backend/executor/nodeBitmapHeapscan.c
+++ b/src/backend/executor/nodeBitmapHeapscan.c
@@ -176,24 +176,10 @@ BitmapHeapNext(BitmapHeapScanState *node)
 		 */
 		if (!scan)
 		{
-			bool		need_tuples = false;
-
-			/*
-			 * We can potentially skip fetching heap pages if we do not need
-			 * any columns of the table, either for checking non-indexable
-			 * quals or for returning data.  This test is a bit simplistic, as
-			 * it checks the stronger condition that there's no qual or return
-			 * tlist at all. But in most cases it's probably not worth working
-			 * harder than that.
-			 */
-			need_tuples = (node->ss.ps.plan->qual != NIL ||
-						   node->ss.ps.plan->targetlist != NIL);
-
 			scan = table_beginscan_bm(node->ss.ss_currentRelation,
 									  node->ss.ps.state->es_snapshot,
 									  0,
-									  NULL,
-									  need_tuples);
+									  NULL);
 
 			node->ss.ss_currentScanDesc = scan;
 		}
@@ -453,7 +439,6 @@ BitmapPrefetch(BitmapHeapScanState *node, TableScanDesc scan)
 			while (node->prefetch_pages < node->prefetch_target)
 			{
 				TBMIterateResult *tbmpre = tbm_iterate(prefetch_iterator);
-				bool		skip_fetch;
 
 				if (tbmpre == NULL)
 				{
@@ -465,20 +450,7 @@ BitmapPrefetch(BitmapHeapScanState *node, TableScanDesc scan)
 				node->prefetch_pages++;
 				node->prefetch_blockno = tbmpre->blockno;
 
-				/*
-				 * If we expect not to have to actually read this heap page,
-				 * skip this prefetch call, but continue to run the prefetch
-				 * logic normally.  (Would it be better not to increment
-				 * prefetch_pages?)
-				 */
-				skip_fetch = (!(scan->rs_flags & SO_NEED_TUPLES) &&
-							  !tbmpre->recheck &&
-							  VM_ALL_VISIBLE(node->ss.ss_currentRelation,
-											 tbmpre->blockno,
-											 &node->pvmbuffer));
-
-				if (!skip_fetch)
-					PrefetchBuffer(scan->rs_rd, MAIN_FORKNUM, tbmpre->blockno);
+				PrefetchBuffer(scan->rs_rd, MAIN_FORKNUM, tbmpre->blockno);
 			}
 		}
 
@@ -495,7 +467,6 @@ BitmapPrefetch(BitmapHeapScanState *node, TableScanDesc scan)
 			{
 				TBMIterateResult *tbmpre;
 				bool		do_prefetch = false;
-				bool		skip_fetch;
 
 				/*
 				 * Recheck under the mutex. If some other process has already
@@ -523,15 +494,7 @@ BitmapPrefetch(BitmapHeapScanState *node, TableScanDesc scan)
 
 				node->prefetch_blockno = tbmpre->blockno;
 
-				/* As above, skip prefetch if we expect not to need page */
-				skip_fetch = (!(scan->rs_flags & SO_NEED_TUPLES) &&
-							  !tbmpre->recheck &&
-							  VM_ALL_VISIBLE(node->ss.ss_currentRelation,
-											 tbmpre->blockno,
-											 &node->pvmbuffer));
-
-				if (!skip_fetch)
-					PrefetchBuffer(scan->rs_rd, MAIN_FORKNUM, tbmpre->blockno);
+				PrefetchBuffer(scan->rs_rd, MAIN_FORKNUM, tbmpre->blockno);
 			}
 		}
 	}
-- 
2.45.2