Workflow [PR], commit [e12f472]

Summary: ❌

• Performance Comparison: Performance dashboard

AI Review

Summary

This PR adds an experimental query plan cache for analyzer-based SELECT queries, including dependency revalidation, access rechecks, per-user quotas, SYSTEM DROP QUERY PLAN CACHE, documentation, and focused stateless coverage. The overall design is close, but I still see one correctness blocker in the cache-hit path, one new concurrency bug in quota accounting, and two remaining query-log metadata gaps.

Findings

❌ Blockers

• [src/Interpreters/executeQuery.cpp:1174] [dismissed by author -- https://github.com/Add query plan cache for complex queries (views, joins, subqueries) #107125#discussion_r3470391281] Validation and execution are still not tied to the same storage identity. validateQueryPlanCacheEntry checks dep.uuid, but materializeCachedQueryPlan deserializes ReadFromTable steps that only store table_name, and QueryPlan::resolveStorages resolves that name again. A concurrent Atomic DROP/CREATE can therefore validate UUID A and then execute the cached plan against UUID B, including an engine/schema that the cache never validated or would normally reject. Minimum fix: carry the validated storage identity into materialization, or make resolveStorages reject when the resolved UUID no longer matches the dependency fingerprint.

⚠️ Majors

• [src/Interpreters/Cache/QueryPlanCache.cpp:234] clear can erase per_user_bytes / entry_weights after a concurrent set has pre-charged them but before Base::set inserts the entry. Because Context::clearQueryPlanCache intentionally runs without holding the context mutex, SYSTEM DROP QUERY PLAN CACHE can leave a resident entry with no tracked weight, so query_plan_cache_size_in_bytes_quota becomes understated until the key is reinserted or the cache is cleared again. Minimum fix: synchronize clear with the pre-charge/insert path, or teach set to notice a concurrent clear and restore/remove the accounting before returning.
• [src/Interpreters/Cache/QueryPlanCacheUtils.cpp:128] [dismissed by author -- https://github.com/Add query plan cache for complex queries (views, joins, subqueries) #107125#discussion_r3470391278] Cache hits still replay stale row-policy names into system.query_log. The dependency fingerprint hashes only the effective filter expression, while entry.used_row_policies is persisted verbatim and later replayed on hits. Replacing or renaming a policy with another one that has the same effective expression keeps validation green but leaves used_row_policies reporting the old names. This is audit metadata, not execution semantics, but it is still incorrect user-visible logging.
• [src/Interpreters/Cache/QueryPlanCacheUtils.cpp:729] [dismissed by author -- https://github.com/Add query plan cache for complex queries (views, joins, subqueries) #107125#discussion_r3501144202] Cache-hit access replay still under-reports accessed objects in system.query_log. columns_unknown dependencies have their column list cleared during dependency merge, so hits log the table name but not the actually read columns, and view dependencies are routed to query_views only even though the miss path records them through addQueryAccessInfo. The result is that query_columns / query_tables on a hit can be materially weaker than on a miss for the same query.

Final Verdict

Status: ❌ Block

Minimum required actions:

• Bind cache-hit validation and materialization to the same storage identity so a concurrent DROP/CREATE cannot execute a cached plan against a different table UUID.
• Fix the clear / set accounting race so query_plan_cache_size_in_bytes_quota remains correct across SYSTEM DROP QUERY PLAN CACHE.

Pushed 230ba5365c9. Merged current master (resolves the conflicting state) and addressed the review.

Docs build (Build docusaurus) — the new query-plan-cache.md was a floating page with no sidebar reference. Linked it from caches.md and documented SYSTEM CLEAR|DROP QUERY PLAN CACHE in the system statements (commit f5a3e4df667).

AI review findings — addressed (commit ec109e09b50, with regression tests 04492/04493; arrayJoin and the parsing wording were already addressed in 5e1f87a999f/a7101abcafc with test 04491):

• Ineligible queries now fall back to the normal interpreter (return nullptr) instead of executing the cacheable logical plan, so cache-enabled execution can't change a query's result or security context.
• SQL SECURITY NONE views are rejected (not only DEFINER); only INVOKER views are eligible.
• Validation re-runs the engine/view-security eligibility check, so a UUID-less DROP/CREATE into an unsupported engine or a view security change is caught even when the schema hash is unchanged.
• computeSchemaHash now includes table constraints and, for views, the SQL security type and definer.
• Access model: duplicate dependencies are merged by unioning column sets (self-joins, tables read in multiple places); scalar-subquery tables are folded out of the plan so they require table-level SELECT on a hit.

Left open for your decision (replied on the threads, not resolved):

• Validation→materialization UUID TOCTOU — the robust fix serializes the expected UUID into ReadFromTable and re-checks in resolveStorages, which touches the plan serialization format used by distributed execution; deferred to a dedicated change.
• CTE-name shadowing — current behavior is fail-close (over-strict, never drops a real dependency); a precise per-scope fix is needed and an imprecise one would risk fail-open.
• Row-policy identity in the fingerprint — affects only logged policy names in query_log, not results or access.

CI — the Hung check failed, possible deadlock found (AST fuzzer) is a master-wide flake tracked by #107941 (40–150 failures/day across dozens of unrelated PRs); the plan cache is off by default, so it is not caused by this PR.

Status update on the AI-review "⚠️ Request changes" verdict (head d66a486e92c):

Addressed

• Docs nit — SQL SECURITY NONE eligibility mismatch (docs/en/operations/query-plan-cache.md): fixed in d66a486e92c. The eligibility text now says only SQL SECURITY INVOKER views are eligible (and that both SQL SECURITY DEFINER and SQL SECURITY NONE views are not cached), matching isStorageEligibleForPlanCache, which rejects any view whose sql_security_type is set to anything other than INVOKER. This was the one open thread not in the earlier "left open" list.

Row-policy audit metadata (Major) — the verdict asks to "fix or explicitly scope out". This was explicitly scoped out on the thread: a same-expression policy rename/replace only makes the used_row_policies names replayed into system.query_log stale; it never affects results or access (the filter-expression hash still revalidates). Folding the sorted policy full names into the dependency fingerprint is the follow-up.

UUID validation→materialization TOCTOU (Blocker) — left as your decision. The window is strictly between validateQueryPlanCacheEntry (checks UUID A) and resolveStorages inside materializeCachedQueryPlan, which re-resolves each leaf by name and pins a StorageSnapshot + lockForShare. Once resolveStorages binds, the snapshot is pinned, so the residual exposure is only that single window.

I did not push a fix for this here: the robust fix touches the query-plan serialization format shared with distributed plan shipping (your stated reason for deferring), and the alternative is a real change to the shared resolveStorages hot path that I cannot build or test in this environment — pushing untested concurrency code to a correctness-sensitive shared path against an explicit maintainer deferral would be the wrong call. For when you do the dedicated change, a non-format-changing option that stays fail-close:

• Have QueryPlan::resolveStorages return (or expose via an out-parameter) the (StorageID, uuid) set it actually bound, then re-validate those bound identities against entry.dependencies after materializeCachedQueryPlan. On any mismatch, treat it as a stale miss (QueryPlanCacheStaleMisses) and fall through to normal planning. Because the bound storages are exactly the ones whose snapshots are pinned, re-checking them fully closes the window without changing the serialized plan format. The serialize-the-expected-UUID-into-ReadFromTable approach remains the more direct fix if the format change is acceptable in that dedicated PR.

CI — the only red checks are Hung check failed, possible deadlock found on Stress (amd_tsan / arm_tsan). The CI report links them to #107941, and the stacks are AST-fuzzer queries (executeASTFuzzerQueries, e.g. SELECT arrayZip('')) with no plan-cache frames; the cache is off by default, so these are the known master-wide flake, not caused by this PR. No re-merge of master was done: the merge-base is from earlier today, the branch is MERGEABLE with no conflicts, and the reds are not fixable by merging master.

Status update on the AI-review ❌ Block verdict (now head 66e3e3eee42).

Fixed — view-column privilege recheck on a hit (the new blocker, QueryPlanCacheUtils.cpp:340)

This was a genuine fail-open access gap and was not one of the previously-deferred items, so I fixed it in 66e3e3eee42. A view is expanded into the cached plan and leaves no ReadFromTable leaf for the view storage itself, so the view dependency had an empty columns set with columns_unknown = false; on a hit checkAccessForQueryPlanCacheHit then only required SELECT on any one column of the view. A miss checks the precise selected view columns in prepareBuildQueryPlanForTableExpression, so a user could warm SELECT a FROM v with SELECT(a) on v, lose SELECT(a) while keeping SELECT(b), and still get a on the hit.

I took the reviewer's second option (require table-level SELECT for view-only dependencies) rather than the first (record the exact selected view columns), because:

• It is consistent with how scalar-subquery tables are already handled — they have the same "no plan leaf, columns unrecoverable" property and already set columns_unknown = true (see case 2 of 04492). Recording precise columns would be the natural follow-up for both cases at once.
• It is the minimal, strictly fail-close change: table-level SELECT is at least as strict as the per-column access a non-cached query checks, so it can never serve a hit with weaker access.

Note this is the deny-on-hit direction, identical to the accepted scalar-subquery behavior: a query that only a column-level grant on a view makes runnable is now denied on a hit (rather than served). I documented this in query-plan-cache.md. If you would prefer column-restricted-view users to still get hits, the precise-column-recording approach is the follow-up (and would cover the scalar-subquery case symmetrically).

Regression: 04494_query_plan_cache_view_column_access — warms SELECT a FROM v under per-column grants on the view, asserts the hit is denied and stays denied after revoking the selected column (another remains), then that granting table-level SELECT makes the hit succeed.

Still open by your decision (unchanged from the previous round)

• UUID validation→materialization TOCTOU (executeQuery.cpp:1174) — left for the dedicated change you described; the robust fix touches the plan serialization format shared with distributed plan shipping. I again did not push speculative concurrency code to that shared path against your explicit deferral.
• Row-policy audit-metadata (Major, QueryPlanCacheUtils.cpp:106) — explicitly scoped out as a follow-up (affects only used_row_policies names replayed into system.query_log, never results or access).
• CTE-shadowing (QueryPlanCacheUtils.cpp:319) — current behavior is fail-close (over-strict), as you noted; a precise per-scope fix is the follow-up.

CI — the only reds on the prior commit were Hung check failed on Stress (amd_tsan / arm_tsan), which the CI report links to #107941 (AST-fuzzer stacks, no plan-cache frames; cache is off by default) — the known master-wide flake, not caused by this PR. A fresh run is now in progress on 66e3e3eee42. No master re-merge: the merge-base is from earlier today, the branch is MERGEABLE with no conflicts, and the reds are not fixable by merging master.

Pushed 9e72ebfaf47: merged current master into the branch. The merge is conflict-free, so the GitHub CONFLICTING/DIRTY state was stale (the branch had fallen ~560 commits behind a fast-moving master); the new merge brings it up to date and should flip the PR back to MERGEABLE. The merge touched only non-overlapping additions in Settings.cpp and SettingsChangesHistory.cpp; all plan-cache settings, history entries, profile events and new files are intact, and there are no leftover conflict markers or duplicate version keys.

CI on the previous head 66e3e3ee — both reds are unrelated, pre-existing flakes:

• Unit tests (tsan, function_prop_fuzzer) → FunctionsStress.stress / AllTests: the function-property fuzzer hit SELECT reinterpret(a, 'Decimal256(21)') ... ARRAY JOIN CAST(... AS Array(Decimal(76, 35))) returning Decimal256 instead of Decimal(76, 21) (INCORRECT_DATA). This is a pre-existing bug in reinterpret's result-scale handling found by the fuzzer, tracked by the CI auto-matching issue FunctionsStress #107242. CIDB shows FunctionsStress.stress failing across 52 distinct PRs on 2026-06-30 alone and every day for the past month — a master-wide flake. This PR touches no Functions/ code, so it cannot be the cause.
• Stress test (amd_tsan) / Stress test (arm_tsan) → Hung check failed, possible deadlock found: the chronic AST-fuzzer Hung-check flake Hung check failed, possible deadlock found #107941 (the plan cache is off by default and the stacks are unrelated BackupsWorker/fuzzer frames).

Open AI-review "⚠️ Request changes" items (left for your decision):

The two remaining "minimum required actions" are, in my reading, both fail-close / audit-only — the same class as the items you already deferred (:106, :319, executeQuery.cpp:1174) — and I did not push speculative fixes for them (I cannot build or test this branch in this environment, and the obvious CTE fix is fail-open; see below):

• QueryPlanCacheUtils.cpp:293 (CTE traversal). The literal suggestion — "traverse ASTWithElement::subquery in table position, like FROM (subquery)" — is fail-open as written. For a CTE referenced only in scalar position, e.g. WITH cte AS (SELECT a FROM t) SELECT (SELECT sum(a) FROM cte), t is folded into a constant and has no plan leaf, yet that change would record it with columns_unknown = false; on a hit checkAccessForQueryPlanCacheHit then only requires "SELECT on any one column", so a user who lost SELECT(a) but kept SELECT(b) would still get the baked a. A correct fix needs per-scope CTE tracking — the same work you deferred on the related thread :319. The current behavior is fail-close: a table CTE is just not cached by default (its body is seen as a scalar subquery) or is over-strict (table-level SELECT on a hit). A safe partial fix that only removes the over-rejection (cache the query, but keep the body's tables columns_unknown = true, i.e. table-level recheck) is possible without scope tracking, but it does not address the over-strictness the reviewer also flags.
• QueryPlanCacheUtils.cpp:686 (cached-hit column logging). This is audit-metadata only — the same class as the row-policy-name audit gap at :106 you scoped out. The precise-column version is not implementable for scalar-subquery tables without re-running the analysis the cache exists to skip (the AST walk only sees raw FROM t, never the resolved column t.b). The only implementable option is the reviewer's conservative fallback: log all columns from the validated metadata for columns_unknown dependencies (over-report rather than under-report). Happy to do that if you want it in this PR rather than as the follow-up that also covers :106.

New CI is running on 9e72ebfaf47.

Pushed 9e72ebfaf47..d11fc3fbdc6 addressing the two new AI-review findings on the current head (both now resolved):

• Blocker — QueryPlanCacheUtils.cpp:177 (row-policy subquery): a SELECT row policy such as USING x <= (SELECT max(v) FROM policy_limits) bakes the subquery result into the plan, but the table it reads never becomes a plan leaf or an AST-closure dependency, so a hit could keep enforcing a stale boundary and it escaped query_plan_cache_allow_scalar_subqueries. fillDependency now makes any plan over a table whose effective row policy filter contains a subquery uncacheable (repeated in validateQueryPlanCacheEntry as defense in depth); a plain policy stays cacheable. New test 04495_query_plan_cache_row_policy_subquery changes the table read by the policy subquery between runs and asserts the query is never cached and stays fresh.
• Major — QueryPlanCache.cpp:193 (per-user quota ghost charge): the per-user quota was charged after Base::set, but a just-inserted probationary entry can be evicted synchronously inside removeOverflow, whose onEntryRemoval then had nothing to subtract, leaving a ghost charge that shrank the user's query_plan_cache_size_in_bytes_quota. The charge is now recorded before Base::set (rolled back if it throws), so a synchronous eviction balances it; the bucket-decrement logic is folded into a decrementUserBytes helper.

Both C++ files pass a clang -fsyntax-only check including thread-safety analysis.

The remaining unresolved threads are your explicit deferrals/dismissals, unchanged: the validate/materialize UUID TOCTOU (executeQuery.cpp:1174), the audit-only row_policy_hash/used_row_policies identity gap (:106), the CTE-scope items (:319, :293), and the columns_unknown column-logging under-report (:686).

Pushed `18e02892983`: fixed the per-user quota admission bug flagged in `QueryPlanCache.cpp` (the new bot finding). canStoreForUser was comparing the user's resident bytes against query_plan_cache_size_in_bytes_quota without releasing the weight of the entry it is about to replace, so a user at the quota could never refresh an invalidated plan — after an ALTER failed validation and the query replanned, the identical-key set was rejected, the stale entry stayed resident, and every later run re-paid validation + replanning. Now the existing key's tracked weight is subtracted before the comparison (mirroring the same-key bookkeeping in set). Added regression test 04496_query_plan_cache_quota_refresh. Verified the change with clang -fsyntax-only (TSA-clean under -Werror).

The remaining 5 unresolved threads are the design/scope calls you reserved (QueryPlanCacheUtils.cpp:128/:332/:358/:729, executeQuery.cpp:1174) — left untouched for your decision.

CI: the sole red is AST fuzzer (amd_debug, targeted) — Logical error: 'Inconsistent AST formatting' on a fuzzed CREATE ... Array(Array(...)) query, which is the known master-wide fuzzer flake (#108372 / #109158) and unrelated to this PR (the plan cache is off by default and the failing query never touches it).

LLVM Coverage Report

Changed lines: Changed C/C++ lines covered: 592/694 (85.30%) · Uncovered code

Full report · Diff report