Hi,
On 2026-03-26 17:43:17 -0400, Melanie Plageman wrote:
> On Wed, Mar 25, 2026 at 5:58 PM Andres Freund <[email protected]> wrote:
> >
> > I'm planning to commit 0001 soon - it hasn't changed in a while. Then I'd
> > like
> > to get 0002 committed soon after, but I'll hold off for that until tomorrow,
> > given that nobody has looked at it (as it's new). I think 0004-0007 can be
> > committed too, but I am not sure if you (Melanie) want to do so.
>
> This is a review of 0002
>
> in read_buffers():
> - you forgot to initialize operation->forknum
Indeed. I happens to work because of palloc0 and MAIN_FORKNUM == 0, but
clearly that's not right.
> - I think the output columns could really use a bit of documentation
You mean in the C function? On the SQL level the column names seem good
enough for something in a test?
Added a comment for every output column. Also renamed need_io to
did_io. Thought about renaming nblocks to nblocks_this_io, but that just
seemed too long.
> - I assume that using uint16 for nblocks and nios is to make sure it
> plays nice with the input nblocks which is an int32 -- it doesn't
> matter for the range of values you're using, but it would be better if
> you could just use uint32 everywhere but I guess because we only have
> int4 in SQL you can't.
I actually had the parameters as int16 (or rather int2), but then the callers
needed casts to actually be able to call the function, so I went back to int4.
> anyway, I find all the many different types of ints and uints in
> read_buffers() pretty sus. Like why do you need this cast to int16?
> that seems...wrong and unnecessary?
> values[3] = Int32GetDatum((int16) nblocks_this_io);
Yea, I went back and forth on the return types and apparently forgot to remove
the cast when going back to to just using int32.
> in the evict_rel() refactor:
> - you need invalidate_one_block() to use the forknum parameter because
> otherwise temp tables won't evict any forks except the main fork
Oops. Fixed.
> for the tests themselves:
> - for the first test,
> # check that one larger read is done as multiple reads
> isn't the comment actually the opposite of what it is testing?
>
> 0|0|t|2 -- would be 1 2 block io starting at 0, no?
Yea, not sure how I ended up with that comment. Adopted yours.
> - for this comment:
> # but if we do it again, i.e. it's in s_b, there will be two operations
> technically you are also doing this for temp tables, so the comment
> isn't entirely correct.
Did s/s_b/buffer pool/
> - For this test:
> # Verify that we aren't doing reads larger than io_combine_limit
> isn't this more just covering the logic in read_buffers()? AFAICT
> StartReadBuffers() only worries about the max IOs it can combine if it
> is near the segment boundary
Fair. I adjusted the comment to remark upon that, but I'm kinda inclined to
keep the test. Just having io_combine_limit sized IOs seems worthwhile?
> - For this:
> $psql_a->query_safe(qq|SELECT invalidate_rel_block('$table', 1)|);
> $psql_a->query_safe(qq|SELECT invalidate_rel_block('$table', 2)|);
> $psql_a->query_safe(qq|SELECT * FROM read_buffers('$table', 3, 2)|);
> psql_like(
> $io_method,
> $psql_a,
> "$persistency: read buffers, miss 1-2, hit 3-4",
> qq|SELECT blockoff, blocknum, needs_io, nblocks FROM
> read_buffers('$table', 1, 4)|,
> qr/^0\|1\|t\|2\n2\|3\|f\|1\n3\|4\|f\|1$/,
> qr/^$/);
>
> I think this is a duplicate. There is one before and after the "verify
> we aren't doing reads larger than io_combine_limit"
Yep.
> - It may be worth adding one more test case which is IO in progress on
> the last block since you have in-progress as the first and the middle
> blocks but not as the last block
Added.
One test used did_io=(t|f). That was actually only needed once "aio: Don't
wait for already in-progress IO" is in, as we might join the foreign IO. I
chose to hide that by making that part of the query "did_io and not
foreign_io", so we would detect if we were to falsely start IO ourselves.
Still need to extend the test as part of the "don't wait" commit, to actually
ensure that we reach the path for joining foreign IO.
Greetings,
Andres Freund
