From 619aea104351cf1bc103fa7161e62e93bbacafa4 Mon Sep 17 00:00:00 2001 From: Thomas Munro 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 Reviewed-by: Tom Lane Reviewed-by: Michael Paquier Reviewed-by: Xuneng Zhou 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)