Re: BitmapHeapScan streaming read user and prelim refactoring

On Tue, May 14, 2024 at 4:05 PM Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org> wrote:
>
> On 2024-May-14, Tomas Vondra wrote:
>
> I wonder why it resets enable_indexscan at all. I see that this query
> first tries a seqscan, then if you disable that it tries an index only
> scan, and if you disable that you get the expected bitmap indexscan.
> But an indexscan doesn't seem to be in the cards.

Ah, yes. That is true. I think I added that when, in an older version
of the test, I had a query that did try an index scan before bitmap
heap scan. I've removed that guc from the attached v7.

> > IMHO if the test requires a specific plan, it's better to do an actual
> > "explain (rows off, costs off)" to check that.
>
> That's already in the patch, right?

Yep.

> I do wonder how do we _know_ that the test is testing what it wants to
> test:
> QUERY PLAN
> ─────────────────────────────────────────────────────────
> Nested Loop Anti Join
> -> Seq Scan on skip_fetch t1
> -> Materialize
> -> Bitmap Heap Scan on skip_fetch t2
> Recheck Cond: (a = 1)
> -> Bitmap Index Scan on skip_fetch_a_idx
> Index Cond: (a = 1)
>
> Is it because of the shape of the index condition? Maybe it's worth
> explaining in the comments for the tests.

There is a comment in the test that explains what it is exercising and
how. We include the explain output (the plan) to ensure it is still
using a bitmap heap scan. The test exercises the skip fetch
optimization in bitmap heap scan when not all of the inner tuples are
emitted.

Without the patch, the test fails, so it is protection against someone
adding back that assert in the future. It is not protection against
someone deleting the line
scan->rs_empty_tuples_pending = 0
That is, it doesn't verify that the empty unused tuples count is
discarded. Do you think that is necessary?

- Melanie