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