Fix rare bug in read_stream.c's split IO handling.

The internal queue of buffers could become corrupted in a rare edge case
that failed to invalidate an entry, causing a stale buffer to be
"forwarded" to StartReadBuffers().  This is a simple fix for the
immediate problem.

A small API change might be able to remove this and related fragility
entirely, but that will have to wait a bit.

Defect in commit ed0b87ca.

Bug: 19006
Backpatch-through: 18
Reported-by: Alexander Lakhin <[email protected]>
Reviewed-by: Tom Lane <[email protected]>
Reviewed-by: Michael Paquier <[email protected]>
Reviewed-by: Xuneng Zhou <[email protected]>
Discussion: https://postgr.es/m/19006-80fcaaf69000377e%40postgresql.org

diff --git a/src/backend/storage/aio/read_stream.c b/src/backend/storage/aio/read_stream.c
index 0e7f5557f5cb93b5c5ac2411167d17f3ba4decc4..031fde9f4cbef1227e33ae5250915621ac158294 100644 (file)
--- 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