Hi,
On 2026-03-31 16:59:14 -0400, Melanie Plageman wrote: > On Tue, Mar 31, 2026 at 12:02 PM Andres Freund <[email protected]> wrote: > > > > 0005+0006: Only increase distance when waiting for IO > > In "aio: io_uring: Trigger async processing for large IOs" (0005), the > first sentence of the commit message is incomplete. Oops. > Is there any reason for both the io size and inflight IOs threshold to > be 4? If they should be the same, I think it would be better if this > was a macro. No, there's no real real they have to be the same. It's just what a bit of experimenting showed working independently for either. > This may not matter, but the old code checked in_flight_before > 5 > before incrementing if for the current IO. The new code counts it > after pushing the current IO onto the submission list. So the new way > is slightly more aggressive. Hm. True. Not sure it matters. I didn't really see a significant difference for anything between 3 and 7, it was only outside of that that I saw worse performance. I have done more validation with the new cutoff value than with the old one, so I'm ever so mildly inclined to use the value currently in the patch, but I won't at all insist on it. > > Unfortuntely with io_uring the situation is more complicated, because > > io_uring performs reads synchronously during submission if the data is > > the > > kernel page cache. This can reduce performance substantially compared > > to > > worker, because it prevents parallelizing the copy from the page cache. > > There is an existing heuristic for that in method_io_uring.c that adds a > > flag to the IO submissions forcing the IO to be processed > > asynchronously, > > allowing for parallelism. Unfortunately the heuristic is triggered by > > the > > number of IOs in flight - which will never become big enough to tgrigger > > after using "needed to wait" to control how far to read ahead. > > > > So 0005 expands the io_uring heuristic to also trigger based on the > > sizes > > of IOs - but that's decidedly not perfect, we e.g. have some experiments > > showing it regressing some parallel bitmap heap scan cases. It may be > > better to somehow tweak the logic to only trigger for worker. > > Trigger which logic only for worker, you mean only increasing the > distance when waiting? Yea. > > As is this has another issue, which is that it prevents IO combining in > > situations where it shouldn't, because right now using the distance to > > control both. See 0008 for an attempt at splitting those concerns. > > Even if you can't combine into a single IO, it seems like a low > distance is problematic because it degrades batching and causes us to > have to call io_uring_enter for every block (I think). I don't think it actually does change the situation around that significantly, because we already end up with "too few" IOs once we hit the distance maximum, as we'll submit another IO as soon as we can. I think we will eventually need some logic to only start submitting again once multiple IOs are possible. But that's another set of heuristics, so a project for another day :) In my experiments the batching was primarily useful to allow to reduce the command submission & interrupt overhead when interacting with storage, i.e. when actually doing IO, not just copying from the page cache. I have seen it help due to reducing syscalls too, but the amount of batching and/or combining seems to have a relatively low ceiling at which it stops helping. > Setting aside more complicated prefetching systems, what it seems like > we are saying is that for all "miss" cases (not in SB) a distance of > above 1 is advantageous (unless we are only doing 1 IO). I wonder if > there is something hacky we can do like not decaying distance below > io_combine_limit if there has been a recent miss or growing it up to > at least io_combine_limit if we aren't getting all hits. I think it's true that if IO execution was all that mattered, we would want a bit more IO in flight at all time. However looking ahead quite deeply also has costs: 1) The resowner and private refcount mechanisms take more CPU cycles if you have more buffers pinned 2) The CPU cache hit ratio goes down if there's a longer time between copying data into s_b and consuming it 3) If you have a scan that won't be consumed to completion, you're wasting more the deeper you look ahead This is actually not hard to show: SET max_parallel_workers_per_gather = 0; SELECT pg_buffercache_evict_relation('pgbench_accounts'); EXPLAIN (ANALYZE, TIMING OFF) SELECT count(*) FROM pgbench_accounts WHERE abalance = 3; (on a tree that has the explain stuff, but not the patches in this thread applied) eic time in ms 1 1326.525 2 1325.240 4 1335.073 8 1343.440 16 1346.189 32 1356.598 64 1398.326 128 1635.081 256 1674.685 512 1677.264 1000 1680.050 This one mainly shows 2) from above, I think, but the others are measurable in other workloads. > > 0007: Make read_stream_reset()/end() not wait for IO > > > > This is a quite experimental, not really correct as-is, patch to avoid > > unnecessarily waiting for in-flight IO when read_stream_reset() is done > > while there's in-flight IO. This is useful for things like nestloop > > antioins with quals on the inner side (without the qual we'd not trigger > > any readahead, as that's deferred in the index prefetching patch). > > > > As-is this will leave IOs visible in pg_aios for a while, potentially > > until the backends exit. That's not right. > > Separating the problems: the handle slot exhaustion seems like it > could be solved by having the backend process discard IOs when it > needs one and there isn't any. I don't think it could lead to exhaustion of handles, pgaio_io_acquire() will call pgaio_io_wait_for_free() which will wait for the oldest IO. > The pg_aios view problems seem solvable with a flag on the IO like > "DISCARDED". But the buffers staying pinned is different. It seems > like you'll need the backend to process the discarded IOs at some > point. Maybe it should do that before idling waiting for input? The easiest way would be to actually leave the IOs registered with the resowner and have it wait for completion at command or transaction end if not already done. But we currently don't really do that with resowners in the !abort case. I'm not sure if anybody would mind doing differently here. Another approach would be to do it in AtEOXact_Aio(), but that would mean the IOs could hang around for a while. A third approach could be to do one of the above, but add some additional that go through in flight IOs and check if they completed, e.g. in pgaio_io_acquire_nb(). > When discarding IOs, I don't really understand why the foreign IO > path, doesn't just clear its own wait ref (not the buffer descriptor > one) and move on -- instead you have it wait. That'd be ok to do, I just didn't want to think about that more complicated and less common case :) > I haven't finished reviewing 0008 yet. I've attached a version of what was 0008 split into two, one to introduce the new helpers, one to introduce the separate combine distance. I've pushed what was 0001 and 0002. Will push the former 0003 shortly. Greetings, Andres Freund
>From 405ab26d42dbf9472bc522b67bc2e6517e162109 Mon Sep 17 00:00:00 2001 From: Andres Freund <[email protected]> Date: Wed, 1 Apr 2026 18:39:21 -0400 Subject: [PATCH v4 01/14] read_stream: Issue IO synchronously while in fast path While in fast-path, execute any IO that we might encounter synchronously. Because we are, in that moment, not reading ahead, dispatching any occasional IO to workers has the dispatch overhead, without any realistic chance of the IO completing before we need it. This helps io_method=worker performance for workloads that have only occasional cache misses, but where those occasional misses still take long enough to matter. It is likely this is only measurable with fast local storage or workloads with the data in the kernel page cache, as with remote storage the IO latency, not the dispatch-to-worker latency, is the determining factor. Reviewed-by: Melanie Plageman <[email protected]> Reviewed-by: Nazir Bilal Yavuz <[email protected]> Discussion: https://postgr.es/m/f3xxfrkafjxpyqxywcxricxgyizjirfceychyxsgn7bwjp5eda@kwbduhy7tfmu Discussion: https://postgr.es/m/CAH2-Wz%3DkMg3PNay96cHMT0LFwtxP-cQSRZTZzh1Cixxf8G%3Dzrw%40mail.gmail.com --- src/backend/storage/aio/read_stream.c | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/src/backend/storage/aio/read_stream.c b/src/backend/storage/aio/read_stream.c index cd54c1a74ac..c9595ea10c7 100644 --- a/src/backend/storage/aio/read_stream.c +++ b/src/backend/storage/aio/read_stream.c @@ -833,6 +833,21 @@ read_stream_next_buffer(ReadStream *stream, void **per_buffer_data) if (stream->advice_enabled) flags |= READ_BUFFERS_ISSUE_ADVICE; + /* + * While in fast-path, execute any IO that we might encounter + * synchronously. Because we are, right now, only looking one + * block ahead, dispatching any occasional IO to workers would + * have the overhead of dispatching to workers, without any + * realistic chance of the IO completing before we need it. We + * will switch to non-synchronous IO after this. + * + * Arguably we should do so only for worker, as there's far less + * dispatch overhead with io_uring. However, tests so far have not + * shown a clear downside and additional io_method awareness here + * seems not great from an abstraction POV. + */ + flags |= READ_BUFFERS_SYNCHRONOUSLY; + /* * Pin a buffer for the next call. Same buffer entry, and * arbitrary I/O entry (they're all free). We don't have to @@ -860,6 +875,12 @@ read_stream_next_buffer(ReadStream *stream, void **per_buffer_data) stream->ios_in_progress = 1; stream->ios[0].buffer_index = oldest_buffer_index; stream->seq_blocknum = next_blocknum + 1; + + /* + * XXX: It might be worth triggering additional read-ahead here, + * to avoid having to effectively do another synchronous IO for + * the next block (if it were also a miss). + */ } else { -- 2.53.0.1.gb2826b52eb
>From 5d23e2b31b60f970884681bd75e1553079d5570b Mon Sep 17 00:00:00 2001 From: Andres Freund <[email protected]> Date: Sat, 28 Mar 2026 10:51:44 -0400 Subject: [PATCH v4 02/14] read_stream: Prevent distance from decaying too quickly Until now we reduced the look-ahead distance by 1 on every hit, and doubled it on every miss. That is problematic because there are very common IO patterns where this prevents us from ever reaching a sufficiently high distance (e.g. a miss followed by a hit will never have the distance grow beyond 2). In many such cases, if we had ever reached a sufficient look-ahead distance, things would have been fine, because we grow the distance faster than we decrease it. One might think that the most obvious answer to this problem would be to never reduce the distance. However, that would not work well, as (particularly with upcoming users of read streams), it is reasonably common to at first have a lot of misses and then to transition to a fully cached workload, e.g. because the same blocks are needed repeatedly within one stream. Doing unnecessarily deep readahead can be costly, due to having to pin a lot more buffers, which increases CPU overhead. Because the cost of a synchronously handled miss can be very high (multiple milliseconds for every IO with commonly used storage) compared to the CPU overhead of keeping the distance too high, we want to err on the side of not reducing the distance too early. The insight that a decrease of the distance by 1 at ever hit may be ok at large distances, but not at low distances, shows a way out: If we only allow decreasing the distance once there were no misses for our maximum look-ahead distance, we will keep the distance high as long as readahead has a chance to do IO asynchronously, but not commonly when not. Several folks have written variants of this patch, at least Thomas Munro, Melanie Plageman and I all have written variants of this. Author: Reviewed-by: Discussion: https://postgr.es/m/ Backpatch: --- src/backend/storage/aio/read_stream.c | 36 ++++++++++++++++++++++++--- 1 file changed, 33 insertions(+), 3 deletions(-) diff --git a/src/backend/storage/aio/read_stream.c b/src/backend/storage/aio/read_stream.c index c9595ea10c7..31f9e35dee3 100644 --- a/src/backend/storage/aio/read_stream.c +++ b/src/backend/storage/aio/read_stream.c @@ -99,6 +99,7 @@ struct ReadStream int16 forwarded_buffers; int16 pinned_buffers; int16 distance; + uint16 distance_decay_holdoff; int16 initialized_buffers; int16 resume_distance; int read_buffers_flags; @@ -364,9 +365,22 @@ read_stream_start_pending_read(ReadStream *stream) /* Remember whether we need to wait before returning this buffer. */ if (!need_wait) { - /* Look-ahead distance decays, no I/O necessary. */ - if (stream->distance > 1) - stream->distance--; + /* + * If there currently is no IO in progress, and we have not needed to + * issue IO recently, decay the look-ahead distance. We detect if we + * had to issue IO recently by having a decay holdoff that's set to + * the max look-ahead distance whenever we need to do IO. This is + * important to ensure we eventually reach a high enough distance to + * perform IO asynchronously when starting out with a small look-ahead + * distance. + */ + if (stream->distance > 1 && stream->ios_in_progress == 0) + { + if (stream->distance_decay_holdoff == 0) + stream->distance--; + else + stream->distance_decay_holdoff--; + } } else { @@ -702,6 +716,7 @@ read_stream_begin_impl(int flags, stream->seq_blocknum = InvalidBlockNumber; stream->seq_until_processed = InvalidBlockNumber; stream->temporary = SmgrIsTemp(smgr); + stream->distance_decay_holdoff = 0; /* * Skip the initial ramp-up phase if the caller says we're going to be @@ -954,6 +969,20 @@ read_stream_next_buffer(ReadStream *stream, void **per_buffer_data) distance = Min(distance, stream->max_pinned_buffers); stream->distance = distance; + /* + * As we needed IO, prevent distance from being reduced within our + * maximum look-ahead window. This avoids having distance collapse too + * quickly in workloads where most of the required blocks are cached, + * but where the remaining IOs are a sufficient enough factor to cause + * a substantial slowdown if executed synchronously. + * + * There are valid arguments for preventing decay for max_ios or for + * max_pinned_buffers. But the argument for max_pinned_buffers seems + * clearer - if we can't see any misses within the maximum look-ahead + * distance, we can't do any useful read-ahead. + */ + stream->distance_decay_holdoff = stream->max_pinned_buffers; + /* * If we've reached the first block of a sequential region we're * issuing advice for, cancel that until the next jump. The kernel @@ -1128,6 +1157,7 @@ read_stream_reset(ReadStream *stream) /* Start off assuming data is cached. */ stream->distance = 1; stream->resume_distance = stream->distance; + stream->distance_decay_holdoff = 0; } /* -- 2.53.0.1.gb2826b52eb
>From 7e608df96c101c5afc2131e2759d1a466355fc75 Mon Sep 17 00:00:00 2001 From: Andres Freund <[email protected]> Date: Tue, 3 Mar 2026 20:23:55 -0500 Subject: [PATCH v4 03/14] aio: io_uring: Trigger async processing for large IOs io_method=io_uring has a heuristic to trigger asynchronous processing of IOs once the IO depth is a bit larger. That heuristic is important when doing buffered IO from the kernel page cache, to allow parallelizing of the memory copy, as otherwise io_method=io_uring would be a lot slower than io_method=worker in that case. An upcoming commit will make read_stream.c only increase the read-ahead distance if we needed to wait for IO to complete. If to-be-read data is in the kernel page cache, io_uring will synchronously execute IO, unless the IO is flagged as async. Therefore the aforementioned change in read_stream.c heuristic would lead to a substantial performance regression with io_uring when data is in the page cache, as we would never reach a deep enough queue to actually trigger the existing heuristic. Parallelizing the copy from the page cache is mainly important when doing a lot of IO, which commonly is only possible when doing largely sequential IO. The reason we don't just mark all io_uring IOs as asynchronous is that the dispatch to a kernel thread has overhead. This overhead is mostly noticeable with small random IOs with a low queue depth, as in that case the gain from parallelizing the memory copy is small and the latency cost high. The facts from the two prior paragraphs show a way out: Use the size of the IO in addition to the depth of the queue to trigger asynchronous processing. One might think that just using the IO size might be enough, but experimentation has shown that not to be the case - with deep look-ahead distances being able to parallelize the memory copy is important even with smaller IOs. Author: Reviewed-by: Discussion: https://postgr.es/m/ Backpatch: --- src/backend/storage/aio/method_io_uring.c | 90 +++++++++++++++++------ 1 file changed, 68 insertions(+), 22 deletions(-) diff --git a/src/backend/storage/aio/method_io_uring.c b/src/backend/storage/aio/method_io_uring.c index 39984df31b4..0c8fe4598c3 100644 --- a/src/backend/storage/aio/method_io_uring.c +++ b/src/backend/storage/aio/method_io_uring.c @@ -409,7 +409,6 @@ static int pgaio_uring_submit(uint16 num_staged_ios, PgAioHandle **staged_ios) { struct io_uring *uring_instance = &pgaio_my_uring_context->io_uring_ring; - int in_flight_before = dclist_count(&pgaio_my_backend->in_flight_ios); Assert(num_staged_ios <= PGAIO_SUBMIT_BATCH_SIZE); @@ -425,27 +424,6 @@ pgaio_uring_submit(uint16 num_staged_ios, PgAioHandle **staged_ios) pgaio_io_prepare_submit(ioh); pgaio_uring_sq_from_io(ioh, sqe); - - /* - * io_uring executes IO in process context if possible. That's - * generally good, as it reduces context switching. When performing a - * lot of buffered IO that means that copying between page cache and - * userspace memory happens in the foreground, as it can't be - * offloaded to DMA hardware as is possible when using direct IO. When - * executing a lot of buffered IO this causes io_uring to be slower - * than worker mode, as worker mode parallelizes the copying. io_uring - * can be told to offload work to worker threads instead. - * - * If an IO is buffered IO and we already have IOs in flight or - * multiple IOs are being submitted, we thus tell io_uring to execute - * the IO in the background. We don't do so for the first few IOs - * being submitted as executing in this process' context has lower - * latency. - */ - if (in_flight_before > 4 && (ioh->flags & PGAIO_HF_BUFFERED)) - io_uring_sqe_set_flags(sqe, IOSQE_ASYNC); - - in_flight_before++; } while (true) @@ -701,10 +679,64 @@ pgaio_uring_check_one(PgAioHandle *ioh, uint64 ref_generation) LWLockRelease(&owner_context->completion_lock); } +/* + * io_uring executes IO in process context if possible. That's generally good, + * as it reduces context switching. When performing a lot of buffered IO that + * means that copying between page cache and userspace memory happens in the + * foreground, as it can't be offloaded to DMA hardware as is possible when + * using direct IO. When executing a lot of buffered IO this causes io_uring + * to be slower than worker mode, as worker mode parallelizes the + * copying. io_uring can be told to offload work to worker threads instead. + * + * If the IOs are small, we only benefit from forcing things into the + * background if there is a lot of IO, as otherwise the overhead from context + * switching is higher than the gain. + * + * If IOs are large, there is benefit from asynchronous processing at lower + * queue depths, as IO latency is less of a crucial factor and parallelizing + * memory copies is more important. In addition, it is important to trigger + * asynchronous processing even at low queue depth, as with foreground + * processing we might never actually reach deep enough IO depths to trigger + * asynchronous processing, which in turn would deprive readahead control + * logic of information about whether a deeper look-ahead distance would be + * advantageous. + * + * We have done some basic benchmarking to validate the thresholds used, but + * it's quite plausible that there are better values. + */ +static bool +pgaio_uring_should_use_async(PgAioHandle *ioh, size_t io_size) +{ + /* + * With DIO there's no benefit from forcing asynchronous processing, as + * io_uring will never process IO completions in the foreground. The + * kernel will use worker threads where appropriate. + */ + if (!(ioh->flags & PGAIO_HF_BUFFERED)) + return false; + + /* + * Once the IO queue depth is not that shallow anymore, the overhead of + * dispatching to the background is a less significant factor. + */ + if (dclist_count(&pgaio_my_backend->in_flight_ios) > 4) + return true; + + /* + * If the IO is larger, the gains from parallelizing the memory copy are + * larger and typically the impact of the latency is smaller. + */ + if (io_size >= (BLCKSZ * 4)) + return true; + + return false; +} + static void pgaio_uring_sq_from_io(PgAioHandle *ioh, struct io_uring_sqe *sqe) { struct iovec *iov; + size_t io_size = 0; switch ((PgAioOp) ioh->op) { @@ -717,6 +749,8 @@ pgaio_uring_sq_from_io(PgAioHandle *ioh, struct io_uring_sqe *sqe) iov->iov_base, iov->iov_len, ioh->op_data.read.offset); + + io_size = iov->iov_len; } else { @@ -726,7 +760,13 @@ pgaio_uring_sq_from_io(PgAioHandle *ioh, struct io_uring_sqe *sqe) ioh->op_data.read.iov_length, ioh->op_data.read.offset); + for (int i = 0; i < ioh->op_data.read.iov_length; i++, iov++) + io_size += iov->iov_len; } + + if (pgaio_uring_should_use_async(ioh, io_size)) + io_uring_sqe_set_flags(sqe, IOSQE_ASYNC); + break; case PGAIO_OP_WRITEV: @@ -747,6 +787,12 @@ pgaio_uring_sq_from_io(PgAioHandle *ioh, struct io_uring_sqe *sqe) ioh->op_data.write.iov_length, ioh->op_data.write.offset); } + + /* + * For now don't trigger use of IOSQE_ASYNC for writes, it's not + * clear there is a performance benefit in doing so. + */ + break; case PGAIO_OP_INVALID: -- 2.53.0.1.gb2826b52eb
>From 4fb43ea17f6f34e9eb3ec7d5f740eb9ceb2c231e Mon Sep 17 00:00:00 2001 From: Andres Freund <[email protected]> Date: Tue, 3 Mar 2026 18:00:53 -0500 Subject: [PATCH v4 04/14] read_stream: Only increase distance when waiting for IO This avoids increasing the distance to the maximum in cases where the IO subsystem is already keeping up. This turns out to be important for performance for two reasons: - Pinning a lot of buffers is not cheap. If additional pins allow us to avoid IO waits, it's definitely worth it, but if we can already do all the necessary readahead at a distance of 16, reading ahead 512 buffers can increase the CPU overhead substantially. This is particularly noticeable when the to-be-read blocks are already in the kernel page cache. - If the read stream is read to completion, reading in data earlier than needed is of limited consequences, leaving aside the CPU costs mentioned above. But if the read stream will not be fully consumed, e.g. because it is on the inner side of a nested loop join, the additional IO can be a serious performance issue. This is not that commonly a problem for current read stream users, but the upcoming work, to use a read stream to fetch table pages as part of an index scan, frequently encounters this. Note that this commit would have substantial performance downsides without earlier commits. In particular the earlier commit to avoid decreasing the readahead distance when there was recent IO is crucial, as otherwise we very often would end up not reading ahead aggressively enough anymore with this commit, due to increasing the distance less often. Author: Reviewed-by: Discussion: https://postgr.es/m/ Backpatch: --- src/backend/storage/aio/read_stream.c | 39 +++++++++++++++++++++++---- 1 file changed, 34 insertions(+), 5 deletions(-) diff --git a/src/backend/storage/aio/read_stream.c b/src/backend/storage/aio/read_stream.c index 31f9e35dee3..4b7db4ab372 100644 --- a/src/backend/storage/aio/read_stream.c +++ b/src/backend/storage/aio/read_stream.c @@ -952,22 +952,51 @@ read_stream_next_buffer(ReadStream *stream, void **per_buffer_data) { int16 io_index = stream->oldest_io_index; int32 distance; /* wider temporary value, clamped below */ + bool needed_wait; /* Sanity check that we still agree on the buffers. */ Assert(stream->ios[io_index].op.buffers == &stream->buffers[oldest_buffer_index]); - WaitReadBuffers(&stream->ios[io_index].op); + needed_wait = WaitReadBuffers(&stream->ios[io_index].op); Assert(stream->ios_in_progress > 0); stream->ios_in_progress--; if (++stream->oldest_io_index == stream->max_ios) stream->oldest_io_index = 0; - /* Look-ahead distance ramps up rapidly after we do I/O. */ - distance = stream->distance * 2; - distance = Min(distance, stream->max_pinned_buffers); - stream->distance = distance; + /* + * If the IO was executed synchronously, we will never see + * WaitReadBuffers() block. This is particularly crucial when + * effective_io_concurrency=0 is used, as all IO will be + * synchronous. Without treating synchronous IO as having waited, we'd + * never allow the distance to get large enough to allow for IO + * combining, resulting in bad performance. + */ + if (stream->ios[io_index].op.flags & READ_BUFFERS_SYNCHRONOUSLY) + needed_wait = true; + + /* + * Have the look-ahead distance ramp up rapidly after we needed to + * wait for IO. We only increase the distance when we needed to wait, + * to avoid increasing the distance further than necessary, as looking + * ahead too far can be costly, both due to the cost of unnecessarily + * pinning many buffers and due to doing IOs that may never be + * consumed if the stream is ended/reset before completion. + * + * If we did not need to wait, the current distance was evidently + * sufficient. + * + * NB: May not increase the distance if we already reached the end of + * the stream, as stream->distance == 0 is used to keep track of + * having reached the end. + */ + if (stream->distance > 0 && needed_wait) + { + distance = stream->distance * 2; + distance = Min(distance, stream->max_pinned_buffers); + stream->distance = distance; + } /* * As we needed IO, prevent distance from being reduced within our -- 2.53.0.1.gb2826b52eb
>From 37037847c723e31211450cfe526525a832d79fb9 Mon Sep 17 00:00:00 2001 From: Andres Freund <[email protected]> Date: Tue, 31 Mar 2026 12:49:28 -0400 Subject: [PATCH v4 05/14] WIP: read_stream: Move logic about IO combining & issuing to helpers The long if statements were hard to read and hard to document. Splitting them into inline helpers makes it much easier to explain each part separately. This is done in preparation for making the logic even more complicated... Author: Reviewed-by: Discussion: https://postgr.es/m/ Backpatch: --- src/backend/storage/aio/read_stream.c | 99 ++++++++++++++++++++++----- 1 file changed, 82 insertions(+), 17 deletions(-) diff --git a/src/backend/storage/aio/read_stream.c b/src/backend/storage/aio/read_stream.c index 4b7db4ab372..5d7bfb8fa00 100644 --- a/src/backend/storage/aio/read_stream.c +++ b/src/backend/storage/aio/read_stream.c @@ -440,6 +440,80 @@ read_stream_start_pending_read(ReadStream *stream) return true; } +/* + * Should we continue to perform look ahead? The look ahead may allow us to + * make the pending IO larger via IO combining or to issue more read ahead. + */ +static inline bool +read_stream_should_look_ahead(ReadStream *stream) +{ + /* never start more IOs than our cap */ + if (stream->ios_in_progress >= stream->max_ios) + return false; + + /* If the callback has signaled end-of-stream, we're done */ + if (stream->distance == 0) + return false; + + /* never pin more buffers than allowed */ + if (stream->pinned_buffers + stream->pending_read_nblocks >= stream->max_pinned_buffers) + return false; + + /* + * Don't start more read-ahead if that'd put us over the distance limit + * for doing read-ahead. + */ + if (stream->pinned_buffers + stream->pending_read_nblocks >= stream->distance) + return false; + + return true; +} + +/* + * We don't start the pending read just because we've hit the distance limit, + * preferring to give it another chance to grow to full io_combine_limit size + * once more buffers have been consumed. But this is not desirable in all + * situations - see below. + */ +static inline bool +read_stream_should_issue_now(ReadStream *stream) +{ + int16 pending_read_nblocks = stream->pending_read_nblocks; + + /* no IO to issue */ + if (pending_read_nblocks == 0) + return false; + + /* never start more IOs than our cap */ + if (stream->ios_in_progress >= stream->max_ios) + return false; + + /* + * If the callback has signaled end-of-stream, start the pending read + * immediately. There is no further potential for IO combining. + */ + if (stream->distance == 0) + return true; + + /* + * If we've already reached io_combine_limit, there's no chance of growing + * the read further. + */ + if (pending_read_nblocks >= stream->io_combine_limit) + return true; + + /* + * If we currently have no reads in flight or prepared, issue the IO once + * we are not looking ahead further. This ensures there's always at least + * one IO prepared. + */ + if (stream->pinned_buffers == 0 && + !read_stream_should_look_ahead(stream)) + return true; + + return false; +} + static void read_stream_look_ahead(ReadStream *stream) { @@ -452,14 +526,13 @@ read_stream_look_ahead(ReadStream *stream) if (stream->batch_mode) pgaio_enter_batchmode(); - while (stream->ios_in_progress < stream->max_ios && - stream->pinned_buffers + stream->pending_read_nblocks < stream->distance) + while (read_stream_should_look_ahead(stream)) { BlockNumber blocknum; int16 buffer_index; void *per_buffer_data; - if (stream->pending_read_nblocks == stream->io_combine_limit) + if (read_stream_should_issue_now(stream)) { read_stream_start_pending_read(stream); continue; @@ -511,21 +584,13 @@ read_stream_look_ahead(ReadStream *stream) } /* - * We don't start the pending read just because we've hit the distance - * limit, preferring to give it another chance to grow to full - * io_combine_limit size once more buffers have been consumed. However, - * if we've already reached io_combine_limit, or we've reached the - * distance limit and there isn't anything pinned yet, or the callback has - * signaled end-of-stream, we start the read immediately. Note that the - * pending read can exceed the distance goal, if the latter was reduced - * after hitting the per-backend buffer limit. + * Check if the pending read should be issued now, or if we should give it + * another chance to grow to the full size. + * + * Note that the pending read can exceed the distance goal, if the latter + * was reduced after hitting the per-backend buffer limit. */ - if (stream->pending_read_nblocks > 0 && - (stream->pending_read_nblocks == stream->io_combine_limit || - (stream->pending_read_nblocks >= stream->distance && - stream->pinned_buffers == 0) || - stream->distance == 0) && - stream->ios_in_progress < stream->max_ios) + if (read_stream_should_issue_now(stream)) read_stream_start_pending_read(stream); /* -- 2.53.0.1.gb2826b52eb
>From e0d086eba083b105dfc59120d9a8d043602c1cb7 Mon Sep 17 00:00:00 2001 From: Andres Freund <[email protected]> Date: Tue, 31 Mar 2026 12:45:41 -0400 Subject: [PATCH v4 06/14] WIP: read stream: Split decision about look ahead for AIO and combining "read_stream: Only increase distance when waiting for IO" caused a regression due to the this conflation, because we ended up not allowing IO combining when never needing to wait for IO (as the distance ended up too small to allow for that), which increased CPU overhead. If we go this way, it probably should be moved ahead of the aforementioned commit. Author: Reviewed-by: Discussion: https://postgr.es/m/ Backpatch: --- src/backend/storage/aio/read_stream.c | 147 ++++++++++++++++++++------ 1 file changed, 115 insertions(+), 32 deletions(-) diff --git a/src/backend/storage/aio/read_stream.c b/src/backend/storage/aio/read_stream.c index 5d7bfb8fa00..9d22a119bef 100644 --- a/src/backend/storage/aio/read_stream.c +++ b/src/backend/storage/aio/read_stream.c @@ -98,10 +98,14 @@ struct ReadStream int16 max_pinned_buffers; int16 forwarded_buffers; int16 pinned_buffers; - int16 distance; + /* limit of how far to look ahead for IO combining */ + int16 combine_distance; + /* limit of how far to look ahead for starting IO early */ + int16 readahead_distance; uint16 distance_decay_holdoff; int16 initialized_buffers; - int16 resume_distance; + int16 resume_readahead_distance; + int16 resume_combine_distance; int read_buffers_flags; bool sync_mode; /* using io_method=sync */ bool batch_mode; /* READ_STREAM_USE_BATCHING */ @@ -332,8 +336,8 @@ read_stream_start_pending_read(ReadStream *stream) /* Shrink distance: no more look-ahead until buffers are released. */ new_distance = stream->pinned_buffers + buffer_limit; - if (stream->distance > new_distance) - stream->distance = new_distance; + if (stream->readahead_distance > new_distance) + stream->readahead_distance = new_distance; /* Unless we have nothing to give the consumer, stop here. */ if (stream->pinned_buffers > 0) @@ -374,12 +378,23 @@ read_stream_start_pending_read(ReadStream *stream) * perform IO asynchronously when starting out with a small look-ahead * distance. */ - if (stream->distance > 1 && stream->ios_in_progress == 0) + if (stream->ios_in_progress == 0) { - if (stream->distance_decay_holdoff == 0) - stream->distance--; - else + if (stream->distance_decay_holdoff > 0) stream->distance_decay_holdoff--; + else + { + if (stream->readahead_distance > 1) + stream->readahead_distance--; + + /* + * XXX: Should we actually reduce this at any time other than + * a reset? For now we have to, as this is also a condition + * for re-enabling fast_path. + */ + if (stream->combine_distance > 1) + stream->combine_distance--; + } } } else @@ -452,18 +467,33 @@ read_stream_should_look_ahead(ReadStream *stream) return false; /* If the callback has signaled end-of-stream, we're done */ - if (stream->distance == 0) + if (stream->readahead_distance == 0) return false; /* never pin more buffers than allowed */ if (stream->pinned_buffers + stream->pending_read_nblocks >= stream->max_pinned_buffers) return false; + /* + * Allow looking further ahead if we have an the process of building a + * larger IO, the IO is not yet big enough and we don't yet have IO in + * flight. Note that this is allowed even if we are reaching the + * read-ahead limit (but not the buffer pin limit). + * + * This is important for cases where either effective_io_concurrency is + * low or we never need to wait for IO and thus are not increasing the + * distance. Without this we would end up with lots of small IOs. + */ + if (stream->pending_read_nblocks > 0 && + stream->pinned_buffers == 0 && + stream->pending_read_nblocks < stream->combine_distance) + return true; + /* * Don't start more read-ahead if that'd put us over the distance limit * for doing read-ahead. */ - if (stream->pinned_buffers + stream->pending_read_nblocks >= stream->distance) + if (stream->pinned_buffers + stream->pending_read_nblocks >= stream->readahead_distance) return false; return true; @@ -492,7 +522,7 @@ read_stream_should_issue_now(ReadStream *stream) * If the callback has signaled end-of-stream, start the pending read * immediately. There is no further potential for IO combining. */ - if (stream->distance == 0) + if (stream->readahead_distance == 0) return true; /* @@ -502,6 +532,11 @@ read_stream_should_issue_now(ReadStream *stream) if (pending_read_nblocks >= stream->io_combine_limit) return true; + /* same if capped not by io_combine_limit but combine_distance */ + if (stream->combine_distance > 0 && + pending_read_nblocks >= stream->combine_distance) + return true; + /* * If we currently have no reads in flight or prepared, issue the IO once * we are not looking ahead further. This ensures there's always at least @@ -552,7 +587,8 @@ read_stream_look_ahead(ReadStream *stream) if (blocknum == InvalidBlockNumber) { /* End of stream. */ - stream->distance = 0; + stream->readahead_distance = 0; + stream->combine_distance = 0; break; } @@ -599,7 +635,7 @@ read_stream_look_ahead(ReadStream *stream) * stream. In the worst case we can always make progress one buffer at a * time. */ - Assert(stream->pinned_buffers > 0 || stream->distance == 0); + Assert(stream->pinned_buffers > 0 || stream->readahead_distance == 0); if (stream->batch_mode) pgaio_exit_batchmode(); @@ -789,10 +825,17 @@ read_stream_begin_impl(int flags, * doing full io_combine_limit sized reads. */ if (flags & READ_STREAM_FULL) - stream->distance = Min(max_pinned_buffers, stream->io_combine_limit); + { + stream->readahead_distance = Min(max_pinned_buffers, stream->io_combine_limit); + stream->combine_distance = stream->io_combine_limit; + } else - stream->distance = 1; - stream->resume_distance = stream->distance; + { + stream->readahead_distance = 1; + stream->combine_distance = 1; + } + stream->resume_readahead_distance = stream->readahead_distance; + stream->resume_combine_distance = stream->combine_distance; /* * Since we always access the same relation, we can initialize parts of @@ -891,7 +934,8 @@ read_stream_next_buffer(ReadStream *stream, void **per_buffer_data) Assert(stream->ios_in_progress == 0); Assert(stream->forwarded_buffers == 0); Assert(stream->pinned_buffers == 1); - Assert(stream->distance == 1); + Assert(stream->readahead_distance == 1); + Assert(stream->combine_distance == 1); Assert(stream->pending_read_nblocks == 0); Assert(stream->per_buffer_data_size == 0); Assert(stream->initialized_buffers > stream->oldest_buffer_index); @@ -965,7 +1009,7 @@ read_stream_next_buffer(ReadStream *stream, void **per_buffer_data) else { /* No more blocks, end of stream. */ - stream->distance = 0; + stream->readahead_distance = 0; stream->oldest_buffer_index = stream->next_buffer_index; stream->pinned_buffers = 0; stream->buffers[oldest_buffer_index] = InvalidBuffer; @@ -981,7 +1025,7 @@ read_stream_next_buffer(ReadStream *stream, void **per_buffer_data) Assert(stream->oldest_buffer_index == stream->next_buffer_index); /* End of stream reached? */ - if (stream->distance == 0) + if (stream->readahead_distance == 0) return InvalidBuffer; /* @@ -995,7 +1039,7 @@ read_stream_next_buffer(ReadStream *stream, void **per_buffer_data) /* End of stream reached? */ if (stream->pinned_buffers == 0) { - Assert(stream->distance == 0); + Assert(stream->readahead_distance == 0); return InvalidBuffer; } } @@ -1016,7 +1060,6 @@ read_stream_next_buffer(ReadStream *stream, void **per_buffer_data) stream->ios[stream->oldest_io_index].buffer_index == oldest_buffer_index) { int16 io_index = stream->oldest_io_index; - int32 distance; /* wider temporary value, clamped below */ bool needed_wait; /* Sanity check that we still agree on the buffers. */ @@ -1056,11 +1099,38 @@ read_stream_next_buffer(ReadStream *stream, void **per_buffer_data) * the stream, as stream->distance == 0 is used to keep track of * having reached the end. */ - if (stream->distance > 0 && needed_wait) + if (stream->readahead_distance > 0 && needed_wait) { - distance = stream->distance * 2; - distance = Min(distance, stream->max_pinned_buffers); - stream->distance = distance; + /* wider temporary value, due to oveflow risk */ + int32 readahead_distance; + + readahead_distance = stream->readahead_distance * 2; + readahead_distance = Min(readahead_distance, stream->max_pinned_buffers); + stream->readahead_distance = readahead_distance; + } + + /* + * Whether we needed to wait or not, allow for more IO combining if we + * needed to do IO. The reason to do so independent of needing to wait + * is that when the data is resident in the kernel page cache, IO + * combining reduces the syscall / dispatch overhead, making it + * worthwhile regardless of needing to wait. + * + * It is also important with io_uring as it will never signal the need + * to wait for reads if all the data is in the page cache. There are + * heuristics to deal with that in method_io_uring.c, but they only + * work when the IO gets large enough. + */ + if (stream->combine_distance > 0 && + stream->combine_distance < stream->io_combine_limit) + { + /* wider temporary value, due to oveflow risk */ + int32 combine_distance; + + combine_distance = stream->combine_distance * 2; + combine_distance = Min(combine_distance, stream->io_combine_limit); + combine_distance = Min(combine_distance, stream->max_pinned_buffers); + stream->combine_distance = combine_distance; } /* @@ -1139,10 +1209,18 @@ 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. */ + /* + * FIXME: It's way too easy to wrongly fast path. I'm pretty sure there's + * several pre-existing cases where it triggers because we are not issuing + * additional prefetching (e.g. because of a small + * effective_io_concurrency) and thus stream->pinned_buffers stays at 1 + * after read_stream_look_ahead(). + */ if (stream->ios_in_progress == 0 && stream->forwarded_buffers == 0 && stream->pinned_buffers == 1 && - stream->distance == 1 && + stream->readahead_distance == 1 && + stream->combine_distance == 1 && stream->pending_read_nblocks == 0 && stream->per_buffer_data_size == 0) { @@ -1188,8 +1266,9 @@ read_stream_next_block(ReadStream *stream, BufferAccessStrategy *strategy) BlockNumber read_stream_pause(ReadStream *stream) { - stream->resume_distance = stream->distance; - stream->distance = 0; + stream->resume_readahead_distance = stream->readahead_distance; + stream->resume_combine_distance = stream->combine_distance; + stream->readahead_distance = 0; return InvalidBlockNumber; } @@ -1201,7 +1280,8 @@ read_stream_pause(ReadStream *stream) void read_stream_resume(ReadStream *stream) { - stream->distance = stream->resume_distance; + stream->readahead_distance = stream->resume_readahead_distance; + stream->combine_distance = stream->resume_combine_distance; } /* @@ -1217,7 +1297,8 @@ read_stream_reset(ReadStream *stream) Buffer buffer; /* Stop looking ahead. */ - stream->distance = 0; + stream->readahead_distance = 0; + stream->combine_distance = 0; /* Forget buffered block number and fast path state. */ stream->buffered_blocknum = InvalidBlockNumber; @@ -1249,8 +1330,10 @@ read_stream_reset(ReadStream *stream) Assert(stream->ios_in_progress == 0); /* Start off assuming data is cached. */ - stream->distance = 1; - stream->resume_distance = stream->distance; + stream->readahead_distance = 1; + stream->combine_distance = 1; + stream->resume_readahead_distance = stream->readahead_distance; + stream->resume_combine_distance = stream->combine_distance; stream->distance_decay_holdoff = 0; } -- 2.53.0.1.gb2826b52eb
>From d8cf3d8c97feb026c6d771ef54571b89ba8ad518 Mon Sep 17 00:00:00 2001 From: Andres Freund <[email protected]> Date: Tue, 31 Mar 2026 13:27:29 -0400 Subject: [PATCH v4 07/14] Hacky implementation of making read_stream_reset()/end() not wait for IO Not waiting for IO during read_stream_reset() can be important for performance in cases where read streams are frequently reset before the end is reached. Current users do not commonly do that, but the upcoming work to use a read stream to prefetch table blocks as part of index scans can do so frequently in some query patterns. E.g. if there is an index scan on the inner side of a nested loop. FIXME: This implementation is problematic though, as there is nothing forcing the discarded IOs to ever be completed if the backend goes idle. That's at the very least problematic because it leads to the underlying buffers continuing to be pinned and the IOs showing up in the pg_aios view. Author: Reviewed-by: Discussion: https://postgr.es/m/ Backpatch: --- src/include/storage/aio.h | 1 + src/backend/storage/aio/aio.c | 26 +++++++++++++++++++ src/backend/storage/aio/read_stream.c | 36 +++++++++++++++++++++------ 3 files changed, 55 insertions(+), 8 deletions(-) diff --git a/src/include/storage/aio.h b/src/include/storage/aio.h index ec543b78409..c184e97a977 100644 --- a/src/include/storage/aio.h +++ b/src/include/storage/aio.h @@ -328,6 +328,7 @@ extern int pgaio_wref_get_id(PgAioWaitRef *iow); extern void pgaio_wref_wait(PgAioWaitRef *iow); extern bool pgaio_wref_check_done(PgAioWaitRef *iow); +extern void pgaio_wref_discard_result(PgAioWaitRef *iow); /* -------------------------------------------------------------------------------- diff --git a/src/backend/storage/aio/aio.c b/src/backend/storage/aio/aio.c index 8f7e26607b9..438d5911126 100644 --- a/src/backend/storage/aio/aio.c +++ b/src/backend/storage/aio/aio.c @@ -1050,6 +1050,32 @@ pgaio_wref_check_done(PgAioWaitRef *iow) return false; } +void +pgaio_wref_discard_result(PgAioWaitRef *iow) +{ + uint64 ref_generation; + bool am_owner; + PgAioHandle *ioh; + PgAioHandleState state; + + ioh = pgaio_io_from_wref(iow, &ref_generation); + + am_owner = ioh->owner_procno == MyProcNumber; + + if (!am_owner) + elog(ERROR, "not you"); + + if (pgaio_io_was_recycled(ioh, ref_generation, &state)) + return; + + pgaio_debug_io(DEBUG2, ioh, + "discarding result %p", + ioh->report_return); + + if (ioh->resowner) + pgaio_io_release_resowner(&ioh->resowner_node, false); +} + /* -------------------------------------------------------------------------------- diff --git a/src/backend/storage/aio/read_stream.c b/src/backend/storage/aio/read_stream.c index 9d22a119bef..397e6576689 100644 --- a/src/backend/storage/aio/read_stream.c +++ b/src/backend/storage/aio/read_stream.c @@ -467,7 +467,7 @@ read_stream_should_look_ahead(ReadStream *stream) return false; /* If the callback has signaled end-of-stream, we're done */ - if (stream->readahead_distance == 0) + if (stream->readahead_distance <= 0) return false; /* never pin more buffers than allowed */ @@ -522,7 +522,7 @@ read_stream_should_issue_now(ReadStream *stream) * If the callback has signaled end-of-stream, start the pending read * immediately. There is no further potential for IO combining. */ - if (stream->readahead_distance == 0) + if (stream->readahead_distance <= 0) return true; /* @@ -635,7 +635,7 @@ read_stream_look_ahead(ReadStream *stream) * stream. In the worst case we can always make progress one buffer at a * time. */ - Assert(stream->pinned_buffers > 0 || stream->readahead_distance == 0); + Assert(stream->pinned_buffers > 0 || stream->readahead_distance <= 0); if (stream->batch_mode) pgaio_exit_batchmode(); @@ -1025,7 +1025,7 @@ read_stream_next_buffer(ReadStream *stream, void **per_buffer_data) Assert(stream->oldest_buffer_index == stream->next_buffer_index); /* End of stream reached? */ - if (stream->readahead_distance == 0) + if (stream->readahead_distance <= 0) return InvalidBuffer; /* @@ -1039,7 +1039,7 @@ read_stream_next_buffer(ReadStream *stream, void **per_buffer_data) /* End of stream reached? */ if (stream->pinned_buffers == 0) { - Assert(stream->readahead_distance == 0); + Assert(stream->readahead_distance <= 0); return InvalidBuffer; } } @@ -1066,7 +1066,27 @@ read_stream_next_buffer(ReadStream *stream, void **per_buffer_data) Assert(stream->ios[io_index].op.buffers == &stream->buffers[oldest_buffer_index]); - needed_wait = WaitReadBuffers(&stream->ios[io_index].op); + /* + * If the stream has been reset, don't even wait for the IO, just + * discard it. + */ + if (stream->readahead_distance < 0) + { + if (pgaio_wref_valid(&stream->ios[io_index].op.io_wref) && + !stream->ios[io_index].op.foreign_io) + { + pgaio_wref_discard_result(&stream->ios[io_index].op.io_wref); + pgaio_wref_clear(&stream->ios[io_index].op.io_wref); + } + else + WaitReadBuffers(&stream->ios[io_index].op); + + needed_wait = false; + } + else + { + needed_wait = WaitReadBuffers(&stream->ios[io_index].op); + } Assert(stream->ios_in_progress > 0); stream->ios_in_progress--; @@ -1297,8 +1317,8 @@ read_stream_reset(ReadStream *stream) Buffer buffer; /* Stop looking ahead. */ - stream->readahead_distance = 0; - stream->combine_distance = 0; + stream->readahead_distance = -1; + stream->combine_distance = -1; /* Forget buffered block number and fast path state. */ stream->buffered_blocknum = InvalidBlockNumber; -- 2.53.0.1.gb2826b52eb
