Throw on a distributed query with unknown client version - #109408
Conversation
A distributed query forwarded to a remote shard must carry a known initiator version: the receiving server uses it for version-gated compatibility decisions (e.g. whether to enable the analyzer, see `TCPHandler`). A zero version means the initiating query context was not populated as an initial query - a real client always reports its version, and a server that (re-)initiates a query fills it with its own version. Sending a zero version silently triggers wrong compatibility downgrades on the remote and can break distributed execution (see the issue below). Fail with a logical error instead of sending a zero version, so such context initialization bugs are caught loudly. Related: #109407 Related: #90651 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Workflow [PR], commit [f659dd5] Summary: ✅
AI ReviewSummaryThis PR hardens distributed query client-version propagation: it rejects zero-version distributed queries in Final Verdict✅ Approve LLVM Coverage Report
Changed lines: Changed C/C++ lines covered: 168/170 (98.82%) · Uncovered code |
clickhouse-gh
Bot
added
the
pr-improvement
The new assertion in `RemoteQueryExecutor` surfaced query-context initialization paths that forward a zero client version to remote shards: a query initiated over an interface that does not report a client version (a raw HTTP request via `curl`, or a MySQL/PostgreSQL client) leaves `client_version_*` at 0.0.0. `Session::makeQueryContext` promoted such a context to an initial query with `setQueryKind`, which - unlike `setInitialQuery` - does not fill the version. This server is the real initiator of the query and of any distributed sub-query it spawns, so fill the version with this server's own version when it is zero. Otherwise remote shards treat the initiator as a pre-23.3 server and disable the analyzer for "compatibility" (see `TCPHandler`), diverging from the initiator - the same class of failure as #90651 - and now the assertion rejects the zero version outright. This fixes the Fast test failures `03021_get_client_http_header`, `01455_opentelemetry_distributed`, `02841_parallel_replicas_summary`, `02531_two_level_aggregation_bug` and `01085_max_distributed_connections_http`, which all issue distributed queries over HTTP. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
![clickhouse-gh[bot]](https://avatars.githubusercontent.com/in/928874?s=60&v=4){kind=small}
The zero-version guard added to `RemoteQueryExecutor` also surfaced two internal query-context constructors that synthesize an `INITIAL_QUERY` context via `Context::createCopy` and never fill `client_version_*`, so they forwarded a zero version to remote shards: - `AsynchronousInsertQueue::processBatchDeadlines` - the async insert flush context. Reached when the flush spawns a distributed read, e.g. a materialized view triggered by the insert reads from a `Distributed` table. - `StorageMaterializedView::createRefreshContext` - the refreshable materialized view refresh context. Reached when the refresh `SELECT` reads from a `Distributed` table. Both are `setQueryKind(INITIAL_QUERY)` (not `setInitialQuery`), which does not fill the version. This server is the real initiator of these queries and of any distributed sub-query they spawn, so fill the version with this server's own version when it is zero - the same fix already applied to `Session::makeQueryContext`. Otherwise remote shards treat the initiator as a pre-23.3 server and apply legacy compatibility downgrades (see `TCPHandler`), and `RemoteQueryExecutor` now rejects the zero version outright (a logical error that aborts a debug server from the background refresh/flush threads). Adds a regression test covering both paths (verified locally: the guard fires twice without this change and not at all with it). Related: #109407 Related: #90651 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Notes on the latest AI Review "Request changes" verdict (commit Blocker 1 — Blocker 2 — remaining server-initiated contexts. This is the same class as the
Each of these can end up running a distributed
Happy to implement whichever you prefer — the per-site fill for |
|
Full-CI update. The fast checks were green at my last note; the deep test suite has since run on the same commit ( 1. 2. External dictionary loading with a remote (2) is the more significant finding: it is a common production feature and it sits outside the |
…query-client-version
…query-client-version
…query-client-version # Conflicts: # src/QueryPipeline/RemoteQueryExecutor.cpp
Instead of patching every server-initiated query context individually (`StorageBuffer`, `StorageKafka`, `StorageNATS`, `StorageRabbitMQ`, `StorageFileLog`, `StorageObjectStorageQueue`, `MaterializedPostgreSQLConsumer`, `ClickHouseDictionarySource`, the asynchronous insert flush, ...), fill a zero client version with this server's own version in `Context::makeQueryContext`, which all of these sites go through via `Context::createCopy(...); makeQueryContext();`. This server is the real initiator of such queries, so its own version is the correct one to forward, and the `RemoteQueryExecutor` guard no longer fires on these paths (this is what made the dictionary integration tests red). Contexts created for real client queries overwrite the client info after `makeQueryContext` (see `Session::makeQueryContextImpl`), so a client-reported version is never masked; the fix in `Session::makeQueryContextImpl` stays because of that overwrite ordering, and the one in `StorageMaterializedView::createRefreshContext` stays because that context never calls `makeQueryContext`. The now-redundant per-site fix in `AsynchronousInsertQueue` is removed. Also add a test for the `Buffer` flush path, which previously left the version at zero. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The existing regressions only proved that the initiator no longer trips the new zero-version guard locally. Check the invariant where it matters: the `client_version_*` that the receiving server records for the forwarded `SECONDARY_QUERY` in `system.query_log`, which is the value it gates compatibility decisions on (disabling the analyzer for a pre-23.3 initiator in `TCPHandler::receiveQuery`). A synthesized server-side query context keeps the default `ClientInfo::Interface::TCP`, and `ClientInfo::write` serializes `client_version_*` exactly for that interface, so the zero version used to be transmitted verbatim and overwrote the non-zero value the receiver had derived from the hello handshake. Verified both directions against a local server: the `Buffer` flush forwards `0.0.0` without the fix, and a forwarded query from a context with a non-zero version arrives with that version.
|
CI update on The remaining reds are all unrelated, fleet-wide failures. Checked against the CI database (
@groeneai, these three have no tracking issue and fail across many unrelated pull requests — please investigate and provide fixes in separate pull requests, or link them here if they are already in progress: |
|
Checked all three against 1 + 2.
|
|
My earlier comment named query 3, Attribution over the 48 hits on
All 34 fat-chunk hits are the final query. Query 3 materializes over a single row, so it allocates in per-column pieces and never asks for one 1.5 GiB chunk. Your insert-time block split is on the query that actually held the allocation. The 14 remaining query-3 hits are collateral, not a test defect. Average headroom when query 3 was selected was Since Still open from your list: One nearby data point, not something I am folding in here: |
|
Correcting my own last paragraph: I said |
|
Fix for Root cause is what I described above: the negative |
…by `current_database` The `system.query_log` check added in the previous commit matched the shard-side row with `query LIKE '%`db`.`agg_src`%'`, which the style check rejects: queries to `system.query_log` must be scoped to the test's own database. The usual `current_database = currentDatabase()` condition does not work here - the sub-query runs on the shard through the cluster connection's own default database (measured: `current_database` is `default`), which is why `Distributed` rewrites the query with fully qualified names. Use `has(databases, currentDatabase())` plus `has(tables, ...)` instead, which the style check also accepts. Verified locally against a server on port 19807: the row is still matched (`count() > 0` is 1) and the assertion still discriminates - it yields a zero version on a binary without the fill.
…uery_plan` `04643_distributed_query_client_version_buffer_flush` failed deterministically in `Stateless tests (amd_asan_ubsan, distributed plan, parallel)`: with `serialize_query_plan = 1` the shard executes a deserialized query plan and never analyses the query, so `Context::addQueryAccessInfo` is never called and the `databases`/`tables` columns of its `system.query_log` row stay empty. The predicate then matched nothing and the test printed `remote_version 0 0`. Measured on a local two-port server (`test_shard_localhost` pointing at itself): with `serialize_query_plan = 1` the shard row has `databases = []`, `tables = []`, `current_database = 'default'`, while `query` is ``SELECT sum(`__table1`.`x`) ... FROM `test205`.`agg_src` AS `__table1``` - so identify the forwarded sub-query by its query text, which is qualified with the test's database in both modes, and keep `has(databases, currentDatabase())` as the equivalent condition for the analysing case. Verified: the test passes against a build of this branch with `serialize_query_plan = 1`, and the predicate still yields `remote_version 1 0` on an unfixed binary, so it keeps discriminating.
Address review: a queued (asynchronous) distributed INSERT does not go through `RemoteQueryExecutor` at all - `DistributedAsyncInsertHeader::read` deserializes the initiator's `ClientInfo` from the on-disk batch header and `DistributedAsyncInsertBatch`/`DistributedAsyncInsertDirectoryQueue` hand it to `RemoteInserter` as is. A batch file written by an older server from a server-initiated query context (a `Buffer` flush, a streaming consumer, an asynchronous insert flush) carries a zero client version, and such files are still replayed after an upgrade, so the receiving shard keeps treating the initiator as an ancient server. Fill the version with this server's own version when reading such a header - the same normalization `Context::makeQueryContext` now does for live query contexts. Normalizing rather than throwing, because a stale batch file on disk is not a programming error and rejecting it would wedge the queue. Confirmed the path is real: on an unfixed 26.7 binary, a `Buffer` flush into a `Distributed` table over `test_cluster_two_shards` (its second shard, `127.0.0.2`, is not a local address, so the insert goes over the network) is replayed on the shard as `is_initial_query = 0`, `interface = TCP`, `client_version_major = 0`. The new test `04654_distributed_async_insert_client_version` covers that path end-to-end: it yields `remote_version 1 0` on an unfixed binary and `remote_version 1 1` on this branch. A stale zero-version header itself cannot be produced by a test on a fixed binary - the live path no longer writes one, and a non-`TCP` interface never serializes the version into the header in the first place - so that leg would need an integration test that upgrades a node with a non-empty distributed queue.
…layouts too The normalization added in `ef79cb137ec` only ran in the branch that deserializes an embedded `ClientInfo`. The two legacy layouts - `DBMS_DISTRIBUTED_SIGNATURE_HEADER_OLD_FORMAT` and the even older plain-query-size header - carry no client info at all and returned early with a default-constructed `ClientInfo`, whose interface is `TCP` and whose version is zero. `RemoteInserter` forwards it verbatim after only flipping `query_kind`, and `TCP` is exactly the interface whose version `ClientInfo::write` serializes, so a node upgraded with such batch files still on disk replayed them as `0.0.0` - the very downgrade this change is about. Move the fill into `DistributedAsyncInsertHeader::read`, which now wraps the parsing in a file-local `readHeader`, so every return path is normalized and a layout added later cannot silently miss it. Behavior for the current layout is unchanged.
|
Addressed the review request about the legacy queued distributed The suggested integration test — seed a legacy batch file, replay it with Verified, not just reasoned about: with the normalization disabled, The remaining wire leg — that Also merged |
alexey-milovidov
left a comment
alexey-milovidov
left a comment
There was a problem hiding this comment.
This is very good.
…on receive During a rolling upgrade an older peer can forward a server-initiated query whose context was never filled with a version, so `ClientInfo::read` in `TCPHandler::processQuery` overwrites the session seed with 0.0.0. The new `ClientInfo::setClientVersionFromConnectionIfUnknown` restores the immediate peer's version from the connection hello (`connection_client_version_*`, which `read` never touches), so version-gated compatibility decisions do not wrongly downgrade and a second distributed hop does not trip the zero-version check in `RemoteQueryExecutor`. A locally synthesized context has no connection version, so a genuine programming error still fails loudly on the sending side. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
LLVM Coverage Report
Changed lines: Changed C/C++ lines covered: 167/169 (98.82%) · Uncovered code |
The queued sender is covered by `04654_distributed_async_insert_client_version`, but the synchronous sender used when `distributed_foreground_insert = 1` takes a different route: `DistributedSink` hands `context->getClientInfo()` straight to `RemoteInserter` without the on-disk batch header round trip. The new test drives that route from a server-created context: with `async_insert = 1` the push runs from the flush context synthesized by `AsynchronousInsertQueue`, so the version the shard records comes from a context this pull request fills. It also covers the `remote` table function, whose storage has no data path and therefore always inserts synchronously. Verified on a local server: the receiving shard reports `client_version_major = 0` without the fill and a non-zero version with it.
|
🕵 @groeneai, investigate the failure: https://s3.amazonaws.com/clickhouse-test-reports/json.html?PR=109408&sha=a3dcc46dcdef9b66033d407101bae11fdf2eb05b&name_0=PR&name_1=Fast%20test%20%28arm_darwin%29 and provide a fix in a separate PR. If the fix is already in progress, link it here.
|
|
Investigated. Your reading is right on both counts, and I can add the cause. There is no fix pull request yet; I am preparing one and will link it here. It is the runner's 60 s per-test cap, not a resource error. The 7 arm_darwin failures cluster within 0.8 s of 60.0 s (60040, 60050, 60070, 60070, 60210, 60220, 60810 ms). The cap is The test did not get slower, the runner's IO tail did, and it has already receded. Per day on It was not specific to this test. Over the same window every IO-heavy arm_darwin fast test inflated: Why this test is the one that breaks. The cost is the fixture, not the parse. Writing the 1 GB file takes ~1 s but flushing it costs ~7.3 s on a fast local NVMe array, while the parse is ~3.3 s. Under contended disk it is the write and flush that stretch, and the file carries no The fix I am sending: shrink the fixture, keep the assertion. The 1 GB is not load-bearing for what the test checks. Peak footprint is independent of file size: bisecting the cap, 1 GiB, 128 MiB and 64 MiB all pass at
|
Build profile diff (arm_release)No arm_release build profile data for commit f659dd5 - the build was skipped, reused from cache, or predates profile upload. |
|
Fix pull request: #113080 It does two things. The fixture drops from 1 GB to 128 MiB and is now removed, which is 87.5 % fewer While shrinking it I found the assertion had been dead since |
…ssion tests The shard-side assertions in `04643_distributed_query_client_version_buffer_flush`, `04654_distributed_async_insert_client_version` and `04692_distributed_foreground_insert_client_version` only checked `min(client_version_major) > 0`, so a regression that forwards a wrong non-zero version (e.g. `23.1.0`, still taking the pre-`23.3.0` compatibility branch in `TCPHandler`) would pass. Server-created contexts are filled with this server's own version and the shard is the same server in these tests, so compare the full `client_version_major`/`client_version_minor`/`client_version_patch` tuple against the components of `version` exactly. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
robot-clickhouse-ci-2
added
the
pr-synced-to-cloud
The reproducer was found by the targeted AST fuzzer and reported on issue ClickHouse#110176. Report: https://s3.amazonaws.com/clickhouse-test-reports/json.html?PR=109408&sha=848d6f40422826ac03b60db0219ec027ebed9b48&name_0=PR&name_1=AST%20fuzzer%20%28amd_debug%2C%20targeted%29 Carrier pull request: ClickHouse#109408
…s discarded
A materialized CTE read behind an IN-subquery is activated through a
DelayedPortsProcessor gate that opens once the CTE has been materialized. Discarding a
totals or extremes port attached a node with no outputs, and
ExecutingGraph::initializeExecution seeds every such node before the pipeline runs.
NullSink closes its input immediately, so when that input was a gate output the gate's
paired input was closed and the reader ran before its data was there:
Reading from materialized CTE '...' before its materialization completed -
DelayedPortsProcessor gate is missing in the query plan. (LOGICAL_ERROR)
The discard now goes through DroppingTransform, which keeps a data output connected and
is therefore never seeded. This covers the drop entry points in Pipe, the single-stream
fallbacks, and completion with a sink or a chain, which is the path EXPLAIN ANALYZE
takes. Uniting extremes across the arms of a set operation cannot discard its input,
because the united extremes value has to be preserved, so it uses an accumulating
ExtremesOnlyTransform that needs no sink at all. Each auxiliary input takes its header
from the port it is connected to, because a totals stream may be finalized while the data
stream is not, which is what the NullSink this replaced did.
The regression tests assert row counts and extrema, which are identical whether the CTE
is materialized or inlined, so each carrier also asserts that its own shape is planned
with materialization. Every such assertion was checked to read 1 today and 0 with
enable_materialized_cte = 0. The INSERT carrier is a shell test, because its pipeline is
built by InterpreterInsertQuery and only EXPLAIN PIPELINE INSERT reflects it.
Report: https://s3.amazonaws.com/clickhouse-test-reports/json.html?PR=109408&sha=848d6f40422826ac03b60db0219ec027ebed9b48&name_0=PR&name_1=AST%20fuzzer%20%28amd_debug%2C%20targeted%29
Carrier pull request: ClickHouse#109408
Closes ClickHouse#110176
3 participants
Follow-up hardening for #90651 / #109407.
A distributed query forwarded to a remote shard must carry a known initiator version: the receiving server uses it for version-gated compatibility decisions (for example, whether to enable the analyzer, see
TCPHandler). A real client always reports its version, and a server that (re-)initiates a query fills it with its own version, so a zero version is always a bug — it means the initiating query context was not populated as an initial query. Sending it silently triggers wrong compatibility downgrades on the remote and can break distributed execution (as in #90651, where theON CLUSTERDDL context left the version at0.0.0, so remote shards disabled the analyzer while the initiator kept it enabled).This change makes
RemoteQueryExecutorthrow aLOGICAL_ERRORinstead of forwarding a zero client version, so such query-context initialization bugs are caught loudly instead of causing subtle, version-dependent failures on remote shards.To make the guard hold as an invariant, server-initiated query contexts are fixed centrally:
Context::makeQueryContextfills a zero client version with this server's own version, covering every site that follows theContext::createCopy(...); makeQueryContext();pattern (background flushes ofBuffertables, streaming consumers such asKafka/NATS/RabbitMQ/FileLog/ObjectStorageQueue,MaterializedPostgreSQLreplication, dictionary reloads, asynchronous insert flushes, ...). This never masks a client-reported version: contexts for real client queries overwrite the client info aftermakeQueryContext(seeSession::makeQueryContextImpl, which keeps its own fill for the interfaces that do not report a version, e.g. raw HTTP), and a refreshable materialized view's refresh context, which never callsmakeQueryContext, is filled at its creation site. The primary path that produced a zero version (theON CLUSTER/ distributed DDL context) was fixed the same way in #109407.Related: #109407
Related: #90651
Changelog category (leave one):
Changelog entry (a user-readable short description of the changes that goes into CHANGELOG.md):
Added a sanity check that a distributed query always carries a known client version, throwing a logical error instead of silently forwarding a zero version to remote shards. Server-initiated queries (background flushes, streaming consumers, dictionary reloads, asynchronous insert flushes) now report the server's own version as the initiator version.
Documentation entry for user-facing changes