Workflow [PR], commit [649a50f]

Summary: ❌

• Performance Comparison: Performance dashboard

AI Review

Summary

This PR adds strict unknown-top-level-key validation for the server config, plus a large compatibility/test matrix for existing dynamic sections such as config:// payloads, custom HTTP handler prefixes, include_from substitutions, and arbitrary-named GraphiteMergeTree rollup sections. The implementation is much closer now, but I still see one backward-compatibility regression in the top-level <include> handling and one fresh false-negative hole in the new Graphite empty-section carveout, so the review remains Request changes.

Findings

❌ Blockers

• [src/Core/ServerSettings.cpp:2703] Recursive top-level <include incl="X"/> imports are still not mirrored. ConfigProcessor::doIncludesRecursive recurses after expanding a top-level <include>, so a valid source like <root><include incl="nested"/></root> imports <nested>'s children into the server root. This validator only records the immediate child names of root, so it inserts include into referenced_top_level_keys and still rejects the actual imported top-level keys with UNKNOWN_ELEMENT_IN_CONFIG. That is still a startup/reload regression for previously valid configs using nested include expansion.
  Suggested fix: recurse through imported <include> nodes the same way doIncludesRecursive does, or reuse the already-processed imported top-level keys from ConfigProcessor, and add a focused regression case for the nested include shape.

⚠️ Majors

• [src/Core/ServerSettings.cpp:2873] The empty-section GraphiteMergeTree compatibility carveout is now too broad: any unknown childless empty top-level tag is treated as a valid rollup section. A config like <my_typo/> or <my_unknown></my_unknown> reaches children.empty() and returns true here, so the new checker silently ignores that unknown option instead of throwing UNKNOWN_ELEMENT_IN_CONFIG. This reintroduces the exact silent-ignore behavior the PR is meant to remove, just for the empty-tag shape.
  Suggested fix: do not treat the empty case as Graphite unless you have an independent signal that the section is actually referenced by GraphiteMergeTree; otherwise this compatibility exception is broader than the feature’s contract.

Final Verdict

❌ Request changes

The failures of "Flaky check" in "functions_bad_arguments" will be fixed by #101994.

The MSan stress test failure (MemorySanitizer: use-of-uninitialized-value, STID 4179-5154 or 4148-3044) is a known pre-existing issue unrelated to this PR. Fix: #102158

Pushed f8708ec114c addressing the AI-Review Request changes verdict on 49f75420b50 — the two remaining checkUnknownSettings false negatives, both of which exempted a top-level section the runtime consumer falls back past and never reads:

1. <protocols>...<handlers>NAME</handlers> exemption. The top-level component of the referenced handler prefix was exempted unconditionally. createHTTPHandlerFactory consults the prefix only when config.has(NAME); otherwise it falls back to the default http_handlers and never reads the named section. A misspelled prefix (<handlers>custom.handlers</handlers> while the config only has <custom><typo>...</typo></custom>, so custom.handlers does not exist) therefore whitelisted the typo'd top-level <custom>. The exemption (and the config:// scan) is now gated on config.has(handler_section), mirroring the consumer.

2. Static handler config:// exemption. config://payload.suffix exempted the top-level <payload> even when the exact path payload.suffix was absent. StaticRequestHandler::writeResponse reads it with getRawString("payload.suffix", "Ok.\n"), so a missing path serves the default Ok.\n and never consumes <payload>. The top-level component is now exempted only when the full referenced path exists.

New negative integration cases (fail to start with UNKNOWN_ELEMENT_IN_CONFIG before the fix, pass after):

• test_protocols_missing_handler_prefix_not_exempted
• test_config_ref_missing_path_does_not_exempt_unknown_key

ServerSettings.cpp compiled clean locally (RelWithDebInfo, zero diagnostics). Ran the focused integration subset (both new cases plus the positive protocols_custom_handlers / protocols_nested / config:// / bracket-index guards): 7 passed, 0 failed, confirming the tightened exemptions still accept every legitimate handler/config:// reference. Both AI-Review threads replied to and resolved.

Pushed 9dff5e9ffbb and 1b10585feab addressing the AI-Review Request changes verdict on f8708ec114c — both Majors, which were also the two failing test_unknown_config_option cases in CI.

1. Missing negative-case config files (9dff5e9ffbb). test_protocols_missing_handler_prefix_not_exempted and test_config_ref_missing_path_does_not_exempt_unknown_key referenced protocols_missing_handler_prefix.xml and static_handler_config_ref_missing_path.xml, which were never committed. ClickHouseInstance.create_dir copies main_configs with shutil.copy before assigning self.logs_dir, so the missing files raised FileNotFoundError at cluster start and the fixtures' except blocks then died with AttributeError: 'ClickHouseInstance' object has no attribute 'logs_dir' (the actual CI error) before reaching the assertions. Added both configs; custom_missing (absent handler prefix) and missing_payload (absent config:// path) are now correctly rejected with UNKNOWN_ELEMENT_IN_CONFIG.

2. Graphite-shape exemption tightened (1b10585feab). looks_like_graphite_rollup only validated the child names of each rollup rule, so a typo such as <some_typo><pattern><regexp>.*</regexp></pattern></some_typo> was exempted even though appendGraphitePattern would reject it. The helper (renamed graphite_rule_valid, taking an is_default flag) now also mirrors: at least one <function>/<retention*> per rule (NO_ELEMENTS_IN_CONFIG), a valid <rule_type> value (BAD_ARGUMENTS), and rule_type == all for the <default> rule (BAD_ARGUMENTS). It only ever rejects sections the parser also rejects, so no valid rollup config breaks. Added one negative test per check: graphite_rule_no_aggregation.xml, graphite_default_rule_type_not_all.xml, graphite_invalid_rule_type.xml.

The remaining red, Unit tests (asan_ubsan, function_prop_fuzzer) (FunctionsStress.stress/AllTests), is unrelated: the function-property fuzzer found a reinterpret/Decimal scale mismatch (Decimal64 returned for a requested Decimal(18, 18)), which a server-config-validation change cannot affect. It is tracked by the open issue #107242 and fails intermittently on master as well.

Did not re-merge master: the branch is MERGEABLE with no conflicts and the merge-base is from earlier today (< 1 day old).

Pushed 256d06be630 addressing the sole remaining unresolved review thread on graphite_rule_valid (ServerSettings.cpp).

graphite_rule_valid now mirrors appendGraphitePattern's retention handling: for each retention* block it requires both <age> and <precision> to be present and to parse as unsigned integers (via config.getUInt, exactly as the parser reads them), and only then counts it toward has_function_or_retention. A block missing or with a non-numeric <age>/<precision> now fails the shape check, so a typo like <some_typo><pattern><retention><precision>60</precision></retention></pattern></some_typo> is rejected with UNKNOWN_ELEMENT_IN_CONFIG instead of being exempted. Added the negative integration test test_graphite_retention_missing_age_rejected (config graphite_retention_missing_age.xml).

Triage of the CI reds on the previous commit 1b10585feab (all pre-existing, unrelated to this config-validation change; a fresh run will re-evaluate on 256d06be630)

• BuzzHouse (amd_tsan) — Logical error: ScatterExchangeStep should have one source shard, got A. Server-side distributed query-plan assertion in makeDistributedPlan/ReadFromMerge (make_distributed_plan=1 + Merge table). Tracked by Logical error: 'ScatterExchangeStep should have one source shard, got 8' with make_distributed_plan=1 + Merge table #107946. This PR touches only server-config validation.
• Upgrade check (amd_release) — Error message in clickhouse-server.log, flagging only mysqlxx::Pool: Failed to connect/PostgreSQLConnectionPool errors against the unreachable fake 192.0.2.1 (DatabaseMySQL/DatabasePostgreSQL background refresh, from stateful tests). Widespread pre-existing flake: the same check with the same signature failed on 141 distinct PRs in the last 30 days.
• Fast test (arm_darwin) — 03230_keeper_cp_mv_commands. macOS-only keeper-client flake: 0 failures on any Linux stateless variant in 30 days, but 5 distinct PRs failed it on Fast test (arm_darwin) (Compress hash join right-side blocks in memory before OOM / spilling #107667, Auto-escape XML special characters in from_env config substitutions #101773, Avoid vertical TTL delete for TTLDrop merges #108626, Fix word boundary and anchor assertions in regexp "match all" functions #108047, this one).

@groeneai, the last two have no tracking issue and are systemic. Could you investigate and provide fixes in separate PRs (or link existing work)?

• Upgrade-check mysqlxx/DatabaseMySQL error-message flake (141 PRs/30d): https://github.com/ClickHouse/ClickHouse/actions/runs/28480807775/job/84434374173
• 03230_keeper_cp_mv_commands on Fast test (arm_darwin): https://github.com/ClickHouse/ClickHouse/actions/runs/28480807775/job/84431358651

@alexey-milovidov Both already have open fixes; linking them:

1. Upgrade-check mysqlxx/DatabaseMySQL (and PostgreSQLConnectionPool) 192.0.2.1 log-noise flake

Fix: #108560 (open, you approved 2026-06-28; currently only UNSTABLE from CI in progress, no red review). It allowlists the benign background/startup connection failures both remote engines emit against the RFC5737 TEST-NET-1 fixture host 192.0.2.1 created by 04210_show_remote_databases_in_system_tables, while still surfacing any non-connection DatabaseMySQL/DatabasePostgreSQL error. It covers the MySQL variant (mysqlxx::Pool: Failed to connect to MySQL, DatabaseMySQL ... Connections to mysql failed) as well as the Postgres one, so this signature is handled.

2. 03230_keeper_cp_mv_commands on Fast test (arm_darwin)

Fix: #107438 (open, approved by alesapin 2026-06-19). Same keeper-client teardown flake as 03135_keeper_client_find_commands / 03161: on macOS, socket.shutdown() inside Coordination::ZooKeeper::finalize() throws ENOTCONN (errno 57) at connection teardown, and the Fast test harness treats any stderr output as a failure. 03230's stderr is exactly that path:

Net Exception: Socket is not connected
3. Poco::Net::SocketImpl::shutdown()
4. Coordination::ZooKeeper::finalize(...)
5. Coordination::ZooKeeper::~ZooKeeper()

#107438 catches POCO_ENOTCONN in both the constructor and finalize() and downgrades it to LOG_TRACE, silencing the benign teardown. CIDB confirms it is macOS-only: 5 hits / 5 PRs in 30d, all Fast test (arm_darwin), 0 Linux, 0 master.

Both are open and ready for merge; no new fix needed.

Pushed d6b625a8ae1 (fix) and merged master (ec66067e82f), addressing the AI-Review Request changes verdict on 256d06be630.

Fix — top-level <include> from external <include_from> sources (ServerSettings.cpp)

checkUnknownSettings skipped every external <include_from> source, treating it as a pure incl lookup table with no top-level contribution. But ConfigProcessor::doIncludesRecursive expands a top-level <include incl="X"/> by inserting the children of node X into the root, so those child tags become real top-level keys of the merged config — and were wrongly rejected with UNKNOWN_ELEMENT_IN_CONFIG, a regression for a previously valid substitution.

The fix collects top-level <include incl="X"/> reference names from the server config only (the main config and merged config.d/conf.d fragments, never the users config), resolves <X> in each <include_from> source exactly as the processor does (getRootNode(include_from)->getNodeByPath(name)), and exempts only the imported children — nothing else, so an unrelated typo stays rejected. Added the positive regression test test_top_level_include_from_external_source_accepted; the negative lookup-table case (test_external_include_from_source_does_not_exempt_unknown_key) is unchanged.

Re-merge

Merged master (was 664 commits behind, merge-base 2026-06-30): known_keys is auto-derived from ServerSettingsImpl().all(), so being far behind risks the validator false-positive-rejecting master's newer default configs across the full test suite. master had also modified ServerSettings.cpp; the merge was conflict-free and the file still compiles.

Triage of the CI reds on 256d06be630 (a fresh run re-evaluates on ec66067e82f)

• Stress test (arm_msan) — Hung check failed, possible deadlock found. Master-wide flake tracked in Hung check failed, possible deadlock found #107941 (32 distinct PRs today, 183 on 2026-06-30). The stack is a server-side AST-fuzzer query (executeASTFuzzerQueries → TCPHandler::run) with no config-validation frames; a config-key validator cannot cause a hung fuzzer query.
• CH Inc sync — internal sync; re-evaluates on the fresh head.

Pushed fbf709911c7 addressing the AI-Review Request changes verdict on ec66067e — both Majors, which are compatibility regressions where a previously valid <include>-based config would now be wrongly rejected with UNKNOWN_ELEMENT_IN_CONFIG. The fix is confined to the top-level-<include> handling in ServerSettings::checkUnknownSettings.

1. Top-level <include> with from_env / from_zk (ServerSettings.cpp:2465)

The scan only recorded the incl attribute, but doIncludesRecursive expands a top-level <include> for any of the three SUBSTITUTION_ATTRS (incl, from_env, from_zk), importing the referenced node's children as top-level keys. It now mirrors all three:

• from_env is resolved deterministically: the environment value is wrapped as <from_env>VALUE</from_env> and parsed exactly as the processor does, and its imported children are added to referenced_top_level_keys.
• from_zk cannot be resolved without a ZooKeeper connection at config-validation time (it runs before <zookeeper> is validated). So when a top-level <include from_zk=.../> is present the whole check is skipped (fail open, with a LOG_WARNING) rather than falsely rejecting the imported keys — a valid config is never refused; a genuine typo alongside such an include is not caught. This extends the existing documented from_zk limitation and is noted in the PR description.

2. Implicit /etc/metrika.xml default include source (ServerSettings.cpp:2504)

When the server config declares no <include_from>, ConfigProcessor::processConfig still uses /etc/metrika.xml if it exists. The validator now seeds the same default (only when it exists and there is a top-level <include incl=X/> to resolve against it), so <X>'s imported children are exempted.

Tests (integration, all reload/start-based)

• test_top_level_include_from_env_accepted — dedicated instance whose env_variables supply <my_env_imported_section>...; the node must start.
• test_top_level_include_from_zk_accepted — creates the ZooKeeper node, reloads a config with a top-level <include from_zk=.../>; reload must succeed.
• test_metrika_default_include_source_top_level_include_accepted — no <include_from>, injects /etc/metrika.xml and a top-level <include incl=.../>; reload must succeed.

ServerSettings.cpp was verified to compile clean (-fsyntax-only, -Werror, this branch's headers, 0 diagnostics); test.py and the new configs parse clean. Both AI-Review threads replied to and resolved. No master re-merge: the branch is MERGEABLE with no conflicts and the merge-base is from earlier today.

Pushed 76bacb45a5e addressing the AI-Review Request changes verdict on 85a141b0 (the two GraphiteMergeTree findings).

Fixed — empty arbitrary-named Graphite section (ServerSettings.cpp:2794, compatibility regression)

looks_like_graphite_rollup returned false for a present-but-empty section, so a config like <retention_cfg/> was rejected with UNKNOWN_ELEMENT_IN_CONFIG and the server did not start. But setGraphitePatternsFromConfig only throws NO_ELEMENTS_IN_CONFIG when the section is missing; an existing empty section is accepted (default column names + empty pattern list). The helper now returns true for the empty case, mirroring the parser. Added the regression test test_graphite_empty_section_accepted.

Verified -fsyntax-only -Werror clean on ServerSettings.cpp (0 diagnostics); test.py py_compiles and the new config XML is well-formed. A full integration run was not feasible this session (the branch is 85 commits from master, no incremental build cache), so CI's integration job is authoritative for the new test. The fix is a loosening (accept → exempt), so it cannot convert a valid config into a rejected one — it cannot cause the server to fail to start.

Deferred as a design decision — function-name validation (ServerSettings.cpp:2737, false-negative)

Fully mirroring appendGraphitePattern's function handling is not possible in checkUnknownSettings: it is a static method with no ContextPtr, whereas the parser resolves the value with getAggregateFunctionNameAndParametersArray(..., context) and constructs the function via AggregateFunctionFactory::instance().get(name, action, {Float64}, params_row, properties) before rejecting one that allocatesMemoryInArena. A partial name-only check would still miss the arena case and risks a new false positive (e.g. rejecting a valid <function>quantile(0.5)</function>), turning a recognition miss into a server-won't-start regression — which I can't verify safely here without a build. This is best-effort recognition (the <skip_check_for_incorrect_settings> escape hatch remains, and a bogus function is still caught by appendGraphitePattern when a table references the section), so I left that thread open for a call: accept the limitation as-is, or thread a ContextPtr in so the recognition can delegate to the real parser.

No master re-merge: MERGEABLE, no conflicts, merge-base is from earlier today (2026-07-01 10:16Z).

Status on 76bacb45a5e:

• AI-Review "Request changes" — both findings are the GraphiteMergeTree semantic false-negatives. The <function>-value one is already dismissed as best-effort by design (above). The new <regexp>-compile one (ServerSettings.cpp:2749) is the same class: it can't be delegated to appendGraphitePattern without a ContextPtr (which the static check lacks and which the parser needs to resolve <function> before it ever compiles the regexp), and a standalone copy of buildTaggedRegex + OptimizedRegularExpression risks false-positive-rejecting a valid tag_list config and stopping the server from starting. Full reasoning and the two options in the inline thread; left open as a design call — no unverifiable tightening pushed.
• Public CI — fresh run on this head, 0 failed (coverage / a couple of flaky shards still in progress).
• CH Inc sync — the private mirror countSubstrings may hang #53245 is MERGEABLE (no conflict) but still at the stale head e2da1d1045a; its reds are the usual cloud-only flakes (db disk / distributed cache / meta in keeper / SharedCatalog, scattered single shards, not a uniform config rejection). The scheduled IncrementalSync is re-running and re-evaluates on the current head.
• No master re-merge — MERGEABLE, no conflicts, merge-base 6ff851961b5 from 10:16Z today; the only red is CH Inc sync, which a re-merge would not change.

Pushed 6b91d23e469 fixing a real, uniform integration-test failure on 76bacb45a5e. After that head's shards finished, all 55 failures were test_unknown_config_option (every negative case, across every shard: asan/arm/tsan/msan/coverage) — CI report. The server was starting successfully with a clearly unknown top-level key (caught_exception empty, reloads succeeding), i.e. the check's core was silently disabled.

Root cause

test_unknown_config_option_rejected last passed on 85a141b01de and failed starting exactly at 76bacb45a5e. The only functional change in that commit was, in looks_like_graphite_rollup:

    if (children.empty())
-       return false;
+       return true;

The intent was to accept a present-but-empty GraphiteMergeTree section (<retention_cfg/>). But Poco::AbstractConfiguration::keys reports only child elements, so a scalar leaf key such as <some_completely_unknown_option>1</...> (a typo'd setting — precisely what this check exists to catch) also has zero children and was therefore exempted from UNKNOWN_ELEMENT_IN_CONFIG. Every unknown top-level key carrying a scalar value silently passed, defeating the feature's primary purpose.

Fix

A GraphiteMergeTree section is always a container and never carries a scalar value of its own, so the two cases are distinguishable: exempt a childless section only when its (whitespace-trimmed) value is empty — a genuine empty container — and let a childless section with a non-empty value fall through to be rejected.

    if (children.empty())
        return Poco::trim(config.getString(section, "")).empty();

Verified against every failing negative case (all inject valued leaf keys: <...>1</...> / text → now rejected) and against test_graphite_empty_section_accepted (<retention_empty/>, valueless → still accepted).

This is orthogonal to the two open graphite_rule_valid threads (the <function>/<regexp> semantic-validation design calls at lines 2749/2750) — those are untouched.

Status on 6b91d23e469:

• Public CI — fresh run on this head, 0 failed (16 jobs, incl. coverage / integration, still in progress).

• CH Inc sync (the sole red) — the private mirror countSubstrings may hang #53245 is MERGEABLE (no conflict) but still at the stale head e2da1d1045a, not yet re-mirrored to this head. Its 7 reds are the usual Cloud-only flakes on scattered single shards (db disk 6/7 ×2, distributed cache / meta in keeper / s3 storage 1/2, amd_tsan 5/6, amd_msan 5/6, SharedCatalog + meta in keeper stress ×2 — everything else green), not a uniform config rejection: if this PR's check were wrongly rejecting a Cloud config key, the server would fail to start on every shard (as in the earlier enable_http_stacktrace / format_alter_operations_with_parentheses cases), which is not what we see here. The scheduled IncrementalSync re-evaluates on the current head.

• AI-Review "Request changes" (16:07Z on this head) — the two non-dismissed Majors are the same design-call class as the already-dismissed <function> / <regexp> graphite findings, left open for a decision rather than tightened blind:

  • graphite_rule_valid retention ordering (ServerSettings.cpp:2760). Mirroring appendGraphitePattern's compareRetentions — a throw-if-not-monotonic comparator invoked inside ::sort (Graphite.cpp:173), sensitive to ties / equal ages, the single-retention case, and the TypeRetention-only guard — in a standalone static check is subtle. More importantly it is the same compatibility trade-off as the other graphite findings: the rollup section is not parsed at server start (only at GraphiteMergeTree table creation), so an out-of-order-retention section currently boots; rejecting it at startup with UNKNOWN_ELEMENT_IN_CONFIG is a behavior change whose downside is a false positive that stops the server from starting.
  • top-level <include incl="X"/> vs users-config include_from (ServerSettings.cpp:2639). Narrowing which include_from sources a server <include> resolves against re-implements ConfigProcessor's server-vs-users scoping; getting it wrong wrongly rejects a legitimately imported top-level key — again a false positive that stops the server from starting.

  Both would need a full build plus the two suggested regression tests to verify safely, which is why they are not tightened in this pass. Leaving them as a design call.

• No master re-merge — MERGEABLE, merge-base 6ff851961b5 from 10:16Z today (behind only by velocity); the only red is CH Inc sync, which a re-merge would not change.

LLVM Coverage Report

Changed lines: Changed C/C++ lines covered: 493/515 (95.73%) · Uncovered code

Full report · Diff report