Re: BitmapHeapScan streaming read user and prelim refactoring

On Sun, Mar 24, 2024 at 01:36:19PM +0100, Tomas Vondra wrote:
>
>
> On 3/23/24 01:26, Melanie Plageman wrote:
> > On Fri, Mar 22, 2024 at 08:22:11PM -0400, Melanie Plageman wrote:
> >> On Tue, Mar 19, 2024 at 02:33:35PM +0200, Heikki Linnakangas wrote:
> >>> On 18/03/2024 17:19, Melanie Plageman wrote:
> >>>> I've attached v7 rebased over this commit.
> >>>
> >>> If we delayed table_beginscan_bm() call further, after starting the TBM
> >>> iterator, we could skip it altogether when the iterator is empty.
> >>>
> >>> That's a further improvement, doesn't need to be part of this patch set.
> >>> Just caught my eye while reading this.
> >>
> >> Hmm. You mean like until after the first call to tbm_[shared]_iterate()?
> >> AFAICT, tbm_begin_iterate() doesn't tell us anything about whether or
> >> not the iterator is "empty". Do you mean cases when the bitmap has no
> >> blocks in it? It seems like we should be able to tell that from the
> >> TIDBitmap.
> >>
> >>>
> >>>> v7-0003-Push-BitmapHeapScan-skip-fetch-optimization-into-.patch
> >>>
> >>> I suggest to avoid the double negative with SO_CAN_SKIP_FETCH, and call the
> >>> flag e.g. SO_NEED_TUPLE.
> >>
> >> Agreed. Done in attached v8. Though I wondered if it was a bit weird
> >> that the flag is set in the common case and not set in the uncommon
> >> case...
> >
> > v8 actually attached this time
>
> I tried to run the benchmarks with v8, but unfortunately it crashes for
> me very quickly (I've only seen 0015 to crash, so I guess the bug is in
> that patch).
>
> The backtrace attached, this doesn't seem right:
>
> (gdb) p hscan->rs_cindex
> $1 = 543516018

Thanks for reporting this! I hadn't seen it crash on my machine, so I
didn't realize that I was no longer initializing rs_cindex and
rs_ntuples on the first call to heapam_bitmap_next_tuple() (since
heapam_bitmap_next_block() wasn't being called first). I've done this in
attached v9.

I haven't had a chance yet to reproduce the regressions you saw in the
streaming read user patch or to look closely at the performance results.
I don't anticipate the streaming read user will have any performance
differences in this v9 from v6, since I haven't yet rebased in Thomas'
latest streaming read API changes nor addressed any other potential
regression sources.

I tried rebasing in Thomas' latest version today and something is
causing a crash that I have yet to figure out. v10 of this patchset will
have his latest version once I get that fixed. I wanted to share this
version with what I think is a bug fix for the crash you saw first.

- Melanie