public inbox for [email protected]
help / color / mirror / Atom feedpgsql: test_aio: Add basic tests for StartReadBuffers()
7+ messages / 4 participants
[nested] [flat]
* pgsql: test_aio: Add basic tests for StartReadBuffers()
@ 2026-03-27 22:54 Andres Freund <[email protected]>
0 siblings, 1 reply; 7+ messages in thread
From: Andres Freund @ 2026-03-27 22:54 UTC (permalink / raw)
To: [email protected]
test_aio: Add basic tests for StartReadBuffers()
Upcoming commits will change StartReadBuffers() and its building blocks,
making it worthwhile to directly test StartReadBuffers().
Reviewed-by: Melanie Plageman <[email protected]>
Discussion: https://postgr.es/m/zljergweqti7x67lg5ije2rzjusie37nslsnkjkkby4laqqbfw@3p3zu522yykv
Branch
------
master
Details
-------
https://git.postgresql.org/pg/commitdiff/020c02bd90896066964d173139b3adb154ec7426
Modified Files
--------------
src/test/modules/test_aio/t/001_aio.pl | 243 ++++++++++++++++++++++++++++
src/test/modules/test_aio/test_aio--1.0.sql | 7 +
src/test/modules/test_aio/test_aio.c | 229 ++++++++++++++++++++++++--
3 files changed, 469 insertions(+), 10 deletions(-)
^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: pgsql: test_aio: Add basic tests for StartReadBuffers()
@ 2026-03-31 10:18 Christoph Berg <[email protected]>
parent: Andres Freund <[email protected]>
0 siblings, 1 reply; 7+ messages in thread
From: Christoph Berg @ 2026-03-31 10:18 UTC (permalink / raw)
To: Andres Freund <[email protected]>; +Cc: [email protected]; Melanie Plageman <[email protected]>
One of the AIO commits around this one (2026-03-28 00:00Z) broke the
postgresql-19 builds on apt.pg.o. Only the Debian unstable (sid,
assertions enabled) builds are working, everything else including the
nearly identical testing (forky) is broken (where assertions are off):
05:01:04 # +++ tap check in src/test/modules/test_aio +++
05:01:04
05:01:04 # Failed test 'worker: normal: read buffers, doesn't combine hits, block 0-1: expected stdout'
05:01:04 # at t/001_aio.pl line 1474.
05:01:04 # '0|0|f|0
05:01:04 # 0|0|f|0'
05:01:04 # doesn't match '(?^:^0\|0\|f\|1\n1\|1\|f\|1$)'
05:01:04
05:01:04 # Failed test 'worker: normal: read buffers, doesn't combine hits, block 0-1: expected stderr'
05:01:04 # at t/001_aio.pl line 1474.
05:01:04 # 'psql:<stdin>:19: WARNING: resource was not closed: [4728] (rel=base/5/16413, blockNum=0, flags=0x83000000, refcount=1 1)
05:01:04 # psql:<stdin>:19: WARNING: resource was not closed: [4729] (rel=base/5/16413, blockNum=1, flags=0x83000000, refcount=1 1)'
05:01:04 # doesn't match '(?^:^$)'
05:01:04
05:01:04 # Failed test 'worker: normal: read buffers, interrupted by hit on 3, block 2-5: expected stdout'
05:01:04 # at t/001_aio.pl line 1490.
05:01:04 # '0|2|t|1
05:01:04 # 1|3|f|0
05:01:04 # 1|3|t|2'
05:01:04 # doesn't match '(?^:^0\|2\|t\|1\n1\|3\|f\|1\n2\|4\|t\|2$)'
05:01:04
05:01:04 # Failed test 'worker: normal: read buffers, interrupted by hit on 3, block 2-5: expected stderr'
05:01:04 # at t/001_aio.pl line 1490.
05:01:04 # 'psql:<stdin>:27: WARNING: resource was not closed: [4730] (rel=base/5/16413, blockNum=3, flags=0x83000000, refcount=1 1)'
05:01:04 # doesn't match '(?^:^$)'
05:01:04
05:01:04 # Failed test 'worker: normal: read buffers, hit, block 0: expected stdout'
05:01:04 # at t/001_aio.pl line 1508.
05:01:04 # '0|0|f|0'
05:01:04 # doesn't match '(?^:^0\|0\|f\|1$)'
05:01:04
05:01:04 # Failed test 'worker: normal: read buffers, hit, block 0: expected stderr'
05:01:04 # at t/001_aio.pl line 1508.
05:01:04 # 'psql:<stdin>:39: WARNING: resource was not closed: [4734] (rel=base/5/16413, blockNum=0, flags=0x83000000, refcount=1 1)'
05:01:04 # doesn't match '(?^:^$)'
05:01:04
05:01:04 # Failed test 'worker: normal: read buffers, hit, block 1: expected stdout'
05:01:04 # at t/001_aio.pl line 1522.
05:01:04 # '0|1|f|0'
05:01:04 # doesn't match '(?^:^0\|1\|f\|1$)'
05:01:04
05:01:04 # Failed test 'worker: normal: read buffers, hit, block 1: expected stderr'
05:01:04 # at t/001_aio.pl line 1522.
05:01:04 # 'psql:<stdin>:47: WARNING: resource was not closed: [4735] (rel=base/5/16413, blockNum=1, flags=0x83000000, refcount=1 1)'
05:01:04 # doesn't match '(?^:^$)'
05:01:04
05:01:04 # Failed test 'worker: normal: read buffers, hit, block 0-1: expected stdout'
05:01:04 # at t/001_aio.pl line 1529.
05:01:04 # '0|0|f|0
05:01:04 # 0|0|f|0'
05:01:04 # doesn't match '(?^:^0\|0\|f\|1\n1\|1\|f\|1$)'
05:01:04
05:01:04 # Failed test 'worker: normal: read buffers, hit, block 0-1: expected stderr'
05:01:04 # at t/001_aio.pl line 1529.
05:01:04 # 'psql:<stdin>:51: WARNING: resource was not closed: [4734] (rel=base/5/16413, blockNum=0, flags=0x83000000, refcount=1 1)
05:01:04 # psql:<stdin>:51: WARNING: resource was not closed: [4735] (rel=base/5/16413, blockNum=1, flags=0x83000000, refcount=1 1)'
05:01:04 # doesn't match '(?^:^$)'
05:01:04
05:01:04 # Failed test 'worker: normal: read buffers, hit 0-1, miss 2: expected stdout'
05:01:04 # at t/001_aio.pl line 1536.
05:01:04 # '0|0|f|0
05:01:04 # 0|0|f|0
05:01:04 # 0|0|t|1'
05:01:04 # doesn't match '(?^:^0\|0\|f\|1\n1\|1\|f\|1\n2\|2\|t\|1$)'
05:01:04
05:01:04 # Failed test 'worker: normal: read buffers, hit 0-1, miss 2: expected stderr'
05:01:04 # at t/001_aio.pl line 1536.
05:01:04 # 'psql:<stdin>:55: WARNING: resource was not closed: [4734] (rel=base/5/16413, blockNum=0, flags=0x83000000, refcount=1 1)
05:01:04 # psql:<stdin>:55: WARNING: resource was not closed: [4735] (rel=base/5/16413, blockNum=1, flags=0x83000000, refcount=1 1)'
05:01:04 # doesn't match '(?^:^$)'
...
https://jengus.postgresql.org/view/Snapshot/job/postgresql-19-binaries-snapshot/
https://jengus.postgresql.org/view/Snapshot/job/postgresql-19-binaries-snapshot/781/architecture=amd...
Christoph
^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: pgsql: test_aio: Add basic tests for StartReadBuffers()
@ 2026-03-31 12:10 Nazir Bilal Yavuz <[email protected]>
parent: Christoph Berg <[email protected]>
0 siblings, 1 reply; 7+ messages in thread
From: Nazir Bilal Yavuz @ 2026-03-31 12:10 UTC (permalink / raw)
To: Christoph Berg <[email protected]>; +Cc: Andres Freund <[email protected]>; [email protected]; Melanie Plageman <[email protected]>
Hi,
On Tue, 31 Mar 2026 at 13:19, Christoph Berg <[email protected]> wrote:
>
> One of the AIO commits around this one (2026-03-28 00:00Z) broke the
> postgresql-19 builds on apt.pg.o. Only the Debian unstable (sid,
> assertions enabled) builds are working, everything else including the
> nearly identical testing (forky) is broken (where assertions are off):
Thank you for the report! I can reproduce this when I disable
assertions. The problem is that, in StartReadBuffersImpl() in
bufmgr.c:
```
#ifdef USE_ASSERT_CHECKING
/*
* Initialize enough of ReadBuffersOperation to make
* CheckReadBuffersOperation() work. Outside of assertions
* that's not necessary when no IO is issued.
*/
operation->buffers = buffers;
operation->blocknum = blockNum;
operation->nblocks = 1;
operation->nblocks_done = 1;
CheckReadBuffersOperation(operation, true);
#endif
```
if (found) and if (i == 0) then we set operation->buffers and
operation->nblocks if the assertions are enabled but AIO tests expect
to read these values. So, read_buffers() in AIO tests read incorrect
values [1] when the assertions are disabled. Moving operation->buffers
and operation->nblocks outside of #ifdef USE_ASSERT_CHECKING like in
the attached fixes the problem.
[1]
read_buffers(PG_FUNCTION_ARGS)
{
...
for (int nio = 0; nio < nios; nio++)
{
ReadBuffersOperation *operation = &operations[nio];
int nblocks_this_io = operation->nblocks;
Datum values[6] = {0};
bool nulls[6] = {0};
ArrayType *buffers_arr;
/* convert buffer array to datum array */
for (int i = 0; i < nblocks_this_io; i++)
{
Buffer buf = operation->buffers[i];
...
}
--
Regards,
Nazir Bilal Yavuz
Microsoft
Attachments:
[text/x-patch] 0001-Set-the-variables-that-callers-might-read-in-StartRe.patch (1.7K, 2-0001-Set-the-variables-that-callers-might-read-in-StartRe.patch)
download | inline diff:
From 56218fc850cdaccaeb87767c2ca8977ee2f26697 Mon Sep 17 00:00:00 2001
From: Nazir Bilal Yavuz <[email protected]>
Date: Tue, 31 Mar 2026 15:00:40 +0300
Subject: [PATCH] Set the variables that callers might read in
StartReadBuffersImpl()
When the first requested block is already in the buffer pool, StartRead-
BuffersImpl() returned early after setting *nblocks = 1 without starting
any I/O. In that path, operation->nblocks and operation->buffers were
only initialised inside #ifdef USE_ASSERT_CHECKING, leaving them as zero
/ NULL in non-assert builds.
Callers might want to inspect these values like in the AIO tests. Fix
that problem by unconditionally setting operation->nblocks and
operation->buffers in the early-return hit path.
---
src/backend/storage/buffer/bufmgr.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/src/backend/storage/buffer/bufmgr.c b/src/backend/storage/buffer/bufmgr.c
index cd21ae3fc36..e6f99ad7668 100644
--- a/src/backend/storage/buffer/bufmgr.c
+++ b/src/backend/storage/buffer/bufmgr.c
@@ -1457,6 +1457,10 @@ StartReadBuffersImpl(ReadBuffersOperation *operation,
{
*nblocks = 1;
+ /* Set the variables that callers might read */
+ operation->buffers = buffers;
+ operation->nblocks = 1;
+
#ifdef USE_ASSERT_CHECKING
/*
@@ -1464,9 +1468,7 @@ StartReadBuffersImpl(ReadBuffersOperation *operation,
* CheckReadBuffersOperation() work. Outside of assertions
* that's not necessary when no IO is issued.
*/
- operation->buffers = buffers;
operation->blocknum = blockNum;
- operation->nblocks = 1;
operation->nblocks_done = 1;
CheckReadBuffersOperation(operation, true);
#endif
--
2.47.3
^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: pgsql: test_aio: Add basic tests for StartReadBuffers()
@ 2026-03-31 12:52 Nazir Bilal Yavuz <[email protected]>
parent: Nazir Bilal Yavuz <[email protected]>
0 siblings, 2 replies; 7+ messages in thread
From: Nazir Bilal Yavuz @ 2026-03-31 12:52 UTC (permalink / raw)
To: Christoph Berg <[email protected]>; +Cc: Andres Freund <[email protected]>; [email protected]; Melanie Plageman <[email protected]>
Hi,
On Tue, 31 Mar 2026 at 15:10, Nazir Bilal Yavuz <[email protected]> wrote:
>
> Hi,
>
> On Tue, 31 Mar 2026 at 13:19, Christoph Berg <[email protected]> wrote:
> >
> > One of the AIO commits around this one (2026-03-28 00:00Z) broke the
> > postgresql-19 builds on apt.pg.o. Only the Debian unstable (sid,
> > assertions enabled) builds are working, everything else including the
> > nearly identical testing (forky) is broken (where assertions are off):
>
> Thank you for the report! I can reproduce this when I disable
> assertions.
Please ignore my previous email, Melanie already shared a better fix
in the original thread [1].
[1] https://postgr.es/m/CAAKRu_ZRf_t4%3D5y5nvq3BKoN8%3D8Dh77O0pi6s2kScXiAnA3eVQ%40mail.gmail.com
--
Regards,
Nazir Bilal Yavuz
Microsoft
^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: pgsql: test_aio: Add basic tests for StartReadBuffers()
@ 2026-03-31 13:14 Christoph Berg <[email protected]>
parent: Nazir Bilal Yavuz <[email protected]>
1 sibling, 0 replies; 7+ messages in thread
From: Christoph Berg @ 2026-03-31 13:14 UTC (permalink / raw)
To: Nazir Bilal Yavuz <[email protected]>; +Cc: Freund <[email protected]>; [email protected]
Re: Nazir Bilal Yavuz
> Please ignore my previous email, Melanie already shared a better fix
> in the original thread [1].
I should probably have done more research looking for an existing fix.
Admittedly I was only looking for threads with "aio" in the subject
while that one has only "IO"...
Thanks,
Christoph
^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: pgsql: test_aio: Add basic tests for StartReadBuffers()
@ 2026-03-31 19:07 Melanie Plageman <[email protected]>
parent: Nazir Bilal Yavuz <[email protected]>
1 sibling, 1 reply; 7+ messages in thread
From: Melanie Plageman @ 2026-03-31 19:07 UTC (permalink / raw)
To: Nazir Bilal Yavuz <[email protected]>; +Cc: Christoph Berg <[email protected]>; Andres Freund <[email protected]>; [email protected]
On Tue, Mar 31, 2026 at 8:52 AM Nazir Bilal Yavuz <[email protected]> wrote:
>
> Please ignore my previous email, Melanie already shared a better fix
> in the original thread [1].
That's been pushed in 8519251ee975.
- Melanie
^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: pgsql: test_aio: Add basic tests for StartReadBuffers()
@ 2026-04-01 08:20 Christoph Berg <[email protected]>
parent: Melanie Plageman <[email protected]>
0 siblings, 0 replies; 7+ messages in thread
From: Christoph Berg @ 2026-04-01 08:20 UTC (permalink / raw)
To: Melanie Plageman <[email protected]>; +Cc: Nazir Bilal Yavuz <[email protected]>; Andres Freund <[email protected]>; [email protected]
Re: Melanie Plageman
> That's been pushed in 8519251ee975.
Confirmed, packages build again!
Thanks,
Christoph
^ permalink raw reply [nested|flat] 7+ messages in thread
end of thread, other threads:[~2026-04-01 08:20 UTC | newest]
Thread overview: 7+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2026-03-27 22:54 pgsql: test_aio: Add basic tests for StartReadBuffers() Andres Freund <[email protected]>
2026-03-31 10:18 ` Christoph Berg <[email protected]>
2026-03-31 12:10 ` Nazir Bilal Yavuz <[email protected]>
2026-03-31 12:52 ` Nazir Bilal Yavuz <[email protected]>
2026-03-31 13:14 ` Christoph Berg <[email protected]>
2026-03-31 19:07 ` Melanie Plageman <[email protected]>
2026-04-01 08:20 ` Christoph Berg <[email protected]>
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox