Re: BitmapHeapScan streaming read user and prelim refactoring

On Wed, Jun 19, 2024 at 12:38 PM Tomas Vondra
<tomas.vondra@enterprisedb.com> wrote:
>
>
>
> On 6/19/24 17:55, Melanie Plageman wrote:
> > On Tue, Jun 18, 2024 at 6:02 PM Tomas Vondra
> > <tomas.vondra@enterprisedb.com> wrote:
>
> > From your v22b-0017-review.patch
> >
> > diff --git a/src/include/access/relscan.h b/src/include/access/relscan.h
> > index 036ef29e7d5..9c711ce0eb0 100644
> > --- a/src/include/access/relscan.h
> > +++ b/src/include/access/relscan.h
> > @@ -52,6 +52,13 @@ typedef struct TableScanDescData
> >  } TableScanDescData;
> >  typedef struct TableScanDescData *TableScanDesc;
> >
> > +/*
> > + * XXX I don't understand why we should have this special node if we
> > + * don't have special nodes for other scan types.
> >
> > In this case, up until the final commit (using the read stream
> > interface), there are six fields required by bitmap heap scan that are
> > not needed by any other user of HeapScanDescData. There are also
> > several members of HeapScanDescData that are not needed by bitmap heap
> > scans and all of the setup in initscan() for those fields is not
> > required for bitmap heap scans.
> >
> > Also, because the BitmapHeapScanDesc needs information like the
> > ParallelBitmapHeapState and prefetch_maximum (for use in prefetching),
> > the scan_begin() callback would have to take those as parameters. I
> > thought adding so much bitmap table scan-specific information to the
> > generic table scan callbacks was a bad idea.
> >
> > Once we add the read stream API code, the number of fields required
> > for bitmap heap scan that are in the scan descriptor goes down to
> > three. So, perhaps we could justify having that many bitmap heap
> > scan-specific fields in the HeapScanDescData.
> >
> > Though, I actually think we could start moving toward having
> > specialized scan descriptors if the requirements for that scan type
> > are appreciably different. I can't think of new code that would be
> > added to the HeapScanDescData that would have to be duplicated over to
> > specialized scan descriptors.
> >
> > With the final read stream state, I can see the argument for bloating
> > the HeapScanDescData with three extra members and avoiding making new
> > scan descriptors. But, for the intermediate patches which have all of
> > the bitmap prefetch members pushed down into the HeapScanDescData, I
> > think it is really not okay. Six members only for bitmap heap scans
> > and two bitmap-specific members to begin_scan() seems bad.
> >
> > What I thought we plan to do is commit the refactoring patches
> > sometime after the branch for 18 is cut and leave the final read
> > stream patch uncommitted so we can do performance testing on it. If
> > you think it is okay to have the six member bloated HeapScanDescData
> > in master until we push the read stream code, I am okay with removing
> > the BitmapTableScanDesc and BitmapHeapScanDesc.
> >
>
> I admit I don't have a very good idea what the ideal / desired state
> look like. My comment is motivated solely by the feeling that it seems
> strange to have one struct serving most scan types, and then a special
> struct for one particular scan type ...

I see what you are saying. We could make BitmapTableScanDesc inherit
from TableScanDescData which would be similar to what we do with other
things like the executor scan nodes themselves. We would waste space
and LOC with initializing the unneeded members, but it might seem less
weird.

Whether we want the specialized scan descriptors at all is probably
the bigger question, though.

The top level BitmapTableScanDesc is motivated by wanting fewer bitmap
table scan specific members passed to scan_begin(). And the
BitmapHeapScanDesc is motivated by this plus wanting to avoid bloating
the HeapScanDescData.

If you look at at HEAD~1 (with my patches applied) and think you would
be okay with
1) the contents of the BitmapHeapScanDesc being in the HeapScanDescData and
2) the extra bitmap table scan-specific parameters in scan_begin_bm()
being passed to scan_begin()

then I will remove the specialized scan descriptors.

The final state (with the read stream) will still have three bitmap
heap scan-specific members in the HeapScanDescData.

Would it help if I do a version like this so you can see what it is like?

> > + * XXX Also, maybe this should do the naming convention with Data at
> > + * the end (mostly for consistency).
> > + */
> >  typedef struct BitmapTableScanDesc
> >  {
> >      Relation    rs_rd;            /* heap relation descriptor */
> >
> > I really want to move away from these Data typedefs. I find them so
> > confusing as a developer, but it's hard to justify ripping out the
> > existing ones because of code churn. If we add new scan descriptors, I
> > had really hoped to start using a different pattern.
> >
>
> Perhaps, I understand that. I'm not a huge fan of Data structs myself,
> but I'm not sure it's a great idea to do both things in the same area of
> code. That's guaranteed to be confusing for everyone ...
>
> If we want to move away from that, I'd rather rename the nearby structs
> and accept the code churn.

Makes sense. I'll do a patch to get rid of the typedefs and rename
TableScanDescData -> TableScanDesc (also for the heap scan desc) if we
end up keeping the specialized scan descriptors.

> > From your v22b-0025-review.patch
> >
> > diff --git a/src/backend/access/table/tableamapi.c
> > b/src/backend/access/table/tableamapi.c
> > index a47527d490a..379b7df619e 100644
> > --- a/src/backend/access/table/tableamapi.c
> > +++ b/src/backend/access/table/tableamapi.c
> > @@ -91,6 +91,9 @@ GetTableAmRoutine(Oid amhandler)
> >
> >      Assert(routine->relation_estimate_size != NULL);
> >
> > +    /* XXX shouldn't this check that _block is not set without _tuple?
> > +     * Also, the commit message says _block is "local helper" but then
> > +     * why would it be part of TableAmRoutine? */
> >      Assert(routine->scan_sample_next_block != NULL);
> >      Assert(routine->scan_sample_next_tuple != NULL);
> >
> > scan_bitmap_next_block() is removed as a table AM callback here, so we
> > don't check if it is set. We do still check if scan_bitmap_next_tuple
> > is set if amgetbitmap is not NULL. heapam_scan_bitmap_next_block() is
> > now a local helper for heapam_scan_bitmap_next_tuple(). Perhaps I
> > should change the name to something like heap_scan_bitmap_next_block()
> > to make it clear it is not an implementation of a table AM callback?
> >
>
> I'm quite confused by this. How could it not be am AM callback when it's
> in the AM routine?

I removed the callback for getting the next block. Now,
BitmapHeapNext() just calls table_scan_bitmap_next_tuple() and the
table AM is responsible for advancing to the next block when the
current block is out of tuples. In the new code structure, there isn't
much point in having a separate table_scan_bitmap_next_block()
callback. Advancing the iterator to get the next block assignment is
already pushed into the table AM itself. So, getting the next block
can be an internal detail of how the table AM gets the next tuple.
Heikki actually originally suggested it, and I thought it made sense.

- Melanie