pg.ddx.io pgsql-bugs@postgresql.org mailing list archive
help / color / mirror / Atom feedBUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer
29+ messages / 6 participants
[nested] [flat]
* BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer
@ 2025-08-03 12:00 PG Bug reporting form <noreply@postgresql.org>
2025-08-04 01:12 ` Re: BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer Michael Paquier <michael@paquier.xyz>
2025-08-04 13:13 ` Re: BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer Xuneng Zhou <xunengzhou@gmail.com>
0 siblings, 2 replies; 29+ messages in thread
From: PG Bug reporting form @ 2025-08-03 12:00 UTC (permalink / raw)
To: pgsql-bugs@lists.postgresql.org; +Cc: exclusion@gmail.com
The following bug has been logged on the website:
Bug reference: 19006
Logged by: Alexander Lakhin
Email address: exclusion@gmail.com
PostgreSQL version: 18beta2
Operating system: Ubuntu 24.04
Description:
The following script (based on the regress scripts):
CREATE TABLE public.tenk1 (
unique1 integer,
unique2 integer,
two integer,
four integer,
ten integer,
twenty integer,
hundred integer,
thousand integer,
twothousand integer,
fivethous integer,
tenthous integer,
odd integer,
even integer,
stringu1 name,
stringu2 name,
string4 name
);
COPY tenk1 FROM '.../src/test/regress/data/tenk.data';
COPY tenk1 FROM '.../src/test/regress/data/tenk.data';
DELETE FROM tenk1;
COPY tenk1 FROM '.../src/test/regress/data/tenk.data';
SELECT COUNT(*) FROM tenk1 t1 LEFT JOIN
LATERAL (SELECT t1.twenty, t2.two FROM tenk1 t2) s
ON t1.two = s.two
WHERE s.twenty < 0;
with
shared_buffers = 16MB
triggers:
TRAP: failed Assert("BufferIsPinned(buffer)"), File: "bufmgr.c", Line: 4233,
PID: 2785555
Core was generated by `postgres: law regression [local] SELECT
'.
Program terminated with signal SIGABRT, Aborted.
...
#0 __pthread_kill_implementation (no_tid=0, signo=6, threadid=<optimized
out>) at ./nptl/pthread_kill.c:44
#1 __pthread_kill_internal (signo=6, threadid=<optimized out>) at
./nptl/pthread_kill.c:78
#2 __GI___pthread_kill (threadid=<optimized out>, signo=signo@entry=6) at
./nptl/pthread_kill.c:89
#3 0x000075efce04527e in __GI_raise (sig=sig@entry=6) at
../sysdeps/posix/raise.c:26
#4 0x000075efce0288ff in __GI_abort () at ./stdlib/abort.c:79
#5 0x00006419a43d5e3f in ExceptionalCondition
(conditionName=conditionName@entry=0x6419a445bb56 "BufferIsPinned(buffer)",
fileName=fileName@entry=0x6419a445b9b7 "bufmgr.c",
lineNumber=lineNumber@entry=4233) at assert.c:66
#6 0x00006419a423f028 in BufferGetBlockNumber (buffer=<optimized out>) at
bufmgr.c:4233
#7 0x00006419a42445c5 in StartReadBuffersImpl (allow_forwarding=true,
flags=0, nblocks=0x7fff9765ee74, blockNum=710, buffers=0x6419bc3d127c,
operation=0x6419bc3d1428) at bufmgr.c:1292
#8 StartReadBuffers (operation=0x6419bc3d1428,
buffers=buffers@entry=0x6419bc3d127c, blockNum=710,
nblocks=nblocks@entry=0x7fff9765ee74, flags=flags@entry=0) at bufmgr.c:1500
#9 0x00006419a423afc4 in read_stream_start_pending_read
(stream=stream@entry=0x6419bc3d1218) at read_stream.c:335
#10 0x00006419a423b3df in read_stream_look_ahead
(stream=stream@entry=0x6419bc3d1218) at read_stream.c:493
#11 0x00006419a423b7b3 in read_stream_next_buffer (stream=0x6419bc3d1218,
per_buffer_data=per_buffer_data@entry=0x0) at read_stream.c:971
#12 0x00006419a3efaede in heap_fetch_next_buffer (dir=<optimized out>,
scan=0x6419bc3d0cf8) at heapam.c:675
#13 heapgettup_pagemode (scan=scan@entry=0x6419bc3d0cf8, dir=<optimized
out>, nkeys=<optimized out>, key=<optimized out>) at heapam.c:1037
#14 0x00006419a3efb5d2 in heap_getnextslot (sscan=0x6419bc3d0cf8,
direction=<optimized out>, slot=0x6419bc3c11a8) at heapam.c:1391
#15 0x00006419a40d1503 in table_scan_getnextslot (slot=0x6419bc3c11a8,
direction=ForwardScanDirection, sscan=<optimized out>) at
../../../src/include/access/tableam.h:1031
#16 SeqNext (node=node@entry=0x6419bc3c0f88) at nodeSeqscan.c:81
#17 0x00006419a40d197f in ExecScanFetch (recheckMtd=0x6419a40d1490
<SeqRecheck>, accessMtd=0x6419a40d14a0 <SeqNext>, epqstate=0x0,
node=<optimized out>) at ../../../src/include/executor/execScan.h:126
#18 ExecScanExtended (projInfo=0x0, qual=0x6419bc38a650, epqstate=0x0,
recheckMtd=0x6419a40d1490 <SeqRecheck>, accessMtd=0x6419a40d14a0 <SeqNext>,
node=0x6419bc3c0f88) at ../../../src/include/executor/execScan.h:187
#19 ExecSeqScanWithQual (pstate=0x6419bc3c0f88) at nodeSeqscan.c:138
#20 0x00006419a40cf97b in ExecProcNode (node=0x6419bc3c0f88) at
../../../src/include/executor/executor.h:313
#21 ExecNestLoop (pstate=<optimized out>) at nodeNestloop.c:159
...
Reproduced starting from 12ce89fd0.
^ permalink raw reply [nested|flat] 29+ messages in thread
* Re: BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer
2025-08-03 12:00 BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer PG Bug reporting form <noreply@postgresql.org>
@ 2025-08-04 01:12 ` Michael Paquier <michael@paquier.xyz>
1 sibling, 0 replies; 29+ messages in thread
From: Michael Paquier @ 2025-08-04 01:12 UTC (permalink / raw)
To: exclusion@gmail.com; pgsql-bugs@lists.postgresql.org
On Sun, Aug 03, 2025 at 12:00:02PM +0000, PG Bug reporting form wrote:
> Reproduced starting from 12ce89fd0.
Added an open item to track this one. Thanks for the report.
--
Michael
Attachments:
[application/pgp-signature] signature.asc (832B, ../../aJAJC_9v-HaMGUdZ@paquier.xyz/2-signature.asc)
download
^ permalink raw reply [nested|flat] 29+ messages in thread
* Re: BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer
2025-08-03 12:00 BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer PG Bug reporting form <noreply@postgresql.org>
@ 2025-08-04 13:13 ` Xuneng Zhou <xunengzhou@gmail.com>
2025-08-04 19:19 ` Re: BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer Thomas Munro <thomas.munro@gmail.com>
1 sibling, 1 reply; 29+ messages in thread
From: Xuneng Zhou @ 2025-08-04 13:13 UTC (permalink / raw)
To: exclusion@gmail.com; pgsql-bugs@lists.postgresql.org; +Cc: thomas.munro@gmail.com, Michael Paquier <michael@paquier.xyz>
Hi,
On Sun, Aug 3, 2025 at 8:58 PM PG Bug reporting form
<noreply@postgresql.org> wrote:
>
> The following bug has been logged on the website:
>
> Bug reference: 19006
> Logged by: Alexander Lakhin
> Email address: exclusion@gmail.com
> PostgreSQL version: 18beta2
> Operating system: Ubuntu 24.04
> Description:
>
> The following script (based on the regress scripts):
> CREATE TABLE public.tenk1 (
> unique1 integer,
> unique2 integer,
> two integer,
> four integer,
> ten integer,
> twenty integer,
> hundred integer,
> thousand integer,
> twothousand integer,
> fivethous integer,
> tenthous integer,
> odd integer,
> even integer,
> stringu1 name,
> stringu2 name,
> string4 name
> );
>
> COPY tenk1 FROM '.../src/test/regress/data/tenk.data';
> COPY tenk1 FROM '.../src/test/regress/data/tenk.data';
> DELETE FROM tenk1;
> COPY tenk1 FROM '.../src/test/regress/data/tenk.data';
>
> SELECT COUNT(*) FROM tenk1 t1 LEFT JOIN
> LATERAL (SELECT t1.twenty, t2.two FROM tenk1 t2) s
> ON t1.two = s.two
> WHERE s.twenty < 0;
>
> with
> shared_buffers = 16MB
> triggers:
> TRAP: failed Assert("BufferIsPinned(buffer)"), File: "bufmgr.c", Line: 4233,
> PID: 2785555
>
> Core was generated by `postgres: law regression [local] SELECT
> '.
> Program terminated with signal SIGABRT, Aborted.
> ...
> #0 __pthread_kill_implementation (no_tid=0, signo=6, threadid=<optimized
> out>) at ./nptl/pthread_kill.c:44
> #1 __pthread_kill_internal (signo=6, threadid=<optimized out>) at
> ./nptl/pthread_kill.c:78
> #2 __GI___pthread_kill (threadid=<optimized out>, signo=signo@entry=6) at
> ./nptl/pthread_kill.c:89
> #3 0x000075efce04527e in __GI_raise (sig=sig@entry=6) at
> ../sysdeps/posix/raise.c:26
> #4 0x000075efce0288ff in __GI_abort () at ./stdlib/abort.c:79
> #5 0x00006419a43d5e3f in ExceptionalCondition
> (conditionName=conditionName@entry=0x6419a445bb56 "BufferIsPinned(buffer)",
> fileName=fileName@entry=0x6419a445b9b7 "bufmgr.c",
> lineNumber=lineNumber@entry=4233) at assert.c:66
> #6 0x00006419a423f028 in BufferGetBlockNumber (buffer=<optimized out>) at
> bufmgr.c:4233
> #7 0x00006419a42445c5 in StartReadBuffersImpl (allow_forwarding=true,
> flags=0, nblocks=0x7fff9765ee74, blockNum=710, buffers=0x6419bc3d127c,
> operation=0x6419bc3d1428) at bufmgr.c:1292
> #8 StartReadBuffers (operation=0x6419bc3d1428,
> buffers=buffers@entry=0x6419bc3d127c, blockNum=710,
> nblocks=nblocks@entry=0x7fff9765ee74, flags=flags@entry=0) at bufmgr.c:1500
> #9 0x00006419a423afc4 in read_stream_start_pending_read
> (stream=stream@entry=0x6419bc3d1218) at read_stream.c:335
> #10 0x00006419a423b3df in read_stream_look_ahead
> (stream=stream@entry=0x6419bc3d1218) at read_stream.c:493
> #11 0x00006419a423b7b3 in read_stream_next_buffer (stream=0x6419bc3d1218,
> per_buffer_data=per_buffer_data@entry=0x0) at read_stream.c:971
> #12 0x00006419a3efaede in heap_fetch_next_buffer (dir=<optimized out>,
> scan=0x6419bc3d0cf8) at heapam.c:675
> #13 heapgettup_pagemode (scan=scan@entry=0x6419bc3d0cf8, dir=<optimized
> out>, nkeys=<optimized out>, key=<optimized out>) at heapam.c:1037
> #14 0x00006419a3efb5d2 in heap_getnextslot (sscan=0x6419bc3d0cf8,
> direction=<optimized out>, slot=0x6419bc3c11a8) at heapam.c:1391
> #15 0x00006419a40d1503 in table_scan_getnextslot (slot=0x6419bc3c11a8,
> direction=ForwardScanDirection, sscan=<optimized out>) at
> ../../../src/include/access/tableam.h:1031
> #16 SeqNext (node=node@entry=0x6419bc3c0f88) at nodeSeqscan.c:81
> #17 0x00006419a40d197f in ExecScanFetch (recheckMtd=0x6419a40d1490
> <SeqRecheck>, accessMtd=0x6419a40d14a0 <SeqNext>, epqstate=0x0,
> node=<optimized out>) at ../../../src/include/executor/execScan.h:126
> #18 ExecScanExtended (projInfo=0x0, qual=0x6419bc38a650, epqstate=0x0,
> recheckMtd=0x6419a40d1490 <SeqRecheck>, accessMtd=0x6419a40d14a0 <SeqNext>,
> node=0x6419bc3c0f88) at ../../../src/include/executor/execScan.h:187
> #19 ExecSeqScanWithQual (pstate=0x6419bc3c0f88) at nodeSeqscan.c:138
> #20 0x00006419a40cf97b in ExecProcNode (node=0x6419bc3c0f88) at
> ../../../src/include/executor/executor.h:313
> #21 ExecNestLoop (pstate=<optimized out>) at nodeNestloop.c:159
> ...
>
> Reproduced starting from 12ce89fd0.
>
Thanks for reporting this. I can reproduce this issue reliably when
shared_buffers is set to 16 MB; raising the value allows the query to
complete normally. However, I don't have the expertise to investigate
and fix it. Perhaps we could CC Thomas on this thread to see whether
he might have time to look into it.
Best,
Xuneng
^ permalink raw reply [nested|flat] 29+ messages in thread
* Re: BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer
2025-08-03 12:00 BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer PG Bug reporting form <noreply@postgresql.org>
2025-08-04 13:13 ` Re: BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer Xuneng Zhou <xunengzhou@gmail.com>
@ 2025-08-04 19:19 ` Thomas Munro <thomas.munro@gmail.com>
2025-08-05 04:29 ` Re: BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer Thomas Munro <thomas.munro@gmail.com>
0 siblings, 1 reply; 29+ messages in thread
From: Thomas Munro @ 2025-08-04 19:19 UTC (permalink / raw)
To: Xuneng Zhou <xunengzhou@gmail.com>; +Cc: exclusion@gmail.com, pgsql-bugs@lists.postgresql.org, Michael Paquier <michael@paquier.xyz>
Thanks. Repro'd here. Looking...
^ permalink raw reply [nested|flat] 29+ messages in thread
* Re: BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer
2025-08-03 12:00 BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer PG Bug reporting form <noreply@postgresql.org>
2025-08-04 13:13 ` Re: BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer Xuneng Zhou <xunengzhou@gmail.com>
2025-08-04 19:19 ` Re: BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer Thomas Munro <thomas.munro@gmail.com>
@ 2025-08-05 04:29 ` Thomas Munro <thomas.munro@gmail.com>
2025-08-05 04:43 ` Re: BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer Tom Lane <tgl@sss.pgh.pa.us>
2025-08-07 15:09 ` Re: BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer Xuneng Zhou <xunengzhou@gmail.com>
0 siblings, 2 replies; 29+ messages in thread
From: Thomas Munro @ 2025-08-05 04:29 UTC (permalink / raw)
To: Xuneng Zhou <xunengzhou@gmail.com>; +Cc: exclusion@gmail.com, pgsql-bugs@lists.postgresql.org, Michael Paquier <michael@paquier.xyz>
Here's my proposed fix. Great reproducer, Alexander, thanks!
Attachments:
[text/x-patch] 0001-Fix-bug-in-read_stream.c-s-split-IO-handling.patch (3.2K, ../../CA+hUKGJyqwQs_kQFB1J=bx4pHMrMcPF8_PgNKyeNFEo+yNvyiQ@mail.gmail.com/2-0001-Fix-bug-in-read_stream.c-s-split-IO-handling.patch)
download | inline diff:
From 3b033fa79e3963cbdeb2300508164d00af61f124 Mon Sep 17 00:00:00 2001
From: Thomas Munro <thomas.munro@gmail.com>
Date: Tue, 5 Aug 2025 14:35:09 +1200
Subject: [PATCH] Fix bug in read_stream.c's split IO handling.
If a circular queue wraparound, a multi-block IO split and a transition
to the fast path happened in a certain sequence, a buffer forwarded
from one StartReadBuffers() call to next would not be cleared out.
cleared out. That could confuse a later queue-wrapping
StartReadBuffers() call by passing it a random unpinned buffer.
Fix, and add some tighter and earlier assertions about the layout and
identity of buffers forwarded across IO splits, and the following
entries that should have been cleared before reuse.
Defect in commit ed0b87ca.
Bug: 19006
Backpatch-through: 18
Reported-by: Alexander Lakhin <exclusion@gmail.com>
Discussion: https://postgr.es/m/19006-80fcaaf69000377e%40postgresql.org
---
src/backend/storage/aio/read_stream.c | 32 +++++++++++++++++++++++++++
1 file changed, 32 insertions(+)
diff --git a/src/backend/storage/aio/read_stream.c b/src/backend/storage/aio/read_stream.c
index 0e7f5557f5c..a9cfe3347c8 100644
--- a/src/backend/storage/aio/read_stream.c
+++ b/src/backend/storage/aio/read_stream.c
@@ -253,6 +253,25 @@ read_stream_start_pending_read(ReadStream *stream)
else
Assert(stream->next_buffer_index == stream->oldest_buffer_index);
+ /*
+ * Pinned buffers forwarded by a preceding StartReadBuffers() call that
+ * had to split the operation should match the leading blocks of this
+ * following StartReadBuffers() call.
+ */
+ Assert(stream->forwarded_buffers <= stream->pending_read_nblocks);
+ for (int i = 0; i < stream->forwarded_buffers; ++i)
+ Assert(BufferGetBlockNumber(stream->buffers[stream->next_buffer_index + i]) ==
+ stream->pending_read_blocknum + i);
+
+ /*
+ * Check that we've cleared the queue/overflow entries corresponding to
+ * the rest of the blocks covered by this read, unless it's the first go
+ * around and we haven't even initialized them yet.
+ */
+ for (int i = stream->forwarded_buffers; i < stream->pending_read_nblocks; ++i)
+ Assert(stream->next_buffer_index + i >= stream->initialized_buffers ||
+ stream->buffers[stream->next_buffer_index + i] == InvalidBuffer);
+
/* Do we need to issue read-ahead advice? */
flags = stream->read_buffers_flags;
if (stream->advice_enabled)
@@ -979,6 +998,19 @@ read_stream_next_buffer(ReadStream *stream, void **per_buffer_data)
stream->pending_read_nblocks == 0 &&
stream->per_buffer_data_size == 0)
{
+ /*
+ * The fast path spins on one buffer entry repeatedly instead of
+ * rotating through the whole queue and clearing the entries behind
+ * it. If the buffer it starts with happened to be forwarded between
+ * StartReadBuffers() calls and also wrapped around the circular queue
+ * partway through, then a copy also exists in the overflow zone, and
+ * it won't clear it out as the regular path would. Do that now, so
+ * it doesn't need code for that.
+ */
+ if (stream->oldest_buffer_index < stream->io_combine_limit - 1)
+ stream->buffers[stream->queue_size + stream->oldest_buffer_index] =
+ InvalidBuffer;
+
stream->fast_path = true;
}
#endif
--
2.47.2
^ permalink raw reply [nested|flat] 29+ messages in thread
* Re: BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer
2025-08-03 12:00 BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer PG Bug reporting form <noreply@postgresql.org>
2025-08-04 13:13 ` Re: BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer Xuneng Zhou <xunengzhou@gmail.com>
2025-08-04 19:19 ` Re: BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer Thomas Munro <thomas.munro@gmail.com>
2025-08-05 04:29 ` Re: BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer Thomas Munro <thomas.munro@gmail.com>
@ 2025-08-05 04:43 ` Tom Lane <tgl@sss.pgh.pa.us>
2025-08-06 08:40 ` Re: BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer Michael Paquier <michael@paquier.xyz>
1 sibling, 1 reply; 29+ messages in thread
From: Tom Lane @ 2025-08-05 04:43 UTC (permalink / raw)
To: Thomas Munro <thomas.munro@gmail.com>; +Cc: Xuneng Zhou <xunengzhou@gmail.com>; exclusion@gmail.com, pgsql-bugs@lists.postgresql.org, Michael Paquier <michael@paquier.xyz>
Thomas Munro <thomas.munro@gmail.com> writes:
> Here's my proposed fix. Great reproducer, Alexander, thanks!
I've not looked at the issue actually being fixed, but a drive-by
comment: these loops
+ for (int i = 0; i < stream->forwarded_buffers; ++i)
+ Assert(BufferGetBlockNumber(stream->buffers[stream->next_buffer_index + i]) ==
+ stream->pending_read_blocknum + i);
should be wrapped in "#ifdef USE_ASSERT_CHECKING". Maybe the
compiler is smart enough to throw away the useless looping logic
in a production build, or maybe it isn't.
regards, tom lane
^ permalink raw reply [nested|flat] 29+ messages in thread
* Re: BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer
2025-08-03 12:00 BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer PG Bug reporting form <noreply@postgresql.org>
2025-08-04 13:13 ` Re: BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer Xuneng Zhou <xunengzhou@gmail.com>
2025-08-04 19:19 ` Re: BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer Thomas Munro <thomas.munro@gmail.com>
2025-08-05 04:29 ` Re: BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer Thomas Munro <thomas.munro@gmail.com>
2025-08-05 04:43 ` Re: BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer Tom Lane <tgl@sss.pgh.pa.us>
@ 2025-08-06 08:40 ` Michael Paquier <michael@paquier.xyz>
2025-08-07 11:37 ` Re: BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer Xuneng Zhou <xunengzhou@gmail.com>
0 siblings, 1 reply; 29+ messages in thread
From: Michael Paquier @ 2025-08-06 08:40 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Thomas Munro <thomas.munro@gmail.com>; Xuneng Zhou <xunengzhou@gmail.com>; exclusion@gmail.com; pgsql-bugs@lists.postgresql.org
On Tue, Aug 05, 2025 at 12:43:06AM -0400, Tom Lane wrote:
> I've not looked at the issue actually being fixed, but a drive-by
> comment: these loops
>
> + for (int i = 0; i < stream->forwarded_buffers; ++i)
> + Assert(BufferGetBlockNumber(stream->buffers[stream->next_buffer_index + i]) ==
> + stream->pending_read_blocknum + i);
>
> should be wrapped in "#ifdef USE_ASSERT_CHECKING". Maybe the
> compiler is smart enough to throw away the useless looping logic
> in a production build, or maybe it isn't.
I'd bet it is usually not that smart.. Embedding these in an extra
#ifdef is a sound defense IMO.
--
Michael
Attachments:
[application/pgp-signature] signature.asc (832B, ../../aJMVAbYRcdImR3-V@paquier.xyz/2-signature.asc)
download
^ permalink raw reply [nested|flat] 29+ messages in thread
* Re: BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer
2025-08-03 12:00 BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer PG Bug reporting form <noreply@postgresql.org>
2025-08-04 13:13 ` Re: BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer Xuneng Zhou <xunengzhou@gmail.com>
2025-08-04 19:19 ` Re: BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer Thomas Munro <thomas.munro@gmail.com>
2025-08-05 04:29 ` Re: BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer Thomas Munro <thomas.munro@gmail.com>
2025-08-05 04:43 ` Re: BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer Tom Lane <tgl@sss.pgh.pa.us>
2025-08-06 08:40 ` Re: BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer Michael Paquier <michael@paquier.xyz>
@ 2025-08-07 11:37 ` Xuneng Zhou <xunengzhou@gmail.com>
0 siblings, 0 replies; 29+ messages in thread
From: Xuneng Zhou @ 2025-08-07 11:37 UTC (permalink / raw)
To: Michael Paquier <michael@paquier.xyz>; +Cc: Tom Lane <tgl@sss.pgh.pa.us>; Thomas Munro <thomas.munro@gmail.com>; exclusion@gmail.com; pgsql-bugs@lists.postgresql.org
Hi,
On Wed, Aug 6, 2025 at 4:40 PM Michael Paquier <michael@paquier.xyz> wrote:
>
> On Tue, Aug 05, 2025 at 12:43:06AM -0400, Tom Lane wrote:
> > I've not looked at the issue actually being fixed, but a drive-by
> > comment: these loops
> >
> > + for (int i = 0; i < stream->forwarded_buffers; ++i)
> > + Assert(BufferGetBlockNumber(stream->buffers[stream->next_buffer_index + i]) ==
> > + stream->pending_read_blocknum + i);
> >
> > should be wrapped in "#ifdef USE_ASSERT_CHECKING". Maybe the
> > compiler is smart enough to throw away the useless looping logic
> > in a production build, or maybe it isn't.
>
> I'd bet it is usually not that smart.. Embedding these in an extra
> #ifdef is a sound defense IMO.
> --
> Michael
+1
Best,
Xuneng
^ permalink raw reply [nested|flat] 29+ messages in thread
* Re: BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer
2025-08-03 12:00 BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer PG Bug reporting form <noreply@postgresql.org>
2025-08-04 13:13 ` Re: BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer Xuneng Zhou <xunengzhou@gmail.com>
2025-08-04 19:19 ` Re: BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer Thomas Munro <thomas.munro@gmail.com>
2025-08-05 04:29 ` Re: BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer Thomas Munro <thomas.munro@gmail.com>
@ 2025-08-07 15:09 ` Xuneng Zhou <xunengzhou@gmail.com>
2025-08-08 02:43 ` Re: BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer Thomas Munro <thomas.munro@gmail.com>
1 sibling, 1 reply; 29+ messages in thread
From: Xuneng Zhou @ 2025-08-07 15:09 UTC (permalink / raw)
To: Thomas Munro <thomas.munro@gmail.com>; +Cc: exclusion@gmail.com, pgsql-bugs@lists.postgresql.org, Michael Paquier <michael@paquier.xyz>; Tom Lane <tgl@sss.pgh.pa.us>
Hi,
On Tue, Aug 5, 2025 at 12:29 PM Thomas Munro <thomas.munro@gmail.com> wrote:
>
> Here's my proposed fix. Great reproducer, Alexander, thanks!
I tested the fix, it works well now. I tried to review the code logic
but found it hard to figure out this line on my own due to the
unfamiliarity with this module currently:
stream->buffers[stream->queue_size + stream->oldest_buffer_index] =
InvalidBuffer;
Could you please elaborate on it a little bit more? Thanks.
Best,
Xuneng
^ permalink raw reply [nested|flat] 29+ messages in thread
* Re: BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer
2025-08-03 12:00 BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer PG Bug reporting form <noreply@postgresql.org>
2025-08-04 13:13 ` Re: BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer Xuneng Zhou <xunengzhou@gmail.com>
2025-08-04 19:19 ` Re: BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer Thomas Munro <thomas.munro@gmail.com>
2025-08-05 04:29 ` Re: BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer Thomas Munro <thomas.munro@gmail.com>
2025-08-07 15:09 ` Re: BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer Xuneng Zhou <xunengzhou@gmail.com>
@ 2025-08-08 02:43 ` Thomas Munro <thomas.munro@gmail.com>
2025-08-08 03:48 ` Re: BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer Thomas Munro <thomas.munro@gmail.com>
2025-08-08 04:39 ` Re: BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer Xuneng Zhou <xunengzhou@gmail.com>
2025-08-13 05:29 ` Re: BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer Xuneng Zhou <xunengzhou@gmail.com>
0 siblings, 3 replies; 29+ messages in thread
From: Thomas Munro @ 2025-08-08 02:43 UTC (permalink / raw)
To: Xuneng Zhou <xunengzhou@gmail.com>; +Cc: exclusion@gmail.com, pgsql-bugs@lists.postgresql.org, Michael Paquier <michael@paquier.xyz>; Tom Lane <tgl@sss.pgh.pa.us>
On Fri, Aug 8, 2025 at 3:09 AM Xuneng Zhou <xunengzhou@gmail.com> wrote:
> Could you please elaborate on it a little bit more? Thanks.
Sure. It's a bit complicated and involves many moving parts, so let
me try to summarise the read_stream.c fast path, forwarded buffers and
the motivation and complexities, and then finally the bug. This email
turned out longer than I feared so sorry for the wall of text, but it
seemed useful to work through it with illustrations to help more
people understand this stuff and contribute improvements, and also
recheck my assumptions at the same time.
New version with #ifdefs. I believe that qualified for trivial dead
code elimination in optimized builds but yeah it's still better this
way, thanks all. This is the version I plan to push if there are no
further comments.
Before digging further, a brief recap of read_stream.c and its fast path:
The goal is to be able to perform the equivalent of a stream of
ReadBuffer() while minimising system calls, IOPS and I/O stalls by
looking a variable distance ahead in the block number stream,
combining consecutive blocks into a wider read operation according to
your io_combine_limit setting, and running some number of I/Os
asynchronously up to your {effective,maintenance}_io_concurrency
setting. It starts off looking just one block into the future, but
adjusts it up and down according to the history of demand and cache
misses, as a prediction of future I/O needs.
If the blocks are all cached, the lookahead distance is minimised and
it becomes very similar to a plain loop of ReadBuffer() calls, since
I/O and thus I/O combining and concurrency aren't predicted to be
useful. It has a special fast path mode to squeeze a bit more
performance out of the extreme case of all-cached blocks that don't
use the facility for streaming optional data along with each block.
It uses specialised code in read_stream.c and bufmgr.c to minimise
bookkeeping, branching and data cache misses through queue maintenance
and movement.
The bug involves multiple fast path transitions and short reads, so
next let me describe the bufmgr.c's interface for combined and
asynchronous I/O and the need for short reads:
read_stream.c works by issuing overlapping pairs of calls to
StartReadBuffers() and WaitReadBuffers(), with stream->ios holding the
queue of IOs with a pending call to WaitReadBuffers(). The interface
of StartReadBuffers() is more complex than ReadBuffer() because it has
to cope with a lot of edge cases. It takes a relation and fork, a
block number, an input/output nblocks count, and an input/output array
of buffers. It returns true if you need to call WaitReadBuffers(),
meaning the operation wasn't completed immediately from cache. The
reason nblocks is input/output is that the operation must sometimes be
"split", also known as a short read.
It splits IOs partly because we want StartReadBuffers() to start
exactly zero or one IOs, and the caller would lose control of the I/O
depth if StartReadBuffers() were capable of starting multiple IOs. We
also want to avoid some avoidable waits. Here are some reasons for
splits:
1. The leading block is BM_VALID, so we choose to give it to you
immediately and not look further (we could look further and return
more than one consecutive BM_VALID block at a time, but this isn't
implemented)
2. An internal block is BM_VALID, so any non-BM_VALID blocks either
side of it can't be combined into one operation
3. An internal block is BM_IO_IN_PROGRESS, meaning that another
backend is reading or writing it
4. An internal or final block is in a different 1GB segment file
(Reason 3 will usually turn in to reason 2 eventually, or the other
backend's IO attempt might in theory fail and then it'll be this
backend's turn to try, and there are some slightly complicated rules
about whether we have to wait for that or split the IO here, for
deadlock avoidance and progress guarantees.)
Forwarded buffers are an artefact of the short reads. Let me
summarize what changed in v18:
In v17, we didn't have real AIO yet, just I/O combining and pseudo-AIO
with POSIX_FADV_WILLNEED, so StartReadBuffers() pinned buffers and
possibly shortened the read for reasons #1 and #2. This meant that it
pinned at most one extra buffer that terminated the operation, and it
used a bit of a kludge do deal with that: for reason #1 you get the
buffer, and for reason #2, it pretended that block was part of the IO,
it just didn't include it in the actual read operation. This was
simple, but not scalable to later work. Then WaitReadBuffers() tried
to acquire the BM_IO_IN_PROGRESS flags for all the blocks, and "hid"
all four split reasons by looping: it skipped buffers that had become
BM_VALID in the meantime (someone else read them) and waited for
BM_IO_IN_PROGRESS to become BM_VALID (or not if another backend's read
failed and this backend could now try), and then md.c internally
looped to handle reason #4. WaitReadBuffers() *had* to acquire
BM_IO_IN_PROGRESS in this second phase when I/O was really synchronous
and StartReadBuffers() was just feeding advice to the kernel: if we
acquired it sooner then other backends might wait for this backend to
reach WaitReadBuffers(), which could deadlock, and we had no
infrastructure to deal with that. In other words, I/Os were already
split for all the same reasons, but #3 and #4 were handled at the
later WaitReadBuffers() stage and in secret.
In v18, the handling of reasons #3 and #4 moved into
StartReadBuffers(). By definition we have to try to acquire
BM_IO_IN_PROGRESS at that earlier stage now, because the IO really is
in progress, and the deadlock risk is gone, because any backend can
drive an IO started by any other backend to completion using the
associated PgAioHandle.
This increased the pin management problem: StartReadBuffers() can
acquire a lot of pins, up to 128 (io_combine_limit=1MB) and then
discover a reason to split the IO. So what do do with all those pins?
A simple answer would be: unpin all the buffers after the split
point.
We didn't want to do that, because read_stream.c is almost certainly
going to call StartReadBuffers() for the rest of the blocks (and
possibly more that it now wants after those ones). So we wanted a way
to transfer them from one StartReadBuffer() call to the following one,
avoiding an unpin-and-repin dance. This was quite a difficult API
design question, and this isn't the only design we considered, but we
settled on making the buffer array that you give to StartReadBuffers()
an input/output array for transporting these extra pinned buffers
between calls, while in v17 it was a output-only array. In v18, you
have to fill it up with InvalidBuffer initially. If it wants to
report that it has pinned buffers but then excluded them from a read
that is now a short read, it writes them into that array, but it
doesn't count them in the *nblocks value that it gives you. The term
I dreamed up for that is "forwarded" buffers. It is the caller's job
to pass them in as input to the next StartReadBuffers() call, or if
for whatever reason it doesn't want to continue, it has to unpin them.
Next, let me describe how read_stream.c manages its circular buffer queue:
|<------------- queue_size ------------>|
+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
| | | | | | |0|1|2|3|4|5|6|7|8| | | | | |
+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
^ ^
| |
oldest_buffer_index next_buffer_index
oldest_buffer_index is the consumer end of the queue, and any
associated IO has to be finished before that buffer can be return by
read_stream_next_buffer(). next_buffer_index is the producer end of
the queue, where IOs are started. StartReadBuffers() takes pointer to
an array of buffers of the size of the requested read, so
read_stream.c gives it the address of that part of its queue.
This means that if StartReadBuffer() forwards some buffers because of
a split, we don't have to do anything at all, because we advance
next_buffer_index to the location of the start of the next operation,
and that is where the forwarded buffers landed, ready for the next
call to StartReadBuffers():
|<------------- queue_size ------------>|
+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
| | | | | | |0|1|2|3|4|5|6|7|8|9|A| | | |
+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
^ ^
| |
oldest_buffer_index next_buffer_index
Well, not quite nothing, we do count how many there are in
stream->forwarded_buffers. That's used by the code that limits
lookahead when an unrealistically small buffer pool reports that we
hold too many pins already. Those are pins that this backend holds,
so they are not counted as extra pins that StartReadBuffer() would
acquire.
That's a nice way for successive StartReadBuffers() calls to
communicate, without having to copy them to and from some other
storage between calls. In exchange for that convenience, we also need
to fill the entries with InvalidBuffer initially so they don't look
like forwarded buffers. We don't want to do that up front: the queue
might be up to {effective,maintenance}_io_concurrency *
io_combine_limit in size (highest possible value: 128,000), and if
this is a small or perfectly cached scan then we will never use most
of the queue. So, we:
* initialize them on the first go around the queue only, tracked in
stream->initialized_buffers
* invalidate entries consumed by read_stream_next_buffer() in
preparation for reuse
The fast path repeatedly uses the same queue entry instead of
advancing oldest_buffer_index and next_buffer_index, so it doesn't
need to invalidate queue entries, saving an instruction and reducing
cache line pollution. Now I am getting pretty close to explaining the
bug, but there is one final complication...
Sometimes a StartReadBuffers() call spans past the physical end of the
queue. It needs to wrap around. We could have chosen to give
StartReadBuffers() an iov, iov_cnt style interface like preadv() so
that we could use iov_cnt = 2 and locate some buffers at the physical
end of the queue and the rest at the physical start of the queue, but
StartReadBuffers()'s interface was complicated enough already. The
solution I went with was to allocate extra array entries after the
queue to allow for the maximum possible overflow if beginning a read
into a buffer located in the final queue entry. The overflowing
buffers are immediately copied to the physical start of the queue,
ready for consumption in regular circular queue position. Note the
twin positions for buffer 2 in this example:
|<------------- queue_size ------------>|<--- overflow_size --->|
+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
|2|3|4|5|6|7| | | | | | | | | | | | |0|1|2|3|4|5|6|7| | | | | | |
+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
^ ^
| |
next_buffer_index oldest_buffer_index
(Perhaps it would be clearer if overflow_size were a member variable
in ReadStream instead of being open coded as io_combine_limit - 1 in a
few places.)
The non-fast-path invalidates the queue entry holding the buffer it is
returning to the caller of read_stream_next_buffer(), as required to
mark it as "not a forwarded buffer" when reused next time we go around
the circular queue. If it's in the low physical entries that also
might have a twin in the overflow zone, it must also be invalidated.
The reason we keep both copies for a bit longer is that we don't know
whether the following StartReadBuffers() will also be split, so we
don't know whether it will also overflow, so we allow for any number
of following StartReadBuffers() calls to overflow too OR move to the
physical start of the queue.
Now I can finally explain the bug :-)
The fast-path doesn't have to invalidate queue entries, because it
spins on the same queue entry repeatedly as an optimisation.
Unfortunately I forgot to consider that there might be a copy of it in
the overflow zone. If there is a multi-block StartReadBuffers() that
spans into the overflow zone but is split, a continuing
StartReadBuffers(), and then all the requirements for entering fast
path mode are met, the fast path begins to spin on one buffer entry
repeatedly near the physical start of the queue, and it initially
holds a buffer that was copied from the overflow zone. Nothing goes
wrong yet, but then we leave fast path mode because we need to do I/O,
and begin to cycle around the queue again in the regular non-fast path
code. Eventually we have another StartReadBuffers() call that spans
into the overflow zone, and bufmgr.c sees a stale buffer number that
we failed to clear out earlier. It thinks it's a forwarded buffer.
Forwarded buffers must be pinned and map to blocknum + i, and it
asserts that to avoid corrupting the buffer pool by overwriting a
random buffer, which failed due to the bug.
I don't want the fast path to do extra steps. The solution I found
was to clear the overflow entry *before* entering the fast path. We
already know which buffer entry the fast path is going to use, and we
also know that there is no I/O split needing to be continued as part
of the conditions of entry to the fast path, so the solution is simply
to clear any corresponding overflow entry, very slightly ahead of the
usual time.
Attachments:
[application/octet-stream] v2-0001-Fix-bug-in-read_stream.c-s-split-IO-handling.patch (3.6K, ../../CA+hUKG+gwez63UpPt1-u2rosh3rW3VeKdve23u-=fi9KWAyz5w@mail.gmail.com/2-v2-0001-Fix-bug-in-read_stream.c-s-split-IO-handling.patch)
download | inline diff:
From 619aea104351cf1bc103fa7161e62e93bbacafa4 Mon Sep 17 00:00:00 2001
From: Thomas Munro <thomas.munro@gmail.com>
Date: Tue, 5 Aug 2025 14:35:09 +1200
Subject: [PATCH v2] Fix bug in read_stream.c's split IO handling.
If a circular queue wraparound, a multi-block IO split and a transition
to the fast path happened in a certain sequence, a buffer forwarded from
one StartReadBuffers() call to next would not be cleared out. That
could confuse a later queue-wrapping StartReadBuffers() call by passing
it a random buffer.
Fix, and add some tighter and earlier assertions about the layout and
identity of buffers forwarded across IO splits, and the following
entries that should have been cleared before reuse.
Defect in commit ed0b87ca.
Bug: 19006
Backpatch-through: 18
Reported-by: Alexander Lakhin <exclusion@gmail.com>
Reviewed-by: Tom Lane <tgl@sss.pgh.pa.us>
Reviewed-by: Michael Paquier <michael@paquier.xyz>
Reviewed-by: Xuneng Zhou <xunengzhou@gmail.com>
Discussion: https://postgr.es/m/19006-80fcaaf69000377e%40postgresql.org
---
src/backend/storage/aio/read_stream.c | 34 +++++++++++++++++++++++++++
1 file changed, 34 insertions(+)
diff --git a/src/backend/storage/aio/read_stream.c b/src/backend/storage/aio/read_stream.c
index 0e7f5557f5c..031fde9f4cb 100644
--- a/src/backend/storage/aio/read_stream.c
+++ b/src/backend/storage/aio/read_stream.c
@@ -247,12 +247,33 @@ read_stream_start_pending_read(ReadStream *stream)
Assert(stream->pinned_buffers + stream->pending_read_nblocks <=
stream->max_pinned_buffers);
+#ifdef USE_ASSERT_CHECKING
/* We had better not be overwriting an existing pinned buffer. */
if (stream->pinned_buffers > 0)
Assert(stream->next_buffer_index != stream->oldest_buffer_index);
else
Assert(stream->next_buffer_index == stream->oldest_buffer_index);
+ /*
+ * Pinned buffers forwarded by a preceding StartReadBuffers() call that
+ * had to split the operation should match the leading blocks of this
+ * following StartReadBuffers() call.
+ */
+ Assert(stream->forwarded_buffers <= stream->pending_read_nblocks);
+ for (int i = 0; i < stream->forwarded_buffers; ++i)
+ Assert(BufferGetBlockNumber(stream->buffers[stream->next_buffer_index + i]) ==
+ stream->pending_read_blocknum + i);
+
+ /*
+ * Check that we've cleared the queue/overflow entries corresponding to
+ * the rest of the blocks covered by this read, unless it's the first go
+ * around and we haven't even initialized them yet.
+ */
+ for (int i = stream->forwarded_buffers; i < stream->pending_read_nblocks; ++i)
+ Assert(stream->next_buffer_index + i >= stream->initialized_buffers ||
+ stream->buffers[stream->next_buffer_index + i] == InvalidBuffer);
+#endif
+
/* Do we need to issue read-ahead advice? */
flags = stream->read_buffers_flags;
if (stream->advice_enabled)
@@ -979,6 +1000,19 @@ read_stream_next_buffer(ReadStream *stream, void **per_buffer_data)
stream->pending_read_nblocks == 0 &&
stream->per_buffer_data_size == 0)
{
+ /*
+ * The fast path spins on one buffer entry repeatedly instead of
+ * rotating through the whole queue and clearing the entries behind
+ * it. If the buffer it starts with happened to be forwarded between
+ * StartReadBuffers() calls and also wrapped around the circular queue
+ * partway through, then a copy also exists in the overflow zone, and
+ * it won't clear it out as the regular path would. Do that now, so
+ * it doesn't need code for that.
+ */
+ if (stream->oldest_buffer_index < stream->io_combine_limit - 1)
+ stream->buffers[stream->queue_size + stream->oldest_buffer_index] =
+ InvalidBuffer;
+
stream->fast_path = true;
}
#endif
--
2.39.5 (Apple Git-154)
^ permalink raw reply [nested|flat] 29+ messages in thread
* Re: BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer
2025-08-03 12:00 BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer PG Bug reporting form <noreply@postgresql.org>
2025-08-04 13:13 ` Re: BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer Xuneng Zhou <xunengzhou@gmail.com>
2025-08-04 19:19 ` Re: BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer Thomas Munro <thomas.munro@gmail.com>
2025-08-05 04:29 ` Re: BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer Thomas Munro <thomas.munro@gmail.com>
2025-08-07 15:09 ` Re: BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer Xuneng Zhou <xunengzhou@gmail.com>
2025-08-08 02:43 ` Re: BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer Thomas Munro <thomas.munro@gmail.com>
@ 2025-08-08 03:48 ` Thomas Munro <thomas.munro@gmail.com>
2 siblings, 0 replies; 29+ messages in thread
From: Thomas Munro @ 2025-08-08 03:48 UTC (permalink / raw)
To: Xuneng Zhou <xunengzhou@gmail.com>; +Cc: exclusion@gmail.com, pgsql-bugs@lists.postgresql.org, Michael Paquier <michael@paquier.xyz>; Tom Lane <tgl@sss.pgh.pa.us>
On Fri, Aug 8, 2025 at 2:43 PM Thomas Munro <thomas.munro@gmail.com> wrote:
> We don't want to do that up front: the queue
> might be up to {effective,maintenance}_io_concurrency *
> io_combine_limit in size (highest possible value: 128,000), and if
> this is a small or perfectly cached scan then we will never use most
> of the queue.
Correction: it's actually clamped to fit into int16_t arithmetic so it
can't really be 128,000 entries with maximum GUCs, and there are also
a few more entries allocated for overflow and other technical reasons,
but that doesn't change the material point about delaying
initialisation to keep stream initialisation cheap with high GUC
values. I'll probably propose to flip it all to regular int
arithmetic soon anyway as int16_t was only wide enough for v17s GUC
limits that have since been increased, and int16_t turned out to be
all pain and no gain anyway :-)
^ permalink raw reply [nested|flat] 29+ messages in thread
* Re: BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer
2025-08-03 12:00 BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer PG Bug reporting form <noreply@postgresql.org>
2025-08-04 13:13 ` Re: BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer Xuneng Zhou <xunengzhou@gmail.com>
2025-08-04 19:19 ` Re: BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer Thomas Munro <thomas.munro@gmail.com>
2025-08-05 04:29 ` Re: BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer Thomas Munro <thomas.munro@gmail.com>
2025-08-07 15:09 ` Re: BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer Xuneng Zhou <xunengzhou@gmail.com>
2025-08-08 02:43 ` Re: BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer Thomas Munro <thomas.munro@gmail.com>
@ 2025-08-08 04:39 ` Xuneng Zhou <xunengzhou@gmail.com>
2025-08-08 15:06 ` Re: BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer Thomas Munro <thomas.munro@gmail.com>
2 siblings, 1 reply; 29+ messages in thread
From: Xuneng Zhou @ 2025-08-08 04:39 UTC (permalink / raw)
To: Thomas Munro <thomas.munro@gmail.com>; +Cc: exclusion@gmail.com, pgsql-bugs@lists.postgresql.org, Michael Paquier <michael@paquier.xyz>; Tom Lane <tgl@sss.pgh.pa.us>
Hi, Thomas!
Thanks a lot for the exceptionally detailed and insightful
explanation—it’s far more comprehensive than I anticipated. It is
excellent for unpacking the intricate interplay and state transitions
between submodules, which is what makes this topic challenging to
grasp. I’ll need some time to fully digest it, so I’m bookmarking this
email for future reference. I’m also considering turning it into a
blog post after studying—it’s the kind of deep, clear writing that
could benefit more interested audiences.
On Fri, Aug 8, 2025 at 10:44 AM Thomas Munro <thomas.munro@gmail.com> wrote:
>
> On Fri, Aug 8, 2025 at 3:09 AM Xuneng Zhou <xunengzhou@gmail.com> wrote:
> > Could you please elaborate on it a little bit more? Thanks.
>
> Sure. It's a bit complicated and involves many moving parts, so let
> me try to summarise the read_stream.c fast path, forwarded buffers and
> the motivation and complexities, and then finally the bug. This email
> turned out longer than I feared so sorry for the wall of text, but it
> seemed useful to work through it with illustrations to help more
> people understand this stuff and contribute improvements, and also
> recheck my assumptions at the same time.
>
> New version with #ifdefs. I believe that qualified for trivial dead
> code elimination in optimized builds but yeah it's still better this
> way, thanks all. This is the version I plan to push if there are no
> further comments.
>
> Before digging further, a brief recap of read_stream.c and its fast path:
>
> The goal is to be able to perform the equivalent of a stream of
> ReadBuffer() while minimising system calls, IOPS and I/O stalls by
> looking a variable distance ahead in the block number stream,
> combining consecutive blocks into a wider read operation according to
> your io_combine_limit setting, and running some number of I/Os
> asynchronously up to your {effective,maintenance}_io_concurrency
> setting. It starts off looking just one block into the future, but
> adjusts it up and down according to the history of demand and cache
> misses, as a prediction of future I/O needs.
>
> If the blocks are all cached, the lookahead distance is minimised and
> it becomes very similar to a plain loop of ReadBuffer() calls, since
> I/O and thus I/O combining and concurrency aren't predicted to be
> useful. It has a special fast path mode to squeeze a bit more
> performance out of the extreme case of all-cached blocks that don't
> use the facility for streaming optional data along with each block.
> It uses specialised code in read_stream.c and bufmgr.c to minimise
> bookkeeping, branching and data cache misses through queue maintenance
> and movement.
>
> The bug involves multiple fast path transitions and short reads, so
> next let me describe the bufmgr.c's interface for combined and
> asynchronous I/O and the need for short reads:
>
> read_stream.c works by issuing overlapping pairs of calls to
> StartReadBuffers() and WaitReadBuffers(), with stream->ios holding the
> queue of IOs with a pending call to WaitReadBuffers(). The interface
> of StartReadBuffers() is more complex than ReadBuffer() because it has
> to cope with a lot of edge cases. It takes a relation and fork, a
> block number, an input/output nblocks count, and an input/output array
> of buffers. It returns true if you need to call WaitReadBuffers(),
> meaning the operation wasn't completed immediately from cache. The
> reason nblocks is input/output is that the operation must sometimes be
> "split", also known as a short read.
>
> It splits IOs partly because we want StartReadBuffers() to start
> exactly zero or one IOs, and the caller would lose control of the I/O
> depth if StartReadBuffers() were capable of starting multiple IOs. We
> also want to avoid some avoidable waits. Here are some reasons for
> splits:
>
> 1. The leading block is BM_VALID, so we choose to give it to you
> immediately and not look further (we could look further and return
> more than one consecutive BM_VALID block at a time, but this isn't
> implemented)
> 2. An internal block is BM_VALID, so any non-BM_VALID blocks either
> side of it can't be combined into one operation
> 3. An internal block is BM_IO_IN_PROGRESS, meaning that another
> backend is reading or writing it
> 4. An internal or final block is in a different 1GB segment file
>
> (Reason 3 will usually turn in to reason 2 eventually, or the other
> backend's IO attempt might in theory fail and then it'll be this
> backend's turn to try, and there are some slightly complicated rules
> about whether we have to wait for that or split the IO here, for
> deadlock avoidance and progress guarantees.)
>
> Forwarded buffers are an artefact of the short reads. Let me
> summarize what changed in v18:
>
> In v17, we didn't have real AIO yet, just I/O combining and pseudo-AIO
> with POSIX_FADV_WILLNEED, so StartReadBuffers() pinned buffers and
> possibly shortened the read for reasons #1 and #2. This meant that it
> pinned at most one extra buffer that terminated the operation, and it
> used a bit of a kludge do deal with that: for reason #1 you get the
> buffer, and for reason #2, it pretended that block was part of the IO,
> it just didn't include it in the actual read operation. This was
> simple, but not scalable to later work. Then WaitReadBuffers() tried
> to acquire the BM_IO_IN_PROGRESS flags for all the blocks, and "hid"
> all four split reasons by looping: it skipped buffers that had become
> BM_VALID in the meantime (someone else read them) and waited for
> BM_IO_IN_PROGRESS to become BM_VALID (or not if another backend's read
> failed and this backend could now try), and then md.c internally
> looped to handle reason #4. WaitReadBuffers() *had* to acquire
> BM_IO_IN_PROGRESS in this second phase when I/O was really synchronous
> and StartReadBuffers() was just feeding advice to the kernel: if we
> acquired it sooner then other backends might wait for this backend to
> reach WaitReadBuffers(), which could deadlock, and we had no
> infrastructure to deal with that. In other words, I/Os were already
> split for all the same reasons, but #3 and #4 were handled at the
> later WaitReadBuffers() stage and in secret.
>
> In v18, the handling of reasons #3 and #4 moved into
> StartReadBuffers(). By definition we have to try to acquire
> BM_IO_IN_PROGRESS at that earlier stage now, because the IO really is
> in progress, and the deadlock risk is gone, because any backend can
> drive an IO started by any other backend to completion using the
> associated PgAioHandle.
>
> This increased the pin management problem: StartReadBuffers() can
> acquire a lot of pins, up to 128 (io_combine_limit=1MB) and then
> discover a reason to split the IO. So what do do with all those pins?
> A simple answer would be: unpin all the buffers after the split
> point.
>
> We didn't want to do that, because read_stream.c is almost certainly
> going to call StartReadBuffers() for the rest of the blocks (and
> possibly more that it now wants after those ones). So we wanted a way
> to transfer them from one StartReadBuffer() call to the following one,
> avoiding an unpin-and-repin dance. This was quite a difficult API
> design question, and this isn't the only design we considered, but we
> settled on making the buffer array that you give to StartReadBuffers()
> an input/output array for transporting these extra pinned buffers
> between calls, while in v17 it was a output-only array. In v18, you
> have to fill it up with InvalidBuffer initially. If it wants to
> report that it has pinned buffers but then excluded them from a read
> that is now a short read, it writes them into that array, but it
> doesn't count them in the *nblocks value that it gives you. The term
> I dreamed up for that is "forwarded" buffers. It is the caller's job
> to pass them in as input to the next StartReadBuffers() call, or if
> for whatever reason it doesn't want to continue, it has to unpin them.
>
> Next, let me describe how read_stream.c manages its circular buffer queue:
>
> |<------------- queue_size ------------>|
> +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
> | | | | | | |0|1|2|3|4|5|6|7|8| | | | | |
> +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
> ^ ^
> | |
> oldest_buffer_index next_buffer_index
>
> oldest_buffer_index is the consumer end of the queue, and any
> associated IO has to be finished before that buffer can be return by
> read_stream_next_buffer(). next_buffer_index is the producer end of
> the queue, where IOs are started. StartReadBuffers() takes pointer to
> an array of buffers of the size of the requested read, so
> read_stream.c gives it the address of that part of its queue.
>
> This means that if StartReadBuffer() forwards some buffers because of
> a split, we don't have to do anything at all, because we advance
> next_buffer_index to the location of the start of the next operation,
> and that is where the forwarded buffers landed, ready for the next
> call to StartReadBuffers():
>
> |<------------- queue_size ------------>|
> +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
> | | | | | | |0|1|2|3|4|5|6|7|8|9|A| | | |
> +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
> ^ ^
> | |
> oldest_buffer_index next_buffer_index
>
> Well, not quite nothing, we do count how many there are in
> stream->forwarded_buffers. That's used by the code that limits
> lookahead when an unrealistically small buffer pool reports that we
> hold too many pins already. Those are pins that this backend holds,
> so they are not counted as extra pins that StartReadBuffer() would
> acquire.
>
> That's a nice way for successive StartReadBuffers() calls to
> communicate, without having to copy them to and from some other
> storage between calls. In exchange for that convenience, we also need
> to fill the entries with InvalidBuffer initially so they don't look
> like forwarded buffers. We don't want to do that up front: the queue
> might be up to {effective,maintenance}_io_concurrency *
> io_combine_limit in size (highest possible value: 128,000), and if
> this is a small or perfectly cached scan then we will never use most
> of the queue. So, we:
>
> * initialize them on the first go around the queue only, tracked in
> stream->initialized_buffers
> * invalidate entries consumed by read_stream_next_buffer() in
> preparation for reuse
>
> The fast path repeatedly uses the same queue entry instead of
> advancing oldest_buffer_index and next_buffer_index, so it doesn't
> need to invalidate queue entries, saving an instruction and reducing
> cache line pollution. Now I am getting pretty close to explaining the
> bug, but there is one final complication...
>
> Sometimes a StartReadBuffers() call spans past the physical end of the
> queue. It needs to wrap around. We could have chosen to give
> StartReadBuffers() an iov, iov_cnt style interface like preadv() so
> that we could use iov_cnt = 2 and locate some buffers at the physical
> end of the queue and the rest at the physical start of the queue, but
> StartReadBuffers()'s interface was complicated enough already. The
> solution I went with was to allocate extra array entries after the
> queue to allow for the maximum possible overflow if beginning a read
> into a buffer located in the final queue entry. The overflowing
> buffers are immediately copied to the physical start of the queue,
> ready for consumption in regular circular queue position. Note the
> twin positions for buffer 2 in this example:
>
> |<------------- queue_size ------------>|<--- overflow_size --->|
> +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
> |2|3|4|5|6|7| | | | | | | | | | | | |0|1|2|3|4|5|6|7| | | | | | |
> +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
> ^ ^
> | |
> next_buffer_index oldest_buffer_index
>
> (Perhaps it would be clearer if overflow_size were a member variable
> in ReadStream instead of being open coded as io_combine_limit - 1 in a
> few places.)
>
> The non-fast-path invalidates the queue entry holding the buffer it is
> returning to the caller of read_stream_next_buffer(), as required to
> mark it as "not a forwarded buffer" when reused next time we go around
> the circular queue. If it's in the low physical entries that also
> might have a twin in the overflow zone, it must also be invalidated.
> The reason we keep both copies for a bit longer is that we don't know
> whether the following StartReadBuffers() will also be split, so we
> don't know whether it will also overflow, so we allow for any number
> of following StartReadBuffers() calls to overflow too OR move to the
> physical start of the queue.
>
> Now I can finally explain the bug :-)
>
> The fast-path doesn't have to invalidate queue entries, because it
> spins on the same queue entry repeatedly as an optimisation.
> Unfortunately I forgot to consider that there might be a copy of it in
> the overflow zone. If there is a multi-block StartReadBuffers() that
> spans into the overflow zone but is split, a continuing
> StartReadBuffers(), and then all the requirements for entering fast
> path mode are met, the fast path begins to spin on one buffer entry
> repeatedly near the physical start of the queue, and it initially
> holds a buffer that was copied from the overflow zone. Nothing goes
> wrong yet, but then we leave fast path mode because we need to do I/O,
> and begin to cycle around the queue again in the regular non-fast path
> code. Eventually we have another StartReadBuffers() call that spans
> into the overflow zone, and bufmgr.c sees a stale buffer number that
> we failed to clear out earlier. It thinks it's a forwarded buffer.
> Forwarded buffers must be pinned and map to blocknum + i, and it
> asserts that to avoid corrupting the buffer pool by overwriting a
> random buffer, which failed due to the bug.
>
> I don't want the fast path to do extra steps. The solution I found
> was to clear the overflow entry *before* entering the fast path. We
> already know which buffer entry the fast path is going to use, and we
> also know that there is no I/O split needing to be continued as part
> of the conditions of entry to the fast path, so the solution is simply
> to clear any corresponding overflow entry, very slightly ahead of the
> usual time.
Best,
Xuneng
^ permalink raw reply [nested|flat] 29+ messages in thread
* Re: BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer
2025-08-03 12:00 BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer PG Bug reporting form <noreply@postgresql.org>
2025-08-04 13:13 ` Re: BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer Xuneng Zhou <xunengzhou@gmail.com>
2025-08-04 19:19 ` Re: BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer Thomas Munro <thomas.munro@gmail.com>
2025-08-05 04:29 ` Re: BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer Thomas Munro <thomas.munro@gmail.com>
2025-08-07 15:09 ` Re: BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer Xuneng Zhou <xunengzhou@gmail.com>
2025-08-08 02:43 ` Re: BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer Thomas Munro <thomas.munro@gmail.com>
2025-08-08 04:39 ` Re: BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer Xuneng Zhou <xunengzhou@gmail.com>
@ 2025-08-08 15:06 ` Thomas Munro <thomas.munro@gmail.com>
2025-08-09 04:03 ` Re: BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer Thomas Munro <thomas.munro@gmail.com>
0 siblings, 1 reply; 29+ messages in thread
From: Thomas Munro @ 2025-08-08 15:06 UTC (permalink / raw)
To: Xuneng Zhou <xunengzhou@gmail.com>; +Cc: exclusion@gmail.com, pgsql-bugs@lists.postgresql.org, Michael Paquier <michael@paquier.xyz>; Tom Lane <tgl@sss.pgh.pa.us>
On Fri, Aug 8, 2025 at 4:39 PM Xuneng Zhou <xunengzhou@gmail.com> wrote:
> Thanks a lot for the exceptionally detailed and insightful
> explanation—it’s far more comprehensive than I anticipated. It is
> excellent for unpacking the intricate interplay and state transitions
> between submodules, which is what makes this topic challenging to
> grasp. I’ll need some time to fully digest it, so I’m bookmarking this
> email for future reference. I’m also considering turning it into a
> blog post after studying—it’s the kind of deep, clear writing that
> could benefit more interested audiences.
Well, one benefit of trying to describe a complex system from the top
is that higher level stupidity can sometimes become clear... I
realised that the queue wouldn't even need to be initialised or
cleared if StartReadBuffers() had an explicit in/out npinned argument
to transmit the count between calls along with the buffers, or
examined after each call. This is a draft alternative fix that I am
studying that removes quite a lot of lines and fragility, though time
is not on my side so I might push the one-liner fix ahead of the
upcoming freeze and then see what to do about this.
Attachments:
[application/octet-stream] 0001-Fix-and-refactor-read_stream.c-s-split-IO-handling.patch (13.6K, ../../CA+hUKGLbAyme3mXVYxp3m5knodZACT8ZXyj2AU+WdtdvEfvVQA@mail.gmail.com/2-0001-Fix-and-refactor-read_stream.c-s-split-IO-handling.patch)
download | inline diff:
From a40f9adf9e0867ab208d15a80b5326aa1ca68d48 Mon Sep 17 00:00:00 2001
From: Thomas Munro <thomas.munro@gmail.com>
Date: Tue, 5 Aug 2025 14:35:09 +1200
Subject: [PATCH] Fix and refactor read_stream.c's split IO handling.
If a circular queue wraparound, a multi-block IO split and a transition
to the fast path happened in a certain sequence, a buffer forwarded from
one StartReadBuffers() call to next would not be cleared out. That
could confuse a later queue-wrapping StartReadBuffers() call by passing
it a random buffer. Fixing that rare case would be a one-liner, but
this slightly larger refactoring avoids the whole class of problem and
saves a few cycles.
Remove the need to initialize and clear stream->buffers[] elements
that will be passed to StartReadBuffers() so that it can distinguish
forwarded buffers. Instead, add an explicit npinned argument to tell it
how many buffers are already pinned. It is an in/out argument, and the
output value can be used directly as input as input to the following
call along with the buffers it counts. This also removes the need to
identify and count them when StartReadBuffers() returns. This number is
now stored in stream->pending_read_npinned, the number of leading
buffers that are already pinned.
Defect in commit ed0b87ca.
XXX WIP
Bug: 19006
Backpatch-through: 18
Reported-by: Alexander Lakhin <exclusion@gmail.com>
Reviewed-by:
Discussion: https://postgr.es/m/19006-80fcaaf69000377e%40postgresql.org
---
src/backend/storage/aio/read_stream.c | 68 ++++++---------------------
src/backend/storage/buffer/bufmgr.c | 44 +++++++++++------
src/include/storage/bufmgr.h | 1 +
3 files changed, 45 insertions(+), 68 deletions(-)
diff --git a/src/backend/storage/aio/read_stream.c b/src/backend/storage/aio/read_stream.c
index 0e7f5557f5c..1b327d0ca7a 100644
--- a/src/backend/storage/aio/read_stream.c
+++ b/src/backend/storage/aio/read_stream.c
@@ -96,10 +96,8 @@ struct ReadStream
int16 ios_in_progress;
int16 queue_size;
int16 max_pinned_buffers;
- int16 forwarded_buffers;
int16 pinned_buffers;
int16 distance;
- int16 initialized_buffers;
int read_buffers_flags;
bool sync_mode; /* using io_method=sync */
bool batch_mode; /* READ_STREAM_USE_BATCHING */
@@ -126,6 +124,7 @@ struct ReadStream
/* The read operation we are currently preparing. */
BlockNumber pending_read_blocknum;
int16 pending_read_nblocks;
+ int pending_read_npinned;
/* Space for buffers and optional per-buffer private data. */
size_t per_buffer_data_size;
@@ -230,10 +229,8 @@ static bool
read_stream_start_pending_read(ReadStream *stream)
{
bool need_wait;
- int requested_nblocks;
int nblocks;
int flags;
- int forwarded;
int16 io_index;
int16 overflow;
int16 buffer_index;
@@ -293,9 +290,9 @@ read_stream_start_pending_read(ReadStream *stream)
buffer_limit = Min(GetAdditionalLocalPinLimit(), PG_INT16_MAX);
else
buffer_limit = Min(GetAdditionalPinLimit(), PG_INT16_MAX);
- Assert(stream->forwarded_buffers <= stream->pending_read_nblocks);
+ Assert(stream->pending_read_npinned <= stream->pending_read_nblocks);
- buffer_limit += stream->forwarded_buffers;
+ buffer_limit += stream->pending_read_npinned;
buffer_limit = Min(buffer_limit, PG_INT16_MAX);
if (buffer_limit == 0 && stream->pinned_buffers == 0)
@@ -322,20 +319,15 @@ read_stream_start_pending_read(ReadStream *stream)
/*
* We say how many blocks we want to read, but it may be smaller on return
- * if the buffer manager decides to shorten the read. Initialize buffers
- * to InvalidBuffer (= not a forwarded buffer) as input on first use only,
- * and keep the original nblocks number so we can check for forwarded
- * buffers as output, below.
+ * if the buffer manager decides to shorten the read.
*/
buffer_index = stream->next_buffer_index;
io_index = stream->next_io_index;
- while (stream->initialized_buffers < buffer_index + nblocks)
- stream->buffers[stream->initialized_buffers++] = InvalidBuffer;
- requested_nblocks = nblocks;
need_wait = StartReadBuffers(&stream->ios[io_index].op,
&stream->buffers[buffer_index],
stream->pending_read_blocknum,
&nblocks,
+ &stream->pending_read_npinned,
flags);
stream->pinned_buffers += nblocks;
@@ -360,28 +352,15 @@ read_stream_start_pending_read(ReadStream *stream)
stream->seq_blocknum = stream->pending_read_blocknum + nblocks;
}
- /*
- * How many pins were acquired but forwarded to the next call? These need
- * to be passed to the next StartReadBuffers() call by leaving them
- * exactly where they are in the queue, or released if the stream ends
- * early. We need the number for accounting purposes, since they are not
- * counted in stream->pinned_buffers but we already hold them.
- */
- forwarded = 0;
- while (nblocks + forwarded < requested_nblocks &&
- stream->buffers[buffer_index + nblocks + forwarded] != InvalidBuffer)
- forwarded++;
- stream->forwarded_buffers = forwarded;
-
/*
* We gave a contiguous range of buffer space to StartReadBuffers(), but
* we want it to wrap around at queue_size. Copy overflowing buffers to
* the front of the array where they'll be consumed, but also leave a copy
* in the overflow zone which the I/O operation has a pointer to (it needs
- * a contiguous array). Both copies will be cleared when the buffers are
- * handed to the consumer.
+ * a contiguous array).
*/
- overflow = (buffer_index + nblocks + forwarded) - stream->queue_size;
+ overflow = (buffer_index + nblocks + stream->pending_read_npinned) -
+ stream->queue_size;
if (overflow > 0)
{
Assert(overflow < stream->queue_size); /* can't overlap */
@@ -400,6 +379,7 @@ read_stream_start_pending_read(ReadStream *stream)
/* Adjust the pending read to cover the remaining portion, if any. */
stream->pending_read_blocknum += nblocks;
stream->pending_read_nblocks -= nblocks;
+ Assert(stream->pending_read_nblocks >= stream->pending_read_npinned);
return true;
}
@@ -786,12 +766,11 @@ read_stream_next_buffer(ReadStream *stream, void **per_buffer_data)
/* Fast path assumptions. */
Assert(stream->ios_in_progress == 0);
- Assert(stream->forwarded_buffers == 0);
Assert(stream->pinned_buffers == 1);
Assert(stream->distance == 1);
Assert(stream->pending_read_nblocks == 0);
+ Assert(stream->pending_read_npinned == 0);
Assert(stream->per_buffer_data_size == 0);
- Assert(stream->initialized_buffers > stream->oldest_buffer_index);
/* We're going to return the buffer we pinned last time. */
oldest_buffer_index = stream->oldest_buffer_index;
@@ -844,7 +823,6 @@ read_stream_next_buffer(ReadStream *stream, void **per_buffer_data)
stream->distance = 0;
stream->oldest_buffer_index = stream->next_buffer_index;
stream->pinned_buffers = 0;
- stream->buffers[oldest_buffer_index] = InvalidBuffer;
}
stream->fast_path = false;
@@ -920,16 +898,6 @@ read_stream_next_buffer(ReadStream *stream, void **per_buffer_data)
stream->seq_until_processed = InvalidBlockNumber;
}
- /*
- * We must zap this queue entry, or else it would appear as a forwarded
- * buffer. If it's potentially in the overflow zone (ie from a
- * multi-block I/O that wrapped around the queue), also zap the copy.
- */
- stream->buffers[oldest_buffer_index] = InvalidBuffer;
- if (oldest_buffer_index < stream->io_combine_limit - 1)
- stream->buffers[stream->queue_size + oldest_buffer_index] =
- InvalidBuffer;
-
#if defined(CLOBBER_FREED_MEMORY) || defined(USE_VALGRIND)
/*
@@ -973,10 +941,10 @@ read_stream_next_buffer(ReadStream *stream, void **per_buffer_data)
#ifndef READ_STREAM_DISABLE_FAST_PATH
/* See if we can take the fast path for all-cached scans next time. */
if (stream->ios_in_progress == 0 &&
- stream->forwarded_buffers == 0 &&
stream->pinned_buffers == 1 &&
stream->distance == 1 &&
stream->pending_read_nblocks == 0 &&
+ stream->pending_read_npinned == 0 &&
stream->per_buffer_data_size == 0)
{
stream->fast_path = true;
@@ -1025,22 +993,14 @@ read_stream_reset(ReadStream *stream)
/* Unpin any unused forwarded buffers. */
index = stream->next_buffer_index;
- while (index < stream->initialized_buffers &&
- (buffer = stream->buffers[index]) != InvalidBuffer)
+ while (stream->pending_read_npinned > 0)
{
- Assert(stream->forwarded_buffers > 0);
- stream->forwarded_buffers--;
- ReleaseBuffer(buffer);
-
- stream->buffers[index] = InvalidBuffer;
- if (index < stream->io_combine_limit - 1)
- stream->buffers[stream->queue_size + index] = InvalidBuffer;
-
+ ReleaseBuffer(stream->buffers[index]);
+ stream->pending_read_npinned--;
if (++index == stream->queue_size)
index = 0;
}
- Assert(stream->forwarded_buffers == 0);
Assert(stream->pinned_buffers == 0);
Assert(stream->ios_in_progress == 0);
diff --git a/src/backend/storage/buffer/bufmgr.c b/src/backend/storage/buffer/bufmgr.c
index 6c9ae83a7a8..e58a1539f37 100644
--- a/src/backend/storage/buffer/bufmgr.c
+++ b/src/backend/storage/buffer/bufmgr.c
@@ -1263,14 +1263,18 @@ StartReadBuffersImpl(ReadBuffersOperation *operation,
Buffer *buffers,
BlockNumber blockNum,
int *nblocks,
+ int *npinned,
int flags,
bool allow_forwarding)
{
int actual_nblocks = *nblocks;
+ int actual_npinned = *npinned;
int maxcombine = 0;
bool did_start_io;
+ Assert(*npinned == 0 || allow_forwarding);
Assert(*nblocks == 1 || allow_forwarding);
+ Assert(*npinned <= *nblocks);
Assert(*nblocks > 0);
Assert(*nblocks <= MAX_IO_COMBINE_LIMIT);
@@ -1278,7 +1282,7 @@ StartReadBuffersImpl(ReadBuffersOperation *operation,
{
bool found;
- if (allow_forwarding && buffers[i] != InvalidBuffer)
+ if (allow_forwarding && i < actual_npinned)
{
BufferDesc *bufHdr;
@@ -1323,6 +1327,7 @@ StartReadBuffersImpl(ReadBuffersOperation *operation,
blockNum + i,
operation->strategy,
&found);
+ actual_npinned++;
}
if (found)
@@ -1336,6 +1341,7 @@ StartReadBuffersImpl(ReadBuffersOperation *operation,
if (i == 0)
{
*nblocks = 1;
+ *npinned = actual_npinned - 1;
#ifdef USE_ASSERT_CHECKING
@@ -1459,22 +1465,30 @@ StartReadBuffersImpl(ReadBuffersOperation *operation,
CheckReadBuffersOperation(operation, !did_start_io);
+ /* Pinned buffers not included in final nblocks, forwarded to next call. */
+ *npinned = actual_npinned - *nblocks;
+
return did_start_io;
}
/*
* Begin reading a range of blocks beginning at blockNum and extending for
- * *nblocks. *nblocks and the buffers array are in/out parameters. On entry,
- * the buffers elements covered by *nblocks must hold either InvalidBuffer or
- * buffers forwarded by an earlier call to StartReadBuffers() that was split
- * and is now being continued. On return, *nblocks holds the number of blocks
- * accepted by this operation. If it is less than the original number then
- * this operation has been split, but buffer elements up to the original
- * requested size may hold forwarded buffers to be used for a continuing
- * operation. The caller must either start a new I/O beginning at the block
- * immediately following the blocks accepted by this call and pass those
- * buffers back in, or release them if it chooses not to. It shouldn't make
- * any other use of or assumptions about forwarded buffers.
+ * *nblocks. *nblocks, *npinned and the buffers array are in/out parameters.
+ *
+ * On entry, *nblocks is the desire number of block to read. On exit, it may
+ * be a smaller number if the operation was split.
+ *
+ * On entry, *npinned is the number of pinned buffers forwarded by an earlier
+ * operation that was split. On exit, it is the number forwarded by this call,
+ * which should be passed to the following call when continuing to read the
+ * same sequence of blocks, along with the corresponding buffers.
+ *
+ * When buffers are forwarded, as reported by *npinned, the caller must either
+ * start a new I/O beginning at the block immediately following the blocks
+ * accepted by this call (*nblocks on exit) and pass those buffers back in head
+ * position, or release them if it chooses not to. They are located in the
+ * buffers array beginning at index *nblocks (on exit). It shouldn't make any
+ * other use of or assumptions about forwarded buffers.
*
* If false is returned, no I/O is necessary and the buffers covered by
* *nblocks on exit are valid and ready to be accessed. If true is returned,
@@ -1490,9 +1504,10 @@ StartReadBuffers(ReadBuffersOperation *operation,
Buffer *buffers,
BlockNumber blockNum,
int *nblocks,
+ int *npinned,
int flags)
{
- return StartReadBuffersImpl(operation, buffers, blockNum, nblocks, flags,
+ return StartReadBuffersImpl(operation, buffers, blockNum, nblocks, npinned, flags,
true /* expect forwarded buffers */ );
}
@@ -1511,9 +1526,10 @@ StartReadBuffer(ReadBuffersOperation *operation,
int flags)
{
int nblocks = 1;
+ int npinned = 0;
bool result;
- result = StartReadBuffersImpl(operation, buffer, blocknum, &nblocks, flags,
+ result = StartReadBuffersImpl(operation, buffer, blocknum, &nblocks, &npinned, flags,
false /* single block, no forwarding */ );
Assert(nblocks == 1); /* single block can't be short */
diff --git a/src/include/storage/bufmgr.h b/src/include/storage/bufmgr.h
index 41fdc1e7693..e5e8d8129d4 100644
--- a/src/include/storage/bufmgr.h
+++ b/src/include/storage/bufmgr.h
@@ -225,6 +225,7 @@ extern bool StartReadBuffers(ReadBuffersOperation *operation,
Buffer *buffers,
BlockNumber blockNum,
int *nblocks,
+ int *npinned,
int flags);
extern void WaitReadBuffers(ReadBuffersOperation *operation);
--
2.39.5 (Apple Git-154)
^ permalink raw reply [nested|flat] 29+ messages in thread
* Re: BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer
2025-08-03 12:00 BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer PG Bug reporting form <noreply@postgresql.org>
2025-08-04 13:13 ` Re: BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer Xuneng Zhou <xunengzhou@gmail.com>
2025-08-04 19:19 ` Re: BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer Thomas Munro <thomas.munro@gmail.com>
2025-08-05 04:29 ` Re: BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer Thomas Munro <thomas.munro@gmail.com>
2025-08-07 15:09 ` Re: BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer Xuneng Zhou <xunengzhou@gmail.com>
2025-08-08 02:43 ` Re: BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer Thomas Munro <thomas.munro@gmail.com>
2025-08-08 04:39 ` Re: BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer Xuneng Zhou <xunengzhou@gmail.com>
2025-08-08 15:06 ` Re: BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer Thomas Munro <thomas.munro@gmail.com>
@ 2025-08-09 04:03 ` Thomas Munro <thomas.munro@gmail.com>
2025-08-10 00:22 ` Re: BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer Michael Paquier <michael@paquier.xyz>
0 siblings, 1 reply; 29+ messages in thread
From: Thomas Munro @ 2025-08-09 04:03 UTC (permalink / raw)
To: Xuneng Zhou <xunengzhou@gmail.com>; +Cc: exclusion@gmail.com, pgsql-bugs@lists.postgresql.org, Michael Paquier <michael@paquier.xyz>; Tom Lane <tgl@sss.pgh.pa.us>
On Sat, Aug 9, 2025 at 3:06 AM Thomas Munro <thomas.munro@gmail.com> wrote:
> Well, one benefit of trying to describe a complex system from the top
> is that higher level stupidity can sometimes become clear... I
> realised that the queue wouldn't even need to be initialised or
> cleared if StartReadBuffers() had an explicit in/out npinned argument
> to transmit the count between calls along with the buffers, or
> examined after each call. This is a draft alternative fix that I am
> studying that removes quite a lot of lines and fragility, though time
> is not on my side so I might push the one-liner fix ahead of the
> upcoming freeze and then see what to do about this.
One-liner fix pushed.
Here is a new version of the slightly more ambitious fix, for
discussion. I need to figure out if there is any measurable
performance impact, but from an interface sanity and fragility POV it
seems infinitely better like this. I recall being paranoid about
function call width, but it still fits in 6 registers. I think
perhaps the InvalidBuffer-based API might have seemed a shade more
reasonable before I reworked the pin limiting logic, but once I had to
start scanning for forwarded buffers after every StartReadBuffers()
call, I really should have reconsidered that interface. In other
words, maybe a bit of myopic path dependency here...
Attachments:
[text/x-patch] v2-0001-Give-StartReadBuffers-a-more-robust-interface.patch (16.6K, ../../CA+hUKG+cK955=G2gWPcK-yeAx1jz5Ln9_q=x9hMVjU=0zH8=dA@mail.gmail.com/2-v2-0001-Give-StartReadBuffers-a-more-robust-interface.patch)
download | inline diff:
From 23b1db84c83f2bd4dbf30c8a5a08fa717c342239 Mon Sep 17 00:00:00 2001
From: Thomas Munro <thomas.munro@gmail.com>
Date: Tue, 5 Aug 2025 14:35:09 +1200
Subject: [PATCH v2] Give StartReadBuffers() a more robust interface.
Remove the need for read_stream.c to initialize and clear
stream->buffers[] elements that will be passed to StartReadBuffers(),
and to count them on return. Instead, add an explicit npinned argument
to tell StartReadBuffers() how many buffers were already pinned by the
previous call (if it was a short read), and to learn how many are pinned
beyond *nblocks after this call returns (if it is a short read). It is
an in/out argument. The output of one call can be passed directly as
input to the next call, just like the buffers themselves.
The new function argument count still fits in registers in Unixoid
calling conventions. It doesn't on Windows/x86, but it didn't before
either. The single-buffer StartReadBuffer() specialization is not
affected.
read_stream.c now uses stream->pending_read_npinned to carry that number
from call to call, replacing stream->forwarded_buffers. It no longer
maintains it itself, which now seems a little fragile, layer-violating
and expensive. It still needs to consult the value when respecting pin
limits, but other than that it's entirely StartReadBuffers()' job to
worry about it. The new member variable name better reflects its
relationship with pending_read_{blocknum,nblocks}. It is the number of
buffers in that range that were already pinned by an earlier short read.
After a short read, _blocknum advances, _nblocks shrinks, and _npinned
has already been filled in by StartReadBuffers(), so we're ready for the
next call at next_buffer_index, where *npinned buffers have been placed
by StartReadBuffers().
Assertions about the identity of forwarded buffers are removed, because
StartReadBuffers() already has such assertions, and assertions about
queue entries after that are no longer relevant.
XXX does the change have any measurable performance impact?
XXX is the single-buffer specialization unaffected due to dead
code elimination, as expected?
xxx unused queue entries could potentially be electrified with
wipe_mem/VALGRIND_MAKE_MEM_NOACCESS, though existing bufmgr.c assertions
are very likely to reveal any fencepost bugs already
XXX perhaps we don't need quite so many _npinned <= _nblocks assertions
Backpatch-through: 18
Discussion: https://postgr.es/m/19006-80fcaaf69000377e%40postgresql.org
---
src/backend/storage/aio/read_stream.c | 102 ++++----------------------
src/backend/storage/buffer/bufmgr.c | 44 +++++++----
src/include/storage/bufmgr.h | 1 +
3 files changed, 45 insertions(+), 102 deletions(-)
diff --git a/src/backend/storage/aio/read_stream.c b/src/backend/storage/aio/read_stream.c
index 031fde9f4cb..1b327d0ca7a 100644
--- a/src/backend/storage/aio/read_stream.c
+++ b/src/backend/storage/aio/read_stream.c
@@ -96,10 +96,8 @@ struct ReadStream
int16 ios_in_progress;
int16 queue_size;
int16 max_pinned_buffers;
- int16 forwarded_buffers;
int16 pinned_buffers;
int16 distance;
- int16 initialized_buffers;
int read_buffers_flags;
bool sync_mode; /* using io_method=sync */
bool batch_mode; /* READ_STREAM_USE_BATCHING */
@@ -126,6 +124,7 @@ struct ReadStream
/* The read operation we are currently preparing. */
BlockNumber pending_read_blocknum;
int16 pending_read_nblocks;
+ int pending_read_npinned;
/* Space for buffers and optional per-buffer private data. */
size_t per_buffer_data_size;
@@ -230,10 +229,8 @@ static bool
read_stream_start_pending_read(ReadStream *stream)
{
bool need_wait;
- int requested_nblocks;
int nblocks;
int flags;
- int forwarded;
int16 io_index;
int16 overflow;
int16 buffer_index;
@@ -247,33 +244,12 @@ read_stream_start_pending_read(ReadStream *stream)
Assert(stream->pinned_buffers + stream->pending_read_nblocks <=
stream->max_pinned_buffers);
-#ifdef USE_ASSERT_CHECKING
/* We had better not be overwriting an existing pinned buffer. */
if (stream->pinned_buffers > 0)
Assert(stream->next_buffer_index != stream->oldest_buffer_index);
else
Assert(stream->next_buffer_index == stream->oldest_buffer_index);
- /*
- * Pinned buffers forwarded by a preceding StartReadBuffers() call that
- * had to split the operation should match the leading blocks of this
- * following StartReadBuffers() call.
- */
- Assert(stream->forwarded_buffers <= stream->pending_read_nblocks);
- for (int i = 0; i < stream->forwarded_buffers; ++i)
- Assert(BufferGetBlockNumber(stream->buffers[stream->next_buffer_index + i]) ==
- stream->pending_read_blocknum + i);
-
- /*
- * Check that we've cleared the queue/overflow entries corresponding to
- * the rest of the blocks covered by this read, unless it's the first go
- * around and we haven't even initialized them yet.
- */
- for (int i = stream->forwarded_buffers; i < stream->pending_read_nblocks; ++i)
- Assert(stream->next_buffer_index + i >= stream->initialized_buffers ||
- stream->buffers[stream->next_buffer_index + i] == InvalidBuffer);
-#endif
-
/* Do we need to issue read-ahead advice? */
flags = stream->read_buffers_flags;
if (stream->advice_enabled)
@@ -314,9 +290,9 @@ read_stream_start_pending_read(ReadStream *stream)
buffer_limit = Min(GetAdditionalLocalPinLimit(), PG_INT16_MAX);
else
buffer_limit = Min(GetAdditionalPinLimit(), PG_INT16_MAX);
- Assert(stream->forwarded_buffers <= stream->pending_read_nblocks);
+ Assert(stream->pending_read_npinned <= stream->pending_read_nblocks);
- buffer_limit += stream->forwarded_buffers;
+ buffer_limit += stream->pending_read_npinned;
buffer_limit = Min(buffer_limit, PG_INT16_MAX);
if (buffer_limit == 0 && stream->pinned_buffers == 0)
@@ -343,20 +319,15 @@ read_stream_start_pending_read(ReadStream *stream)
/*
* We say how many blocks we want to read, but it may be smaller on return
- * if the buffer manager decides to shorten the read. Initialize buffers
- * to InvalidBuffer (= not a forwarded buffer) as input on first use only,
- * and keep the original nblocks number so we can check for forwarded
- * buffers as output, below.
+ * if the buffer manager decides to shorten the read.
*/
buffer_index = stream->next_buffer_index;
io_index = stream->next_io_index;
- while (stream->initialized_buffers < buffer_index + nblocks)
- stream->buffers[stream->initialized_buffers++] = InvalidBuffer;
- requested_nblocks = nblocks;
need_wait = StartReadBuffers(&stream->ios[io_index].op,
&stream->buffers[buffer_index],
stream->pending_read_blocknum,
&nblocks,
+ &stream->pending_read_npinned,
flags);
stream->pinned_buffers += nblocks;
@@ -381,28 +352,15 @@ read_stream_start_pending_read(ReadStream *stream)
stream->seq_blocknum = stream->pending_read_blocknum + nblocks;
}
- /*
- * How many pins were acquired but forwarded to the next call? These need
- * to be passed to the next StartReadBuffers() call by leaving them
- * exactly where they are in the queue, or released if the stream ends
- * early. We need the number for accounting purposes, since they are not
- * counted in stream->pinned_buffers but we already hold them.
- */
- forwarded = 0;
- while (nblocks + forwarded < requested_nblocks &&
- stream->buffers[buffer_index + nblocks + forwarded] != InvalidBuffer)
- forwarded++;
- stream->forwarded_buffers = forwarded;
-
/*
* We gave a contiguous range of buffer space to StartReadBuffers(), but
* we want it to wrap around at queue_size. Copy overflowing buffers to
* the front of the array where they'll be consumed, but also leave a copy
* in the overflow zone which the I/O operation has a pointer to (it needs
- * a contiguous array). Both copies will be cleared when the buffers are
- * handed to the consumer.
+ * a contiguous array).
*/
- overflow = (buffer_index + nblocks + forwarded) - stream->queue_size;
+ overflow = (buffer_index + nblocks + stream->pending_read_npinned) -
+ stream->queue_size;
if (overflow > 0)
{
Assert(overflow < stream->queue_size); /* can't overlap */
@@ -421,6 +379,7 @@ read_stream_start_pending_read(ReadStream *stream)
/* Adjust the pending read to cover the remaining portion, if any. */
stream->pending_read_blocknum += nblocks;
stream->pending_read_nblocks -= nblocks;
+ Assert(stream->pending_read_nblocks >= stream->pending_read_npinned);
return true;
}
@@ -807,12 +766,11 @@ read_stream_next_buffer(ReadStream *stream, void **per_buffer_data)
/* Fast path assumptions. */
Assert(stream->ios_in_progress == 0);
- Assert(stream->forwarded_buffers == 0);
Assert(stream->pinned_buffers == 1);
Assert(stream->distance == 1);
Assert(stream->pending_read_nblocks == 0);
+ Assert(stream->pending_read_npinned == 0);
Assert(stream->per_buffer_data_size == 0);
- Assert(stream->initialized_buffers > stream->oldest_buffer_index);
/* We're going to return the buffer we pinned last time. */
oldest_buffer_index = stream->oldest_buffer_index;
@@ -865,7 +823,6 @@ read_stream_next_buffer(ReadStream *stream, void **per_buffer_data)
stream->distance = 0;
stream->oldest_buffer_index = stream->next_buffer_index;
stream->pinned_buffers = 0;
- stream->buffers[oldest_buffer_index] = InvalidBuffer;
}
stream->fast_path = false;
@@ -941,16 +898,6 @@ read_stream_next_buffer(ReadStream *stream, void **per_buffer_data)
stream->seq_until_processed = InvalidBlockNumber;
}
- /*
- * We must zap this queue entry, or else it would appear as a forwarded
- * buffer. If it's potentially in the overflow zone (ie from a
- * multi-block I/O that wrapped around the queue), also zap the copy.
- */
- stream->buffers[oldest_buffer_index] = InvalidBuffer;
- if (oldest_buffer_index < stream->io_combine_limit - 1)
- stream->buffers[stream->queue_size + oldest_buffer_index] =
- InvalidBuffer;
-
#if defined(CLOBBER_FREED_MEMORY) || defined(USE_VALGRIND)
/*
@@ -994,25 +941,12 @@ read_stream_next_buffer(ReadStream *stream, void **per_buffer_data)
#ifndef READ_STREAM_DISABLE_FAST_PATH
/* See if we can take the fast path for all-cached scans next time. */
if (stream->ios_in_progress == 0 &&
- stream->forwarded_buffers == 0 &&
stream->pinned_buffers == 1 &&
stream->distance == 1 &&
stream->pending_read_nblocks == 0 &&
+ stream->pending_read_npinned == 0 &&
stream->per_buffer_data_size == 0)
{
- /*
- * The fast path spins on one buffer entry repeatedly instead of
- * rotating through the whole queue and clearing the entries behind
- * it. If the buffer it starts with happened to be forwarded between
- * StartReadBuffers() calls and also wrapped around the circular queue
- * partway through, then a copy also exists in the overflow zone, and
- * it won't clear it out as the regular path would. Do that now, so
- * it doesn't need code for that.
- */
- if (stream->oldest_buffer_index < stream->io_combine_limit - 1)
- stream->buffers[stream->queue_size + stream->oldest_buffer_index] =
- InvalidBuffer;
-
stream->fast_path = true;
}
#endif
@@ -1059,22 +993,14 @@ read_stream_reset(ReadStream *stream)
/* Unpin any unused forwarded buffers. */
index = stream->next_buffer_index;
- while (index < stream->initialized_buffers &&
- (buffer = stream->buffers[index]) != InvalidBuffer)
+ while (stream->pending_read_npinned > 0)
{
- Assert(stream->forwarded_buffers > 0);
- stream->forwarded_buffers--;
- ReleaseBuffer(buffer);
-
- stream->buffers[index] = InvalidBuffer;
- if (index < stream->io_combine_limit - 1)
- stream->buffers[stream->queue_size + index] = InvalidBuffer;
-
+ ReleaseBuffer(stream->buffers[index]);
+ stream->pending_read_npinned--;
if (++index == stream->queue_size)
index = 0;
}
- Assert(stream->forwarded_buffers == 0);
Assert(stream->pinned_buffers == 0);
Assert(stream->ios_in_progress == 0);
diff --git a/src/backend/storage/buffer/bufmgr.c b/src/backend/storage/buffer/bufmgr.c
index fd7e21d96d3..512c3cd1f42 100644
--- a/src/backend/storage/buffer/bufmgr.c
+++ b/src/backend/storage/buffer/bufmgr.c
@@ -1263,14 +1263,18 @@ StartReadBuffersImpl(ReadBuffersOperation *operation,
Buffer *buffers,
BlockNumber blockNum,
int *nblocks,
+ int *npinned,
int flags,
bool allow_forwarding)
{
int actual_nblocks = *nblocks;
+ int actual_npinned = *npinned;
int maxcombine = 0;
bool did_start_io;
+ Assert(*npinned == 0 || allow_forwarding);
Assert(*nblocks == 1 || allow_forwarding);
+ Assert(*npinned <= *nblocks);
Assert(*nblocks > 0);
Assert(*nblocks <= MAX_IO_COMBINE_LIMIT);
@@ -1278,7 +1282,7 @@ StartReadBuffersImpl(ReadBuffersOperation *operation,
{
bool found;
- if (allow_forwarding && buffers[i] != InvalidBuffer)
+ if (allow_forwarding && i < actual_npinned)
{
BufferDesc *bufHdr;
@@ -1323,6 +1327,7 @@ StartReadBuffersImpl(ReadBuffersOperation *operation,
blockNum + i,
operation->strategy,
&found);
+ actual_npinned++;
}
if (found)
@@ -1336,6 +1341,7 @@ StartReadBuffersImpl(ReadBuffersOperation *operation,
if (i == 0)
{
*nblocks = 1;
+ *npinned = actual_npinned - 1;
#ifdef USE_ASSERT_CHECKING
@@ -1459,22 +1465,30 @@ StartReadBuffersImpl(ReadBuffersOperation *operation,
CheckReadBuffersOperation(operation, !did_start_io);
+ /* Pinned buffers not included in final nblocks, forwarded to next call. */
+ *npinned = actual_npinned - *nblocks;
+
return did_start_io;
}
/*
* Begin reading a range of blocks beginning at blockNum and extending for
- * *nblocks. *nblocks and the buffers array are in/out parameters. On entry,
- * the buffers elements covered by *nblocks must hold either InvalidBuffer or
- * buffers forwarded by an earlier call to StartReadBuffers() that was split
- * and is now being continued. On return, *nblocks holds the number of blocks
- * accepted by this operation. If it is less than the original number then
- * this operation has been split, but buffer elements up to the original
- * requested size may hold forwarded buffers to be used for a continuing
- * operation. The caller must either start a new I/O beginning at the block
- * immediately following the blocks accepted by this call and pass those
- * buffers back in, or release them if it chooses not to. It shouldn't make
- * any other use of or assumptions about forwarded buffers.
+ * *nblocks. *nblocks, *npinned and the buffers array are in/out parameters.
+ *
+ * On entry, *nblocks is the desire number of block to read. On exit, it may
+ * be a smaller number if the operation was split.
+ *
+ * On entry, *npinned is the number of pinned buffers forwarded by an earlier
+ * operation that was split. On exit, it is the number forwarded by this call,
+ * which should be passed to the following call when continuing to read the
+ * same sequence of blocks, along with the corresponding buffers.
+ *
+ * When buffers are forwarded, as reported by *npinned, the caller must either
+ * start a new I/O beginning at the block immediately following the blocks
+ * accepted by this call (*nblocks on exit) and pass those buffers back in head
+ * position, or release them if it chooses not to. They are located in the
+ * buffers array beginning at index *nblocks (on exit). It shouldn't make any
+ * other use of or assumptions about forwarded buffers.
*
* If false is returned, no I/O is necessary and the buffers covered by
* *nblocks on exit are valid and ready to be accessed. If true is returned,
@@ -1490,9 +1504,10 @@ StartReadBuffers(ReadBuffersOperation *operation,
Buffer *buffers,
BlockNumber blockNum,
int *nblocks,
+ int *npinned,
int flags)
{
- return StartReadBuffersImpl(operation, buffers, blockNum, nblocks, flags,
+ return StartReadBuffersImpl(operation, buffers, blockNum, nblocks, npinned, flags,
true /* expect forwarded buffers */ );
}
@@ -1511,9 +1526,10 @@ StartReadBuffer(ReadBuffersOperation *operation,
int flags)
{
int nblocks = 1;
+ int npinned = 0;
bool result;
- result = StartReadBuffersImpl(operation, buffer, blocknum, &nblocks, flags,
+ result = StartReadBuffersImpl(operation, buffer, blocknum, &nblocks, &npinned, flags,
false /* single block, no forwarding */ );
Assert(nblocks == 1); /* single block can't be short */
diff --git a/src/include/storage/bufmgr.h b/src/include/storage/bufmgr.h
index 41fdc1e7693..e5e8d8129d4 100644
--- a/src/include/storage/bufmgr.h
+++ b/src/include/storage/bufmgr.h
@@ -225,6 +225,7 @@ extern bool StartReadBuffers(ReadBuffersOperation *operation,
Buffer *buffers,
BlockNumber blockNum,
int *nblocks,
+ int *npinned,
int flags);
extern void WaitReadBuffers(ReadBuffersOperation *operation);
--
2.47.2
^ permalink raw reply [nested|flat] 29+ messages in thread
* Re: BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer
2025-08-03 12:00 BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer PG Bug reporting form <noreply@postgresql.org>
2025-08-04 13:13 ` Re: BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer Xuneng Zhou <xunengzhou@gmail.com>
2025-08-04 19:19 ` Re: BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer Thomas Munro <thomas.munro@gmail.com>
2025-08-05 04:29 ` Re: BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer Thomas Munro <thomas.munro@gmail.com>
2025-08-07 15:09 ` Re: BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer Xuneng Zhou <xunengzhou@gmail.com>
2025-08-08 02:43 ` Re: BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer Thomas Munro <thomas.munro@gmail.com>
2025-08-08 04:39 ` Re: BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer Xuneng Zhou <xunengzhou@gmail.com>
2025-08-08 15:06 ` Re: BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer Thomas Munro <thomas.munro@gmail.com>
2025-08-09 04:03 ` Re: BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer Thomas Munro <thomas.munro@gmail.com>
@ 2025-08-10 00:22 ` Michael Paquier <michael@paquier.xyz>
2025-08-10 02:01 ` Re: BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer Thomas Munro <thomas.munro@gmail.com>
0 siblings, 1 reply; 29+ messages in thread
From: Michael Paquier @ 2025-08-10 00:22 UTC (permalink / raw)
To: Thomas Munro <thomas.munro@gmail.com>; +Cc: Xuneng Zhou <xunengzhou@gmail.com>; exclusion@gmail.com, pgsql-bugs@lists.postgresql.org, Tom Lane <tgl@sss.pgh.pa.us>
On Sat, Aug 09, 2025 at 04:03:41PM +1200, Thomas Munro wrote:
> Here is a new version of the slightly more ambitious fix, for
> discussion. I need to figure out if there is any measurable
> performance impact, but from an interface sanity and fragility POV it
> seems infinitely better like this. I recall being paranoid about
> function call width, but it still fits in 6 registers. I think
> perhaps the InvalidBuffer-based API might have seemed a shade more
> reasonable before I reworked the pin limiting logic, but once I had to
> start scanning for forwarded buffers after every StartReadBuffers()
> call, I really should have reconsidered that interface. In other
> words, maybe a bit of myopic path dependency here...
>
> XXX does the change have any measurable performance impact?
> XXX is the single-buffer specialization unaffected due to dead
> code elimination, as expected?
> xxx unused queue entries could potentially be electrified with
> wipe_mem/VALGRIND_MAKE_MEM_NOACCESS, though existing bufmgr.c assertions
> are very likely to reveal any fencepost bugs already
> XXX perhaps we don't need quite so many _npinned <= _nblocks assertions
With all these comments and b421223172a2 already applied, are you sure
that it is a good idea to play with the v18 branch more than
necessary? We are in a baked beta2 state, and it looks like all these
could qualify as HEAD-only improvements.
Perhaps the open item can be closed then?
--
Michael
Attachments:
[application/pgp-signature] signature.asc (832B, ../../aJfmMuR0ERs394_c@paquier.xyz/2-signature.asc)
download
^ permalink raw reply [nested|flat] 29+ messages in thread
* Re: BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer
2025-08-03 12:00 BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer PG Bug reporting form <noreply@postgresql.org>
2025-08-04 13:13 ` Re: BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer Xuneng Zhou <xunengzhou@gmail.com>
2025-08-04 19:19 ` Re: BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer Thomas Munro <thomas.munro@gmail.com>
2025-08-05 04:29 ` Re: BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer Thomas Munro <thomas.munro@gmail.com>
2025-08-07 15:09 ` Re: BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer Xuneng Zhou <xunengzhou@gmail.com>
2025-08-08 02:43 ` Re: BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer Thomas Munro <thomas.munro@gmail.com>
2025-08-08 04:39 ` Re: BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer Xuneng Zhou <xunengzhou@gmail.com>
2025-08-08 15:06 ` Re: BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer Thomas Munro <thomas.munro@gmail.com>
2025-08-09 04:03 ` Re: BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer Thomas Munro <thomas.munro@gmail.com>
2025-08-10 00:22 ` Re: BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer Michael Paquier <michael@paquier.xyz>
@ 2025-08-10 02:01 ` Thomas Munro <thomas.munro@gmail.com>
2025-08-11 19:00 ` Re: BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer Nathan Bossart <nathandbossart@gmail.com>
2025-08-12 14:41 ` Re: BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer Xuneng Zhou <xunengzhou@gmail.com>
0 siblings, 2 replies; 29+ messages in thread
From: Thomas Munro @ 2025-08-10 02:01 UTC (permalink / raw)
To: Michael Paquier <michael@paquier.xyz>; +Cc: Xuneng Zhou <xunengzhou@gmail.com>; exclusion@gmail.com, pgsql-bugs@lists.postgresql.org, Tom Lane <tgl@sss.pgh.pa.us>
On Sun, Aug 10, 2025 at 12:22 PM Michael Paquier <michael@paquier.xyz> wrote:
> With all these comments and b421223172a2 already applied, are you sure
> that it is a good idea to play with the v18 branch more than
> necessary? We are in a baked beta2 state, and it looks like all these
> could qualify as HEAD-only improvements.
Yeah, b421223172a2 closes the known bug and thus the critical path item for 18.
I am happy to propose this patch as an improvement for master, and I
am not aware of remaining bugs in this area in v18. I just wanted to
publish the patch with analysis ASAP once the existing
read_stream.c/bufmgr.c interaction began to seem egregiously
suboptimal to me and I saw what to do about, having addressed the bug
with a low risk patch as a first priority. That keeps the options
open, even if not very wide given the date: on the off-chance that
others think the status quo isn't good enough and perhaps the
fragility of the existing API is actually riskier than the bigger
change, at least there's a patch on the table to discuss. I have no
plans to take any action without such a consensus.
> Perhaps the open item can be closed then?
Done.
^ permalink raw reply [nested|flat] 29+ messages in thread
* Re: BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer
2025-08-03 12:00 BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer PG Bug reporting form <noreply@postgresql.org>
2025-08-04 13:13 ` Re: BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer Xuneng Zhou <xunengzhou@gmail.com>
2025-08-04 19:19 ` Re: BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer Thomas Munro <thomas.munro@gmail.com>
2025-08-05 04:29 ` Re: BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer Thomas Munro <thomas.munro@gmail.com>
2025-08-07 15:09 ` Re: BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer Xuneng Zhou <xunengzhou@gmail.com>
2025-08-08 02:43 ` Re: BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer Thomas Munro <thomas.munro@gmail.com>
2025-08-08 04:39 ` Re: BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer Xuneng Zhou <xunengzhou@gmail.com>
2025-08-08 15:06 ` Re: BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer Thomas Munro <thomas.munro@gmail.com>
2025-08-09 04:03 ` Re: BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer Thomas Munro <thomas.munro@gmail.com>
2025-08-10 00:22 ` Re: BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer Michael Paquier <michael@paquier.xyz>
2025-08-10 02:01 ` Re: BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer Thomas Munro <thomas.munro@gmail.com>
@ 2025-08-11 19:00 ` Nathan Bossart <nathandbossart@gmail.com>
1 sibling, 0 replies; 29+ messages in thread
From: Nathan Bossart @ 2025-08-11 19:00 UTC (permalink / raw)
To: Thomas Munro <thomas.munro@gmail.com>; +Cc: Michael Paquier <michael@paquier.xyz>; Xuneng Zhou <xunengzhou@gmail.com>; exclusion@gmail.com, pgsql-bugs@lists.postgresql.org, Tom Lane <tgl@sss.pgh.pa.us>
On Sun, Aug 10, 2025 at 02:01:22PM +1200, Thomas Munro wrote:
> On Sun, Aug 10, 2025 at 12:22 PM Michael Paquier <michael@paquier.xyz> wrote:
>> Perhaps the open item can be closed then?
>
> Done.
This was marked as closed before 18rc1, but it appears to have been
committed in time for 18beta3, so I moved it to the "resolved before
18beta3" section.
--
nathan
^ permalink raw reply [nested|flat] 29+ messages in thread
* Re: BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer
2025-08-03 12:00 BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer PG Bug reporting form <noreply@postgresql.org>
2025-08-04 13:13 ` Re: BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer Xuneng Zhou <xunengzhou@gmail.com>
2025-08-04 19:19 ` Re: BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer Thomas Munro <thomas.munro@gmail.com>
2025-08-05 04:29 ` Re: BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer Thomas Munro <thomas.munro@gmail.com>
2025-08-07 15:09 ` Re: BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer Xuneng Zhou <xunengzhou@gmail.com>
2025-08-08 02:43 ` Re: BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer Thomas Munro <thomas.munro@gmail.com>
2025-08-08 04:39 ` Re: BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer Xuneng Zhou <xunengzhou@gmail.com>
2025-08-08 15:06 ` Re: BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer Thomas Munro <thomas.munro@gmail.com>
2025-08-09 04:03 ` Re: BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer Thomas Munro <thomas.munro@gmail.com>
2025-08-10 00:22 ` Re: BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer Michael Paquier <michael@paquier.xyz>
2025-08-10 02:01 ` Re: BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer Thomas Munro <thomas.munro@gmail.com>
@ 2025-08-12 14:41 ` Xuneng Zhou <xunengzhou@gmail.com>
1 sibling, 0 replies; 29+ messages in thread
From: Xuneng Zhou @ 2025-08-12 14:41 UTC (permalink / raw)
To: Thomas Munro <thomas.munro@gmail.com>; +Cc: Michael Paquier <michael@paquier.xyz>; exclusion@gmail.com, pgsql-bugs@lists.postgresql.org, Tom Lane <tgl@sss.pgh.pa.us>; nathandbossart@gmail.com
Hi,
On Sun, Aug 10, 2025 at 10:01 AM Thomas Munro <thomas.munro@gmail.com> wrote:
>
> On Sun, Aug 10, 2025 at 12:22 PM Michael Paquier <michael@paquier.xyz> wrote:
> > With all these comments and b421223172a2 already applied, are you sure
> > that it is a good idea to play with the v18 branch more than
> > necessary? We are in a baked beta2 state, and it looks like all these
> > could qualify as HEAD-only improvements.
>
> Yeah, b421223172a2 closes the known bug and thus the critical path item for 18.
>
> I am happy to propose this patch as an improvement for master, and I
> am not aware of remaining bugs in this area in v18. I just wanted to
> publish the patch with analysis ASAP once the existing
> read_stream.c/bufmgr.c interaction began to seem egregiously
> suboptimal to me and I saw what to do about, having addressed the bug
> with a low risk patch as a first priority. That keeps the options
> open, even if not very wide given the date: on the off-chance that
> others think the status quo isn't good enough and perhaps the
> fragility of the existing API is actually riskier than the bigger
> change, at least there's a patch on the table to discuss. I have no
> plans to take any action without such a consensus.
>
> > Perhaps the open item can be closed then?
>
> Done.
I read the high level explanation in the prior email; it's a pretty
dense reading. I wouldn't claim to have a very good understanding of
it, but the one line clean-up seems to fix the bug well. This overall
idea of the follow-up patch looks sound; however it feels more like
performance improvement enabled by enhanced design. I also did a rough
read on the patch itself and found two minor typos.
"the output value can be used directly (as input as input) to the following
call along with the buffers it counts."
"On entry, *nblocks is the (desire) number of block to read."
Best,
Xuneng
^ permalink raw reply [nested|flat] 29+ messages in thread
* Re: BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer
2025-08-03 12:00 BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer PG Bug reporting form <noreply@postgresql.org>
2025-08-04 13:13 ` Re: BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer Xuneng Zhou <xunengzhou@gmail.com>
2025-08-04 19:19 ` Re: BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer Thomas Munro <thomas.munro@gmail.com>
2025-08-05 04:29 ` Re: BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer Thomas Munro <thomas.munro@gmail.com>
2025-08-07 15:09 ` Re: BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer Xuneng Zhou <xunengzhou@gmail.com>
2025-08-08 02:43 ` Re: BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer Thomas Munro <thomas.munro@gmail.com>
@ 2025-08-13 05:29 ` Xuneng Zhou <xunengzhou@gmail.com>
2025-08-13 06:01 ` Re: BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer Thomas Munro <thomas.munro@gmail.com>
2 siblings, 1 reply; 29+ messages in thread
From: Xuneng Zhou @ 2025-08-13 05:29 UTC (permalink / raw)
To: Thomas Munro <thomas.munro@gmail.com>; +Cc: exclusion@gmail.com, pgsql-bugs@lists.postgresql.org, Michael Paquier <michael@paquier.xyz>; Tom Lane <tgl@sss.pgh.pa.us>; nathandbossart@gmail.com
Hi,
> 1. The leading block is BM_VALID, so we choose to give it to you
> immediately and not look further (we could look further and return
> more than one consecutive BM_VALID block at a time, but this isn't
> implemented)
I am curious about why this isn't implemented. It looks helpful.
Is there any blocking issue or trade-off for not doing so?
> Next, let me describe how read_stream.c manages its circular buffer queue:
>
> |<------------- queue_size ------------>|
> +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
> | | | | | | |0|1|2|3|4|5|6|7|8| | | | | |
> +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
> ^ ^
> | |
> oldest_buffer_index next_buffer_index
>
> oldest_buffer_index is the consumer end of the queue, and any
> associated IO has to be finished before that buffer can be return by
> read_stream_next_buffer(). next_buffer_index is the producer end of
> the queue, where IOs are started. StartReadBuffers() takes pointer to
> an array of buffers of the size of the requested read, so
> read_stream.c gives it the address of that part of its queue.
>
> This means that if StartReadBuffer() forwards some buffers because of
> a split, we don't have to do anything at all, because we advance
> next_buffer_index to the location of the start of the next operation,
> and that is where the forwarded buffers landed, ready for the next
> call to StartReadBuffers():
>
> |<------------- queue_size ------------>|
> +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
> | | | | | | |0|1|2|3|4|5|6|7|8|9|A| | | |
> +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
> ^ ^
> | |
> oldest_buffer_index next_buffer_index
>
The format of this part is not aligned well in gmail, so I copy it into vs
code.
Is this layout right? I found second illustration somewhat hard to follow,
especially
the "do nothing" trick and the movement of next_buffef_index in the second
queue.
Maybe I need to read the corresponding code.
[image: image.png]
Best,
Xuneng
Attachments:
[image/png] image.png (279.5K, ../../CABPTF7UaVaw6NC9w-y_xexxpP1odHKrqAK4z+H5yg9n9Rrfo7w@mail.gmail.com/3-image.png)
download | view image
^ permalink raw reply [nested|flat] 29+ messages in thread
* Re: BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer
2025-08-03 12:00 BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer PG Bug reporting form <noreply@postgresql.org>
2025-08-04 13:13 ` Re: BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer Xuneng Zhou <xunengzhou@gmail.com>
2025-08-04 19:19 ` Re: BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer Thomas Munro <thomas.munro@gmail.com>
2025-08-05 04:29 ` Re: BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer Thomas Munro <thomas.munro@gmail.com>
2025-08-07 15:09 ` Re: BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer Xuneng Zhou <xunengzhou@gmail.com>
2025-08-08 02:43 ` Re: BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer Thomas Munro <thomas.munro@gmail.com>
2025-08-13 05:29 ` Re: BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer Xuneng Zhou <xunengzhou@gmail.com>
@ 2025-08-13 06:01 ` Thomas Munro <thomas.munro@gmail.com>
2025-08-13 13:35 ` Re: BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer Xuneng Zhou <xunengzhou@gmail.com>
2025-08-22 11:58 ` Re: BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer Xuneng Zhou <xunengzhou@gmail.com>
0 siblings, 2 replies; 29+ messages in thread
From: Thomas Munro @ 2025-08-13 06:01 UTC (permalink / raw)
To: Xuneng Zhou <xunengzhou@gmail.com>; +Cc: exclusion@gmail.com, pgsql-bugs@lists.postgresql.org, Michael Paquier <michael@paquier.xyz>; Tom Lane <tgl@sss.pgh.pa.us>; nathandbossart@gmail.com
On Wed, Aug 13, 2025 at 5:29 PM Xuneng Zhou <xunengzhou@gmail.com> wrote:
> > 1. The leading block is BM_VALID, so we choose to give it to you
> > immediately and not look further (we could look further and return
> > more than one consecutive BM_VALID block at a time, but this isn't
> > implemented)
>
> I am curious about why this isn't implemented. It looks helpful.
> Is there any blocking issue or trade-off for not doing so?
stream->distance tends to be 1 for well cached data (it grows when IO
is needed and it is worth looking far ahead, and shrinks when data is
found in cache). So we won't currently have a distance > 1 for well
cached data, so we wouldn't benefit much. That may not always be
true: if the buffer mapping table is changed to a tree data structure
(like the way kernels manage virtual memory pages associated with a
file or other VM object), then we might have an incentive to look up
lots of cached blocks at the same time, and then we might want to
change the distance tuning algorithm, and then we might want
StartReadBuffers() to be able to return more than one cached block at
a time. We've focused mainly on I/O so far, and just tried not to
make the well cached cases worse.
> The format of this part is not aligned well in gmail, so I copy it into vs code.
> Is this layout right?
Yes.
> I found second illustration somewhat hard to follow, especially
> the "do nothing" trick and the movement of next_buffef_index in the second queue.
> Maybe I need to read the corresponding code.
Suppose you have pending_read_blocknum = 100, pending_read_nblock = 5,
next_buffer_index = 200. Now you decide to start that read, because
the next block the caller wants can't be combined, so you call
StartReadBuffers(blocknum = 100, *nblocks = 4, buffers =
&buffers[200]). StartReadBuffers() pins 5 buffers and starts an IO,
but finds a reason to split it at size 2. It sets *nblocks to 2, and
returns. Now read_stream.c adds 2 to pending_read_blocknum, subtracts
2 from pending_read_nblocks, so that it represents the continuation of
the previous read. It also adds 2 to next_buffer_index with modulo
arithmetic (so it wraps around the queue), because that is the
location of the buffers for the next read, and sets forwarded_buffers
to 3, because there are 3 buffers already pinned (in that patch ^ it
is renamed to pending_read_npinned). The three buffers themselves are
already in the queue at the right position for StartReadBuffers() to
receive them and know that it doesn't have to pin them again. What I
was referring to with "doing nothing" is the way that if you keep
sliding StartReadBuffers() call along the queue, the extra buffers it
spits out become its input next time, without having to be copied
anywhere. In that picture there are two forwarded buffers, labeled 9
and A.
^ permalink raw reply [nested|flat] 29+ messages in thread
* Re: BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer
2025-08-03 12:00 BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer PG Bug reporting form <noreply@postgresql.org>
2025-08-04 13:13 ` Re: BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer Xuneng Zhou <xunengzhou@gmail.com>
2025-08-04 19:19 ` Re: BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer Thomas Munro <thomas.munro@gmail.com>
2025-08-05 04:29 ` Re: BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer Thomas Munro <thomas.munro@gmail.com>
2025-08-07 15:09 ` Re: BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer Xuneng Zhou <xunengzhou@gmail.com>
2025-08-08 02:43 ` Re: BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer Thomas Munro <thomas.munro@gmail.com>
2025-08-13 05:29 ` Re: BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer Xuneng Zhou <xunengzhou@gmail.com>
2025-08-13 06:01 ` Re: BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer Thomas Munro <thomas.munro@gmail.com>
@ 2025-08-13 13:35 ` Xuneng Zhou <xunengzhou@gmail.com>
2025-08-22 03:25 ` Re: BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer Xuneng Zhou <xunengzhou@gmail.com>
1 sibling, 1 reply; 29+ messages in thread
From: Xuneng Zhou @ 2025-08-13 13:35 UTC (permalink / raw)
To: Thomas Munro <thomas.munro@gmail.com>; +Cc: exclusion@gmail.com, pgsql-bugs@lists.postgresql.org, Michael Paquier <michael@paquier.xyz>; Tom Lane <tgl@sss.pgh.pa.us>; nathandbossart@gmail.com
Hi,
On Wed, Aug 13, 2025 at 2:02 PM Thomas Munro <thomas.munro@gmail.com> wrote:
>
> On Wed, Aug 13, 2025 at 5:29 PM Xuneng Zhou <xunengzhou@gmail.com> wrote:
> > > 1. The leading block is BM_VALID, so we choose to give it to you
> > > immediately and not look further (we could look further and return
> > > more than one consecutive BM_VALID block at a time, but this isn't
> > > implemented)
> >
> > I am curious about why this isn't implemented. It looks helpful.
> > Is there any blocking issue or trade-off for not doing so?
>
> stream->distance tends to be 1 for well cached data (it grows when IO
> is needed and it is worth looking far ahead, and shrinks when data is
> found in cache). So we won't currently have a distance > 1 for well
> cached data, so we wouldn't benefit much. That may not always be
> true: if the buffer mapping table is changed to a tree data structure
> (like the way kernels manage virtual memory pages associated with a
> file or other VM object), then we might have an incentive to look up
> lots of cached blocks at the same time, and then we might want to
> change the distance tuning algorithm, and then we might want
> StartReadBuffers() to be able to return more than one cached block at
> a time. We've focused mainly on I/O so far, and just tried not to
> make the well cached cases worse.
Understood. I came across Alexander’s article on the topic. It appears
to require a major re-architecture, which could be challenging to land
HEAD.
https://www.orioledb.com/blog/buffer-management
>
> > The format of this part is not aligned well in gmail, so I copy it into vs code.
> > Is this layout right?
>
> Yes.
>
> > I found second illustration somewhat hard to follow, especially
> > the "do nothing" trick and the movement of next_buffef_index in the second queue.
> > Maybe I need to read the corresponding code.
>
> Suppose you have pending_read_blocknum = 100, pending_read_nblock = 5,
> next_buffer_index = 200. Now you decide to start that read, because
> the next block the caller wants can't be combined, so you call
> StartReadBuffers(blocknum = 100, *nblocks = 4, buffers =
> &buffers[200]). StartReadBuffers() pins 5 buffers and starts an IO,
> but finds a reason to split it at size 2. It sets *nblocks to 2, and
> returns. Now read_stream.c adds 2 to pending_read_blocknum, subtracts
> 2 from pending_read_nblocks, so that it represents the continuation of
> the previous read. It also adds 2 to next_buffer_index with modulo
> arithmetic (so it wraps around the queue), because that is the
> location of the buffers for the next read, and sets forwarded_buffers
> to 3, because there are 3 buffers already pinned (in that patch ^ it
> is renamed to pending_read_npinned). The three buffers themselves are
> already in the queue at the right position for StartReadBuffers() to
> receive them and know that it doesn't have to pin them again. What I
> was referring to with "doing nothing" is the way that if you keep
> sliding StartReadBuffers() call along the queue, the extra buffers it
> spits out become its input next time, without having to be copied
> anywhere. In that picture there are two forwarded buffers, labeled 9
> and A.
Got it. Elegant technique. Patch 2 also reads as a robustness
improvement now. :-) I'll try to map the high-level design to code and
review patch 2.
Thanks for the great guidance!
Best,
Xuneng
^ permalink raw reply [nested|flat] 29+ messages in thread
* Re: BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer
2025-08-03 12:00 BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer PG Bug reporting form <noreply@postgresql.org>
2025-08-04 13:13 ` Re: BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer Xuneng Zhou <xunengzhou@gmail.com>
2025-08-04 19:19 ` Re: BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer Thomas Munro <thomas.munro@gmail.com>
2025-08-05 04:29 ` Re: BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer Thomas Munro <thomas.munro@gmail.com>
2025-08-07 15:09 ` Re: BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer Xuneng Zhou <xunengzhou@gmail.com>
2025-08-08 02:43 ` Re: BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer Thomas Munro <thomas.munro@gmail.com>
2025-08-13 05:29 ` Re: BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer Xuneng Zhou <xunengzhou@gmail.com>
2025-08-13 06:01 ` Re: BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer Thomas Munro <thomas.munro@gmail.com>
2025-08-13 13:35 ` Re: BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer Xuneng Zhou <xunengzhou@gmail.com>
@ 2025-08-22 03:25 ` Xuneng Zhou <xunengzhou@gmail.com>
0 siblings, 0 replies; 29+ messages in thread
From: Xuneng Zhou @ 2025-08-22 03:25 UTC (permalink / raw)
To: pgsql-bugs@lists.postgresql.org
Hi,
The mailing list seems to prefer compressed attachments over many
scattered ones.
On Fri, Aug 22, 2025 at 11:08 AM Xuneng Zhou <xunengzhou@gmail.com> wrote:
>
> Hi,
>
> XXX does the change have any measurable performance impact?
> I did some preliminary performance tests on this patch and Head. Not
> sure whether the methodology of tests is right, still post some
> findings in here:
>
> 1) Query runtimes (EXPLAIN (ANALYZE, BUFFERS), ms)
>
> Bitmap heap scan
> • HEAD (5 reps): [94.121, 89.600, 92.732, 78.383, 78.068]
> mean 86.58 ms, median 89.60, min 78.07, max 94.12. Buffers: shared hit only.
> • PATCHED (5 reps): [89.130, 80.375, 88.227, 98.661, 88.113]
> mean 88.90 ms, median 88.227, min 80.38, max 98.66. Buffers: shared hit only.
>
> Delta (PATCHED vs HEAD): mean +2.68% (slower), median −1.53% (faster).
>
> Sequential scan
> • HEAD (5 reps): [2718.212, 2691.739, 3253.613, 3060.852, 2963.142]
> mean 2937.51 ms, median 2963.14.
> • PATCHED (5 reps): [2719.971, 2770.082, 2697.245, 2610.932, 2662.142]
> mean 2692.07 ms, median 2697.25. Plans + buffer counts shown in the
> attached file (example snippets include Buffers: shared hit ~600–900,
> read ~343–344k; I/O Timings ~10–16 ms).
>
> Delta (PATCHED vs HEAD): mean −8.36%, median −8.97%.
>
>
> 2) CPU micro-latencies (bpftrace) — hot read-stream functions
>
> Functions: StartReadBuffers, read_stream_start_pending_read,
> read_stream_next_buffer. Averages are µs; totals are over the ~60 s
> run.
>
> Bitmap
> • HEAD — calls: SRB 2,697,243, start_pending 2,696,591, next_buffer 2,697,105;
> avg_us: SRB ≈4, start_pending ≈5, next_buffer ≈10.
> • PATCHED — calls: SRB 2,677,563, start_pending 2,676,940, next_buffer
> 2,677,477;
> avg_us: SRB ≈4, start_pending ≈5, next_buffer ≈10.
>
> Seq (control)
> • HEAD — calls: SRB 712,927, start_pending 2,042,992, next_buffer 5,528,071;
> avg_us: SRB ≈8, start_pending ≈3, next_buffer ≈6.
> • PATCHED — calls: SRB 727,083, start_pending 2,083,866, next_buffer 5,636,554;
> avg_us: SRB ≈8, start_pending ≈3, next_buffer ≈6.
>
> Read: Per-call costs are indistinguishable. Small call-count drift is
> noise-level and doesn’t suggest a behavioral change.
>
>
> 3) perf stat (-d) — counters over ~60 s (single backend)
>
> Key: task-clock (ms), CPU util (=task-clock/elapsed), cycles,
> instructions, IPC, branch-miss rate, L1D miss rate.
>
> Bitmap
> • HEAD → PATCHED (Δ)
> • task-clock: 8528.28 → 8542.37 ms (+0.17%)
> • CPU util: 0.1421 → 0.1424 (+0.16%)
> • cycles: 16.844 G → 16.796 G (−0.28%)
> • instructions: 51.161 G → 51.435 G (+0.54%)
> • IPC: 3.037 → 3.062 (+0.82%)
> • branch-miss rate: 0.125% → 0.115% (−7.6%)
> • L1D miss rate: 0.379% → 0.383% (+1.1%)
>
> Read: CPU-side efficiency is neutral-to-slightly better with the patch
> (↑IPC, ↓cycles, ↓branch-miss), matching the bpftrace picture.
>
> Seq (control)
>
> • HEAD → PATCHED (Δ)
> • task-clock: 10,862.53 → 8,896.26 ms (−18.1%)
> • CPU util: 0.181 → 0.148 (−18.1%)
> • cycles: 16.275 G → 13.049 G (−19.8%)
> • instructions: 39.782 G → 33.343 G (−16.2%)
> • IPC: 2.444 → 2.555 (+4.5%)
> • branch-miss rate: 0.380% → 0.350% (−8.0%)
> • L1D miss rate: 1.009% → 0.997% (−1.2%)
>
> Read: Control path remains healthy: IPC improves; miss rates improve slightly.
>
>
> Takeaways
> • The new *npinned interface and removal of “forwarded buffer”
> bookkeeping appears neutral-to-positive under warm-cache conditions.
> • Bitmap workload: unchanged micro-latencies, modest CPU-efficiency
> uplift (↑IPC ~0.8%, ↓cycles), and runtimes that straddle zero with
> identical plans/buffer behavior.
> • Seq control: unchanged micro-latencies; perf shows higher IPC and
> slightly better miss rates; EXPLAIN shows a modest runtime improvement
> (~9% median).
>
> Best,
> Xuneng
Attachments:
[application/zip] perftests.zip (169.4K, ../../CABPTF7U=1G_3fGxDcS_YceWu7zRGi+w-zVNzc9H7ux00cErEVA@mail.gmail.com/2-perftests.zip)
download
^ permalink raw reply [nested|flat] 29+ messages in thread
* Re: BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer
2025-08-03 12:00 BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer PG Bug reporting form <noreply@postgresql.org>
2025-08-04 13:13 ` Re: BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer Xuneng Zhou <xunengzhou@gmail.com>
2025-08-04 19:19 ` Re: BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer Thomas Munro <thomas.munro@gmail.com>
2025-08-05 04:29 ` Re: BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer Thomas Munro <thomas.munro@gmail.com>
2025-08-07 15:09 ` Re: BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer Xuneng Zhou <xunengzhou@gmail.com>
2025-08-08 02:43 ` Re: BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer Thomas Munro <thomas.munro@gmail.com>
2025-08-13 05:29 ` Re: BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer Xuneng Zhou <xunengzhou@gmail.com>
2025-08-13 06:01 ` Re: BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer Thomas Munro <thomas.munro@gmail.com>
@ 2025-08-22 11:58 ` Xuneng Zhou <xunengzhou@gmail.com>
2025-10-11 12:32 ` Re: BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer Xuneng Zhou <xunengzhou@gmail.com>
1 sibling, 1 reply; 29+ messages in thread
From: Xuneng Zhou @ 2025-08-22 11:58 UTC (permalink / raw)
To: Thomas Munro <thomas.munro@gmail.com>; +Cc: exclusion@gmail.com, pgsql-bugs@lists.postgresql.org, Michael Paquier <michael@paquier.xyz>; Tom Lane <tgl@sss.pgh.pa.us>; nathandbossart@gmail.com
Hi,
These are the results of a cold-cache(not strictly) bench:
Cold-cache readstream microbench (20M rows, shared_buffers=128MB). Use
a somewhat unrealistic setting to amplify the potential impact.
Patch: Make StartReadBuffers() carry npinned in/out
Workload: single-session COUNT(*) on large_table
• Seq scan: SET enable_{indexscan,indexonlyscan,bitmapscan}=off;
SELECT count(*) FROM large_table;
• Bitmap scan: 16 wide tiles over id (contiguous ranges)
Machine: AMD, 16G mem, 8 cores
Config (per run):
shared_buffers=128MB, io_combine_limit=8, track_io_timing=on, jit=off,
effective_io_concurrency=32. Table is clustered by id. N≈20M rows
(int, text(100)).
1) Query runtimes (EXPLAIN ANALYZE)
Sequential scan — 5 reps each
• BASE: mean 2937 ms, median 2963 ms, p95 3254 ms
(per-run: 2719, 2692, 3254, 3061, 2963 ms)
• PATCHED: mean 2692 ms, median 2697 ms, p95 2770 ms
(per-run: 2720, 2770, 2697, 2611, 2662 ms)
PATCHED faster: mean –8.4%, p95 –14.8% vs BASE.
Bitmap scan — 5 reps each
• BASE: mean 86.6 ms, median 89.6 ms, p95 94.1 ms
• PATCHED: mean 88.9 ms, median 88.2 ms, p95 98.7 ms
2) perf stat (60s pgbench drive of the same SQL)
Sequential scan
metric BASE PATCHED
task-clock (ms) 10,862.53 8,896.26 –18.1%
cycles 16.27e9 13.05e9 –19.8%
instructions 39.78e9 33.34e9 –16.2%
IPC 2.44 2.56 +4.5%
branches 8.62e9 7.33e9 –14.9%
branch-misses 32.74e6 24.47e6 –25.2%
Bitmap scan
metric BASE PATCHED
task-clock (ms) 8,528.28 8,542.37 +0.2%
cycles 16.84e9 16.80e9 –0.3%
instructions 51.16e9 51.43e9 +0.5%
IPC 3.04 3.06 +0.8%
branch-misses 11.86e6 11.05e6 –6.9%
3) bpftrace micro-latency on the readstream path
(averages in µs; also showing call counts and total time seen in probes)
Sequential scan (60s)
function BASE avg (µs) PATCHED avg (µs) BASE calls PATCHED calls BASE
sum (s) PATCHED sum (s)
StartReadBuffers 8 8 712,927 727,083 6.14 6.23
read_stream_start_pending_read 3 3 2,042,992 2,083,866 7.52 7.61
read_stream_next_buffer 6 6 5,528,071 5,636,554 35.40 35.36
Bitmap scan (60s)
function BASE avg (µs) PATCHED avg (µs) BASE calls PATCHED calls BASE
sum (s) PATCHED sum (s)
StartReadBuffers 4 4 2,697,243 2,677,563 11.28 11.26
read_stream_start_pending_read 5 5 2,696,591 2,676,940 14.72 14.60
read_stream_next_buffer 10 10 2,697,105 2,677,477 28.74 28.67
Indistinguishable between BASE and PATCHED
Best,
Xuneng
Attachments:
[application/zip] cold_cache_perftest.zip (140.4K, ../../CABPTF7W6gBEh27hU0GVMM8g=GhEhD+nw_oQGS3Lae_h_n03ejA@mail.gmail.com/2-cold_cache_perftest.zip)
download
^ permalink raw reply [nested|flat] 29+ messages in thread
* Re: BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer
2025-08-03 12:00 BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer PG Bug reporting form <noreply@postgresql.org>
2025-08-04 13:13 ` Re: BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer Xuneng Zhou <xunengzhou@gmail.com>
2025-08-04 19:19 ` Re: BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer Thomas Munro <thomas.munro@gmail.com>
2025-08-05 04:29 ` Re: BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer Thomas Munro <thomas.munro@gmail.com>
2025-08-07 15:09 ` Re: BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer Xuneng Zhou <xunengzhou@gmail.com>
2025-08-08 02:43 ` Re: BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer Thomas Munro <thomas.munro@gmail.com>
2025-08-13 05:29 ` Re: BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer Xuneng Zhou <xunengzhou@gmail.com>
2025-08-13 06:01 ` Re: BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer Thomas Munro <thomas.munro@gmail.com>
2025-08-22 11:58 ` Re: BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer Xuneng Zhou <xunengzhou@gmail.com>
@ 2025-10-11 12:32 ` Xuneng Zhou <xunengzhou@gmail.com>
2025-12-11 04:16 ` Re: BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer Xuneng Zhou <xunengzhou@gmail.com>
0 siblings, 1 reply; 29+ messages in thread
From: Xuneng Zhou @ 2025-10-11 12:32 UTC (permalink / raw)
To: Thomas Munro <thomas.munro@gmail.com>; +Cc: exclusion@gmail.com, pgsql-bugs@lists.postgresql.org, Michael Paquier <michael@paquier.xyz>; Tom Lane <tgl@sss.pgh.pa.us>; nathandbossart@gmail.com
Hi,
shared_buffers = '128MB'
track_io_timing = on
jit = off
effective_io_concurrency = 500
log_min_messages = warning
max_parallel_workers_per_gather = 0
io_combine_limit = ${IO_COMBINE_LIMIT:-36}
NROWS="${NROWS:-20000000}"
shared_buffers = '64GB'
track_io_timing = on
jit = off
effective_io_concurrency = 500
log_min_messages = warning
max_parallel_workers_per_gather = 0
io_combine_limit = ${IO_COMBINE_LIMIT:-36}
NROWS="${NROWS:-200000000}"
With these settings, running the benchmark script on a larger machine
(AX162-R from hetzner) installed with NVMe SSDs did not reveal
consistent or obvious performance gains or regressions in the patched
version, although it did show slightly better results for bitmap scans
in some runs.
Best,
Xuneng
^ permalink raw reply [nested|flat] 29+ messages in thread
* Re: BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer
2025-08-03 12:00 BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer PG Bug reporting form <noreply@postgresql.org>
2025-08-04 13:13 ` Re: BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer Xuneng Zhou <xunengzhou@gmail.com>
2025-08-04 19:19 ` Re: BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer Thomas Munro <thomas.munro@gmail.com>
2025-08-05 04:29 ` Re: BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer Thomas Munro <thomas.munro@gmail.com>
2025-08-07 15:09 ` Re: BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer Xuneng Zhou <xunengzhou@gmail.com>
2025-08-08 02:43 ` Re: BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer Thomas Munro <thomas.munro@gmail.com>
2025-08-13 05:29 ` Re: BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer Xuneng Zhou <xunengzhou@gmail.com>
2025-08-13 06:01 ` Re: BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer Thomas Munro <thomas.munro@gmail.com>
2025-08-22 11:58 ` Re: BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer Xuneng Zhou <xunengzhou@gmail.com>
2025-10-11 12:32 ` Re: BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer Xuneng Zhou <xunengzhou@gmail.com>
@ 2025-12-11 04:16 ` Xuneng Zhou <xunengzhou@gmail.com>
2026-04-11 01:55 ` Re: BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer Thomas Munro <thomas.munro@gmail.com>
0 siblings, 1 reply; 29+ messages in thread
From: Xuneng Zhou @ 2025-12-11 04:16 UTC (permalink / raw)
To: Thomas Munro <thomas.munro@gmail.com>; +Cc: exclusion@gmail.com, pgsql-bugs@lists.postgresql.org, Michael Paquier <michael@paquier.xyz>; Tom Lane <tgl@sss.pgh.pa.us>; nathandbossart@gmail.com
Hi,
I've reviewed patch v2 carefully and found no correctness problems.
Although the performance benefit it brings might be intangible, the
new interface is more robust and simpler than the current one. It does
save a lot of bookkeeping:
1) Initialize slots to InvalidBuffer: Before calling StartReadBuffers,
it had to zero out buffer slots:
while (stream->initialized_buffers < buffer_index + nblocks)
stream->buffers[stream->initialized_buffers++] = InvalidBuffer;
2) Scan for forwarded buffers: After the call, it had to scan the
array to count how many buffers were forwarded:
forwarded = 0;
while (nblocks + forwarded < requested_nblocks &&
stream->buffers[buffer_index + nblocks + forwarded] != InvalidBuffer)
forwarded++;
stream->forwarded_buffers = forwarded;
3) Clear consumed slots: When returning a buffer to the consumer, it
had to clear the slot:
stream->buffers[oldest_buffer_index] = InvalidBuffer;
This could be fragile because:
- read_stream.c must maintain the invariant that unused slots are InvalidBuffer.
- The scanning loop is O(n) and "layer-violating" (it's re-discovering
information that StartReadBuffers already knew).
- Missing a = InvalidBuffer assignment anywhere would cause silent corruption.
With the new interface -- Make npinned an explicit in/out parameter.
1) No initialization needed: StartReadBuffers doesn't look at
uninitialized slots. It only reads buffers[0..npinned-1].
2) No scanning needed: The count is returned directly: *npinned =
actual_npinned - *nblocks; // Buffer manager tells you the count
3) No clearing needed: The "is it forwarded?" check is now i <
actual_npinned, not buffers[i] != InvalidBuffer.
As for the xxx questions, here're some thoughts:
XXX is the single-buffer specialization unaffected due to dead code
elimination, as expected?
- The single-buffer specialization stays unchanged: StartReadBuffer()
still passes npinned=0 with allow_forwarding=false, so the extra
parameter is dead code there and the compiler will inline/strip it
just as before.
XXX unused queue entries could potentially be electrified with
wipe_mem/VALGRIND_MAKE_MEM_NOACCESS, though existing bufmgr.c
assertions are very likely to reveal any fencepost bugs already
- The queue-entry sanitization step removed should not affect
behavior, since StartReadBuffers() now relies on npinned rather than
InvalidBuffer sentinels. Leaving stale values in the array is likely
harmless. If desired, using wipe_mem or VALGRIND_MAKE_MEM_NOACCESS
would simply provide additional debugging hardening.
XXX perhaps we don't need quite so many _npinned <= _nblocks assertions
- I think this makes sense. In my experience, reading through the
read_stream module already requires a fair amount of effort and focus.
There are many edge cases to watch, and the complexity is amplified by
managing two arrays, pointer arithmetic across function calls, and
numerous assertions to validate those operations. Removing assertions
that are not strictly necessary can make the code more readable and
easier to follow, without weakening its defensive properties.
// In read_stream_start_pending_read():
// REMOVE this pre-call assertion:
- Assert(stream->pending_read_npinned <= stream->pending_read_nblocks);
// Entry in StartReadBuffersImpl (bufmgr.c):
- Assert(*npinned <= *nblocks);
// KEEP this post-call assertion:
Assert(stream->pending_read_nblocks >= stream->pending_read_npinned);
The pre-call assertion in read_stream_start_pending_read looks less
needed since the entry assertion in StartReadBuffersImpl catches
exactly a subset of bugs that the entry assertion catches, with no
intervening code that could obscure the failure or change program
state.
--
Best,
Xuneng
^ permalink raw reply [nested|flat] 29+ messages in thread
* Re: BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer
2025-08-03 12:00 BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer PG Bug reporting form <noreply@postgresql.org>
2025-08-04 13:13 ` Re: BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer Xuneng Zhou <xunengzhou@gmail.com>
2025-08-04 19:19 ` Re: BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer Thomas Munro <thomas.munro@gmail.com>
2025-08-05 04:29 ` Re: BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer Thomas Munro <thomas.munro@gmail.com>
2025-08-07 15:09 ` Re: BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer Xuneng Zhou <xunengzhou@gmail.com>
2025-08-08 02:43 ` Re: BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer Thomas Munro <thomas.munro@gmail.com>
2025-08-13 05:29 ` Re: BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer Xuneng Zhou <xunengzhou@gmail.com>
2025-08-13 06:01 ` Re: BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer Thomas Munro <thomas.munro@gmail.com>
2025-08-22 11:58 ` Re: BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer Xuneng Zhou <xunengzhou@gmail.com>
2025-10-11 12:32 ` Re: BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer Xuneng Zhou <xunengzhou@gmail.com>
2025-12-11 04:16 ` Re: BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer Xuneng Zhou <xunengzhou@gmail.com>
@ 2026-04-11 01:55 ` Thomas Munro <thomas.munro@gmail.com>
2026-04-11 02:22 ` Re: BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 1 reply; 29+ messages in thread
From: Thomas Munro @ 2026-04-11 01:55 UTC (permalink / raw)
To: Xuneng Zhou <xunengzhou@gmail.com>; +Cc: exclusion@gmail.com, pgsql-bugs@lists.postgresql.org, Michael Paquier <michael@paquier.xyz>; Tom Lane <tgl@sss.pgh.pa.us>; nathandbossart@gmail.com, Melanie Plageman <melanieplageman@gmail.com>; Heikki Linnakangas <hlinnaka@iki.fi>
On Thu, Dec 11, 2025 at 5:16 PM Xuneng Zhou <xunengzhou@gmail.com> wrote:
> I've reviewed patch v2 carefully and found no correctness problems.
> Although the performance benefit it brings might be intangible, the
> new interface is more robust and simpler than the current one. It does
> save a lot of bookkeeping:
Thanks Xuneng for all your work reviewing and testing this back in December.
Unfortunately this fell through the cracks (sorry) and I didn't push
it before the freeze. Any objections to pushing it now? I can live
with deferring it until master reopens if that's the call (CC RMT),
but it would be nice to tidy up this design wart if we can.
A short restatement of the situation (but see earlier if you want a
... really long one):
* it's already the case that the StartReadBuffers() sometimes stores
and "forwards" already-pinned buffers in its in/out buffers[] argument
* that currently means the caller has to fill it with InvalidBuffer so
they can be distinguished
* it forwards buffers when it decides to boot them out of the current
IO operation and into the next one (that it assumes the caller will
usually make)
* an example reason to split is when an overlapping already
in-progress IO is encountered after it has the pins
* in the current coding, read_stream.c has to initialise its internal
queue with InvalidBuffer and clear them after use, and count them
after each calls
* in the 18 beta phase a bug in an edge case of that accounting was
fixed by b4212231
This patch shifts some of that responsibility to a more natural place:
* it now seems obvious that StartReadBuffers() should just allow an
in/out npinned counter to travel along with the in/out buffers array
* read_stream.c still needs to know how many there are for pin limit purposes
* it also needs to know in the unusual case that the stream ends
earlier and it has to unpin them
* other than that, it's StartReadBuffers()'s private business to manage them
* StartReadBuffers() can do that with trivial arithmetic, no need to
distinguish and count the buffers
* the end result is much simpler and more robust
(Heikki may recall that we wrestled with versions of this
how-to-avoid-unpinning-across-split-IOs question back in the v17 cycle
when he was reviewing the first vectored I/O + read_stream.c patches,
and there were "two phase" and the "three phase" API ideas, which
eventually came back as "forwarded buffers" in v18, and this would be
a simplification for v19, "forwarded buffers + a counter".)
Attachments:
[text/x-patch] v3-0001-Give-StartReadBuffers-a-more-robust-interface.patch (18.0K, ../../CA+hUKG+VjgOp2Tk-JHhbV3F3fpcsFE2g53==Cu55Gv+hP-pabw@mail.gmail.com/2-v3-0001-Give-StartReadBuffers-a-more-robust-interface.patch)
download | inline diff:
From fd18aa6530277487fcf2c602c570f36c274bbc81 Mon Sep 17 00:00:00 2001
From: Thomas Munro <thomas.munro@gmail.com>
Date: Tue, 5 Aug 2025 14:35:09 +1200
Subject: [PATCH v3] Give StartReadBuffers() a more robust interface.
Remove the need for read_stream.c to initialize and clear
stream->buffers[] elements that will be passed to StartReadBuffers(),
and to count them on return. Instead, add an explicit npinned argument
to tell StartReadBuffers() how many buffers were already pinned by the
previous call (if it was a short read), and to learn how many are pinned
beyond *nblocks after this call returns (if it is a short read). It is
an in/out argument. The output of one call can be passed directly as
input to the next call, just like the buffers themselves.
The new function argument count still fits in registers in Unixoid
calling conventions. It doesn't on Windows/x86, but it didn't before
either. The single-buffer StartReadBuffer() specialization is not
affected.
read_stream.c now uses stream->pending_read_npinned to carry that number
from call to call, replacing stream->forwarded_buffers. It no longer
maintains it itself, which now seems a little fragile, layer-violating
and expensive. It still needs to consult the value when respecting pin
limits, but other than that it's entirely StartReadBuffers()' job to
worry about it. The new member variable name better reflects its
relationship with pending_read_{blocknum,nblocks}. It is the number of
buffers in that range that were already pinned by an earlier short read.
After a short read, _blocknum advances, _nblocks shrinks, and _npinned
has already been filled in by StartReadBuffers(), so we're ready for the
next call at next_buffer_index, where *npinned buffers have been placed
by StartReadBuffers().
Assertions about the identity of forwarded buffers are removed, because
StartReadBuffers() already has such assertions, and assertions about
queue entries after that are no longer relevant.
Reviewed-by: Xuneng Zhou <xunengzhou@gmail.com>
Discussion: https://postgr.es/m/19006-80fcaaf69000377e%40postgresql.org
---
src/backend/storage/aio/read_stream.c | 107 +++++---------------------
src/backend/storage/buffer/bufmgr.c | 44 +++++++----
src/include/storage/bufmgr.h | 1 +
src/test/modules/test_aio/test_aio.c | 11 +++
4 files changed, 60 insertions(+), 103 deletions(-)
diff --git a/src/backend/storage/aio/read_stream.c b/src/backend/storage/aio/read_stream.c
index 2374b4cd507..a6169188a53 100644
--- a/src/backend/storage/aio/read_stream.c
+++ b/src/backend/storage/aio/read_stream.c
@@ -99,7 +99,6 @@ struct ReadStream
int16 ios_in_progress;
int16 queue_size;
int16 max_pinned_buffers;
- int16 forwarded_buffers;
int16 pinned_buffers;
/*
@@ -115,7 +114,6 @@ struct ReadStream
int16 combine_distance;
int16 readahead_distance;
uint16 distance_decay_holdoff;
- int16 initialized_buffers;
int16 resume_readahead_distance;
int16 resume_combine_distance;
int read_buffers_flags;
@@ -147,6 +145,7 @@ struct ReadStream
/* The read operation we are currently preparing. */
BlockNumber pending_read_blocknum;
int16 pending_read_nblocks;
+ int pending_read_npinned;
/* Space for buffers and optional per-buffer private data. */
size_t per_buffer_data_size;
@@ -318,10 +317,8 @@ static bool
read_stream_start_pending_read(ReadStream *stream)
{
bool need_wait;
- int requested_nblocks;
int nblocks;
int flags;
- int forwarded;
int16 io_index;
int16 overflow;
int16 buffer_index;
@@ -335,33 +332,12 @@ read_stream_start_pending_read(ReadStream *stream)
Assert(stream->pinned_buffers + stream->pending_read_nblocks <=
stream->max_pinned_buffers);
-#ifdef USE_ASSERT_CHECKING
/* We had better not be overwriting an existing pinned buffer. */
if (stream->pinned_buffers > 0)
Assert(stream->next_buffer_index != stream->oldest_buffer_index);
else
Assert(stream->next_buffer_index == stream->oldest_buffer_index);
- /*
- * Pinned buffers forwarded by a preceding StartReadBuffers() call that
- * had to split the operation should match the leading blocks of this
- * following StartReadBuffers() call.
- */
- Assert(stream->forwarded_buffers <= stream->pending_read_nblocks);
- for (int i = 0; i < stream->forwarded_buffers; ++i)
- Assert(BufferGetBlockNumber(stream->buffers[stream->next_buffer_index + i]) ==
- stream->pending_read_blocknum + i);
-
- /*
- * Check that we've cleared the queue/overflow entries corresponding to
- * the rest of the blocks covered by this read, unless it's the first go
- * around and we haven't even initialized them yet.
- */
- for (int i = stream->forwarded_buffers; i < stream->pending_read_nblocks; ++i)
- Assert(stream->next_buffer_index + i >= stream->initialized_buffers ||
- stream->buffers[stream->next_buffer_index + i] == InvalidBuffer);
-#endif
-
/* Do we need to issue read-ahead advice? */
flags = stream->read_buffers_flags;
if (stream->advice_enabled)
@@ -402,9 +378,9 @@ read_stream_start_pending_read(ReadStream *stream)
buffer_limit = Min(GetAdditionalLocalPinLimit(), PG_INT16_MAX);
else
buffer_limit = Min(GetAdditionalPinLimit(), PG_INT16_MAX);
- Assert(stream->forwarded_buffers <= stream->pending_read_nblocks);
+ Assert(stream->pending_read_npinned <= stream->pending_read_nblocks);
- buffer_limit += stream->forwarded_buffers;
+ buffer_limit += stream->pending_read_npinned;
buffer_limit = Min(buffer_limit, PG_INT16_MAX);
if (buffer_limit == 0 && stream->pinned_buffers == 0)
@@ -431,20 +407,15 @@ read_stream_start_pending_read(ReadStream *stream)
/*
* We say how many blocks we want to read, but it may be smaller on return
- * if the buffer manager decides to shorten the read. Initialize buffers
- * to InvalidBuffer (= not a forwarded buffer) as input on first use only,
- * and keep the original nblocks number so we can check for forwarded
- * buffers as output, below.
+ * if the buffer manager decides to shorten the read.
*/
buffer_index = stream->next_buffer_index;
io_index = stream->next_io_index;
- while (stream->initialized_buffers < buffer_index + nblocks)
- stream->buffers[stream->initialized_buffers++] = InvalidBuffer;
- requested_nblocks = nblocks;
need_wait = StartReadBuffers(&stream->ios[io_index].op,
&stream->buffers[buffer_index],
stream->pending_read_blocknum,
&nblocks,
+ &stream->pending_read_npinned,
flags);
stream->pinned_buffers += nblocks;
@@ -502,28 +473,15 @@ read_stream_start_pending_read(ReadStream *stream)
read_stream_count_io(stream, nblocks, stream->ios_in_progress);
}
- /*
- * How many pins were acquired but forwarded to the next call? These need
- * to be passed to the next StartReadBuffers() call by leaving them
- * exactly where they are in the queue, or released if the stream ends
- * early. We need the number for accounting purposes, since they are not
- * counted in stream->pinned_buffers but we already hold them.
- */
- forwarded = 0;
- while (nblocks + forwarded < requested_nblocks &&
- stream->buffers[buffer_index + nblocks + forwarded] != InvalidBuffer)
- forwarded++;
- stream->forwarded_buffers = forwarded;
-
/*
* We gave a contiguous range of buffer space to StartReadBuffers(), but
* we want it to wrap around at queue_size. Copy overflowing buffers to
* the front of the array where they'll be consumed, but also leave a copy
* in the overflow zone which the I/O operation has a pointer to (it needs
- * a contiguous array). Both copies will be cleared when the buffers are
- * handed to the consumer.
+ * a contiguous array).
*/
- overflow = (buffer_index + nblocks + forwarded) - stream->queue_size;
+ overflow = (buffer_index + nblocks + stream->pending_read_npinned) -
+ stream->queue_size;
if (overflow > 0)
{
Assert(overflow < stream->queue_size); /* can't overlap */
@@ -542,6 +500,7 @@ read_stream_start_pending_read(ReadStream *stream)
/* Adjust the pending read to cover the remaining portion, if any. */
stream->pending_read_blocknum += nblocks;
stream->pending_read_nblocks -= nblocks;
+ Assert(stream->pending_read_nblocks >= stream->pending_read_npinned);
return true;
}
@@ -1036,13 +995,12 @@ read_stream_next_buffer(ReadStream *stream, void **per_buffer_data)
/* Fast path assumptions. */
Assert(stream->ios_in_progress == 0);
- Assert(stream->forwarded_buffers == 0);
Assert(stream->pinned_buffers == 1);
Assert(stream->readahead_distance == 1);
Assert(stream->combine_distance == 1);
Assert(stream->pending_read_nblocks == 0);
+ Assert(stream->pending_read_npinned == 0);
Assert(stream->per_buffer_data_size == 0);
- Assert(stream->initialized_buffers > stream->oldest_buffer_index);
/* We're going to return the buffer we pinned last time. */
oldest_buffer_index = stream->oldest_buffer_index;
@@ -1124,7 +1082,6 @@ read_stream_next_buffer(ReadStream *stream, void **per_buffer_data)
stream->combine_distance = 0;
stream->oldest_buffer_index = stream->next_buffer_index;
stream->pinned_buffers = 0;
- stream->buffers[oldest_buffer_index] = InvalidBuffer;
}
stream->fast_path = false;
@@ -1274,16 +1231,6 @@ read_stream_next_buffer(ReadStream *stream, void **per_buffer_data)
stream->seq_until_processed = InvalidBlockNumber;
}
- /*
- * We must zap this queue entry, or else it would appear as a forwarded
- * buffer. If it's potentially in the overflow zone (ie from a
- * multi-block I/O that wrapped around the queue), also zap the copy.
- */
- stream->buffers[oldest_buffer_index] = InvalidBuffer;
- if (oldest_buffer_index < stream->io_combine_limit - 1)
- stream->buffers[stream->queue_size + oldest_buffer_index] =
- InvalidBuffer;
-
#if defined(CLOBBER_FREED_MEMORY) || defined(USE_VALGRIND)
/*
@@ -1329,26 +1276,13 @@ read_stream_next_buffer(ReadStream *stream, void **per_buffer_data)
#ifndef READ_STREAM_DISABLE_FAST_PATH
/* See if we can take the fast path for all-cached scans next time. */
if (stream->ios_in_progress == 0 &&
- stream->forwarded_buffers == 0 &&
stream->pinned_buffers == 1 &&
stream->readahead_distance == 1 &&
stream->combine_distance == 1 &&
stream->pending_read_nblocks == 0 &&
+ stream->pending_read_npinned == 0 &&
stream->per_buffer_data_size == 0)
{
- /*
- * The fast path spins on one buffer entry repeatedly instead of
- * rotating through the whole queue and clearing the entries behind
- * it. If the buffer it starts with happened to be forwarded between
- * StartReadBuffers() calls and also wrapped around the circular queue
- * partway through, then a copy also exists in the overflow zone, and
- * it won't clear it out as the regular path would. Do that now, so
- * it doesn't need code for that.
- */
- if (stream->oldest_buffer_index < stream->io_combine_limit - 1)
- stream->buffers[stream->queue_size + stream->oldest_buffer_index] =
- InvalidBuffer;
-
stream->fast_path = true;
}
#endif
@@ -1421,24 +1355,19 @@ read_stream_reset(ReadStream *stream)
while ((buffer = read_stream_next_buffer(stream, NULL)) != InvalidBuffer)
ReleaseBuffer(buffer);
- /* Unpin any unused forwarded buffers. */
+ /*
+ * Unpin any unused forwarded buffers. See comments above
+ * StartReadBuffers() for why we do this.
+ */
index = stream->next_buffer_index;
- while (index < stream->initialized_buffers &&
- (buffer = stream->buffers[index]) != InvalidBuffer)
+ while (stream->pending_read_npinned > 0)
{
- Assert(stream->forwarded_buffers > 0);
- stream->forwarded_buffers--;
- ReleaseBuffer(buffer);
-
- stream->buffers[index] = InvalidBuffer;
- if (index < stream->io_combine_limit - 1)
- stream->buffers[stream->queue_size + index] = InvalidBuffer;
-
+ ReleaseBuffer(stream->buffers[index]);
+ stream->pending_read_npinned--;
if (++index == stream->queue_size)
index = 0;
}
- Assert(stream->forwarded_buffers == 0);
Assert(stream->pinned_buffers == 0);
Assert(stream->ios_in_progress == 0);
diff --git a/src/backend/storage/buffer/bufmgr.c b/src/backend/storage/buffer/bufmgr.c
index 3cc0b0bdd92..b966a107b9a 100644
--- a/src/backend/storage/buffer/bufmgr.c
+++ b/src/backend/storage/buffer/bufmgr.c
@@ -1369,16 +1369,20 @@ StartReadBuffersImpl(ReadBuffersOperation *operation,
Buffer *buffers,
BlockNumber blockNum,
int *nblocks,
+ int *npinned,
int flags,
bool allow_forwarding)
{
int actual_nblocks = *nblocks;
+ int actual_npinned = *npinned;
int maxcombine = 0;
bool did_start_io;
IOContext io_context;
IOObject io_object;
+ Assert(*npinned == 0 || allow_forwarding);
Assert(*nblocks == 1 || allow_forwarding);
+ Assert(*npinned <= *nblocks);
Assert(*nblocks > 0);
Assert(*nblocks <= MAX_IO_COMBINE_LIMIT);
@@ -1397,7 +1401,7 @@ StartReadBuffersImpl(ReadBuffersOperation *operation,
{
bool found;
- if (allow_forwarding && buffers[i] != InvalidBuffer)
+ if (allow_forwarding && i < actual_npinned)
{
BufferDesc *bufHdr;
@@ -1443,6 +1447,7 @@ StartReadBuffersImpl(ReadBuffersOperation *operation,
operation->strategy,
io_object, io_context,
&found);
+ actual_npinned++;
}
if (found)
@@ -1456,6 +1461,7 @@ StartReadBuffersImpl(ReadBuffersOperation *operation,
if (i == 0)
{
*nblocks = 1;
+ *npinned = actual_npinned - 1;
#ifdef USE_ASSERT_CHECKING
@@ -1579,22 +1585,30 @@ StartReadBuffersImpl(ReadBuffersOperation *operation,
CheckReadBuffersOperation(operation, !did_start_io);
+ /* Pinned buffers not included in final nblocks, forwarded to next call. */
+ *npinned = actual_npinned - *nblocks;
+
return did_start_io;
}
/*
* Begin reading a range of blocks beginning at blockNum and extending for
- * *nblocks. *nblocks and the buffers array are in/out parameters. On entry,
- * the buffers elements covered by *nblocks must hold either InvalidBuffer or
- * buffers forwarded by an earlier call to StartReadBuffers() that was split
- * and is now being continued. On return, *nblocks holds the number of blocks
- * accepted by this operation. If it is less than the original number then
- * this operation has been split, but buffer elements up to the original
- * requested size may hold forwarded buffers to be used for a continuing
- * operation. The caller must either start a new I/O beginning at the block
- * immediately following the blocks accepted by this call and pass those
- * buffers back in, or release them if it chooses not to. It shouldn't make
- * any other use of or assumptions about forwarded buffers.
+ * *nblocks. *nblocks, *npinned and the buffers array are in/out parameters.
+ *
+ * On entry, *nblocks is the desire number of block to read. On exit, it may
+ * be a smaller number if the operation was split.
+ *
+ * On entry, *npinned is the number of pinned buffers forwarded by an earlier
+ * operation that was split. On exit, it is the number forwarded by this call,
+ * which should be passed to the following call when continuing to read the
+ * same sequence of blocks, along with the corresponding buffers.
+ *
+ * When buffers are forwarded, as reported by *npinned, the caller must either
+ * start a new I/O beginning at the block immediately following the blocks
+ * accepted by this call (*nblocks on exit) and pass those buffers back in head
+ * position, or release them if it chooses not to. They are located in the
+ * buffers array beginning at index *nblocks (on exit). It shouldn't make any
+ * other use of or assumptions about forwarded buffers.
*
* If false is returned, no I/O is necessary and the buffers covered by
* *nblocks on exit are valid and ready to be accessed. If true is returned,
@@ -1610,9 +1624,10 @@ StartReadBuffers(ReadBuffersOperation *operation,
Buffer *buffers,
BlockNumber blockNum,
int *nblocks,
+ int *npinned,
int flags)
{
- return StartReadBuffersImpl(operation, buffers, blockNum, nblocks, flags,
+ return StartReadBuffersImpl(operation, buffers, blockNum, nblocks, npinned, flags,
true /* expect forwarded buffers */ );
}
@@ -1631,9 +1646,10 @@ StartReadBuffer(ReadBuffersOperation *operation,
int flags)
{
int nblocks = 1;
+ int npinned = 0;
bool result;
- result = StartReadBuffersImpl(operation, buffer, blocknum, &nblocks, flags,
+ result = StartReadBuffersImpl(operation, buffer, blocknum, &nblocks, &npinned, flags,
false /* single block, no forwarding */ );
Assert(nblocks == 1); /* single block can't be short */
diff --git a/src/include/storage/bufmgr.h b/src/include/storage/bufmgr.h
index 6837b35fc6d..262dc4262d0 100644
--- a/src/include/storage/bufmgr.h
+++ b/src/include/storage/bufmgr.h
@@ -250,6 +250,7 @@ extern bool StartReadBuffers(ReadBuffersOperation *operation,
Buffer *buffers,
BlockNumber blockNum,
int *nblocks,
+ int *npinned,
int flags);
extern bool WaitReadBuffers(ReadBuffersOperation *operation);
diff --git a/src/test/modules/test_aio/test_aio.c b/src/test/modules/test_aio/test_aio.c
index 35efba1a5e3..e7c4864136c 100644
--- a/src/test/modules/test_aio/test_aio.c
+++ b/src/test/modules/test_aio/test_aio.c
@@ -709,6 +709,7 @@ read_buffers(PG_FUNCTION_ARGS)
Datum *buffers_datum;
bool *io_reqds;
int *nblocks_per_io;
+ int npinned;
Assert(nblocks > 0);
@@ -724,6 +725,15 @@ read_buffers(PG_FUNCTION_ARGS)
rel = relation_open(relid, AccessShareLock);
smgr = RelationGetSmgr(rel);
+ /*
+ * Pins might be forwarded between calls, if IOs are split after pins are
+ * obtained. Such buffers will be provided to the next call in the
+ * buffers array, and their count will be carrier between calls in this
+ * variable. (Only if we decided to give up the loop below would we need
+ * to consult this, to unpin them.)
+ */
+ npinned = 0;
+
/*
* Do StartReadBuffers() until IO for all the required blocks has been
* started (if required).
@@ -744,6 +754,7 @@ read_buffers(PG_FUNCTION_ARGS)
&buffers[nblocks_done],
startblock + nblocks_done,
&nblocks_this_io,
+ &npinned,
0);
nblocks_per_io[nios] = nblocks_this_io;
nios++;
--
2.53.0
^ permalink raw reply [nested|flat] 29+ messages in thread
* Re: BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer
2025-08-03 12:00 BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer PG Bug reporting form <noreply@postgresql.org>
2025-08-04 13:13 ` Re: BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer Xuneng Zhou <xunengzhou@gmail.com>
2025-08-04 19:19 ` Re: BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer Thomas Munro <thomas.munro@gmail.com>
2025-08-05 04:29 ` Re: BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer Thomas Munro <thomas.munro@gmail.com>
2025-08-07 15:09 ` Re: BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer Xuneng Zhou <xunengzhou@gmail.com>
2025-08-08 02:43 ` Re: BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer Thomas Munro <thomas.munro@gmail.com>
2025-08-13 05:29 ` Re: BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer Xuneng Zhou <xunengzhou@gmail.com>
2025-08-13 06:01 ` Re: BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer Thomas Munro <thomas.munro@gmail.com>
2025-08-22 11:58 ` Re: BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer Xuneng Zhou <xunengzhou@gmail.com>
2025-10-11 12:32 ` Re: BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer Xuneng Zhou <xunengzhou@gmail.com>
2025-12-11 04:16 ` Re: BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer Xuneng Zhou <xunengzhou@gmail.com>
2026-04-11 01:55 ` Re: BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer Thomas Munro <thomas.munro@gmail.com>
@ 2026-04-11 02:22 ` Tom Lane <tgl@sss.pgh.pa.us>
2026-04-13 17:24 ` Re: BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer Michael Paquier <michael@paquier.xyz>
0 siblings, 1 reply; 29+ messages in thread
From: Tom Lane @ 2026-04-11 02:22 UTC (permalink / raw)
To: Thomas Munro <thomas.munro@gmail.com>; +Cc: Xuneng Zhou <xunengzhou@gmail.com>; exclusion@gmail.com, pgsql-bugs@lists.postgresql.org, Michael Paquier <michael@paquier.xyz>; nathandbossart@gmail.com, Melanie Plageman <melanieplageman@gmail.com>; Heikki Linnakangas <hlinnaka@iki.fi>
Thomas Munro <thomas.munro@gmail.com> writes:
> Unfortunately this fell through the cracks (sorry) and I didn't push
> it before the freeze. Any objections to pushing it now? I can live
> with deferring it until master reopens if that's the call (CC RMT),
> but it would be nice to tidy up this design wart if we can.
This doesn't seem to me to be a "new feature", so I'm not sure that
feature freeze applies.
> This patch shifts some of that responsibility to a more natural place:
> * it now seems obvious that StartReadBuffers() should just allow an
> in/out npinned counter to travel along with the in/out buffers array
> * read_stream.c still needs to know how many there are for pin limit purposes
> * it also needs to know in the unusual case that the stream ends
> earlier and it has to unpin them
> * other than that, it's StartReadBuffers()'s private business to manage them
> * StartReadBuffers() can do that with trivial arithmetic, no need to
> distinguish and count the buffers
> * the end result is much simpler and more robust
IIUC, this is basically fixing StartReadBuffers' API, and if we don't
do it now then the v19 code will differ from both earlier and later
branches. That doesn't seem like a great place to be when you think
about having to back-patch bug fixes in this area.
So yeah, squeezing this in now seems like a good bet to me.
regards, tom lane
^ permalink raw reply [nested|flat] 29+ messages in thread
* Re: BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer
2025-08-03 12:00 BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer PG Bug reporting form <noreply@postgresql.org>
2025-08-04 13:13 ` Re: BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer Xuneng Zhou <xunengzhou@gmail.com>
2025-08-04 19:19 ` Re: BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer Thomas Munro <thomas.munro@gmail.com>
2025-08-05 04:29 ` Re: BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer Thomas Munro <thomas.munro@gmail.com>
2025-08-07 15:09 ` Re: BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer Xuneng Zhou <xunengzhou@gmail.com>
2025-08-08 02:43 ` Re: BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer Thomas Munro <thomas.munro@gmail.com>
2025-08-13 05:29 ` Re: BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer Xuneng Zhou <xunengzhou@gmail.com>
2025-08-13 06:01 ` Re: BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer Thomas Munro <thomas.munro@gmail.com>
2025-08-22 11:58 ` Re: BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer Xuneng Zhou <xunengzhou@gmail.com>
2025-10-11 12:32 ` Re: BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer Xuneng Zhou <xunengzhou@gmail.com>
2025-12-11 04:16 ` Re: BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer Xuneng Zhou <xunengzhou@gmail.com>
2026-04-11 01:55 ` Re: BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer Thomas Munro <thomas.munro@gmail.com>
2026-04-11 02:22 ` Re: BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer Tom Lane <tgl@sss.pgh.pa.us>
@ 2026-04-13 17:24 ` Michael Paquier <michael@paquier.xyz>
2026-04-15 01:30 ` Re: BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer Xuneng Zhou <xunengzhou@gmail.com>
0 siblings, 1 reply; 29+ messages in thread
From: Michael Paquier @ 2026-04-13 17:24 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Thomas Munro <thomas.munro@gmail.com>; Xuneng Zhou <xunengzhou@gmail.com>; exclusion@gmail.com, pgsql-bugs@lists.postgresql.org, nathandbossart@gmail.com, Melanie Plageman <melanieplageman@gmail.com>; Heikki Linnakangas <hlinnaka@iki.fi>
On Fri, Apr 10, 2026 at 10:22:56PM -0400, Tom Lane wrote:
> Thomas Munro <thomas.munro@gmail.com> writes:
>> Unfortunately this fell through the cracks (sorry) and I didn't push
>> it before the freeze. Any objections to pushing it now? I can live
>> with deferring it until master reopens if that's the call (CC RMT),
>> but it would be nice to tidy up this design wart if we can.
>
> This doesn't seem to me to be a "new feature", so I'm not sure that
> feature freeze applies.
I have read the patch and I would agree this stance.
>> * it now seems obvious that StartReadBuffers() should just allow an
>> in/out npinned counter to travel along with the in/out buffers array
>> * read_stream.c still needs to know how many there are for pin limit purposes
>> * it also needs to know in the unusual case that the stream ends
>> earlier and it has to unpin them
>> * other than that, it's StartReadBuffers()'s private business to manage them
>> * StartReadBuffers() can do that with trivial arithmetic, no need to
>> distinguish and count the buffers
>> * the end result is much simpler and more robust
>
> IIUC, this is basically fixing StartReadBuffers' API, and if we don't
> do it now then the v19 code will differ from both earlier and later
> branches. That doesn't seem like a great place to be when you think
> about having to back-patch bug fixes in this area.
>
> So yeah, squeezing this in now seems like a good bet to me.
+1 for doing it now. It's also worth noting that this shaves code
line-wise.
--
Michael
Attachments:
[application/pgp-signature] signature.asc (832B, ../../ad0mtIJhfwiGkxCD@paquier.xyz/2-signature.asc)
download
^ permalink raw reply [nested|flat] 29+ messages in thread
* Re: BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer
2025-08-03 12:00 BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer PG Bug reporting form <noreply@postgresql.org>
2025-08-04 13:13 ` Re: BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer Xuneng Zhou <xunengzhou@gmail.com>
2025-08-04 19:19 ` Re: BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer Thomas Munro <thomas.munro@gmail.com>
2025-08-05 04:29 ` Re: BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer Thomas Munro <thomas.munro@gmail.com>
2025-08-07 15:09 ` Re: BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer Xuneng Zhou <xunengzhou@gmail.com>
2025-08-08 02:43 ` Re: BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer Thomas Munro <thomas.munro@gmail.com>
2025-08-13 05:29 ` Re: BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer Xuneng Zhou <xunengzhou@gmail.com>
2025-08-13 06:01 ` Re: BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer Thomas Munro <thomas.munro@gmail.com>
2025-08-22 11:58 ` Re: BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer Xuneng Zhou <xunengzhou@gmail.com>
2025-10-11 12:32 ` Re: BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer Xuneng Zhou <xunengzhou@gmail.com>
2025-12-11 04:16 ` Re: BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer Xuneng Zhou <xunengzhou@gmail.com>
2026-04-11 01:55 ` Re: BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer Thomas Munro <thomas.munro@gmail.com>
2026-04-11 02:22 ` Re: BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer Tom Lane <tgl@sss.pgh.pa.us>
2026-04-13 17:24 ` Re: BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer Michael Paquier <michael@paquier.xyz>
@ 2026-04-15 01:30 ` Xuneng Zhou <xunengzhou@gmail.com>
0 siblings, 0 replies; 29+ messages in thread
From: Xuneng Zhou @ 2026-04-15 01:30 UTC (permalink / raw)
To: Michael Paquier <michael@paquier.xyz>; +Cc: Tom Lane <tgl@sss.pgh.pa.us>; Thomas Munro <thomas.munro@gmail.com>; exclusion@gmail.com, pgsql-bugs@lists.postgresql.org, nathandbossart@gmail.com, Melanie Plageman <melanieplageman@gmail.com>; Heikki Linnakangas <hlinnaka@iki.fi>
On Tue, Apr 14, 2026 at 1:24 AM Michael Paquier <michael@paquier.xyz> wrote:
>
> On Fri, Apr 10, 2026 at 10:22:56PM -0400, Tom Lane wrote:
> > Thomas Munro <thomas.munro@gmail.com> writes:
> >> Unfortunately this fell through the cracks (sorry) and I didn't push
> >> it before the freeze. Any objections to pushing it now? I can live
> >> with deferring it until master reopens if that's the call (CC RMT),
> >> but it would be nice to tidy up this design wart if we can.
> >
> > This doesn't seem to me to be a "new feature", so I'm not sure that
> > feature freeze applies.
>
> I have read the patch and I would agree this stance.
>
> >> * it now seems obvious that StartReadBuffers() should just allow an
> >> in/out npinned counter to travel along with the in/out buffers array
> >> * read_stream.c still needs to know how many there are for pin limit purposes
> >> * it also needs to know in the unusual case that the stream ends
> >> earlier and it has to unpin them
> >> * other than that, it's StartReadBuffers()'s private business to manage them
> >> * StartReadBuffers() can do that with trivial arithmetic, no need to
> >> distinguish and count the buffers
> >> * the end result is much simpler and more robust
> >
> > IIUC, this is basically fixing StartReadBuffers' API, and if we don't
> > do it now then the v19 code will differ from both earlier and later
> > branches. That doesn't seem like a great place to be when you think
> > about having to back-patch bug fixes in this area.
> >
> > So yeah, squeezing this in now seems like a good bet to me.
>
> +1 for doing it now. It's also worth noting that this shaves code
> line-wise.
+1.
--
Best,
Xuneng
^ permalink raw reply [nested|flat] 29+ messages in thread
end of thread, other threads:[~2026-04-15 01:30 UTC | newest]
Thread overview: 29+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2025-08-03 12:00 BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer PG Bug reporting form <noreply@postgresql.org>
2025-08-04 01:12 ` Michael Paquier <michael@paquier.xyz>
2025-08-04 13:13 ` Xuneng Zhou <xunengzhou@gmail.com>
2025-08-04 19:19 ` Thomas Munro <thomas.munro@gmail.com>
2025-08-05 04:29 ` Thomas Munro <thomas.munro@gmail.com>
2025-08-05 04:43 ` Tom Lane <tgl@sss.pgh.pa.us>
2025-08-06 08:40 ` Michael Paquier <michael@paquier.xyz>
2025-08-07 11:37 ` Xuneng Zhou <xunengzhou@gmail.com>
2025-08-07 15:09 ` Xuneng Zhou <xunengzhou@gmail.com>
2025-08-08 02:43 ` Thomas Munro <thomas.munro@gmail.com>
2025-08-08 03:48 ` Thomas Munro <thomas.munro@gmail.com>
2025-08-08 04:39 ` Xuneng Zhou <xunengzhou@gmail.com>
2025-08-08 15:06 ` Thomas Munro <thomas.munro@gmail.com>
2025-08-09 04:03 ` Thomas Munro <thomas.munro@gmail.com>
2025-08-10 00:22 ` Michael Paquier <michael@paquier.xyz>
2025-08-10 02:01 ` Thomas Munro <thomas.munro@gmail.com>
2025-08-11 19:00 ` Nathan Bossart <nathandbossart@gmail.com>
2025-08-12 14:41 ` Xuneng Zhou <xunengzhou@gmail.com>
2025-08-13 05:29 ` Xuneng Zhou <xunengzhou@gmail.com>
2025-08-13 06:01 ` Thomas Munro <thomas.munro@gmail.com>
2025-08-13 13:35 ` Xuneng Zhou <xunengzhou@gmail.com>
2025-08-22 03:25 ` Xuneng Zhou <xunengzhou@gmail.com>
2025-08-22 11:58 ` Xuneng Zhou <xunengzhou@gmail.com>
2025-10-11 12:32 ` Xuneng Zhou <xunengzhou@gmail.com>
2025-12-11 04:16 ` Xuneng Zhou <xunengzhou@gmail.com>
2026-04-11 01:55 ` Thomas Munro <thomas.munro@gmail.com>
2026-04-11 02:22 ` Tom Lane <tgl@sss.pgh.pa.us>
2026-04-13 17:24 ` Michael Paquier <michael@paquier.xyz>
2026-04-15 01:30 ` Xuneng Zhou <xunengzhou@gmail.com>
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox