Hi, Thank you for working on this!
On Wed, 26 Aug 2026 at 16:46, Yuhang Qiu <[email protected]> wrote: > > I tested this with a 64 GiB table containing 8,388,608 pages, with one > row per page. PostgreSQL was restarted before each run to clear shared > buffers. The main settings were: > debug_io_direct=data > io_method=worker > shared_buffers=128MB > effective_io_concurrency=16 > > Results: > master patch speedup > SYSTEM(0.1) 829ms 121ms 6.8x > SYSTEM(1) 7.53s 0.89s 8.4x > SYSTEM(10) 70.84s 10.62s 6.7x > SYSTEM_ROWS(100000) 9.05s 0.97s 9.3x > BERNOULLI(100) LIMIT 100000 1.83s 0.56s 3.3x > > With 8 concurrent clients running `SYSTEM(1)`, throughput increased > from 1.06 TPS to 4.29 TPS. Nice results! > Performance was almost unchanged with `effective_io_concurrency=0`. > With worker I/O and `effective_io_concurrency=1`, the patch was about > 8% slower. When all data was cached, the regression was about 1% to 2%. > Queries that stop early, such as `LIMIT` and `SYSTEM_ROWS`, may also > issue a small number of unused speculative reads. I think that is expected. I started reviewing the patches, but it will take some time. Meanwhile, I think the initscan() strategy changes in 0001 and 0002 fix an independent bug and could be committed separately. Melanie is CCed since she may know more. Bug: heap_beginscan() creates a read stream using the current scan->rs_strategy. On rescan, initscan() recalculates strategy eligibility and may allocate or free the strategy if the relation crosses the NBuffers / 4 threshold. However, the reused read stream retains the original scan->rs_strategy. This can cause the stream to either keep using no strategy after one is allocated or retain a dangling pointer after one is freed. Also, review for the 0001: +void +read_stream_set_strategy(ReadStream *stream, BufferAccessStrategy strategy) +{ + Assert(stream->pinned_buffers == 0); + Assert(stream->ios_in_progress == 0); + Assert(stream->pending_read_nblocks == 0); + + stream->max_pinned_buffers = Min(stream->queue_size - 1, + GetAccessStrategyPinLimit(strategy)); stream->max_pinned_buffers might be 0 after this, it should be at least 1 to proceed. Another thing is, GetAccessStrategyPinLimit() could return higher number than current stream->max_pinned_buffers, it would make sense to re-create stream on some of this cases because it might be faster. I am not sure how to calculate that, though. + + if (stream->stats) + stream->stats->distance_capacity = stream->max_pinned_buffers; You change distance_capacity but we are still in the same scan. I think distance_capacity should be highest stream->max_pinned_buffers, which is the first one as it might only decrease. So, no need to change it. -- Regards, Nazir Bilal Yavuz Microsoft
