Hi, Here is rest of my review from [1].
On Wed, 26 Aug 2026 at 16:46, Yuhang Qiu <[email protected]> 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
