Re: Streaming I/O, vectored I/O (WIP)

Some quick comments:

On 12/03/2024 15:02, Thomas Munro wrote:
> src/backend/storage/aio/streaming_read.c
> src/include/storage/streaming_read.h

Standard file header comments missing.

It would be nice to have a comment at the top of streaming_read.c,
explaining at a high level how the circular buffer, lookahead and all
that works. Maybe even some diagrams.

For example, what is head and what is tail? Before reading the code, I
assumed that 'head' was the next block range to return in
pg_streaming_read_buffer_get_next(). But I think it's actually the other
way round?

> /*
> * Create a new streaming read object that can be used to perform the
> * equivalent of a series of ReadBuffer() calls for one fork of one relation.
> * Internally, it generates larger vectored reads where possible by looking
> * ahead.
> */
> PgStreamingRead *
> pg_streaming_read_buffer_alloc(int flags,
> void *pgsr_private,
> size_t per_buffer_data_size,
> BufferAccessStrategy strategy,
> BufferManagerRelation bmr,
> ForkNumber forknum,
> PgStreamingReadBufferCB next_block_cb)

I'm not a fan of the name, especially the 'alloc' part. Yeah, most of
the work it does is memory allocation. But I'd suggest something like
'pg_streaming_read_begin' instead.

Do we really need the pg_ prefix in these?

> Buffer
> pg_streaming_read_buffer_get_next(PgStreamingRead *pgsr, void **per_buffer_data)

Maybe 'pg_streaming_read_next_buffer' or just 'pg_streaming_read_next',
for a shorter name.

>
> /*
> * pgsr->ranges is a circular buffer. When it is empty, head == tail.
> * When it is full, there is an empty element between head and tail. Head
> * can also be empty (nblocks == 0), therefore we need two extra elements
> * for non-occupied ranges, on top of max_pinned_buffers to allow for the
> * maxmimum possible number of occupied ranges of the smallest possible
> * size of one.
> */
> size = max_pinned_buffers + 2;

I didn't understand this explanation for why it's + 2.

> /*
> * Skip the initial ramp-up phase if the caller says we're going to be
> * reading the whole relation. This way we start out doing full-sized
> * reads.
> */
> if (flags & PGSR_FLAG_FULL)
> pgsr->distance = Min(MAX_BUFFERS_PER_TRANSFER, pgsr->max_pinned_buffers);
> else
> pgsr->distance = 1;

Should this be "Max(MAX_BUFFERS_PER_TRANSFER,
pgsr->max_pinned_buffers)"? max_pinned_buffers cannot be smaller than
MAX_BUFFERS_PER_TRANSFER though, given how it's initialized earlier. So
perhaps just 'pgsr->distance = pgsr->max_pinned_buffers' ?

--
Heikki Linnakangas
Neon (https://neon.tech)