Re: BitmapHeapScan streaming read user and prelim refactoring

On Sun, Apr 7, 2024 at 12:17 AM Melanie Plageman
<melanieplageman(at)gmail(dot)com> wrote:
>
> After we decided not to pursue streaming bitmapheapscan for 17, I wanted
> to make sure we removed the prefetch code table AM violation -- since we
> weren't deleting that code. So what started out as me looking for a way
> to clean up one commit ended up becoming a much larger project. Sorry
> about that last minute code explosion! I do think there is a way to do
> it right and make it nice. Also that violation would be gone if we
> figure out how to get streaming bitmapheapscan behaving correctly.
>
> So, there's just more motivation to make streaming bitmapheapscan
> awesome for 18!

Attached v21 is the rest of the patches to make bitmap heap scan use
the read stream API.
Don't be alarmed by the 20 patches in the set. I tried to keep the
individual patches as small and easy to review as possible.

Patches 0001-0003 implement the async-friendly behavior needed both to
push down the VM lookups for prefetching and eventually to use the
read stream API.

Patches 0004-0006 add and make use of a common interface for the
shared and private (parallel and serial) bitmap iterator per Heikki's
suggestion in [1].

Patches 0008 - 0012 make new scan descriptors for bitmap table scans
and the heap AM implementation. It is not possible to remove the
layering violations mentioned for bitmap table scans in tableam.h
without passing more bitmap-specific parameters to
table_begin/end/rescan(). Because bitmap heap scans use a fairly
reduced set of the members in the HeapScanDescData, it made sense to
enable specialized scan descriptors for bitmap table scans.

0013 pushes the primary iterator setup down into heap code.

0014 and 0015 push all of the prefetch code down into heap AM code as
suggested by Heikki in [1].

0017 removes scan_bitmap_next_block() per Heikki's suggestion in [1].

After all of these patches, we've removed the layering violations
mentioned previously in tableam.h. There is no use of the visibility
map anymore in generic bitmap table scan code. Almost all
block-specific logic is gone. The table AMs own the iterator almost
completely.

The one relic of iterator ownership is that for parallel bitmap heap
scan, a single process must scan the index, construct the bitmap, and
call tbm_prepare_shared_iterate() to set up the iterator for all the
processes. I didn't see a way to push this down (because to build the
bitmap we have to scan the index and call ExecProcNode()). I wonder if
this creates an odd split of responsibilities. I could use some other
synchronization mechanism to communicate which process built the
bitmap in the generic bitmap table scan code and thus should set up
the iterator in the heap implementation, but that sounds like a pretty
bad idea. Also, I'm not sure how a non-block-based table AM would use
TBMIterators (or even TIDBitmap) anyway.

As a side note, I've started naming all new structs in the bitmap
table scan code with the `BitmapTableScan` convention. It seems like
it might be a good time to rename the executor node from
BitmapHeapScanState to BitmapTableScanState. I didn't because I
wondered if I also needed to rename the file (nodeBitmapHeapscan.c ->
nodeBitmapTablescan.c) and update all other places using the
`BitmapHeapScan` convention.

Patches 0018 and 0019 make some changes to the TIDBitmap API to
support having multiple TBMIterateResults at the same time instead of
reusing the same one when iterating. With async execution we may have
more than one TBMIterateResult at a time.

There is one MTODO in the whole patch set -- in 0019 -- related to
resetting state in the TBMIterateResult. See that patch for the full
question.

And, finally, 0020 uses the read stream API and removes all the
bespoke prefetching code from bitmap heap scan.

Assuming we all get to a happy place with the code up until 0020, the
next step is to go back and investigate the performance regression
with bitmap heap scan and the read stream API first reported by Tomas
Vondra in [2].

I'd be very happy for code review on any of the patches, answers to my
questions above, or help investigating the regression Tomas found.

- Melanie

[1] https://www.postgresql.org/message-id/5a172d1e-d69c-409a-b1fa-6521214c81c2%40iki.fi
[2] https://www.postgresql.org/message-id/5d5954ed-6f43-4f1a-8e19-ece75b2b7362@enterprisedb.com