Re: BitmapHeapScan streaming read user and prelim refactoring

On Sun, Apr 7, 2024 at 7:38 AM Tomas Vondra
<tomas(dot)vondra(at)enterprisedb(dot)com> wrote:
>
>
>
> On 4/7/24 06:17, Melanie Plageman wrote:
> > On Sun, Apr 07, 2024 at 02:27:43AM +0200, Tomas Vondra wrote:
> >> On 4/6/24 23:34, Melanie Plageman wrote:
> >>> ...
> >>>>
> >>>> I realized it makes more sense to add a FIXME (I used XXX. I'm not when
> >>>> to use what) with a link to the message where Andres describes why he
> >>>> thinks it is a bug. If we plan on fixing it, it is good to have a record
> >>>> of that. And it makes it easier to put a clear and accurate comment.
> >>>> Done in 0009.
> >>>>
> >>>>> OK, thanks. If think 0001-0008 are ready to go, with some minor tweaks
> >>>>> per above (tuple vs. tuples etc.), and the question about the recheck
> >>>>> flag. If you can do these tweaks, I'll get that committed today and we
> >>>>> can try to get a couple more patches in tomorrow.
> >>>
> >>> Attached v19 rebases the rest of the commits from v17 over the first
> >>> nine patches from v18. All patches 0001-0009 are unchanged from v18. I
> >>> have made updates and done cleanup on 0010-0021.
> >>>
> >>
> >> I've pushed 0001-0005, I'll get back to this tomorrow and see how much
> >> more we can get in for v17.
> >
> > Thanks! I thought about it a bit more, and I got worried about the
> >
> > Assert(scan->rs_empty_tuples_pending == 0);
> >
> > in heap_rescan() and heap_endscan().
> >
> > I was worried if we don't complete the scan it could end up tripping
> > incorrectly.
> >
> > I tried to come up with a query which didn't end up emitting all of the
> > tuples on the page (using a LIMIT clause), but I struggled to come up
> > with an example that qualified for the skip fetch optimization and also
> > returned before completing the scan.
> >
> > I could work a bit harder tomorrow to try and come up with something.
> > However, I think it might be safer to just change these to:
> >
> > scan->rs_empty_tuples_pending = 0
> >
>
> Hmmm, good point. I haven't tried, but wouldn't something like "SELECT 1
> FROM t WHERE column = X LIMIT 1" do the trick? Probably in a join, as a
> correlated subquery?

Unfortunately (or fortunately, I guess) that exact thing won't work
because even constant values in the target list disqualify it for the
skip fetch optimization.

Being a bit too lazy to look at planner code this morning, I removed
the target list requirement like this:

- need_tuples = (node->ss.ps.plan->qual != NIL ||
- node->ss.ps.plan->targetlist != NIL);
+ need_tuples = (node->ss.ps.plan->qual != NIL);

And can easily trip the assert with this:

create table foo (a int);
insert into foo select i from generate_series(1,10)i;
create index on foo(a);
vacuum foo;
select 1 from (select 2 from foo limit 3);

Anyway, I don't know if we could find a query that does actually hit
this. The only bitmap heap scan queries in the regress suite that meet
the
BitmapHeapScanState->ss.ps.plan->targetlist == NIL
condition are aggregates (all are count(*)).

I'll dig a bit more later, but do you think this is worth adding an
open item for? Even though I don't have a repro yet?

- Melanie