On Tue, Feb 13, 2024 at 11:34 PM Mark Dilger <mark.dil...@enterprisedb.com> wrote: > > > On Feb 13, 2024, at 3:11 PM, Melanie Plageman <melanieplage...@gmail.com> > > wrote: > > Thanks for the patch... > > > Attached is a patch set which refactors BitmapHeapScan such that it > > can use the streaming read API [1]. It also resolves the long-standing > > FIXME in the BitmapHeapScan code suggesting that the skip fetch > > optimization should be pushed into the table AMs. Additionally, it > > moves table scan initialization to after the index scan and bitmap > > initialization. > > > > patches 0001-0002 are assorted cleanup needed later in the set. > > patches 0003 moves the table scan initialization to after bitmap creation > > patch 0004 is, I think, a bug fix. see [2]. > > patches 0005-0006 push the skip fetch optimization into the table AMs > > patches 0007-0009 change the control flow of BitmapHeapNext() to match > > that required by the streaming read API > > patch 0010 is the streaming read code not yet in master > > patch 0011 is the actual bitmapheapscan streaming read user. > > > > patches 0001-0009 apply on top of master but 0010 and 0011 must be > > applied on top of a commit before a 21d9c3ee4ef74e2 (until a rebased > > version of the streaming read API is on the mailing list). > > I followed your lead and applied them to > 6a8ffe812d194ba6f4f26791b6388a4837d17d6c. `make check` worked fine, though I > expect you know that already.
Thanks for taking a look! > > The caveat is that these patches introduce breaking changes to two > > table AM functions for bitmapheapscan: table_scan_bitmap_next_block() > > and table_scan_bitmap_next_tuple(). > > You might want an independent perspective on how much of a hassle those > breaking changes are, so I took a stab at that. Having written a custom > proprietary TAM for postgresql 15 here at EDB, and having ported it and > released it for postgresql 16, I thought I'd try porting it to the the above > commit with your patches. Even without your patches, I already see breaking > changes coming from commit f691f5b80a85c66d715b4340ffabb503eb19393e, which > creates a similar amount of breakage for me as does your patches. Dealing > with the combined breakage might amount to a day of work, including testing, > half of which I think I've already finished. In other words, it doesn't seem > like a big deal. > > Were postgresql 17 shaping up to be compatible with TAMs written for 16, your > patch would change that qualitatively, but since things are already > incompatible, I think you're in the clear. Oh, good to know! I'm very happy to have the perspective of a table AM author. Just curious, did your table AM implement table_scan_bitmap_next_block() and table_scan_bitmap_next_tuple(), and, if so, did you use the TBMIterateResult? Since it is not used in BitmapHeapNext() in my version, table AMs would have to change how they use TBMIterateResults anyway. But I assume they could add it to a table AM specific scan descriptor if they want access to a TBMIterateResult of their own making in both table_san_bitmap_next_block() and next_tuple()? - Melanie
