Re: [PATCH] Use streaming read I/O in sample scans

Hi,

Thank you for working on this!

On Wed, 26 Aug 2026 at 16:46, Yuhang Qiu <iamqyh(at)gmail(dot)com> 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