Remove HeapBitmapScan's skip_fetch optimization

The optimization does not take the removal of TIDs by a concurrent vacuum into
account. The concurrent vacuum can remove dead TIDs and make pages ALL_VISIBLE
while those dead TIDs are referenced in the bitmap. This can lead to a
skip_fetch scan returning too many tuples.

It likely would be possible to implement this optimization safely, but we
don't have the necessary infrastructure in place. Nor is it clear that it's
worth building that infrastructure, given how limited the skip_fetch
optimization is.

In the backbranches we just disable the optimization by always passing
need_tuples=true to table_beginscan_bm(). We can't perform API/ABI changes in
the backbranches and we want to make the change as minimal as possible.

Author: Matthias van de Meent <boekewurm+postgres@gmail.com>
Reported-By: Konstantin Knizhnik <knizhnik@garret.ru>
Discussion: https://postgr.es/m/CAEze2Wg3gXXZTr6_rwC+s4-o2ZVFB5F985uUSgJTsECx6AmGcQ@mail.gmail.com
Backpatch-through: 13

1 files changed, 8 insertions, 53 deletions

diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c
index cedaa195cb6..5b3fe4a1d3b 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;
 	}
 
@@ -1124,8 +1099,10 @@ heap_beginscan(Relation relation, Snapshot snapshot,
 	{
 		BitmapHeapScanDesc bscan = palloc(sizeof(BitmapHeapScanDescData));
 
-		bscan->rs_vmbuffer = InvalidBuffer;
-		bscan->rs_empty_tuples_pending = 0;
+		/*
+		 * Bitmap Heap scans do not have any fields that a normal Heap Scan
+		 * does not have, so no special initializations required here.
+		 */
 		scan = (HeapScanDesc) bscan;
 	}
 	else
@@ -1280,23 +1257,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 +1289,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.
 	 */