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: > https://postgr.es/m/tencent_9D2274B5F0ABB631C546C61D6FE120DB4E05%40qq.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
v2-0001-read_stream-Allow-changing-the-buffer-access-stra.patch
Description: Binary data
v2-0002-heapam-Keep-read-stream-strategy-in-sync-on-resca.patch
Description: Binary data
v2-0003-heapam-Use-streaming-read-I-O-in-sample-scans.patch
Description: Binary data
v2-0004-explain-Report-streaming-I-O-statistics-for-sampl.patch
Description: Binary data
