[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@gmail.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

Hi,

Here is rest of my review from [1]/messages/by-id/CAN55FZ3Lik2m9J+EQjzEHSMSBNgAXU5Myt8LFg=D2OQ0i_jUMQ@mail.gmail.com.

On Wed, 26 Aug 2026 at 16:46, Yuhang Qiu <iamqyh@gmail.com> wrote:

`SYSTEM_TIME` needs some explanation. It currently checks the time only
in `NextSampleBlock()`, so the existing implementation is not a strict
time limit and may exceed the requested time while processing the
current page. A ReadStream may request a limited number of blocks in
advance, so the deviation can grow to the look-ahead window. This patch
does not add special handling for `SYSTEM_TIME`.

I believe we need to change the documentation at least, since it
mentions: 'maximum number of milliseconds to spend reading the table'.
You can perhaps disable read-streams and fall back to the original
method. Alternatively, you can create a read-stream flag to block
look-ahead distance increases, but I think that doesn't make sense
since it contradicts how read streams work.

Code review:

0002:

Other than the SYSTEM_TIME point above, the code looks good to me. As
mentioned in [1]/messages/by-id/CAN55FZ3Lik2m9J+EQjzEHSMSBNgAXU5Myt8LFg=D2OQ0i_jUMQ@mail.gmail.com, I would move the `heapam.c` changes to 0001 and
submit that patch separately as an independent bug fix.

0003: LGTM.

Besides SampleScan, I found several paths that still read blocks one at
a time without using ReadStream:

- `ProcessSingleRelationFork()` while enabling online checksums;

There is already a thread for this:
/messages/by-id/tencent_9D2274B5F0ABB631C546C61D6FE120DB4E05@qq.com

- `log_newpage_range()`, which reads a block range while generating WAL;

That would make sense.

- exact heap and index scans in pgstattuple;

pgstat_btree_page(), pgstat_hash_page(), and pgstat_gist_page() each
read one page, but pgstat_index() calls them while scanning every
index block. I think pgstat_index() can be refactored to use
read-streams.

- nbtree and GIN verification in amcheck;

These seem very complicated to streamify. I am not sure if the effort
will pay off.

- index-driven heap fetch, which has also been discussed separately [2].

I don't have an opinion on this.

There is also a broader question. Some in-core pathes and many external
extensions still call `ReadBuffer()` directly, and some higher-level
workloads have predictable block access patterns that are not exposed to
the buffer manager. Adapting individual core paths cannot cover all of
these cases.

It may be worth discussing whether DIO needs a more general heuristic
read-ahead mechanism. If there is interest, I can start a separate thread.
That is outside the scope of this patch.

We must use AIO and DIO together; otherwise, DIO without AIO will
perform badly. And to use AIO for reads, read-streams are the general
solution instead of using AIO's own functions. Could you please
explain your idea in more detail?

[1]: /messages/by-id/CAN55FZ3Lik2m9J+EQjzEHSMSBNgAXU5Myt8LFg=D2OQ0i_jUMQ@mail.gmail.com

--
Regards,
Nazir Bilal Yavuz
Microsoft

Hi Nazir,

Thanks for reviewing! Attached is v2.

stream->max_pinned_buffers might be 0 after this, it should be at
least 1 to proceed.

Fixed. `max_pinned_buffers` is now at least 1.

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.

`read_stream_set_strategy()` remains limited by the existing queue
capacity. When a heap scan adds a strategy, it reuses the existing stream.
When it removes a strategy, it recreates the stream so that it can use the
potentially larger queue available without the strategy.

I think distance_capacity should be highest stream->max_pinned_buffers

The patch no longer updates `distance_capacity`.

I would move the `heapam.c` changes to 0001 and submit that patch
separately as an independent bug fix.

The rescan issue is now completely separate from the SampleScan changes.
The independent bug fix is 0002 rather than 0001 because it depends on the
`read_stream_set_strategy()` API added by 0001. Patch 0001 only adds the
API, 0002 fixes the heap rescan issue, and 0003 and 0004 contain the
SampleScan and EXPLAIN changes respectively.

I believe we need to change the documentation at least, since it
mentions: 'maximum number of milliseconds to spend reading the table'.

Agreed. I changed the `SYSTEM_TIME` documentation to say:

specifies approximately how many milliseconds to spend reading the table.
This gives you approximate control over how long the query takes

The example and source comment now also use `approximately`. I kept
ReadStream enabled because `SYSTEM_TIME` was not a strict time limit in the
first place. The documentation now reflects that while retaining the
benefits of streaming I/O.

There is already a thread for this:
/messages/by-id/tencent_9D2274B5F0ABB631C546C61D6FE120DB4E05@qq.com

Got it.

`log_newpage_range()`, which reads a block range while generating WAL;

That would make sense.

I think pgstat_index() can be refactored to use read-streams.

After finishing the work in this thread, I will evaluate whether converting
`log_newpage_range()` and `pgstat_index()` to ReadStream is feasible and
worthwhile.

- nbtree and GIN verification in amcheck;

These seem very complicated to streamify. I am not sure if the effort
will pay off.

Got it.

We must use AIO and DIO together; otherwise, DIO without AIO will
perform badly. And to use AIO for reads, read-streams are the general
solution instead of using AIO's own functions. Could you please
explain your idea in more detail?

I agree that DIO needs to be used with AIO, and that ReadStream should be
the general interface for AIO reads. Paths with an explicit block sequence
should still be converted to ReadStream first.

The heuristic read-ahead I have in mind would also use AIO. It is not an
alternative to AIO. The motivation is that once DIO bypasses the page
cache, we no longer have an equivalent mechanism for some cases that were
previously covered by Linux readahead.

For example, many in-core paths and external extensions have not yet been
adapted to ReadStream. Some access patterns only emerge from temporal and
spatial locality across individual reads, sometimes even across multiple SQL
statements, rather than from an explicit block sequence known in advance.
The idea would be to detect such locality heuristically from observed
relation/block accesses and use AIO to read likely subsequent blocks ahead.

Best Regards,
Yuhang Qiu

Hi,

On Fri, 28 Aug 2026 at 13:05, Yuhang Qiu <iamqyh@gmail.com> wrote:

The rescan issue is now completely separate from the SampleScan changes.
The independent bug fix is 0002 rather than 0001 because it depends on the
`read_stream_set_strategy()` API added by 0001. Patch 0001 only adds the
API, 0002 fixes the heap rescan issue, and 0003 and 0004 contain the
SampleScan and EXPLAIN changes respectively.

Thanks, 0001 and 0002 LGTM.

I believe we need to change the documentation at least, since it
mentions: 'maximum number of milliseconds to spend reading the table'.

Agreed. I changed the `SYSTEM_TIME` documentation to say:

specifies approximately how many milliseconds to spend reading the table.
This gives you approximate control over how long the query takes

I don't think changing "maximum" to "approximately" fully explains the
behavioral change. Providing users with the exact reason would work
better, perhaps something along these lines:

```
The time limit is checked while selecting blocks. As blocks can be
selected and read ahead, any blocks already queued when the limit
expires are still processed. The query can therefore exceed the
requested duration by an amount that depends on the look-ahead
distance and how long those blocks take to read and process.
```

I am also not yet convinced that changing the documentation is
sufficient for `SYSTEM_TIME`. It would be useful to get more opinions
on whether that change is acceptable or whether `SYSTEM_TIME` should
avoid to use read streams.

Other than this point, 0003 looks good to me.

We must use AIO and DIO together; otherwise, DIO without AIO will
perform badly. And to use AIO for reads, read-streams are the general
solution instead of using AIO's own functions. Could you please
explain your idea in more detail?

I agree that DIO needs to be used with AIO, and that ReadStream should be
the general interface for AIO reads. Paths with an explicit block sequence
should still be converted to ReadStream first.

The heuristic read-ahead I have in mind would also use AIO. It is not an
alternative to AIO. The motivation is that once DIO bypasses the page
cache, we no longer have an equivalent mechanism for some cases that were
previously covered by Linux readahead.

For example, many in-core paths and external extensions have not yet been
adapted to ReadStream. Some access patterns only emerge from temporal and
spatial locality across individual reads, sometimes even across multiple SQL
statements, rather than from an explicit block sequence known in advance.
The idea would be to detect such locality heuristically from observed
relation/block accesses and use AIO to read likely subsequent blocks ahead.

It is clearer now, thanks. I can see this being useful as a fallback
for unmodified code with simple and stable access patterns. For paths
where the upcoming block sequence is known, using a read stream makes
sense.

For access patterns that are difficult to predict, mispredictions
could waste I/O and CPU and cause cache pollution. I would therefore
be cautious about introducing this as a general mechanism.

I think creating a separate thread makes sense so that the relevant
details can be discussed. As a small suggestion, I would start by
sharing the idea first rather than beginning implementation; to see
what others think first.

--
Regards,
Nazir Bilal Yavuz
Microsoft

Hi Nazir,

Thanks, 0001 and 0002 LGTM.

Thanks for the review. Attached is v3. Compared with v2:

- 0001 now clamps the current look-ahead distances and those saved for
resuming a paused stream when changing to a strategy with a smaller
pin limit.
- In 0003, I moved `stream_flags` into the block that uses it and removed
the temporary `blockno` variable.
- 0002 and 0004 are unchanged.

I am also not yet convinced that changing the documentation is
sufficient for `SYSTEM_TIME`. It would be useful to get more opinions
on whether that change is acceptable or whether `SYSTEM_TIME` should
avoid to use read streams.

For `SYSTEM_TIME`, I compared the read-stream patch with the unpatched
base using:
```sql
SELECT count(*) FROM wide TABLESAMPLE system_time(100);
```

The table was 1 GiB, with one row per page. I used `shared_buffers=2GB`,
`io_combine_limit=128kB` and `debug_io_direct=data`, evicting the table from
shared buffers before each query. Worker mode used two I/O workers.

Each entry is the median execution time of five `EXPLAIN ANALYZE` runs:
```
io_method effective_io_concurrency Unpatched (ms) Patched (ms)
worker 16 100.14 100.83
worker 256 100.11 103.23
sync 16 100.08 101.51
sync 256 100.11 123.31
io_uring 16 100.10 100.17
io_uring 256 100.10 100.19
```

As you noted, pages selected before the deadline can still be read and
processed afterwards. I'd like to hear what you and others think about
this change to `SYSTEM_TIME`.

Regards,
Yuhang Qiu