Re: BitmapHeapScan streaming read user and prelim refactoring

On Sun, Mar 24, 2024 at 06:37:20PM -0400, Melanie Plageman wrote:
> On Sun, Mar 24, 2024 at 5:59 PM Tomas Vondra
> <tomas(dot)vondra(at)enterprisedb(dot)com> wrote:
> >
> > BTW when you say "up to 'Make table_scan_bitmap_next_block() async
> > friendly'" do you mean including that patch, or that this is the first
> > patch that is not one of the independently useful patches.
>
> I think the code is easier to understand with "Make
> table_scan_bitmap_next_block() async friendly". Prior to that commit,
> table_scan_bitmap_next_block() could return false even when the bitmap
> has more blocks and expects the caller to handle this and invoke it
> again. I think that interface is very confusing. The downside of the
> code in that state is that the code for prefetching is still in the
> BitmapHeapNext() code and the code for getting the current block is in
> the heap AM-specific code. I took a stab at fixing this in v9's 0013,
> but the outcome wasn't very attractive.
>
> What I will do tomorrow is reorder and group the commits such that all
> of the commits that are useful independent of streaming read are first
> (I think 0014 and 0015 are independently valuable but they are on top
> of some things that are only useful to streaming read because they are
> more recently requested changes). I think I can actually do a bit of
> simplification in terms of how many commits there are and what is in
> each. Just to be clear, v9 is still reviewable. I am just going to go
> back and change what is included in each commit.

So, attached v10 does not include the new version of streaming read API.
I focused instead on the refactoring patches commit regrouping I
mentioned here.

I realized "Remove table_scan_bitmap_next_block()" can't easily be moved
down below "Push BitmapHeapScan prefetch code into heapam.c" because we
have to do BitmapAdjustPrefetchTarget() and
BitmapAdjustPrefetchIterator() on either side of getting the next block
(via table_scan_bitmap_next_block()).

"Push BitmapHeapScan prefetch code into heapam.c" isn't very nice
because it adds a lot of bitmapheapscan specific members to
TableScanDescData and HeapScanDescData. I thought about wrapping all of
those members in some kind of BitmapHeapScanTableState struct -- but I
don't like that because the members are spread out across
HeapScanDescData and TableScanDescData so not all of them would go in
BitmapHeapScanTableState. I could move the ones I put in
HeapScanDescData back into TableScanDescData and then wrap that in a
BitmapHeapScanTableState. I haven't done that in this version.

I did manage to move "Unify parallel and serial BitmapHeapScan iterator
interfaces" down below the line of patches which are only useful if
the streaming read user also goes in.

In attached v10, all patches up to and including "Unify parallel and
serial BitmapHeapScan iterator interfaces" (0010) are proposed for
master with or without the streaming read API.

0010 does add additional indirection and thus pointer dereferencing for
accessing the iterators, which doesn't feel good. But, it does simplify
the code.

Perhaps it is worth renaming the existing TableScanDescData->rs_parallel
(a ParallelTableScanDescData) to something like rs_seq_parallel. It is
only for sequential scans and scans of tables when building indexes but
the comments say it is for parallel scans in general. There is a similar
member in HeapScanDescData called rs_parallelworkerdata.

- Melanie