Workflow [PR], commit [6d928ab]

Summary: ✅

• Performance Comparison: Performance dashboard

AI Review

Summary

This PR extends KEYED BY normalized_query_hash quota accounting from the query-count counters to the read/result/write/error resource counters, and it threads the normalized hash through the main execution pipeline, async inserts, SQL-security contexts, refreshable materialized views, EXPLAIN WHATIF, and dictGetKeys. Most earlier gaps are fixed, but one read-progress path still bypasses quota accounting: callers that only use QueryPipeline::setLimitsAndQuota do not populate the pipeline's stored quota, so WATCH still builds its ReadProgressCallback with quota == nullptr and leaves READ_ROWS / READ_BYTES untracked.

Findings

❌ Blockers

• [src/QueryPipeline/QueryPipeline.cpp:657] setLimitsAndQuota updates the result-limit transform but not QueryPipeline::quota, while PullingPipelineExecutor later builds ReadProgressCallback from that stored field. InterpreterWatchQuery::execute is a current caller that only uses setLimitsAndQuota, so WATCH still skips READ_ROWS / READ_BYTES accounting entirely. That leaves a quota-bypass path for MAX read_rows / MAX read_bytes, including KEYED BY normalized_query_hash quotas. Suggested fix: store quota_ in QueryPipeline::quota here (or call setQuota on the watch pipeline before execution) and add a focused WATCH regression.

Tests

• ⚠️ Add a focused WATCH quota regression that forces the read-progress callback to fire and proves READ_ROWS / READ_BYTES hit the quota bucket. The current suite covers ordinary pulls, async inserts, MV writes, EXPLAIN WHATIF, and dictGetKeys, but not this custom watch pipeline.

Final Verdict

• Status: ⚠️ Request changes
• Minimum required actions: wire the WATCH read-progress path into quota accounting, then cover it with a focused regression.

Addressed the AI review's two required actions (commits bf4c908, b64f595):

• SHOW CREATE QUOTA introspection (EnabledQuota.h): the current-quota form now routes through getQuotaUsages and lists every quota governing the user, consistent with the multi-quota enforcement, instead of an arbitrary single one. New test 05019_show_create_quota_lists_all_current_quotas (user assigned two quotas of different key types → both shown). The single-valued getUsage/getQuotaUsage are kept to avoid touching possible downstream consumers.
• Test coverage: 04341_normalized_query_hash_quota_resources now also covers written_bytes (insert CountingTransform + pre-check) and errors (exception callbacks), each demonstrating independent per-hash buckets for two query patterns. execution_time is omitted as non-deterministic.

Both findings validated locally against a freshly built server.

The two CI reds are unrelated flakes: the Stress test (amd_tsan) Unexpected exception in refresh scheduling is the known open #106737 (MaterializedView refresh scheduling, the other two stress sub-failures cascade from it), and the Performance Comparison array_reduce slowdowns are perf noise (the cross-shard verdict is "No significant performance changes detected").

Addressed the AI review's required action (async-insert written_bytes bucketing) and merged master.

• Async insert flush hash (294ad3494e5): AsynchronousInsertQueue::processData now sets normalized_query_hash on the flush context before constructing InterpreterInsertQuery, mirroring executeQuery. Previously async inserts charged WRITTEN_BYTES to hash 0, so all async patterns shared one bucket under a KEYED BY normalized_query_hash quota. New test 04403_normalized_query_hash_quota_async_insert (Values vs JSONEachRow → distinct hashes) verifies the buckets are independent; I confirmed it fails when the one-line fix is reverted and passes with it. (Inline thread r3442853435 resolved.)
• Merged master (was behind, with a red style check). The merge was conflict-free.
• test_numbers_check gap (f77a41d93ca): the SHOW CREATE QUOTA test landed at 05019 (an artifact of unrelated local working-tree files), far above the highest real test number, so the style check failed with Gap (4364, 5019) > 100. Renumbered it to 04402.

Built locally and re-ran 04341, 04402 and the new 04403 against a freshly built server — all pass.

Picked this up after #107664 merged into master (the branch was then CONFLICTING). Pushed f77a41d93ca..35b70700fde:

1. Re-merged master — the only conflict was in InterpreterShowCreateAccessEntityQuery.cpp, where master had independently adopted the same multi-quota getQuotaUsages() loop (via Enforce all quotas assigned to a user instead of only one #107664); kept the loop + the explanatory comment.

2. Fixed the two red tests 04341_normalized_query_hash_quota_resources and 04403_normalized_query_hash_quota_async_insert (deterministic, only in the ParallelReplicas config). Root cause: the ContextData copy constructor did not copy the new normalized_query_hash member, so Context::createCopy reset it to 0. Under parallel replicas the insert path does Context::createCopy(context) to disable parallel replicas for the write (InterpreterInsertQuery), so the WRITTEN_BYTES accounting charged every INSERT SELECT pattern to the shared hash-0 bucket. One-line fix in the copy constructor (10669e4b30d).

3. Addressed the AI review ⚠️ Request changes Majors (35b70700fde) — the SQL-security/refreshable-MV contexts that still dropped the hash:

   • getSQLSecurityOverriddenContext now carries the outer query hash into the DEFINER/NONE overridden context (covers the definer view-read and the materialized-view dependency-insert sites).
   • RefreshTask now sets the computed refresh hash on refresh_context before building the insert.
   • New test 04404_normalized_query_hash_quota_sql_security verifies a SQL SECURITY DEFINER materialized view's written_bytes is bucketed per outer insert pattern; I confirmed the bucket key equals the outer insert's normalizedQueryHash (propagated, not 0), and verified the refreshable-MV path produces distinct per-hash buckets.

All review threads resolved. The two remaining stress-test reds are known master-wide flakes — Unexpected exception in refresh scheduling (#106737) and Hung check failed (#107941) — unrelated to this change; a fresh CI run on the re-merged branch will re-test them.

Addressed the AI review's ⚠️ Request changes (both required actions) and merged master. Pushed 861237c3503..e8736c8897a:

1. EXPLAIN WHATIF per-hash accounting (8735d389174): the empirical-scan pipeline in WhatIfIndexEstimator called setQuota but never set the normalized query hash, so under a KEYED BY normalized_query_hash quota the scan's read_rows/read_bytes went to the shared hash-0 bucket, and two structurally different EXPLAIN WHATIF patterns shared one bucket. It now calls setNormalizedQueryHash(context->getNormalizedQueryHash()) next to setQuota (the Context copy in WhatIfIndexEstimator::run already preserves the hash). New test 04490_normalized_query_hash_quota_whatif proves two structurally different EXPLAIN WHATIF patterns get independent read_rows buckets.

2. Allocation-free multi-counter usedForQuery (e8736c8897a): the std::vector overload from 861237c3503 was switched to std::initializer_list<std::pair<QuotaType, QuotaValue>>. The braced lists passed by the hot-path callers (ReadProgressCallback::onProgress, LimitsCheckingTransform::checkQuota) now use stack-backed storage and no longer heap-allocate before the empty fast path — while keeping the single combined signature (no overload zoo). All three review threads are addressed and resolved.

3. Merged master (the branch was ~1 week behind and red). The merge was conflict-free; the net diff vs master is exactly the PR's own files plus the two fixes above.

The previous red was the Stress test (arm_msan) Logical error: 'Unexpected exception in refresh scheduling' (STID 2508-3e7b) and the two stress sub-failures cascading from it (the server couldn't start). That is the known #106737 in RefreshTask::doScheduling, unrelated to quotas; it was CLOSED on 2026-06-29, fixed by #108441, whose fix (931d5fd893d) is now included via the master merge — so the fresh CI run picks it up.

CI on the current head e8736c8897a is still finishing; the reds so far are all unrelated to this quota change, and each is already tracked:

• Integration tests (arm_binary, distributed plan, 3/4) → test_storage_iceberg_with_trino::test_clickhouse_writes_trino_reads — Trino infra flake (Query … failed: No nodes available to run query, i.e. the Trino worker didn't come up). It fails the same way on unrelated PRs (DeltaLake : Use new create table transaction #106011, Add unified SYSTEM STOP/PAUSE/CANCEL/REFRESH for streaming engines and materialized views #107476, Use LLVM-libc memory functions instead of glibc ones #107566), and this PR touches no Iceberg/Trino/integration code. Fix already in flight: Fix flaky test test_clickhouse_writes_trino_reads (No nodes available to run query) #108834.
• Stress test (amd_tsan / arm_tsan / arm_asan_ubsan) → Hung check failed, possible deadlock found — the known master-wide flake Hung check failed, possible deadlock found #107941 (all three are bot-linked to it). The hung thread is in QueryAnalyzer::resolveQuery (query analysis, before pipeline execution), and none of the stress-run logs contain any quota frames (EnabledQuota / usedForQuery / checkExceededForQuery / ReadProgressCallback / LimitsCheckingTransform / CountingTransform), so the new accounting paths are not involved.
• CH Inc sync — internal private-repo sync.

AI review is ✅ Approve on this commit and there are no unresolved review threads, so the PR is ready modulo CI completion and merge.

Re-triggered the failed CI jobs on head `e8736c8897a` (run 28426257065) — the remaining reds are confirmed unrelated flakes:

• Stress test (amd_tsan / arm_tsan / arm_asan_ubsan) → Hung check failed = Hung check failed, possible deadlock found #107941 (OPEN, master-wide). All three are bot-linked to it; the hung thread is in QueryAnalyzer::resolveQuery, none of the stress logs touch the quota paths.
• Integration tests (arm_binary, distributed plan, 3/4) → test_storage_iceberg_with_trino::test_clickhouse_writes_trino_reads = Trino infra flake, fix in flight in Fix flaky test test_clickhouse_writes_trino_reads (No nodes available to run query) #108834 (test name and No nodes available to run query error match exactly). This PR touches no Iceberg/Trino code.

No re-merge needed (merge-base is from today, <1 day old). AI review is ✅ Approve, all review threads are resolved, and the PR is MERGEABLE. Ready modulo the flaky reruns going green and merge.

The latest CI on head e8736c8897a (run 28426257065, completed 2026-07-01 01:56 UTC) re-hit only the known master-wide flake:

• Stress test (amd_tsan / arm_tsan) → Hung check failed, possible deadlock found = #107941 (OPEN). Both are bot-linked to it; the hung stacks are in executeASTFuzzerQueries → QueryAnalyzer (query analysis, before pipeline execution), and none of the stress logs contain any quota frames (EnabledQuota / usedForQuery / checkExceededForQuery / ReadProgressCallback / LimitsCheckingTransform / CountingTransform), so the new accounting paths are not involved. These are non-required checks, so the PR is MERGEABLE past them.

I re-triggered the failed stress jobs to give them a fresh roll. No re-merge: the merge-base is still <1 day old, and the only PR-touched file master has evolved since then is Context.cpp, whose new commits are in the background-schedule-pool area — disjoint from this PR's ContextData copy-constructor / normalized_query_hash changes, and the branch remains conflict-free.

AI review is ✅ Approve on this commit and there are no unresolved review threads, so the PR is ready modulo the flaky reruns and merge.

Re-merged master (branch was 3 days behind, merge-base 2f4799932b5 from 2026-06-30) and pushed e8736c8897a..05d90ad53de to give the flaky stress checks a fresh CI roll.

The merge was conflict-free — the six PR-touched files that master has since evolved (Context.cpp/.h, executeQuery.cpp, QueryPipeline.cpp, RefreshTask.cpp, WhatIfIndexEstimator.cpp) auto-merged cleanly, and the net diff vs master is unchanged (34 files, +503/-31, exactly this PR's own files).

The previous head e8736c8897a was red only on the known master-wide flake #107941 Hung check failed, possible deadlock found (Stress test (amd_tsan) and (arm_tsan)). I re-confirmed it is unrelated to this change: both architectures hung on the identical AST-fuzzer query SELECT formatQuery($$SELECT [[[[…$$) (deeply nested arrays, read_rows = 0, is_cancelled = 1), and neither hung_check.log contains any quota frames (EnabledQuota / usedForQuery / checkExceededForQuery / ReadProgressCallback / LimitsCheckingTransform / CountingTransform). These are non-required checks, so the PR stays MERGEABLE.

AI review is ✅ Approve on the prior head and there are no unresolved review threads, so the PR is ready modulo the fresh CI going green and merge.

Addressed the AI review's ⚠️ Request changes verdict on the fresh head — pushed 6d928ab9238.

The re-review surfaced one more direct ReadProgressCallback caller that was missed until now: FunctionDictGetKeys::executeConstPath set the quota but not the normalized query hash, so a dictGetKeys dictionary scan charged READ_ROWS/READ_BYTES to the shared hash-0 bucket. That is the same class of gap already fixed for the EXPLAIN WHATIF empirical-scan path.

• Fix: propagate helper.context->getNormalizedQueryHash() into that callback (one line, next to setQuota).
• Test: 04498_dict_get_keys_normalized_query_hash_quota — under a KEYED BY normalized_query_hash MAX read_rows quota, two structurally different dictGetKeys patterns get independent read_rows buckets (verified locally: the second call of pattern A hits QUOTA_EXCEEDED, while a differently-shaped pattern B stays within its own bucket).

Also noticed on this fresh CI roll: the 30+-pass Hung check flake (#107941) is now green on all 11 stress configs, confirming it was the stale-binary flake. The only remaining reds are unrelated master-wide flakes — 04319_skip_unavailable_shards_mode_table_missing on arm_darwin (already tracked by #109254) and the Upgrade check backup-encryption error (~120 PRs over 7d).

LLVM Coverage Report

Changed lines: Changed C/C++ lines covered: 151/157 (96.18%) · Uncovered code

Full report · Diff report

Fast test (arm_darwin) failed on 04319_skip_unavailable_shards_mode_table_missing and Some queries hung — the known arm_darwin hang flake #103235 (the test connects to a second loopback shard 127.0.0.2, which hangs on the macOS CI runners). It is unrelated to the quota accounting changes here: this PR touches no shard / skip_unavailable_shards test. Everything else is green, including all 11 stress jobs. I re-ran the failed arm_darwin job to try to clear the flake.