Re: BitmapHeapScan streaming read user and prelim refactoring

On Sun, Mar 24, 2024 at 2:22 PM Tomas Vondra
<tomas.vondra@enterprisedb.com> wrote:
>
> On 3/24/24 18:38, Melanie Plageman wrote:
> > 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.
>
> So you tried to reproduce it and didn't hit the issue? Or didn't have
> time to look into that yet? FWIW with v7 it failed almost immediately
> (only a couple queries until hitting one triggering the issue), but v9
> that's not the case (hundreds of queries without an error).

I haven't started trying to reproduce it yet.

> I however wonder what the plan with these patches is - do we still plan
> to get some of this into v17? It seems to me we're getting uncomfortably
> close to the end of the cycle, with a fairly incomplete idea of how it
> affects performance.
>
> Which is why I've been focusing more on the refactoring patches (up to
> 0015), to make sure those don't cause regressions if committed. And I
> think that's generally true.

Thank you for testing the refactoring patches with this in mind! Out
of the refactoring patches, I think there is a subset of them that
have independent value without the streaming read user. I think it is
worth committing the first few patches because they remove a table AM
layering violation. IMHO, all of the patches up to "Make
table_scan_bitmap_next_block() async friendly" make the code nicer and
better. And, if folks like the patch "Remove
table_scan_bitmap_next_block()", then I think I could rebase that back
in on top of "Make table_scan_bitmap_next_block() async friendly".
This would mean table AMs would only have to implement one callback
(table_scan_bitmap_next_tuple()) which I also think is a net
improvement and simplification.

The other refactoring patches may not be interesting without the
streaming read user.

> But for the main StreamingRead API the situation is very different.

My intent for the bitmapheapscan streaming read user was to get it
into 17, but I'm not sure that looks likely. The main issues Thomas is
looking into right now are related to regressions for a fully cached
scan (noticeable with the pg_prewarm streaming read user). With all of
these fixed, I anticipate we will still see enough behavioral
differences with the bitmapheap scan streaming read user that it may
not be committable in time. Though, I have yet to work on reproducing
the regressions with the BHS streaming read user mostly because I was
focused on getting the refactoring ready and not as much because the
streaming read API is unstable.

- Melanie