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)