Object storage: prefetch upcoming files ahead of the current one - #2137
Open
VighneshPath wants to merge 6 commits into
Open
Object storage: prefetch upcoming files ahead of the current one#2137VighneshPath wants to merge 6 commits into
VighneshPath wants to merge 6 commits into
Conversation
StorageObjectStorageSource read files one at a time, opening the next file's reader only after the current one is fully consumed; the number of concurrently open files was therefore tied to num_streams (~max_threads), even though object storage reads are IO-wait-bound rather than CPU-bound and idle cores could be overlapping many more files' fetch latency instead of decode threads' worth. A one-file-ahead reader was already built asynchronously (createReaderAsync via create_reader_pool), but only the pipeline objects were constructed in advance - IInputFormat::read() (where Parquet's ReadManager/Prefetcher actually start fetching) was never called until the reader became current, so no bytes were fetched early. Add a new setting, object_storage_max_files_to_prefetch (default 1, matching prior behaviour exactly), that widens the single lookahead future into a queue of futures and, for slots beyond the first, calls a new IInputFormat::prefetch() hook (no-op by default; overridden by ParquetV3BlockInputFormat to eagerly run initializeIfNeeded()) so their background reads are already in flight by the time generate() reaches them. Per-stream row order is unaffected, since files within a stream are still consumed strictly front-to-back from the queue. Locally, against a latency-injected MinIO/Iceberg REST catalog harness (max_threads=2, cacheless), raising this setting from 1 to 4 took wall time from ~42s to ~15s on a 70-file scan, with identical result checksums throughout. Signed-off-by: VighneshPath <pathrikarvighnesh@gmail.com>
VighneshPath
force-pushed
the
feature/antalya-26.3/object-storage-file-lookahead
branch
from
July 31, 2026 05:09
73a0492 to
12ab361
Compare
Mirrors MergeTreePrefetchedReadPool's observability/testability conventions for its own background prefetch mechanism: - ObjectStorageWaitPrefetchedReaderMicroseconds measures time spent blocked in reader_futures.front().get(), the same way WaitPrefetchTaskMicroseconds does for MergeTree's prefetched reads. - object_storage_file_prefetch_failpoint lets tests deterministically exercise a failed prefetch, the same way prefetched_reader_pool_failpoint does for MergeTree. Signed-off-by: VighneshPath <pathrikarvighnesh@gmail.com>
Each .cpp using a FailPoints:: symbol needs its own local extern declaration (matching MergeTreePrefetchedReadPool.cpp's pattern) - this was missing, causing "use of undeclared identifier 'FailPoints'". Signed-off-by: VighneshPath <pathrikarvighnesh@gmail.com>
… empty `object_storage_max_files_to_prefetch` above 2 could make a scan silently return fewer rows than the files contain. The queued tasks each call `file_iterator->next` themselves, so a task queued earlier can lose the race and resolve to no file while tasks queued later already hold real files. `generate` treated the first empty slot as proof the file list was exhausted and stopped, abandoning those readers. With a single future in flight, as before this setting existed, the two were the same fact. `takeNextQueuedReader` now skips empty slots and reports exhaustion only once every queued slot has been drained. Also move the `FailPoints` extern declaration inside `namespace DB`. At global scope it names a different symbol than the one `FailPoint.cpp` defines, so the build did not link. Note this means row order within a stream is not deterministic above 1, which contradicts the claim in 2e3df41; only which rows are returned is fixed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: VighneshPath <pathrikarvighnesh@gmail.com>
…ource Each source built its own `ThreadPool` sized from `object_storage_max_files_to_prefetch`, so a query's prefetch threads came to `streams * max_files_to_prefetch` with no global ceiling - 1024 threads at 64 streams and depth 16. Nothing bounded that across concurrent queries either. Submit to `Context::getPrefetchThreadpool` instead, the server-wide pool that `MergeTreePrefetchedReadPool` already uses for remote reads and that is capped by `prefetch_threadpool_pool_size`. Reader construction stays off the format parsing pool, so the nested shared-pool deadlock described in `threadPoolCallbackRunner.h` is still avoided. The pool now outlives the source while the scheduled work still captures `this`, so the destructor waits for this source's own outstanding futures. Waiting on the whole pool, as before, would now block on unrelated queries. Drop the `create_reader_pool->wait` that ran when the lookahead was 1: it let a private one-thread pool release its thread before the next task was scheduled, and there is no private thread any more. Taking a reader from the queue already implies its callback finished, which is what that wait gave us. Teardown can still wait for a queued task to reach the front of the shared pool. That is memory-safe but not prompt; source-local cancellation would be a separate change. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: VighneshPath <pathrikarvighnesh@gmail.com>
…ers files Above 1, a stream prepares several files at once and they are claimed in whatever order the background tasks reach the shared file list, so the order files are read in is not deterministic. Which rows come back is unaffected, but rows arrive in a different order between runs when there is no `ORDER BY`. That was worth stating where a user actually looks before raising the setting. `2e3df41adc8` claimed the opposite, and the code comments and the test were corrected earlier; this is the same correction for the user-facing text. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: VighneshPath <pathrikarvighnesh@gmail.com>
VighneshPath
force-pushed
the
feature/antalya-26.3/object-storage-file-lookahead
branch
from
July 31, 2026 05:35
12ab361 to
fbb6fb5
Compare
ianton-ru
reviewed
Jul 31, 2026
|
|
||
| void StorageObjectStorageSource::refillReaderFutures() | ||
| { | ||
| while (reader_futures.size() < max_files_to_prefetch) |
There was a problem hiding this comment.
How to turn the feature off? As far as I understand, with max_files_to_prefetch=0 is equal to 'max_files_to_prefetch=1', but it changes behavior compared to state before PR.
Author
There was a problem hiding this comment.
I think even before this change, ClickHouse already started building the next file's reader in the background. Setting this to 1 should keep that same behavior AFAIK.
Though what does change is, now setting it to 1, uses a shared prefetch pool instead of a dedicated thread
There was a problem hiding this comment.
As I understand, now global shared prefetch thread pool (returned by getPrefetchThreadpool) is used instead of independent pool with one thread before. It may affect other queries used the same prefetch pool.
Does MinIO runs in docker on the same host, or on different host with communication over real network? What about memory usage with large files? |
Author
|
For these numbers, same host, MinIO in Docker, with toxiproxy in front of it to inject latency, not a real network hop. I also tested this on S3 and K8s with this benchmark https://github.com/Altinity/IcebergBench, for around 100M rows, without FS or page cache enabled, CH went from 8.33 s hot / 9.44 s cold to 5.63 / 5.97 (-32%/-37%). Though this does seem to cause some regression when caching is enabled and the setting is > 1. Hot queries with Cache: 2.99 s → 3.06 s (-2.3%, worse), Cold queries with Cache: 3.69 s → 3.40 s (+7.9%, better) I haven't yet tested large files or memory usage yet. I have a Docker setup with toxiproxy already wired up, so I can run a matrix of file sizes and latencies and get back to you with memory numbers. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
StorageObjectStorageSourcereads files one at a time, opening the next file's reader only after the current one is fully consumed. The number of concurrently open files is therefore tied tonum_streams(roughlymax_threads), even though object storage reads are IO-wait-bound rather than CPU-bound, so idle cores could be overlapping many more files' fetch latency instead of a decode thread's worth.A one-file-ahead reader was already built asynchronously, but only the pipeline objects were constructed in advance -
IInputFormat::read(where Parquet'sReadManageractually starts fetching) was not called until the reader became current, so no bytes were fetched early.This adds a setting,
object_storage_max_files_to_prefetch(default1, matching prior behaviour exactly), that widens the single lookahead future into a queue of futures and, for slots beyond the first, calls a newIInputFormat::prefetchhook - a no-op by default, overridden byParquetV3BlockInputFormatto eagerly runinitializeIfNeeded- so their background reads are already in flight by the timegeneratereaches them.Against a latency-injected (Via Toxiproxy) MinIO/Iceberg REST catalog harness (
max_threads=2, without any fs or page cache), raising the setting from 1 to 4 took wall time from ~42s to ~15s on a 70-file scan, with identical result checksums.Notes for review:
Context::getPrefetchThreadpool, the server-wide poolMergeTreePrefetchedReadPoolalready uses, rather than a per-source pool.1, queued tasks race each other forfile_iterator->next, so the order files are read in is not deterministic. Which rows are returned is unaffected, but rows arrive in a different order between runs when there is noORDER BY. This is stated in the setting description.This is a port of a branch developed against upstream
master; the commits were cherry-picked ontoantalya-26.3.Changelog category (leave one):
Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):
Added a setting
object_storage_max_files_to_prefetchthat lets each reading stream of thes3/azureBlobStorage/hdfsand data lake table engines and table functions start the reads of upcoming files ahead of the file it is currently consuming, overlapping object storage fetch latency instead of tying file-level read concurrency to the number of processing threads. The default of1preserves the previous behaviour. Above1, files within a stream are claimed in non-deterministic order, so rows arrive in a different order between runs when there is noORDER BY.Documentation entry for user-facing changes
The new setting
object_storage_max_files_to_prefetchis documented in itsDECLAREdescription insrc/Core/Settings.cpp, including the row-order caveat.CI/CD Options
Exclude tests:
Regression jobs to run: