From 71c770623725f16e66d4a302415e8c9c66bae2bc Mon Sep 17 00:00:00 2001 From: Matthias van de Meent 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 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