public inbox for [email protected]
help / color / mirror / Atom feedFrom: Melanie Plageman <[email protected]>
To: Alexander Lakhin <[email protected]>
Cc: 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: Mon, 30 Mar 2026 18:37:27 -0400
Message-ID: <CAAKRu_ZRf_t4=5y5nvq3BKoN8=8Dh77O0pi6s2kScXiAnA3eVQ@mail.gmail.com> (raw)
In-Reply-To: <CAAKRu_Y2NeYkqf1wymzErYt9aWFXU5ebUSi2xWfsv3wY4HMEbw@mail.gmail.com>
References: <prkj4fv7dvyew4lbad2g5l346nn6u66inndwx7nzzap5c3bdmh@fds54c2hx3yd>
<42rdu4q44kvsq53fz5qgzuawqpaytvnemsnquynlfch5mqhc2m@6ytnlgivtzro>
<CAAKRu_YiaJgbmHGTFHRJEP9+AtVCt+gHTd1rmXm05agT_1Mx2A@mail.gmail.com>
<rk56uxuqxi3udzu5oqc7qft5u7nncau5d2avlbui6obd7ybols@wlu2euvawj7t>
<CAAKRu_bxajARZ0sXg_P=ZN76f1GofBmojE6HbQd7chjjhzWnRg@mail.gmail.com>
<tjhnl6hiooo4aj6tktyehf4pn4yxcaqndlqebgpmynw4tmticq@odfcssnjf2qu>
<CAAKRu_a8htJqvgMAMtA54zX35EZKhx3j7nLDXhksbciCxiNaJQ@mail.gmail.com>
<wmqrglrv4eq2m2kzgbjbtmmuctklg7q4ylwo22rks7r7mlbf3w@dn6txz6znmtr>
<us6m5jdjxmveyan77tdg2tysjls4yu3tsohpvjjedz6pnjkyn7@2g6b2asvgug3>
<CAAKRu_Zp8qY1hYxRp-XLQi=3Dff=j5VmS8b_KJu+MSGDHnbP-A@mail.gmail.com>
<bhbitjpkaosi75kxqkemt7257rbz67j5vqjp4ya73uwl5zonnx@zz6z4vzqd7zh>
<[email protected]>
<CAAKRu_Y2NeYkqf1wymzErYt9aWFXU5ebUSi2xWfsv3wY4HMEbw@mail.gmail.com>
On Mon, Mar 30, 2026 at 3:14 PM Melanie Plageman
<[email protected]> wrote:
>
> On Mon, Mar 30, 2026 at 3:00 PM Alexander Lakhin <[email protected]> wrote:
> >
> > As copperhead showed [1], tests added in 020c02bd9 fail when postgres is
> > built without --enable-cassert. I've reproduced the failure locally with:
>
> Yes, it's because read_buffers() (in test_aio.c) uses
> operation->nblocks and that's only intialized for buffer hits in
> assert builds. The test code could just use the correctly initialized
> nblocks out parameter.
Fix was a little more invasive than that. Looks like we were using
operation in more places than I thought. See attached.
- Melanie
Attachments:
[text/x-patch] v1-0001-Fix-test_aio-read_buffers-to-work-without-cassert.patch (3.6K, 2-v1-0001-Fix-test_aio-read_buffers-to-work-without-cassert.patch)
download | inline diff:
From 8db56562d13300ca8e1620fbc4ea4e6e3102e3fb Mon Sep 17 00:00:00 2001
From: Melanie Plageman <[email protected]>
Date: Mon, 30 Mar 2026 18:25:46 -0400
Subject: [PATCH v1] Fix test_aio read_buffers() to work without cassert
In a production build, StartReadBuffers() doesn't population all fields
of a ReadBuffersOperation for a buffer hit because no callers use them
(they are populated in assert builds).
The read_buffers() test function relied on some of these fields, though,
so AIO tests failed on non-assert builds (discovered on the
buildfarm after commit 020c02bd908).
Fix by tracking the required information ourselves in read_buffers() and
avoiding reliance on the ReadBuffersOperation unless we know that we did
IO.
Reported-by: Alexander Lakhin <[email protected]>
Discussion: https://postgr.es/m/9ce8f5d8-8ab2-4aa2-b062-c5d74161069c%40gmail.com
---
src/test/modules/test_aio/test_aio.c | 22 ++++++++++------------
1 file changed, 10 insertions(+), 12 deletions(-)
diff --git a/src/test/modules/test_aio/test_aio.c b/src/test/modules/test_aio/test_aio.c
index a8267192cb7..d7530681192 100644
--- a/src/test/modules/test_aio/test_aio.c
+++ b/src/test/modules/test_aio/test_aio.c
@@ -719,6 +719,7 @@ read_buffers(PG_FUNCTION_ARGS)
Buffer *buffers;
Datum *buffers_datum;
bool *io_reqds;
+ int *nblocks_per_io;
Assert(nblocks > 0);
@@ -729,6 +730,7 @@ read_buffers(PG_FUNCTION_ARGS)
buffers = palloc0(sizeof(Buffer) * nblocks);
buffers_datum = palloc0(sizeof(Datum) * nblocks);
io_reqds = palloc0(sizeof(bool) * nblocks);
+ nblocks_per_io = palloc0(sizeof(int) * nblocks);
rel = relation_open(relid, AccessShareLock);
smgr = RelationGetSmgr(rel);
@@ -754,6 +756,7 @@ read_buffers(PG_FUNCTION_ARGS)
startblock + nblocks_done,
&nblocks_this_io,
0);
+ nblocks_per_io[nios] = nblocks_this_io;
nios++;
nblocks_done += nblocks_this_io;
}
@@ -777,7 +780,7 @@ read_buffers(PG_FUNCTION_ARGS)
for (int nio = 0; nio < nios; nio++)
{
ReadBuffersOperation *operation = &operations[nio];
- int nblocks_this_io = operation->nblocks;
+ int nblocks_this_io = nblocks_per_io[nio];
Datum values[6] = {0};
bool nulls[6] = {0};
ArrayType *buffers_arr;
@@ -785,9 +788,8 @@ read_buffers(PG_FUNCTION_ARGS)
/* convert buffer array to datum array */
for (int i = 0; i < nblocks_this_io; i++)
{
- Buffer buf = operation->buffers[i];
+ Buffer buf = buffers[nblocks_disp + i];
- Assert(buffers[nblocks_disp + i] == buf);
Assert(BufferGetBlockNumber(buf) == startblock + nblocks_disp + i);
buffers_datum[nblocks_disp + i] = Int32GetDatum(buf);
@@ -809,8 +811,8 @@ read_buffers(PG_FUNCTION_ARGS)
values[2] = BoolGetDatum(io_reqds[nio]);
nulls[2] = false;
- /* foreign IO */
- values[3] = BoolGetDatum(operation->foreign_io);
+ /* foreign IO - only valid when IO was required */
+ values[3] = BoolGetDatum(io_reqds[nio] ? operation->foreign_io : false);
nulls[3] = false;
/* nblocks */
@@ -827,13 +829,8 @@ read_buffers(PG_FUNCTION_ARGS)
}
/* release pins on all the buffers */
- for (int nio = 0; nio < nios; nio++)
- {
- ReadBuffersOperation *operation = &operations[nio];
-
- for (int i = 0; i < operation->nblocks; i++)
- ReleaseBuffer(operation->buffers[i]);
- }
+ for (int i = 0; i < nblocks_done; i++)
+ ReleaseBuffer(buffers[i]);
/*
* Free explicitly, to have a chance to detect potential issues with too
@@ -843,6 +840,7 @@ read_buffers(PG_FUNCTION_ARGS)
pfree(buffers);
pfree(buffers_datum);
pfree(io_reqds);
+ pfree(nblocks_per_io);
relation_close(rel, NoLock);
--
2.43.0
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], [email protected]
Subject: Re: Don't synchronously wait for already-in-progress IO in read stream
In-Reply-To: <CAAKRu_ZRf_t4=5y5nvq3BKoN8=8Dh77O0pi6s2kScXiAnA3eVQ@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