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

Hi,

Here is rest of my review from [1].

On Wed, 26 Aug 2026 at 16:46, Yuhang Qiu <iamqyh(at)gmail(dot)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], 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:
https://postgr.es/m/tencent_9D2274B5F0ABB631C546C61D6FE120DB4E05%40qq.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] https://postgr.es/m/CAN55FZ3Lik2m9J%2BEQjzEHSMSBNgAXU5Myt8LFg%3DD2OQ0i_jUMQ%40mail.gmail.com

--
Regards,
Nazir Bilal Yavuz
Microsoft