Incorrect result of bitmap heap scan.

From 07739e5af83664b67ea02d3db7a461a106d74040 Mon Sep 17 00:00:00 2001
From: Matthias van de Meent <boekewurm+postgres@gmail.com>
Date: Mon, 2 Dec 2024 15:59:35 +0100
Subject: [PATCH v1] Disable 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               |  9 +++--
 src/include/access/tableam.h              |  3 +-
 src/backend/access/heap/heapam.c          | 13 -------
 src/backend/access/heap/heapam_handler.c  | 28 ---------------
 src/backend/executor/nodeBitmapHeapscan.c | 42 ++---------------------
 5 files changed, 8 insertions(+), 87 deletions(-)

diff --git a/src/include/access/heapam.h b/src/include/access/heapam.h
index 65999dd64e..42f28109e2 100644
--- a/src/include/access/heapam.h
+++ b/src/include/access/heapam.h
@@ -92,11 +92,10 @@ typedef struct HeapScanDescData
 	ParallelBlockTableScanWorkerData *rs_parallelworkerdata;
 
 	/*
-	 * These fields are only used for bitmap scans for the "skip fetch"
-	 * optimization. Bitmap scans needing no fields from the heap may skip
-	 * fetching an all visible block, instead using the number of tuples per
-	 * block reported by the bitmap to determine how many NULL-filled tuples
-	 * to return.
+	 * These fields were kept around to guarantee ABI stability, but are
+	 * otherwise unused. They were only used for bitmap scans for the
+	 * "skip fetch" optimization, which turned out to be unsound when the
+	 * second phase of vacuum ran concurrently.
 	 */
 	Buffer		rs_vmbuffer;
 	int			rs_empty_tuples_pending;
diff --git a/src/include/access/tableam.h b/src/include/access/tableam.h
index da661289c1..5d54b0a18b 100644
--- a/src/include/access/tableam.h
+++ b/src/include/access/tableam.h
@@ -955,8 +955,7 @@ table_beginscan_bm(Relation rel, Snapshot snapshot,
 {
 	uint32		flags = SO_TYPE_BITMAPSCAN | SO_ALLOW_PAGEMODE;
 
-	if (need_tuple)
-		flags |= SO_NEED_TUPLES;
+	flags |= SO_NEED_TUPLES;
 
 	return rel->rd_tableam->scan_begin(rel, snapshot, nkeys, key, NULL, flags);
 }
diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c
index bbe64b1e53..ca2dbb0b75 100644
--- a/src/backend/access/heap/heapam.c
+++ b/src/backend/access/heap/heapam.c
@@ -1188,19 +1188,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
diff --git a/src/backend/access/heap/heapam_handler.c b/src/backend/access/heap/heapam_handler.c
index 6f8b1b7929..93980be98a 100644
--- a/src/backend/access/heap/heapam_handler.c
+++ b/src/backend/access/heap/heapam_handler.c
@@ -2131,24 +2131,6 @@ heapam_scan_bitmap_next_block(TableScanDesc scan,
 	hscan->rs_cindex = 0;
 	hscan->rs_ntuples = 0;
 
-	/*
-	 * 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;
-	}
-
 	/*
 	 * Ignore any claimed entries past what we think is the end of the
 	 * relation. It may have been extended after the start of our scan (we
@@ -2261,16 +2243,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 6b48a6d835..82402c0ac0 100644
--- a/src/backend/executor/nodeBitmapHeapscan.c
+++ b/src/backend/executor/nodeBitmapHeapscan.c
@@ -185,24 +185,11 @@ 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);
+									  true);
 
 			node->ss.ss_currentScanDesc = scan;
 		}
@@ -459,7 +446,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)
 				{
@@ -470,20 +456,7 @@ BitmapPrefetch(BitmapHeapScanState *node, TableScanDesc scan)
 				}
 				node->prefetch_pages++;
 
-				/*
-				 * 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);
 			}
 		}
 
@@ -500,7 +473,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
@@ -526,15 +498,7 @@ BitmapPrefetch(BitmapHeapScanState *node, TableScanDesc scan)
 					break;
 				}
 
-				/* 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

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

From eed8c4b613b5c44113e55bc6917ef3564d4632f8 Mon Sep 17 00:00:00 2001
From: Matthias van de Meent <boekewurm+postgres@gmail.com>
Date: Mon, 2 Dec 2024 16:05:09 +0100
Subject: [PATCH v1] Disable 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 still 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/backend/executor/nodeBitmapHeapscan.c | 64 ++---------------------
 1 file changed, 4 insertions(+), 60 deletions(-)

diff --git a/src/backend/executor/nodeBitmapHeapscan.c b/src/backend/executor/nodeBitmapHeapscan.c
index 1cf0bbddd4..214c129472 100644
--- a/src/backend/executor/nodeBitmapHeapscan.c
+++ b/src/backend/executor/nodeBitmapHeapscan.c
@@ -186,8 +186,6 @@ BitmapHeapNext(BitmapHeapScanState *node)
 
 	for (;;)
 	{
-		bool		skip_fetch;
-
 		CHECK_FOR_INTERRUPTS();
 
 		/*
@@ -212,32 +210,7 @@ BitmapHeapNext(BitmapHeapScanState *node)
 			else
 				node->lossy_pages++;
 
-			/*
-			 * We can skip fetching the heap page if we don't need any fields
-			 * from the heap, and the bitmap entries don't need rechecking,
-			 * and all tuples on the page are visible to our transaction.
-			 *
-			 * XXX: It's a layering violation that we do these checks above
-			 * tableam, they should probably moved below it at some point.
-			 */
-			skip_fetch = (node->can_skip_fetch &&
-						  !tbmres->recheck &&
-						  VM_ALL_VISIBLE(node->ss.ss_currentRelation,
-										 tbmres->blockno,
-										 &node->vmbuffer));
-
-			if (skip_fetch)
-			{
-				/* can't be lossy in the skip_fetch case */
-				Assert(tbmres->ntuples >= 0);
-
-				/*
-				 * The number of tuples on this page is put into
-				 * node->return_empty_tuples.
-				 */
-				node->return_empty_tuples = tbmres->ntuples;
-			}
-			else if (!table_scan_bitmap_next_block(scan, tbmres))
+			if (!table_scan_bitmap_next_block(scan, tbmres))
 			{
 				/* AM doesn't think this block is valid, skip */
 				continue;
@@ -475,7 +448,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)
 				{
@@ -486,25 +458,7 @@ BitmapPrefetch(BitmapHeapScanState *node, TableScanDesc scan)
 				}
 				node->prefetch_pages++;
 
-				/*
-				 * 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?)
-				 *
-				 * This depends on the assumption that the index AM will
-				 * report the same recheck flag for this future heap page as
-				 * it did for the current heap page; which is not a certainty
-				 * but is true in many cases.
-				 */
-				skip_fetch = (node->can_skip_fetch &&
-							  (node->tbmres ? !node->tbmres->recheck : false) &&
-							  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);
 			}
 		}
 
@@ -521,7 +475,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
@@ -547,15 +500,7 @@ BitmapPrefetch(BitmapHeapScanState *node, TableScanDesc scan)
 					break;
 				}
 
-				/* As above, skip prefetch if we expect not to need page */
-				skip_fetch = (node->can_skip_fetch &&
-							  (node->tbmres ? !node->tbmres->recheck : false) &&
-							  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);
 			}
 		}
 	}
@@ -749,8 +694,7 @@ ExecInitBitmapHeapScan(BitmapHeapScan *node, EState *estate, int eflags)
 	 * 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.
 	 */
-	scanstate->can_skip_fetch = (node->scan.plan.qual == NIL &&
-								 node->scan.plan.targetlist == NIL);
+	scanstate->can_skip_fetch = false;
 
 	/*
 	 * Miscellaneous initialization
-- 
2.45.2

From a666e6da7af9a0af31ae506de8c2c229b713f8a6 Mon Sep 17 00:00:00 2001
From: Andres Freund <andres@anarazel.de>
Date: Mon, 2 Dec 2024 14:38:11 -0500
Subject: [PATCH v1] isolationtester showing broken index-only bitmap heap scan

---
 .../expected/index-only-bitmapscan.out        | 28 ++++++
 src/test/isolation/isolation_schedule         |  1 +
 .../specs/index-only-bitmapscan.spec          | 85 +++++++++++++++++++
 3 files changed, 114 insertions(+)
 create mode 100644 src/test/isolation/expected/index-only-bitmapscan.out
 create mode 100644 src/test/isolation/specs/index-only-bitmapscan.spec

diff --git a/src/test/isolation/expected/index-only-bitmapscan.out b/src/test/isolation/expected/index-only-bitmapscan.out
new file mode 100644
index 00000000000..132ff1bda70
--- /dev/null
+++ b/src/test/isolation/expected/index-only-bitmapscan.out
@@ -0,0 +1,28 @@
+Parsed test spec with 2 sessions
+
+starting permutation: s2_vacuum s2_mod s1_begin s1_prepare s1_fetch_1 s2_vacuum s1_fetch_all s1_commit
+step s2_vacuum: VACUUM (TRUNCATE false) ios_bitmap;
+step s2_mod: 
+  DELETE FROM ios_bitmap WHERE a > 1;
+
+step s1_begin: BEGIN;
+step s1_prepare: 
+    DECLARE foo NO SCROLL CURSOR FOR SELECT row_number() OVER () FROM ios_bitmap WHERE a > 0 or b > 0;
+
+step s1_fetch_1: 
+    FETCH FROM foo;
+
+row_number
+----------
+         1
+(1 row)
+
+step s2_vacuum: VACUUM (TRUNCATE false) ios_bitmap;
+step s1_fetch_all: 
+    FETCH ALL FROM foo;
+
+row_number
+----------
+(0 rows)
+
+step s1_commit: COMMIT;
diff --git a/src/test/isolation/isolation_schedule b/src/test/isolation/isolation_schedule
index 143109aa4da..e3c669a29c7 100644
--- a/src/test/isolation/isolation_schedule
+++ b/src/test/isolation/isolation_schedule
@@ -17,6 +17,7 @@ test: partial-index
 test: two-ids
 test: multiple-row-versions
 test: index-only-scan
+test: index-only-bitmapscan
 test: predicate-lock-hot-tuple
 test: update-conflict-out
 test: deadlock-simple
diff --git a/src/test/isolation/specs/index-only-bitmapscan.spec b/src/test/isolation/specs/index-only-bitmapscan.spec
new file mode 100644
index 00000000000..9962b8dc531
--- /dev/null
+++ b/src/test/isolation/specs/index-only-bitmapscan.spec
@@ -0,0 +1,85 @@
+# index-only-bitmapscan test showing wrong results
+#
+setup
+{
+    -- by using a low fillfactor and a wide tuple we can get multiple blocks
+    -- with just few rows
+    CREATE TABLE ios_bitmap (a int NOT NULL, b int not null, pad char(1024) default '')
+    WITH (AUTOVACUUM_ENABLED = false, FILLFACTOR = 10);
+
+    INSERT INTO ios_bitmap SELECT g.i, g.i FROM generate_series(1, 10) g(i);
+
+    CREATE INDEX ios_bitmap_a ON ios_bitmap(a);
+    CREATE INDEX ios_bitmap_b ON ios_bitmap(b);
+}
+
+teardown
+{
+    DROP TABLE ios_bitmap;
+}
+
+
+session s1
+
+setup {
+    SET enable_seqscan = false;
+}
+
+step s1_begin { BEGIN; }
+step s1_commit { COMMIT; }
+
+
+# The test query uses an or between two indexes to ensure make it more likely
+# to use a bitmap index scan
+#
+# The row_number() hack is a way to have something returned (isolationtester
+# doesn't display empty rows) while still allowing for the index-only scan
+# optimization in bitmap heap scans, which requires an empty targetlist.
+step s1_prepare {
+    DECLARE foo NO SCROLL CURSOR FOR SELECT row_number() OVER () FROM ios_bitmap WHERE a > 0 or b > 0;
+}
+
+step s1_fetch_1 {
+    FETCH FROM foo;
+}
+
+step s1_fetch_all {
+    FETCH ALL FROM foo;
+}
+
+
+session s2
+
+# Don't delete row 1 so we have a row for the cursor to "rest" on.
+step s2_mod
+{
+  DELETE FROM ios_bitmap WHERE a > 1;
+}
+
+# Disable truncation, as otherwise we'll just wait for a timeout while trying
+# to acquire the lock
+step s2_vacuum  { VACUUM (TRUNCATE false) ios_bitmap; }
+
+permutation
+  # Vacuum first, to ensure VM exists, otherwise the bitmapscan will consider
+  # VM to be size 0, due to caching. Can't do that in setup because
+  s2_vacuum
+
+  # delete nearly all rows, to make issue visible
+  s2_mod
+  # create a cursor
+  s1_begin
+  s1_prepare
+
+  # fetch one row from the cursor, that ensures the index scan portion is done
+  # before the vacuum in the next step
+  s1_fetch_1
+
+  # with the bug this vacuum will mark pages as all-visible that the scan in
+  # the next step then considers all-visible, despite all rows from those
+  # pages having been removed.
+  s2_vacuum
+  # if this returns any rows, we're busted
+  s1_fetch_all
+
+  s1_commit
-- 
2.45.2.746.g06e570c0df.dirty

From 3b189a59b2e59220ed192d8dde4080659f76fec9 Mon Sep 17 00:00:00 2001
From: Peter Geoghegan <pg@bowt.ie>
Date: Mon, 2 Dec 2024 15:50:04 -0500
Subject: [PATCH v1] isolationtester showing broken index-only scans with GiST

---
 .../expected/index-only-gistscan.out          | 28 +++++++
 src/test/isolation/isolation_schedule         |  1 +
 .../isolation/specs/index-only-gistscan.spec  | 79 +++++++++++++++++++
 3 files changed, 108 insertions(+)
 create mode 100644 src/test/isolation/expected/index-only-gistscan.out
 create mode 100644 src/test/isolation/specs/index-only-gistscan.spec

diff --git a/src/test/isolation/expected/index-only-gistscan.out b/src/test/isolation/expected/index-only-gistscan.out
new file mode 100644
index 000000000..5c97d0f27
--- /dev/null
+++ b/src/test/isolation/expected/index-only-gistscan.out
@@ -0,0 +1,28 @@
+Parsed test spec with 2 sessions
+
+starting permutation: s2_vacuum s2_mod s1_begin s1_prepare s1_fetch_1 s2_vacuum s1_fetch_all s1_commit
+step s2_vacuum: VACUUM (TRUNCATE false) ios_bitmap;
+step s2_mod: 
+  DELETE FROM ios_bitmap WHERE a > 1;
+
+step s1_begin: BEGIN;
+step s1_prepare: 
+    DECLARE foo NO SCROLL CURSOR FOR SELECT a FROM ios_bitmap WHERE a > 0;
+
+step s1_fetch_1: 
+    FETCH FROM foo;
+
+a
+-
+1
+(1 row)
+
+step s2_vacuum: VACUUM (TRUNCATE false) ios_bitmap;
+step s1_fetch_all: 
+    FETCH ALL FROM foo;
+
+a
+-
+(0 rows)
+
+step s1_commit: COMMIT;
diff --git a/src/test/isolation/isolation_schedule b/src/test/isolation/isolation_schedule
index 143109aa4..e3c669a29 100644
--- a/src/test/isolation/isolation_schedule
+++ b/src/test/isolation/isolation_schedule
@@ -17,6 +17,7 @@ test: partial-index
 test: two-ids
 test: multiple-row-versions
 test: index-only-scan
+test: index-only-bitmapscan
 test: predicate-lock-hot-tuple
 test: update-conflict-out
 test: deadlock-simple
diff --git a/src/test/isolation/specs/index-only-gistscan.spec b/src/test/isolation/specs/index-only-gistscan.spec
new file mode 100644
index 000000000..f42bcd80f
--- /dev/null
+++ b/src/test/isolation/specs/index-only-gistscan.spec
@@ -0,0 +1,79 @@
+# index-only-scan test showing wrong results with GiST
+#
+setup
+{
+    -- by using a low fillfactor and a wide tuple we can get multiple blocks
+    -- with just few rows
+    create extension btree_gist;
+    CREATE TABLE ios_bitmap (a int NOT NULL, b int not null, pad char(1024) default '')
+    WITH (AUTOVACUUM_ENABLED = false, FILLFACTOR = 10);
+
+    INSERT INTO ios_bitmap SELECT g.i, g.i FROM generate_series(1, 10) g(i);
+
+    CREATE INDEX ios_gist_a ON ios_bitmap USING GIST (a);
+}
+
+teardown
+{
+    DROP TABLE ios_bitmap;
+}
+
+
+session s1
+
+setup {
+    SET enable_seqscan = false;
+    SET enable_bitmapscan = false;
+}
+
+step s1_begin { BEGIN; }
+step s1_commit { COMMIT; }
+
+step s1_prepare {
+    DECLARE foo NO SCROLL CURSOR FOR SELECT a FROM ios_bitmap WHERE a > 0;
+}
+
+step s1_fetch_1 {
+    FETCH FROM foo;
+}
+
+step s1_fetch_all {
+    FETCH ALL FROM foo;
+}
+
+
+session s2
+
+# Don't delete row 1 so we have a row for the cursor to "rest" on.
+step s2_mod
+{
+  DELETE FROM ios_bitmap WHERE a > 1;
+}
+
+# Disable truncation, as otherwise we'll just wait for a timeout while trying
+# to acquire the lock
+step s2_vacuum  { VACUUM (TRUNCATE false) ios_bitmap; }
+
+permutation
+  # Vacuum first, to ensure VM exists, otherwise the bitmapscan will consider
+  # VM to be size 0, due to caching. Can't do that in setup because
+  s2_vacuum
+
+  # delete nearly all rows, to make issue visible
+  s2_mod
+  # create a cursor
+  s1_begin
+  s1_prepare
+
+  # fetch one row from the cursor, that ensures the index scan portion is done
+  # before the vacuum in the next step
+  s1_fetch_1
+
+  # with the bug this vacuum will mark pages as all-visible that the scan in
+  # the next step then considers all-visible, despite all rows from those
+  # pages having been removed.
+  s2_vacuum
+  # if this returns any rows, we're busted
+  s1_fetch_all
+
+  s1_commit
-- 
2.45.2

From f5e90fc3ac68b8606cc14bba42d72d062c25eab1 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 v3 1/2] 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 7d06dad83fc..84a1f30aecb 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 09b9b394e0e..74c8befef6b 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,13 +948,10 @@ 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)
 {
 	uint32		flags = SO_TYPE_BITMAPSCAN | SO_ALLOW_PAGEMODE;
 
-	if (need_tuple)
-		flags |= SO_NEED_TUPLES;
-
 	return rel->rd_tableam->scan_begin(rel, snapshot, nkeys, key,
 									   NULL, flags);
 }
diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c
index 485525f4d64..905dd9d04a5 100644
--- a/src/backend/access/heap/heapam.c
+++ b/src/backend/access/heap/heapam.c
@@ -1056,8 +1056,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.
@@ -1173,19 +1171,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
@@ -1213,9 +1198,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 e817f8f8f84..0ea4f029793 100644
--- a/src/backend/access/heap/heapam_handler.c
+++ b/src/backend/access/heap/heapam_handler.c
@@ -2155,24 +2155,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;
 
 	/*
@@ -2287,16 +2269,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 be616683f98..e5d7fab4dbe 100644
--- a/src/backend/executor/nodeBitmapHeapscan.c
+++ b/src/backend/executor/nodeBitmapHeapscan.c
@@ -158,24 +158,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;
 		}
@@ -434,7 +420,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)
 				{
@@ -445,20 +430,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);
 			}
 		}
 
@@ -475,7 +447,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
@@ -502,15 +473,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

From 11c89a393ebf194d5d5f6617d6bd70f753f6da74 Mon Sep 17 00:00:00 2001
From: Andres Freund <andres@anarazel.de>
Date: Mon, 2 Dec 2024 14:38:11 -0500
Subject: [PATCH v3 2/2] isolationtester showing broken index-only bitmap heap
 scan

---
 .../expected/index-only-bitmapscan.out        | 28 ++++++
 src/test/isolation/isolation_schedule         |  1 +
 .../specs/index-only-bitmapscan.spec          | 85 +++++++++++++++++++
 3 files changed, 114 insertions(+)
 create mode 100644 src/test/isolation/expected/index-only-bitmapscan.out
 create mode 100644 src/test/isolation/specs/index-only-bitmapscan.spec

diff --git a/src/test/isolation/expected/index-only-bitmapscan.out b/src/test/isolation/expected/index-only-bitmapscan.out
new file mode 100644
index 00000000000..132ff1bda70
--- /dev/null
+++ b/src/test/isolation/expected/index-only-bitmapscan.out
@@ -0,0 +1,28 @@
+Parsed test spec with 2 sessions
+
+starting permutation: s2_vacuum s2_mod s1_begin s1_prepare s1_fetch_1 s2_vacuum s1_fetch_all s1_commit
+step s2_vacuum: VACUUM (TRUNCATE false) ios_bitmap;
+step s2_mod: 
+  DELETE FROM ios_bitmap WHERE a > 1;
+
+step s1_begin: BEGIN;
+step s1_prepare: 
+    DECLARE foo NO SCROLL CURSOR FOR SELECT row_number() OVER () FROM ios_bitmap WHERE a > 0 or b > 0;
+
+step s1_fetch_1: 
+    FETCH FROM foo;
+
+row_number
+----------
+         1
+(1 row)
+
+step s2_vacuum: VACUUM (TRUNCATE false) ios_bitmap;
+step s1_fetch_all: 
+    FETCH ALL FROM foo;
+
+row_number
+----------
+(0 rows)
+
+step s1_commit: COMMIT;
diff --git a/src/test/isolation/isolation_schedule b/src/test/isolation/isolation_schedule
index 143109aa4da..e3c669a29c7 100644
--- a/src/test/isolation/isolation_schedule
+++ b/src/test/isolation/isolation_schedule
@@ -17,6 +17,7 @@ test: partial-index
 test: two-ids
 test: multiple-row-versions
 test: index-only-scan
+test: index-only-bitmapscan
 test: predicate-lock-hot-tuple
 test: update-conflict-out
 test: deadlock-simple
diff --git a/src/test/isolation/specs/index-only-bitmapscan.spec b/src/test/isolation/specs/index-only-bitmapscan.spec
new file mode 100644
index 00000000000..9962b8dc531
--- /dev/null
+++ b/src/test/isolation/specs/index-only-bitmapscan.spec
@@ -0,0 +1,85 @@
+# index-only-bitmapscan test showing wrong results
+#
+setup
+{
+    -- by using a low fillfactor and a wide tuple we can get multiple blocks
+    -- with just few rows
+    CREATE TABLE ios_bitmap (a int NOT NULL, b int not null, pad char(1024) default '')
+    WITH (AUTOVACUUM_ENABLED = false, FILLFACTOR = 10);
+
+    INSERT INTO ios_bitmap SELECT g.i, g.i FROM generate_series(1, 10) g(i);
+
+    CREATE INDEX ios_bitmap_a ON ios_bitmap(a);
+    CREATE INDEX ios_bitmap_b ON ios_bitmap(b);
+}
+
+teardown
+{
+    DROP TABLE ios_bitmap;
+}
+
+
+session s1
+
+setup {
+    SET enable_seqscan = false;
+}
+
+step s1_begin { BEGIN; }
+step s1_commit { COMMIT; }
+
+
+# The test query uses an or between two indexes to ensure make it more likely
+# to use a bitmap index scan
+#
+# The row_number() hack is a way to have something returned (isolationtester
+# doesn't display empty rows) while still allowing for the index-only scan
+# optimization in bitmap heap scans, which requires an empty targetlist.
+step s1_prepare {
+    DECLARE foo NO SCROLL CURSOR FOR SELECT row_number() OVER () FROM ios_bitmap WHERE a > 0 or b > 0;
+}
+
+step s1_fetch_1 {
+    FETCH FROM foo;
+}
+
+step s1_fetch_all {
+    FETCH ALL FROM foo;
+}
+
+
+session s2
+
+# Don't delete row 1 so we have a row for the cursor to "rest" on.
+step s2_mod
+{
+  DELETE FROM ios_bitmap WHERE a > 1;
+}
+
+# Disable truncation, as otherwise we'll just wait for a timeout while trying
+# to acquire the lock
+step s2_vacuum  { VACUUM (TRUNCATE false) ios_bitmap; }
+
+permutation
+  # Vacuum first, to ensure VM exists, otherwise the bitmapscan will consider
+  # VM to be size 0, due to caching. Can't do that in setup because
+  s2_vacuum
+
+  # delete nearly all rows, to make issue visible
+  s2_mod
+  # create a cursor
+  s1_begin
+  s1_prepare
+
+  # fetch one row from the cursor, that ensures the index scan portion is done
+  # before the vacuum in the next step
+  s1_fetch_1
+
+  # with the bug this vacuum will mark pages as all-visible that the scan in
+  # the next step then considers all-visible, despite all rows from those
+  # pages having been removed.
+  s2_vacuum
+  # if this returns any rows, we're busted
+  s1_fetch_all
+
+  s1_commit
-- 
2.45.2

From 0eb1060da55d488717445878b364d1a255089657 Mon Sep 17 00:00:00 2001
From: Andres Freund <andres@anarazel.de>
Date: Mon, 2 Dec 2024 14:38:11 -0500
Subject: [PATCH v4 2/2] isolationtester showing broken index-only bitmap heap
 scan

---
 .../expected/index-only-bitmapscan.out        | 28 ++++++
 src/test/isolation/isolation_schedule         |  1 +
 .../specs/index-only-bitmapscan.spec          | 85 +++++++++++++++++++
 3 files changed, 114 insertions(+)
 create mode 100644 src/test/isolation/expected/index-only-bitmapscan.out
 create mode 100644 src/test/isolation/specs/index-only-bitmapscan.spec

diff --git a/src/test/isolation/expected/index-only-bitmapscan.out b/src/test/isolation/expected/index-only-bitmapscan.out
new file mode 100644
index 00000000000..132ff1bda70
--- /dev/null
+++ b/src/test/isolation/expected/index-only-bitmapscan.out
@@ -0,0 +1,28 @@
+Parsed test spec with 2 sessions
+
+starting permutation: s2_vacuum s2_mod s1_begin s1_prepare s1_fetch_1 s2_vacuum s1_fetch_all s1_commit
+step s2_vacuum: VACUUM (TRUNCATE false) ios_bitmap;
+step s2_mod: 
+  DELETE FROM ios_bitmap WHERE a > 1;
+
+step s1_begin: BEGIN;
+step s1_prepare: 
+    DECLARE foo NO SCROLL CURSOR FOR SELECT row_number() OVER () FROM ios_bitmap WHERE a > 0 or b > 0;
+
+step s1_fetch_1: 
+    FETCH FROM foo;
+
+row_number
+----------
+         1
+(1 row)
+
+step s2_vacuum: VACUUM (TRUNCATE false) ios_bitmap;
+step s1_fetch_all: 
+    FETCH ALL FROM foo;
+
+row_number
+----------
+(0 rows)
+
+step s1_commit: COMMIT;
diff --git a/src/test/isolation/isolation_schedule b/src/test/isolation/isolation_schedule
index 143109aa4da..e3c669a29c7 100644
--- a/src/test/isolation/isolation_schedule
+++ b/src/test/isolation/isolation_schedule
@@ -17,6 +17,7 @@ test: partial-index
 test: two-ids
 test: multiple-row-versions
 test: index-only-scan
+test: index-only-bitmapscan
 test: predicate-lock-hot-tuple
 test: update-conflict-out
 test: deadlock-simple
diff --git a/src/test/isolation/specs/index-only-bitmapscan.spec b/src/test/isolation/specs/index-only-bitmapscan.spec
new file mode 100644
index 00000000000..9962b8dc531
--- /dev/null
+++ b/src/test/isolation/specs/index-only-bitmapscan.spec
@@ -0,0 +1,85 @@
+# index-only-bitmapscan test showing wrong results
+#
+setup
+{
+    -- by using a low fillfactor and a wide tuple we can get multiple blocks
+    -- with just few rows
+    CREATE TABLE ios_bitmap (a int NOT NULL, b int not null, pad char(1024) default '')
+    WITH (AUTOVACUUM_ENABLED = false, FILLFACTOR = 10);
+
+    INSERT INTO ios_bitmap SELECT g.i, g.i FROM generate_series(1, 10) g(i);
+
+    CREATE INDEX ios_bitmap_a ON ios_bitmap(a);
+    CREATE INDEX ios_bitmap_b ON ios_bitmap(b);
+}
+
+teardown
+{
+    DROP TABLE ios_bitmap;
+}
+
+
+session s1
+
+setup {
+    SET enable_seqscan = false;
+}
+
+step s1_begin { BEGIN; }
+step s1_commit { COMMIT; }
+
+
+# The test query uses an or between two indexes to ensure make it more likely
+# to use a bitmap index scan
+#
+# The row_number() hack is a way to have something returned (isolationtester
+# doesn't display empty rows) while still allowing for the index-only scan
+# optimization in bitmap heap scans, which requires an empty targetlist.
+step s1_prepare {
+    DECLARE foo NO SCROLL CURSOR FOR SELECT row_number() OVER () FROM ios_bitmap WHERE a > 0 or b > 0;
+}
+
+step s1_fetch_1 {
+    FETCH FROM foo;
+}
+
+step s1_fetch_all {
+    FETCH ALL FROM foo;
+}
+
+
+session s2
+
+# Don't delete row 1 so we have a row for the cursor to "rest" on.
+step s2_mod
+{
+  DELETE FROM ios_bitmap WHERE a > 1;
+}
+
+# Disable truncation, as otherwise we'll just wait for a timeout while trying
+# to acquire the lock
+step s2_vacuum  { VACUUM (TRUNCATE false) ios_bitmap; }
+
+permutation
+  # Vacuum first, to ensure VM exists, otherwise the bitmapscan will consider
+  # VM to be size 0, due to caching. Can't do that in setup because
+  s2_vacuum
+
+  # delete nearly all rows, to make issue visible
+  s2_mod
+  # create a cursor
+  s1_begin
+  s1_prepare
+
+  # fetch one row from the cursor, that ensures the index scan portion is done
+  # before the vacuum in the next step
+  s1_fetch_1
+
+  # with the bug this vacuum will mark pages as all-visible that the scan in
+  # the next step then considers all-visible, despite all rows from those
+  # pages having been removed.
+  s2_vacuum
+  # if this returns any rows, we're busted
+  s1_fetch_all
+
+  s1_commit
-- 
2.45.2

From 46a2670a20ed2e55f2c6c8dce6c211496123e544 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 v4 1/2] Remove HeapBitmapScan'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               | 12 +---------
 src/backend/access/heap/heapam.c          | 29 -----------------------
 src/backend/access/heap/heapam_handler.c  | 28 ----------------------
 src/backend/executor/nodeBitmapHeapscan.c | 27 ++-------------------
 4 files changed, 3 insertions(+), 93 deletions(-)

diff --git a/src/include/access/heapam.h b/src/include/access/heapam.h
index 1640d9c32f7..e48fe434cd3 100644
--- a/src/include/access/heapam.h
+++ b/src/include/access/heapam.h
@@ -103,17 +103,7 @@ typedef struct BitmapHeapScanDescData
 {
 	HeapScanDescData rs_heap_base;
 
-	/*
-	 * These fields are only used for bitmap scans for the "skip fetch"
-	 * optimization. Bitmap scans needing no fields from the heap may skip
-	 * fetching an all visible block, instead using the number of tuples per
-	 * block reported by the bitmap to determine how many NULL-filled tuples
-	 * to return. They are common to parallel and serial BitmapHeapScans
-	 */
-
-	/* page of VM containing info for current block */
-	Buffer		rs_vmbuffer;
-	int			rs_empty_tuples_pending;
+	/* Holds no data */
 }			BitmapHeapScanDescData;
 typedef struct BitmapHeapScanDescData *BitmapHeapScanDesc;
 
diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c
index fa7935a0ed3..4714e2bfaea 100644
--- a/src/backend/access/heap/heapam.c
+++ b/src/backend/access/heap/heapam.c
@@ -1054,8 +1054,6 @@ heap_beginscan(Relation relation, Snapshot snapshot,
 	{
 		BitmapHeapScanDesc bscan = palloc(sizeof(BitmapHeapScanDescData));
 
-		bscan->rs_vmbuffer = InvalidBuffer;
-		bscan->rs_empty_tuples_pending = 0;
 		scan = (HeapScanDesc) bscan;
 	}
 	else
@@ -1182,24 +1180,6 @@ heap_rescan(TableScanDesc sscan, ScanKey key, bool set_params,
 	if (BufferIsValid(scan->rs_cbuf))
 		ReleaseBuffer(scan->rs_cbuf);
 
-	if (scan->rs_base.rs_flags & SO_TYPE_BITMAPSCAN)
-	{
-		BitmapHeapScanDesc bscan = (BitmapHeapScanDesc) scan;
-
-		/*
-		 * Reset empty_tuples_pending, a field only used by bitmap heap scan,
-		 * to avoid incorrectly emitting NULL-filled tuples from a previous
-		 * scan on rescan.
-		 */
-		bscan->rs_empty_tuples_pending = 0;
-
-		if (BufferIsValid(bscan->rs_vmbuffer))
-		{
-			ReleaseBuffer(bscan->rs_vmbuffer);
-			bscan->rs_vmbuffer = InvalidBuffer;
-		}
-	}
-
 	/*
 	 * The read stream is reset on rescan. This must be done before
 	 * initscan(), as some state referred to by read_stream_reset() is reset
@@ -1227,15 +1207,6 @@ heap_endscan(TableScanDesc sscan)
 	if (BufferIsValid(scan->rs_cbuf))
 		ReleaseBuffer(scan->rs_cbuf);
 
-	if (scan->rs_base.rs_flags & SO_TYPE_BITMAPSCAN)
-	{
-		BitmapHeapScanDesc bscan = (BitmapHeapScanDesc) sscan;
-
-		bscan->rs_empty_tuples_pending = 0;
-		if (BufferIsValid(bscan->rs_vmbuffer))
-			ReleaseBuffer(bscan->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 c0bec014154..00503fd3c82 100644
--- a/src/backend/access/heap/heapam_handler.c
+++ b/src/backend/access/heap/heapam_handler.c
@@ -2160,24 +2160,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, &bscan->rs_vmbuffer))
-	{
-		/* can't be lossy in the skip_fetch case */
-		Assert(tbmres->ntuples >= 0);
-		Assert(bscan->rs_empty_tuples_pending >= 0);
-
-		bscan->rs_empty_tuples_pending += tbmres->ntuples;
-
-		return true;
-	}
-
 	block = tbmres->blockno;
 
 	/*
@@ -2293,16 +2275,6 @@ heapam_scan_bitmap_next_tuple(TableScanDesc scan,
 	Page		page;
 	ItemId		lp;
 
-	if (bscan->rs_empty_tuples_pending > 0)
-	{
-		/*
-		 * If we don't have to fetch the tuple, just return nulls.
-		 */
-		ExecStoreAllNullTuple(slot);
-		bscan->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 be0d24d901b..49270634d54 100644
--- a/src/backend/executor/nodeBitmapHeapscan.c
+++ b/src/backend/executor/nodeBitmapHeapscan.c
@@ -442,7 +442,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)
 				{
@@ -453,20 +452,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);
 			}
 		}
 
@@ -483,7 +469,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
@@ -510,15 +495,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

From e664d1033a2866ff5dd126a62186fb646e63c54b Mon Sep 17 00:00:00 2001
From: Andres Freund <andres@anarazel.de>
Date: Mon, 2 Dec 2024 14:38:11 -0500
Subject: [PATCH v5 2/2] isolationtester showing broken index-only bitmap heap
 scan

---
 .../expected/index-only-bitmapscan.out        | 28 ++++++
 src/test/isolation/isolation_schedule         |  1 +
 .../specs/index-only-bitmapscan.spec          | 85 +++++++++++++++++++
 3 files changed, 114 insertions(+)
 create mode 100644 src/test/isolation/expected/index-only-bitmapscan.out
 create mode 100644 src/test/isolation/specs/index-only-bitmapscan.spec

diff --git a/src/test/isolation/expected/index-only-bitmapscan.out b/src/test/isolation/expected/index-only-bitmapscan.out
new file mode 100644
index 00000000000..132ff1bda70
--- /dev/null
+++ b/src/test/isolation/expected/index-only-bitmapscan.out
@@ -0,0 +1,28 @@
+Parsed test spec with 2 sessions
+
+starting permutation: s2_vacuum s2_mod s1_begin s1_prepare s1_fetch_1 s2_vacuum s1_fetch_all s1_commit
+step s2_vacuum: VACUUM (TRUNCATE false) ios_bitmap;
+step s2_mod: 
+  DELETE FROM ios_bitmap WHERE a > 1;
+
+step s1_begin: BEGIN;
+step s1_prepare: 
+    DECLARE foo NO SCROLL CURSOR FOR SELECT row_number() OVER () FROM ios_bitmap WHERE a > 0 or b > 0;
+
+step s1_fetch_1: 
+    FETCH FROM foo;
+
+row_number
+----------
+         1
+(1 row)
+
+step s2_vacuum: VACUUM (TRUNCATE false) ios_bitmap;
+step s1_fetch_all: 
+    FETCH ALL FROM foo;
+
+row_number
+----------
+(0 rows)
+
+step s1_commit: COMMIT;
diff --git a/src/test/isolation/isolation_schedule b/src/test/isolation/isolation_schedule
index 143109aa4da..e3c669a29c7 100644
--- a/src/test/isolation/isolation_schedule
+++ b/src/test/isolation/isolation_schedule
@@ -17,6 +17,7 @@ test: partial-index
 test: two-ids
 test: multiple-row-versions
 test: index-only-scan
+test: index-only-bitmapscan
 test: predicate-lock-hot-tuple
 test: update-conflict-out
 test: deadlock-simple
diff --git a/src/test/isolation/specs/index-only-bitmapscan.spec b/src/test/isolation/specs/index-only-bitmapscan.spec
new file mode 100644
index 00000000000..9962b8dc531
--- /dev/null
+++ b/src/test/isolation/specs/index-only-bitmapscan.spec
@@ -0,0 +1,85 @@
+# index-only-bitmapscan test showing wrong results
+#
+setup
+{
+    -- by using a low fillfactor and a wide tuple we can get multiple blocks
+    -- with just few rows
+    CREATE TABLE ios_bitmap (a int NOT NULL, b int not null, pad char(1024) default '')
+    WITH (AUTOVACUUM_ENABLED = false, FILLFACTOR = 10);
+
+    INSERT INTO ios_bitmap SELECT g.i, g.i FROM generate_series(1, 10) g(i);
+
+    CREATE INDEX ios_bitmap_a ON ios_bitmap(a);
+    CREATE INDEX ios_bitmap_b ON ios_bitmap(b);
+}
+
+teardown
+{
+    DROP TABLE ios_bitmap;
+}
+
+
+session s1
+
+setup {
+    SET enable_seqscan = false;
+}
+
+step s1_begin { BEGIN; }
+step s1_commit { COMMIT; }
+
+
+# The test query uses an or between two indexes to ensure make it more likely
+# to use a bitmap index scan
+#
+# The row_number() hack is a way to have something returned (isolationtester
+# doesn't display empty rows) while still allowing for the index-only scan
+# optimization in bitmap heap scans, which requires an empty targetlist.
+step s1_prepare {
+    DECLARE foo NO SCROLL CURSOR FOR SELECT row_number() OVER () FROM ios_bitmap WHERE a > 0 or b > 0;
+}
+
+step s1_fetch_1 {
+    FETCH FROM foo;
+}
+
+step s1_fetch_all {
+    FETCH ALL FROM foo;
+}
+
+
+session s2
+
+# Don't delete row 1 so we have a row for the cursor to "rest" on.
+step s2_mod
+{
+  DELETE FROM ios_bitmap WHERE a > 1;
+}
+
+# Disable truncation, as otherwise we'll just wait for a timeout while trying
+# to acquire the lock
+step s2_vacuum  { VACUUM (TRUNCATE false) ios_bitmap; }
+
+permutation
+  # Vacuum first, to ensure VM exists, otherwise the bitmapscan will consider
+  # VM to be size 0, due to caching. Can't do that in setup because
+  s2_vacuum
+
+  # delete nearly all rows, to make issue visible
+  s2_mod
+  # create a cursor
+  s1_begin
+  s1_prepare
+
+  # fetch one row from the cursor, that ensures the index scan portion is done
+  # before the vacuum in the next step
+  s1_fetch_1
+
+  # with the bug this vacuum will mark pages as all-visible that the scan in
+  # the next step then considers all-visible, despite all rows from those
+  # pages having been removed.
+  s2_vacuum
+  # if this returns any rows, we're busted
+  s1_fetch_all
+
+  s1_commit
-- 
2.45.2

From 71c770623725f16e66d4a302415e8c9c66bae2bc 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 v5 1/2] Remove HeapBitmapScan'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               | 12 +---------
 src/backend/access/heap/heapam.c          | 29 -----------------------
 src/backend/access/heap/heapam_handler.c  | 29 -----------------------
 src/backend/executor/nodeBitmapHeapscan.c | 27 ++-------------------
 4 files changed, 3 insertions(+), 94 deletions(-)

diff --git a/src/include/access/heapam.h b/src/include/access/heapam.h
index 1640d9c32f7..e48fe434cd3 100644
--- a/src/include/access/heapam.h
+++ b/src/include/access/heapam.h
@@ -103,17 +103,7 @@ typedef struct BitmapHeapScanDescData
 {
 	HeapScanDescData rs_heap_base;
 
-	/*
-	 * These fields are only used for bitmap scans for the "skip fetch"
-	 * optimization. Bitmap scans needing no fields from the heap may skip
-	 * fetching an all visible block, instead using the number of tuples per
-	 * block reported by the bitmap to determine how many NULL-filled tuples
-	 * to return. They are common to parallel and serial BitmapHeapScans
-	 */
-
-	/* page of VM containing info for current block */
-	Buffer		rs_vmbuffer;
-	int			rs_empty_tuples_pending;
+	/* Holds no data */
 }			BitmapHeapScanDescData;
 typedef struct BitmapHeapScanDescData *BitmapHeapScanDesc;
 
diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c
index fa7935a0ed3..4714e2bfaea 100644
--- a/src/backend/access/heap/heapam.c
+++ b/src/backend/access/heap/heapam.c
@@ -1054,8 +1054,6 @@ heap_beginscan(Relation relation, Snapshot snapshot,
 	{
 		BitmapHeapScanDesc bscan = palloc(sizeof(BitmapHeapScanDescData));
 
-		bscan->rs_vmbuffer = InvalidBuffer;
-		bscan->rs_empty_tuples_pending = 0;
 		scan = (HeapScanDesc) bscan;
 	}
 	else
@@ -1182,24 +1180,6 @@ heap_rescan(TableScanDesc sscan, ScanKey key, bool set_params,
 	if (BufferIsValid(scan->rs_cbuf))
 		ReleaseBuffer(scan->rs_cbuf);
 
-	if (scan->rs_base.rs_flags & SO_TYPE_BITMAPSCAN)
-	{
-		BitmapHeapScanDesc bscan = (BitmapHeapScanDesc) scan;
-
-		/*
-		 * Reset empty_tuples_pending, a field only used by bitmap heap scan,
-		 * to avoid incorrectly emitting NULL-filled tuples from a previous
-		 * scan on rescan.
-		 */
-		bscan->rs_empty_tuples_pending = 0;
-
-		if (BufferIsValid(bscan->rs_vmbuffer))
-		{
-			ReleaseBuffer(bscan->rs_vmbuffer);
-			bscan->rs_vmbuffer = InvalidBuffer;
-		}
-	}
-
 	/*
 	 * The read stream is reset on rescan. This must be done before
 	 * initscan(), as some state referred to by read_stream_reset() is reset
@@ -1227,15 +1207,6 @@ heap_endscan(TableScanDesc sscan)
 	if (BufferIsValid(scan->rs_cbuf))
 		ReleaseBuffer(scan->rs_cbuf);
 
-	if (scan->rs_base.rs_flags & SO_TYPE_BITMAPSCAN)
-	{
-		BitmapHeapScanDesc bscan = (BitmapHeapScanDesc) sscan;
-
-		bscan->rs_empty_tuples_pending = 0;
-		if (BufferIsValid(bscan->rs_vmbuffer))
-			ReleaseBuffer(bscan->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 e78682c3cef..014aecca51a 100644
--- a/src/backend/access/heap/heapam_handler.c
+++ b/src/backend/access/heap/heapam_handler.c
@@ -2167,25 +2167,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, &bscan->rs_vmbuffer))
-	{
-		/* can't be lossy in the skip_fetch case */
-		Assert(!tbmres->lossy);
-		Assert(bscan->rs_empty_tuples_pending >= 0);
-		Assert(noffsets > -1);
-
-		bscan->rs_empty_tuples_pending += noffsets;
-
-		return true;
-	}
-
 	block = tbmres->blockno;
 
 	/*
@@ -2304,16 +2285,6 @@ heapam_scan_bitmap_next_tuple(TableScanDesc scan,
 	Page		page;
 	ItemId		lp;
 
-	if (bscan->rs_empty_tuples_pending > 0)
-	{
-		/*
-		 * If we don't have to fetch the tuple, just return nulls.
-		 */
-		ExecStoreAllNullTuple(slot);
-		bscan->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 be0d24d901b..49270634d54 100644
--- a/src/backend/executor/nodeBitmapHeapscan.c
+++ b/src/backend/executor/nodeBitmapHeapscan.c
@@ -442,7 +442,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)
 				{
@@ -453,20 +452,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);
 			}
 		}
 
@@ -483,7 +469,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
@@ -510,15 +495,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

From 14dbf4bd4c03d8610c92cbe7020ec979bd55dd74 Mon Sep 17 00:00:00 2001
From: Andres Freund <andres@anarazel.de>
Date: Mon, 2 Dec 2024 14:38:11 -0500
Subject: [PATCH v6 2/2] isolationtester showing broken index-only bitmap heap
 scan

---
 .../expected/index-only-bitmapscan.out        | 28 ++++++
 src/test/isolation/isolation_schedule         |  1 +
 .../specs/index-only-bitmapscan.spec          | 85 +++++++++++++++++++
 3 files changed, 114 insertions(+)
 create mode 100644 src/test/isolation/expected/index-only-bitmapscan.out
 create mode 100644 src/test/isolation/specs/index-only-bitmapscan.spec

diff --git a/src/test/isolation/expected/index-only-bitmapscan.out b/src/test/isolation/expected/index-only-bitmapscan.out
new file mode 100644
index 00000000000..132ff1bda70
--- /dev/null
+++ b/src/test/isolation/expected/index-only-bitmapscan.out
@@ -0,0 +1,28 @@
+Parsed test spec with 2 sessions
+
+starting permutation: s2_vacuum s2_mod s1_begin s1_prepare s1_fetch_1 s2_vacuum s1_fetch_all s1_commit
+step s2_vacuum: VACUUM (TRUNCATE false) ios_bitmap;
+step s2_mod: 
+  DELETE FROM ios_bitmap WHERE a > 1;
+
+step s1_begin: BEGIN;
+step s1_prepare: 
+    DECLARE foo NO SCROLL CURSOR FOR SELECT row_number() OVER () FROM ios_bitmap WHERE a > 0 or b > 0;
+
+step s1_fetch_1: 
+    FETCH FROM foo;
+
+row_number
+----------
+         1
+(1 row)
+
+step s2_vacuum: VACUUM (TRUNCATE false) ios_bitmap;
+step s1_fetch_all: 
+    FETCH ALL FROM foo;
+
+row_number
+----------
+(0 rows)
+
+step s1_commit: COMMIT;
diff --git a/src/test/isolation/isolation_schedule b/src/test/isolation/isolation_schedule
index 143109aa4da..e3c669a29c7 100644
--- a/src/test/isolation/isolation_schedule
+++ b/src/test/isolation/isolation_schedule
@@ -17,6 +17,7 @@ test: partial-index
 test: two-ids
 test: multiple-row-versions
 test: index-only-scan
+test: index-only-bitmapscan
 test: predicate-lock-hot-tuple
 test: update-conflict-out
 test: deadlock-simple
diff --git a/src/test/isolation/specs/index-only-bitmapscan.spec b/src/test/isolation/specs/index-only-bitmapscan.spec
new file mode 100644
index 00000000000..9962b8dc531
--- /dev/null
+++ b/src/test/isolation/specs/index-only-bitmapscan.spec
@@ -0,0 +1,85 @@
+# index-only-bitmapscan test showing wrong results
+#
+setup
+{
+    -- by using a low fillfactor and a wide tuple we can get multiple blocks
+    -- with just few rows
+    CREATE TABLE ios_bitmap (a int NOT NULL, b int not null, pad char(1024) default '')
+    WITH (AUTOVACUUM_ENABLED = false, FILLFACTOR = 10);
+
+    INSERT INTO ios_bitmap SELECT g.i, g.i FROM generate_series(1, 10) g(i);
+
+    CREATE INDEX ios_bitmap_a ON ios_bitmap(a);
+    CREATE INDEX ios_bitmap_b ON ios_bitmap(b);
+}
+
+teardown
+{
+    DROP TABLE ios_bitmap;
+}
+
+
+session s1
+
+setup {
+    SET enable_seqscan = false;
+}
+
+step s1_begin { BEGIN; }
+step s1_commit { COMMIT; }
+
+
+# The test query uses an or between two indexes to ensure make it more likely
+# to use a bitmap index scan
+#
+# The row_number() hack is a way to have something returned (isolationtester
+# doesn't display empty rows) while still allowing for the index-only scan
+# optimization in bitmap heap scans, which requires an empty targetlist.
+step s1_prepare {
+    DECLARE foo NO SCROLL CURSOR FOR SELECT row_number() OVER () FROM ios_bitmap WHERE a > 0 or b > 0;
+}
+
+step s1_fetch_1 {
+    FETCH FROM foo;
+}
+
+step s1_fetch_all {
+    FETCH ALL FROM foo;
+}
+
+
+session s2
+
+# Don't delete row 1 so we have a row for the cursor to "rest" on.
+step s2_mod
+{
+  DELETE FROM ios_bitmap WHERE a > 1;
+}
+
+# Disable truncation, as otherwise we'll just wait for a timeout while trying
+# to acquire the lock
+step s2_vacuum  { VACUUM (TRUNCATE false) ios_bitmap; }
+
+permutation
+  # Vacuum first, to ensure VM exists, otherwise the bitmapscan will consider
+  # VM to be size 0, due to caching. Can't do that in setup because
+  s2_vacuum
+
+  # delete nearly all rows, to make issue visible
+  s2_mod
+  # create a cursor
+  s1_begin
+  s1_prepare
+
+  # fetch one row from the cursor, that ensures the index scan portion is done
+  # before the vacuum in the next step
+  s1_fetch_1
+
+  # with the bug this vacuum will mark pages as all-visible that the scan in
+  # the next step then considers all-visible, despite all rows from those
+  # pages having been removed.
+  s2_vacuum
+  # if this returns any rows, we're busted
+  s1_fetch_all
+
+  s1_commit
-- 
2.45.2

From 7544de9eb5d5122651f00878be8847c1c14a07a3 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 v6 1/2] Remove HeapBitmapScan'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               | 12 +----
 src/include/access/tableam.h              | 12 +----
 src/backend/access/heap/heapam.c          | 62 +++--------------------
 src/backend/access/heap/heapam_handler.c  | 44 +---------------
 src/backend/executor/nodeBitmapHeapscan.c | 15 +-----
 5 files changed, 13 insertions(+), 132 deletions(-)

diff --git a/src/include/access/heapam.h b/src/include/access/heapam.h
index 1640d9c32f7..e48fe434cd3 100644
--- a/src/include/access/heapam.h
+++ b/src/include/access/heapam.h
@@ -103,17 +103,7 @@ typedef struct BitmapHeapScanDescData
 {
 	HeapScanDescData rs_heap_base;
 
-	/*
-	 * These fields are only used for bitmap scans for the "skip fetch"
-	 * optimization. Bitmap scans needing no fields from the heap may skip
-	 * fetching an all visible block, instead using the number of tuples per
-	 * block reported by the bitmap to determine how many NULL-filled tuples
-	 * to return. They are common to parallel and serial BitmapHeapScans
-	 */
-
-	/* page of VM containing info for current block */
-	Buffer		rs_vmbuffer;
-	int			rs_empty_tuples_pending;
+	/* Holds no data */
 }			BitmapHeapScanDescData;
 typedef struct BitmapHeapScanDescData *BitmapHeapScanDesc;
 
diff --git a/src/include/access/tableam.h b/src/include/access/tableam.h
index b8cb1e744ad..8713e12cbfb 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;
 
 /*
@@ -920,13 +913,10 @@ 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)
 {
 	uint32		flags = SO_TYPE_BITMAPSCAN | SO_ALLOW_PAGEMODE;
 
-	if (need_tuple)
-		flags |= SO_NEED_TUPLES;
-
 	return rel->rd_tableam->scan_begin(rel, snapshot, nkeys, key,
 									   NULL, flags);
 }
diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c
index cedaa195cb6..c13288d83b7 100644
--- a/src/backend/access/heap/heapam.c
+++ b/src/backend/access/heap/heapam.c
@@ -314,31 +314,6 @@ bitmapheap_stream_read_next(ReadStream *pgsr, void *private_data,
 			tbmres->blockno >= hscan->rs_nblocks)
 			continue;
 
-		/*
-		 * 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 (!(sscan->rs_flags & SO_NEED_TUPLES) &&
-			!tbmres->recheck &&
-			VM_ALL_VISIBLE(sscan->rs_rd, tbmres->blockno, &bscan->rs_vmbuffer))
-		{
-			OffsetNumber offsets[TBM_MAX_TUPLES_PER_PAGE];
-			int			noffsets;
-
-			/* can't be lossy in the skip_fetch case */
-			Assert(!tbmres->lossy);
-			Assert(bscan->rs_empty_tuples_pending >= 0);
-
-			/*
-			 * We throw away the offsets, but this is the easiest way to get a
-			 * count of tuples.
-			 */
-			noffsets = tbm_extract_page_tuple(tbmres, offsets, TBM_MAX_TUPLES_PER_PAGE);
-			bscan->rs_empty_tuples_pending += noffsets;
-			continue;
-		}
-
 		return tbmres->blockno;
 	}
 
@@ -1123,9 +1098,10 @@ heap_beginscan(Relation relation, Snapshot snapshot,
 	if (flags & SO_TYPE_BITMAPSCAN)
 	{
 		BitmapHeapScanDesc bscan = palloc(sizeof(BitmapHeapScanDescData));
-
-		bscan->rs_vmbuffer = InvalidBuffer;
-		bscan->rs_empty_tuples_pending = 0;
+		/*
+		 * Bitmap Heap scans do not have any new fields that a normal Heap
+		 * Scan does not have, so no special initializations required here.
+		 */
 		scan = (HeapScanDesc) bscan;
 	}
 	else
@@ -1280,23 +1256,10 @@ heap_rescan(TableScanDesc sscan, ScanKey key, bool set_params,
 		scan->rs_cbuf = InvalidBuffer;
 	}
 
-	if (scan->rs_base.rs_flags & SO_TYPE_BITMAPSCAN)
-	{
-		BitmapHeapScanDesc bscan = (BitmapHeapScanDesc) scan;
-
-		/*
-		 * Reset empty_tuples_pending, a field only used by bitmap heap scan,
-		 * to avoid incorrectly emitting NULL-filled tuples from a previous
-		 * scan on rescan.
-		 */
-		bscan->rs_empty_tuples_pending = 0;
-
-		if (BufferIsValid(bscan->rs_vmbuffer))
-		{
-			ReleaseBuffer(bscan->rs_vmbuffer);
-			bscan->rs_vmbuffer = InvalidBuffer;
-		}
-	}
+	/*
+	 * SO_TYPE_BITMAPSCAN would be cleaned up here, but it does not hold any
+	 * additional data vs a normal HeapScan
+	 */
 
 	/*
 	 * The read stream is reset on rescan. This must be done before
@@ -1325,15 +1288,6 @@ heap_endscan(TableScanDesc sscan)
 	if (BufferIsValid(scan->rs_cbuf))
 		ReleaseBuffer(scan->rs_cbuf);
 
-	if (scan->rs_base.rs_flags & SO_TYPE_BITMAPSCAN)
-	{
-		BitmapHeapScanDesc bscan = (BitmapHeapScanDesc) sscan;
-
-		bscan->rs_empty_tuples_pending = 0;
-		if (BufferIsValid(bscan->rs_vmbuffer))
-			ReleaseBuffer(bscan->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 24d3765aa20..453470e5aee 100644
--- a/src/backend/access/heap/heapam_handler.c
+++ b/src/backend/access/heap/heapam_handler.c
@@ -2137,32 +2137,6 @@ heapam_scan_bitmap_next_tuple(TableScanDesc scan,
 	 */
 	while (hscan->rs_cindex >= hscan->rs_ntuples)
 	{
-		/*
-		 * Emit empty tuples before advancing to the next block
-		 */
-		if (bscan->rs_empty_tuples_pending > 0)
-		{
-			/*
-			 * If we don't have to fetch the tuple, just return nulls.
-			 */
-			ExecStoreAllNullTuple(slot);
-			bscan->rs_empty_tuples_pending--;
-
-			/*
-			 * We do not recheck all NULL tuples. Because the streaming read
-			 * API only yields TBMIterateResults for blocks actually fetched
-			 * from the heap, we must unset `recheck` ourselves here to ensure
-			 * correct results.
-			 *
-			 * Our read stream callback accrues a count of empty tuples to
-			 * emit and then emits them after emitting tuples from the next
-			 * fetched block. If no blocks need fetching, we'll emit the
-			 * accrued count at the end of the scan.
-			 */
-			*recheck = false;
-			return true;
-		}
-
 		/*
 		 * Returns false if the bitmap is exhausted and there are no further
 		 * blocks we need to scan.
@@ -2516,24 +2490,10 @@ BitmapHeapScanNextBlock(TableScanDesc scan,
 
 	if (BufferIsInvalid(hscan->rs_cbuf))
 	{
-		if (BufferIsValid(bscan->rs_vmbuffer))
-		{
-			ReleaseBuffer(bscan->rs_vmbuffer);
-			bscan->rs_vmbuffer = InvalidBuffer;
-		}
-
 		/*
-		 * The bitmap is exhausted. Now emit any remaining empty tuples. The
-		 * read stream API only returns TBMIterateResults for blocks actually
-		 * fetched from the heap. Our callback will accrue a count of empty
-		 * tuples to emit for all blocks we skipped fetching. So, if we skip
-		 * fetching heap blocks at the end of the relation (or no heap blocks
-		 * are fetched) we need to ensure we emit empty tuples before ending
-		 * the scan. We don't recheck empty tuples so ensure `recheck` is
-		 * unset.
+		 * The bitmap is exhausted.
 		 */
-		*recheck = false;
-		return bscan->rs_empty_tuples_pending > 0;
+		return false;
 	}
 
 	Assert(per_buffer_data);
diff --git a/src/backend/executor/nodeBitmapHeapscan.c b/src/backend/executor/nodeBitmapHeapscan.c
index 3e33360c0fc..bf24f3d7fe0 100644
--- a/src/backend/executor/nodeBitmapHeapscan.c
+++ b/src/backend/executor/nodeBitmapHeapscan.c
@@ -105,24 +105,11 @@ BitmapTableScanSetup(BitmapHeapScanState *node)
 	 */
 	if (!node->ss.ss_currentScanDesc)
 	{
-		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);
-
 		node->ss.ss_currentScanDesc =
 			table_beginscan_bm(node->ss.ss_currentRelation,
 							   node->ss.ps.state->es_snapshot,
 							   0,
-							   NULL,
-							   need_tuples);
+							   NULL);
 	}
 
 	node->ss.ss_currentScanDesc->st.rs_tbmiterator = tbmiterator;
-- 
2.45.2