Re: Asynchronous and "direct" IO support for PostgreSQL.

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