Re: Incorrect result of bitmap heap scan. - Mailing list pgsql-hackers

Hi,

On 2025-04-02 18:58:11 +0200, Matthias van de Meent wrote:
> And here it is, v6 of the patchset, rebased up to master @ 764d501d.

Thanks!

Does anybody have an opinion about how non-invasive to be in the
back-branches? The minimal version is something like this diff:

diff --git i/src/backend/executor/nodeBitmapHeapscan.c w/src/backend/executor/nodeBitmapHeapscan.c
index 6b48a6d8350..0bd8b3404c1 100644
--- i/src/backend/executor/nodeBitmapHeapscan.c
+++ w/src/backend/executor/nodeBitmapHeapscan.c
@@ -187,6 +187,19 @@ BitmapHeapNext(BitmapHeapScanState *node)
         {
             bool        need_tuples = false;

+            /*
+             * Unfortunately it turns out that the below 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 would lead to a !need_tuples scan returning too
+             * many tuples.
+             *
+             * In the back-branches, we therefore simply disable the
+             * optimization. Removing all the relevant code would be too
+             * invasive (and a major backpatching pain).
+             */
+#ifdef NOT_ANYMORE
             /*
              * We can potentially skip fetching heap pages if we do not need
              * any columns of the table, either for checking non-indexable
@@ -197,6 +210,7 @@ BitmapHeapNext(BitmapHeapScanState *node)
              */
             need_tuples = (node->ss.ps.plan->qual != NIL ||
                            node->ss.ps.plan->targetlist != NIL);
+#endif


But it seems a bit weird to continue checking SO_NEED_TUPLES (which is what
need_tuples ends up as) in other parts of the code. But it's less work to
backpatch that way.  Obviously we can't remove the relevant struct fields in
the backbranches.



Other notes:

- Should we commit the test showing that the naive implementation of
  index-only-bitmap-heapscan is broken, in case somebody wants to re-introduce
  it?

  If so, I think we should not backpatch the test. If it turns out to not be
  stable, it's a pain to fix test stability issues over multiple branches.

  And the patch would need a bit of comment editing to explain that, easy
  enough.


- We have some superfluous includes in nodeBitmapHeapscan.c - but I think
  that's not actually the fault of this patch. Seems the read-stream patch
  should have removed the at least the includes of visibilitymap.h, bufmgr.h
  and spccache.h?  And b09ff53667f math.h...


Greetings,

Andres Freund