Re: BitmapHeapScan streaming read user and prelim refactoring

On Thu, Mar 14, 2024 at 03:32:04PM +0200, Heikki Linnakangas wrote:
> On 14/03/2024 12:55, Dilip Kumar wrote:
> > On Thu, Mar 14, 2024 at 4:07 PM Heikki Linnakangas  wrote:
> > > _SPI_execute_plan() has code to deal with the possibility that the
> > > active snapshot is not set. That seems fishy; do we really support SPI
> > > without any snapshot? I'm inclined to turn that into an error. I ran the
> > > regression tests with an "Assert(ActiveSnapshotSet())" there, and
> > > everything worked.
> > 
> > IMHO, we can call SPI_Connect() and SPI_Execute() from any C
> > extension, so I don't think there we can guarantee that the snapshot
> > must be set, do we?
> 
> I suppose, although the things you could do without a snapshot would be
> pretty limited. The query couldn't access any tables. Could it even look up
> functions in the parser? Not sure.
> 
> > Maybe for now we can just handle this specific case to remove the
> > snapshot serializing for the BitmapHeapScan as you are doing in the
> > patch.  After looking into the code your theory seems correct that we
> > are just copying the ActiveSnapshot while building the query
> > descriptor and from there we are copying into the Estate so logically
> > there should not be any reason for these two to be different.
> 
> Ok, committed that for now. Thanks for looking!

Attached v6 is rebased over your new commit. It also has the "fix" in
0010 which moves BitmapAdjustPrefetchIterator() back above
table_scan_bitmap_next_block(). I've also updated the Streaming Read API
commit (0013) to Thomas' v7 version from [1]. This has the update that
we theorize should address some of the regressions in the bitmapheapscan
streaming read user in 0014.

- Melanie

[1] https://www.postgresql.org/message-id/CA%2BhUKGLJi%2Bc5jB3j6UvkgMYHky-qu%2BLPCsiNahUGSa5Z4DvyVA%40mail.gmail.com