Re: Incorrect result of bitmap heap scan.

On Wed, 2 Apr 2025 at 19:48, Andres Freund <andres@anarazel.de> wrote:
>
> 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:
[...]
> 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.

A minimal version seems fine to me.

> 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?

I'd prefer that, yes.

>   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.

That's fair. But if the reason for not adding a test is potential
instability we could just as well add it now and remove it if it
actually proves to be unstable.

> - 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...

I have no strong opinion about this.

Thank you for picking this up!

Kind regards,

Matthias van de Meent
Neon (https://neon.tech)