public inbox for [email protected]
help / color / mirror / Atom feedFrom: Melanie Plageman <[email protected]>
To: Andres Freund <[email protected]>
Cc: Nazir Bilal Yavuz <[email protected]>
Cc: Thomas Munro <[email protected]>
Cc: [email protected]
Cc: Peter Geoghegan <[email protected]>
Cc: Tomas Vondra <[email protected]>
Subject: Re: Don't synchronously wait for already-in-progress IO in read stream
Date: Thu, 26 Mar 2026 17:43:17 -0400
Message-ID: <CAAKRu_a8htJqvgMAMtA54zX35EZKhx3j7nLDXhksbciCxiNaJQ@mail.gmail.com> (raw)
In-Reply-To: <tjhnl6hiooo4aj6tktyehf4pn4yxcaqndlqebgpmynw4tmticq@odfcssnjf2qu>
References: <CAAKRu_ZM1epxTdt2=4-g4ff6UC09zne+0xg_gNv3d7LEcxEvNA@mail.gmail.com>
<CAN55FZ0ymY=XfaHQXHKG+Mbit6BZzZ9d-UWb6dmRXYm_xCvfQg@mail.gmail.com>
<CAAKRu_YV0D_9wWgGkHdWgQsooqmcc08Uqb3tqxzrwcDXHajDtA@mail.gmail.com>
<u73un3xeljr4fiidzwi4ikcr6vm7oqugn4fo5vqpstjio6anl2@hph6fvdiiria>
<CAAKRu_bCZfWpmfmfUiQjDgFK12T8FUox-hV9YFDy6XHTh=i0ew@mail.gmail.com>
<prkj4fv7dvyew4lbad2g5l346nn6u66inndwx7nzzap5c3bdmh@fds54c2hx3yd>
<42rdu4q44kvsq53fz5qgzuawqpaytvnemsnquynlfch5mqhc2m@6ytnlgivtzro>
<CAAKRu_YiaJgbmHGTFHRJEP9+AtVCt+gHTd1rmXm05agT_1Mx2A@mail.gmail.com>
<rk56uxuqxi3udzu5oqc7qft5u7nncau5d2avlbui6obd7ybols@wlu2euvawj7t>
<CAAKRu_bxajARZ0sXg_P=ZN76f1GofBmojE6HbQd7chjjhzWnRg@mail.gmail.com>
<tjhnl6hiooo4aj6tktyehf4pn4yxcaqndlqebgpmynw4tmticq@odfcssnjf2qu>
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
- I think the output columns could really use a bit of documentation
- 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.
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);
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
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?
seems like something like
# check that consecutive misses are combined into one read
would be better
- 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.
- 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
- 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"
- 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
# Test in-progress IO on the last block of the range
$psql_a->query_safe(qq|SELECT evict_rel('$table')|);
$psql_a->query_safe(
qq|SELECT read_rel_block_ll('$table', 3, wait_complete=>false)|);
psql_like(
$io_method,
$psql_a,
"$persistency: read buffers, in-progress 3, read 1-3",
qq|SELECT blockoff, blocknum, needs_io, nblocks FROM
read_buffers('$table', 1, 3)|,
qr/^0\|1\|t\|2\n2\|3\|f\|1$/,
qr/^$/);
- Melanie
view thread (31+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected], [email protected], [email protected]
Subject: Re: Don't synchronously wait for already-in-progress IO in read stream
In-Reply-To: <CAAKRu_a8htJqvgMAMtA54zX35EZKhx3j7nLDXhksbciCxiNaJQ@mail.gmail.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox