Parallelize PromQL selector scans by budgeting read streams for decode-heavy columns - #116932
Parallelize PromQL selector scans by budgeting read streams for decode-heavy columns#116932nikitamikhaylov wants to merge 1 commit into
Conversation
|
Workflow [PR], commit [5a027e1] Summary: ❌
AI ReviewSummaryThis PR lowers the Findings❌ Blockers
Tests
Final VerdictChanges requested. The local parallelization improvement is reasonable, but the remote-filesystem path still violates the PR's explicit-budget contract. LLVM Coverage ReportMeasured on commit 5a027e1.
Changed lines: Changed C/C++ lines covered: 12/12 (100.00%) · Uncovered code |
|
|
||
| /// Reads over remote filesystems (e.g. object storage) take their per-stream budget from the | ||
| /// `..._for_remote_filesystem` counterpart - budget them the same way. | ||
| if (!context->getSettingsRef().isChanged("merge_tree_min_bytes_for_concurrent_read_for_remote_filesystem")) |
There was a problem hiding this comment.
ReadFromMergeTree does not fall back from the remote thresholds to the generic ones (src/Processors/QueryPlan/ReadFromMergeTree.cpp:1128-1131), so this branch still overrides object-storage selector scans even when the user explicitly sets merge_tree_min_bytes_for_concurrent_read in the query. For example, the opt-out that the new test exercises on local storage:
SELECT count()
FROM timeSeriesSelector(...)
SETTINGS merge_tree_min_bytes_for_concurrent_read = 251658240still gets merge_tree_min_bytes_for_concurrent_read_for_remote_filesystem = 4 MiB here when the samples table lives on remote storage, because the deprecated remote setting usually remains "unchanged" and is the only one the remote path reads. That breaks the "respect an explicit byte budget" contract for Cloud/object-storage deployments. The remote override needs to treat a changed generic setting as an opt-out too (or copy the generic value into the remote counterpart when the remote setting is unset).
Build profile diff (arm_release)Comparing ✅ No significant changes. Binary sizes
Only the stripped binary is compared: the official master build keeps debug symbols while PR builds strip them, so the other binaries differ by construction. Compile time of recompiled translation units7 translation units recompiled, 11 s compile time in total, 7 of them have a recent master baseline. |
…or decode-heavy columns A PromQL selector scan reads a few narrow, decode-heavy columns (DoubleDelta timestamps, Gorilla values) from a handful of primary-key-selected granule ranges of the samples table. The generic per-stream read budget merge_tree_min_bytes_for_concurrent_read (240 MiB) assumes bytes are a proxy for work, which heavy codecs break: with the samples-table geometry (32768-row granules, index_granularity_bytes = 10 MiB) the budget demands 24 granules per stream, so a dashboard-sized scan of ~20 granules (hundreds of thousands of rows) is planned as a single-stream in-order read and all the decompression is serialized on one core. Lower the byte budget on the selector's query context to 4 MiB - the byte equivalent of the default row budget (merge_tree_min_rows_for_concurrent_read = 163840 rows at ~30 bytes per row) - so the row budget becomes the binding constraint: each stream still gets 1-2 ms of decode work and the scan parallelizes. Applied only when the user has not set the setting; same for the remote-filesystem counterpart. Measured on a 9-billion-row samples table (10 series over 7 days, 720K rows scanned): the selector SQL drops from 35.8 ms to 13 ms (ReadPoolInOrder x1 -> ReadPool x3); the end-to-end PromQL query_range latency over the Prometheus HTTP API drops from 25 ms to 20 ms warm and from 60 ms to 32 ms after a server restart with a cold page cache. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
74853fd to
5a027e1
Compare
|
📊 Cloud Performance Report ✅ AI verdict: no significant changes detected. K_source=6 K_base=30 flagged=0/65 clickbench🟢 No significant changes tpch_adapted_1_official🟢 No significant changes Debug info
|
Changelog category (leave one):
Changelog entry (a user-readable short description of the changes that goes into CHANGELOG.md):
Scans of the TimeSeries samples table performed by PromQL selectors are now parallelized: the per-stream read budget on the selector's query context is lowered from the generic
merge_tree_min_bytes_for_concurrent_read(240 MiB) to 4 MiB, so the row budget becomes the binding constraint for these narrow, decode-heavy columns.Documentation entry for user-facing changes
A PromQL selector scan reads a few narrow, decode-heavy columns (DoubleDelta timestamps, Gorilla values) from a handful of primary-key-selected granule ranges of the samples table. The generic per-stream read budget
merge_tree_min_bytes_for_concurrent_read(240 MiB) assumes bytes are a proxy for work, which heavy codecs break: with the samples-table geometry (32768-row granules,index_granularity_bytes= 10 MiB) the budget demands 24 granules per stream, so a dashboard-sized scan of ~20 granules (hundreds of thousands of rows) is planned as a single-stream in-order read (ReadPoolInOrder × 1), serializing all the decompression on one core.This PR lowers the byte budget on the selector's query context to 4 MiB — the byte equivalent of the default row budget (
merge_tree_min_rows_for_concurrent_read= 163840 rows at ~30 bytes per row of id + timestamp + value ≈ 5 MiB) — so the row budget becomes the binding constraint: each stream still gets 1–2 ms of decode work, and the scan parallelizes. The override applies only when the setting is not explicitly changed by the user; the..._for_remote_filesystemcounterpart is budgeted the same way.Measured on a 9-billion-row samples table (ClockBench d02: 10 series over 7 days, ~720K rows scanned, 32 cores):
ReadPoolInOrder × 1→ReadPool × 3);query_rangeover the Prometheus HTTP API: 25 ms → 20 ms warm, 60 ms → 32 ms on the first query after a server restart with a dropped page cache.A stateless test asserts the pipeline shape both ways (parallel by default, single-stream when the user pins the generic budget) and that both plans return identical results.
Workflow [PR]
Sync PR [sync-upstream/pr/116932]