Hi, On 2021-07-28 13:37:48 -0400, Melanie Plageman wrote: > Attached is a patch on top of the AIO branch which does bitmapheapscan > prefetching using the PgStreamingRead helper already used by sequential > scan and vacuum on the AIO branch. > > The prefetch iterator is removed and the main iterator in the > BitmapHeapScanState node is now used by the PgStreamingRead helper.
Cool! I'm heartened to see "12 files changed, 272 insertions(+), 495 deletions(-)" It's worth calling out that this fixes some abstraction leakyness around tableam too... > Each IO will have its own TBMIterateResult allocated and returned by the > PgStreamingRead helper and freed later by > heapam_scan_bitmap_next_block() before requesting the next block. > Previously it was allocated once and saved in the TBMIterator in the > BitmapHeapScanState node and reused. Because of this, the table AM API > routine, table_scan_bitmap_next_block() now defines the TBMIterateResult > as an output parameter. > > I haven't made the GIN code reasonable yet either (it uses the TID > bitmap functions that I've changed). I don't quite understand the need to change the tidbitmap interface, or maybe rather I'm not convinced that pessimistically preallocating space is a good idea? > I don't see a need for it right now. If you wanted you > Because the PgStreamingReadHelper needs to be set up with the > BitmapHeapScanState node but also needs some table AM specific > functions, I thought it made more sense to initialize it using a new > table AM API routine. Instead of fully implementing that I just wrote a > wrapper function, table_bitmap_scan_setup() which just calls > bitmapheap_pgsr_alloc() to socialize the idea before implementing it. That makes sense. > static bool > heapam_scan_bitmap_next_block(TableScanDesc scan, > - TBMIterateResult > *tbmres) > + TBMIterateResult **tbmres) ISTM that we possibly shouldn't expose the TBMIterateResult outside of the AM after this change? It feels somewhat like an implementation detail now. It seems somewhat odd to expose a ** to set a pointer that nodeBitmapHeapscan.c then doesn't really deal with itself. > @@ -695,8 +693,7 @@ tbm_begin_iterate(TIDBitmap *tbm) > * Create the TBMIterator struct, with enough trailing space to serve > the > * needs of the TBMIterateResult sub-struct. > */ > - iterator = (TBMIterator *) palloc(sizeof(TBMIterator) + > - > MAX_TUPLES_PER_PAGE * sizeof(OffsetNumber)); > + iterator = (TBMIterator *) palloc(sizeof(TBMIterator)); > iterator->tbm = tbm; Hm? > diff --git a/src/include/storage/aio.h b/src/include/storage/aio.h > index 9a07f06b9f..8e1aa48827 100644 > --- a/src/include/storage/aio.h > +++ b/src/include/storage/aio.h > @@ -39,7 +39,7 @@ typedef enum IoMethod > } IoMethod; > > /* We'll default to bgworker. */ > -#define DEFAULT_IO_METHOD IOMETHOD_WORKER > +#define DEFAULT_IO_METHOD IOMETHOD_IO_URING I agree with the sentiment, but ... :) Greetings, Andres Freund
