ClickHouse / ClickHouse/ClickHouse
Clamp applied to a copy: nested `use_query_cache` still escapes `readonly`/`CONST`
- Dominant language
- C++
- Stars
- 49.9k
- Forks
- 9k
- Avg merge
- 21h 32m
- Merged PRs (30d)
- 515
Description
### Describe what's wrong
**A session that may not change settings (`readonly = 1`, or a `use_query_cache ... CONST` profile constraint) can still turn on the query result cache from inside a subquery/CTE, and then reads stale rows from it. `SELECT sum(x) FROM (SELECT x FROM t SETTINGS use_query_cache = 1) SETTINGS readonly = 1` returns the cached value after the table changed, while the top-level form `SELECT sum(x) FROM t SETTINGS use_query_cache = 1, readonly = 1` is refused with `READONLY`. Second symptom: a non-castable value in the same clause now surfaces `BAD_GET` (code 170) instead of the parse error it raised before this PR.**
- **Root cause:** [`src/Analyzer/QueryTreeBuilder.cpp:319`](https://github.com/ClickHouse/ClickHouse/blob/dd7889a115c63e/src/Analyzer/QueryTreeBuilder.cpp#L319) - `settings_changes = set_query.changes;` assigns the raw clause. The added comment claims the copy is only cosmetic ("the tree's AST and hash are unchanged"), but `Planner.cpp:2472` consumes the same list semantically, so the clamp never reaches the one setting the Planner reads off the node. For a value that fails `castValueUtil`, `SettingsConstraints::checkOrClamp` erases it silently ([`src/Access/SettingsConstraints.cpp:290`](https://github.com/ClickHouse/ClickHouse/blob/dd7889a115c63e/src/Access/SettingsConstraints.cpp#L290)) instead of throwing, and the Planner then calls `safeGet` on the raw `String` Field.
Analysis details (evidence, affected locations, impact)
**Why we believe this is a bug:** `QueryTreeBuilder::buildSelectExpression` ([`src/Analyzer/QueryTreeBuilder.cpp:316-319`](https://github.com/ClickHouse/ClickHouse/blob/dd7889a115c63e/src/Analyzer/QueryTreeBuilder.cpp#L316-L319)) clamps a COPY and stores the unclamped original on the `QueryNode`. `Planner::buildPlanForQueryNode` -> `shouldUseQueryCacheForSubquery` ([`src/Planner/Planner.cpp:2472-2475`](https://github.com/ClickHouse/ClickHouse/blob/dd7889a115c63e/src/Planner/Planner.cpp#L2472-L2475)) reads that unclamped list, not the node's context, and returns `change.value.safeGet()` for `use_query_cache`.
**Affected locations:**
- [`src/Analyzer/QueryTreeBuilder.cpp:319`](https://github.com/ClickHouse/ClickHouse/blob/dd7889a115c63e/src/Analyzer/QueryTreeBuilder.cpp#L319) — unclamped clause stored on QueryNode
- [`src/Planner/Planner.cpp:2472`](https://github.com/ClickHouse/ClickHouse/blob/dd7889a115c63e/src/Planner/Planner.cpp#L2472) — shouldUseQueryCacheForSubquery reads the unclamped list
- [`src/Access/SettingsConstraints.cpp:290`](https://github.com/ClickHouse/ClickHouse/blob/dd7889a115c63e/src/Access/SettingsConstraints.cpp#L290) — checkOrClamp erases uncastable/unknown changes instead of throwing
**Impact:** The constraint bypass this PR exists to close remains open for `use_query_cache`, and the resulting cached entry is served back to the same constrained user, so the query returns stale rows after the underlying table changes. Additionally, the query-cache machinery now runs in a state the context says is off, producing errors such as `QUERY_CACHE_USED_WITH_SYSTEM_TABLE` for a `readonly` session that never enabled the cache, and `BAD_GET` for a mistyped value.
### Does it reproduce on most recent release?
Yes — confirmed on current `master` (commit `dd7889a115c63e`).
### How to reproduce
[▶ Run on ClickHouse Fiddle](https://fiddle.clickhouse.com/510dc931-0537-4495-8b39-a00e326a8ba6)
Reproducer
```sql
-- Tag no-parallel: reads the server-wide query result cache, which neighbouring
-- 03381_query_result_cache_* tests drop.
-- A SETTINGS clause nested in a subquery must not enable the query result cache
-- when the session may not change settings.
DROP TABLE IF EXISTS t_qc_readonly;
CREATE TABLE t_qc_readonly (x UInt64) ENGINE = MergeTree ORDER BY x;
INSERT INTO t_qc_readonly VALUES (1);
-- Control: a nested clause is dropped for a session that may not change settings.
SELECT s = getSetting('max_block_size') FROM (SELECT getSetting('max_block_size') AS s SETTINGS max_block_size = 999) SETTINGS readonly = 1;
SELECT sum(x) FROM (SELECT x FROM t_qc_readonly SETTINGS use_query_cache = 1) SETTINGS readonly = 1;
SELECT count() FROM system.query_cache WHERE is_subquery = 1 AND query LIKE '%t_qc_readonly%';
INSERT INTO t_qc_readonly VALUES (10);
SELECT sum(x) FROM (SELECT x FROM t_qc_readonly SETTINGS use_query_cache = 1) SETTINGS readonly = 1;
DROP TABLE IF EXISTS t_qc_readonly;
```
### Expected behavior
```
1
1
0
11
```
### Error message and/or stacktrace
```
1
1
1
1
```
Suggested fix
Store the clamped list on the node: `settings_changes = checked_changes;` (or clamp `set_query.changes` in place). If the raw clause must be kept for AST/hash stability, `shouldUseQueryCacheForSubquery` must read `use_query_cache` from the node's context instead of from `getSettingsChanges()`.
Additional context
**Open risks:**
- `TableFunctionNode::getSettingsChanges` (src/Analyzer/QueryTreeBuilder.cpp:861) carries a `SETTINGS` clause from a table function argument and is not clamped at all by this PR - a separate path that was not in scope here.
Found during automated review of [PR #115397](https://github.com/ClickHouse/ClickHouse/pull/115397). Severity P2 · Finding `h_pr115397_001`
cc @tempel-it @pufit (from #115397)
Contributor guide
Assessment
This issue has not been assessed yet.