public inbox for [email protected]
help / color / mirror / Atom feedFrom: Thomas Munro <[email protected]>
To: Xuneng Zhou <[email protected]>
Cc: [email protected]
Cc: [email protected]
Cc: Michael Paquier <[email protected]>
Subject: Re: BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer
Date: Tue, 5 Aug 2025 16:29:17 +1200
Message-ID: <CA+hUKGJyqwQs_kQFB1J=bx4pHMrMcPF8_PgNKyeNFEo+yNvyiQ@mail.gmail.com> (raw)
In-Reply-To: <CA+hUKGK5x5Yekufz7a+Jr1neahwE9cLZn9xt3gSzSZgE=Spjpw@mail.gmail.com>
References: <[email protected]>
<CABPTF7VaW0Hw2-KXoiYFTH40LeUgr06gE5q09sq9LXQPH-vjPA@mail.gmail.com>
<CA+hUKGK5x5Yekufz7a+Jr1neahwE9cLZn9xt3gSzSZgE=Spjpw@mail.gmail.com>
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, 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 <[email protected]>
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 <[email protected]>
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
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected], [email protected]
Subject: Re: BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer
In-Reply-To: <CA+hUKGJyqwQs_kQFB1J=bx4pHMrMcPF8_PgNKyeNFEo+yNvyiQ@mail.gmail.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox