The previous approach was unsafe:
- Swallowing `EPIPE` in `WriteBufferFromFileDescriptor::nextImpl` affected every
pipe, also on the server: data written to a child process that exited early was
silently dropped, and `UserDefinedExecutableFunctionDriverInvoker` and
`clickhouse-disks sed`, which catch the `EPIPE` `ErrnoException`, would get a
`LOGICAL_ERROR` exception from `finalize` of the silently canceled buffer.
- Ignoring `SIGPIPE` in the client was redundant: Poco already blocks it in all
threads, which is why the write fails with `EPIPE` at all. And an ignored signal
is inherited through `exec`, while `ShellCommand` resets only the signal mask, so
the pager and executable UDFs started by `clickhouse-local` would inherit it.
Instead, the console output buffer of the client remembers that its write failed
with `EPIPE`. Then the client does not print the error, does not run further
queries (even with `--ignore-error`), and exits with code 0.
Closes: https://github.com/ClickHouse/ClickHouse/issues/126443
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
For an account with a time-based one-time password, the server returned
`REQUIRED_SECOND_FACTOR` only when the password was right and the code was
missing, and `AUTHENTICATION_FAILED` otherwise. The two responses let an
attacker brute-force the password without ever knowing a code.
Now the missing code is reported before the password is looked at, so a
right and a wrong password without a code get the same error, and the
password is verified only together with the code. When a code is present
both factors are evaluated unconditionally, so the timing does not tell
which one failed. `REQUIRED_SECOND_FACTOR` maps to HTTP 403 like the other
authentication errors instead of 500.
`clickhouse-client` now gets `REQUIRED_SECOND_FACTOR` already for the first
attempt without any password, so on that error it asks for the password
alone first (it may contain the code as `password+code`) and asks for the
code only if the server asks again. Users with a password see the same
prompts as before; users with `no_password` plus TOTP get a password prompt
(press Enter) before the code prompt.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
A program that formats an AST hiding secrets without installing a finder would show them. Programs that don't register the engines now install `NoSecretArgumentsFinder` explicitly.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
`createJwtProvider` chose `CloudJWTProvider` by hostname alone. For a Cloud
host with a custom IdP, it filled in the Cloud client ID when
`--oauth-client-id` was missing (sending it to the custom IdP, which fails
with `invalid_client`), replaced `--oauth-audience` with `token-exchange`,
and swapped the IdP token through the Cloud API, which only accepts tokens
from the Cloud IdP.
Now a Cloud host uses the Cloud login flow only when no `--oauth-*` option
is passed (`JWTProviderOptions::empty`). Otherwise the generic device flow
is used, as for any other host, and the IdP token is sent as the JWT (for
example, a Cloud service with JWT authentication through a custom IdP).
The check that the options are enough for the generic flow moves from
`Client::login` into `createJwtProvider`, so it also applies to Cloud hosts
and fails before contacting the IdP.
Addresses review comment:
https://github.com/ClickHouse/ClickHouse/pull/110718#discussion_r4106903410
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Resolve conflicts:
- `docs/en/interfaces/client.md`, `docs/en/operations/external-authenticators/jwt.md`: deleted on master (legacy Docusaurus docs removed); the same changes are already present in the `.mdx` counterparts.
- `docs/concepts/features/interfaces/client.mdx`: keep the PR's expanded OAuth option rows, with master's `--login` flag spelling.
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Rewrite all assignments inside `if`/`while` conditions and inside
operands of `&&`/`||`. The type-dispatch chains of the form
`(res = execute<T1>(...)) || (res = execute<T2>(...)) || ...` in
`Functions/` now use `TypeListUtils::forEach` over a `TypeList`; the
other sites declare the variable in the condition, use an
init-statement, or assign before the check. Evaluation order and
short-circuiting are preserved, so there are no behavior changes.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
`ignore_error` does not say who asked for it. Besides the user's `--ignore-error`,
`Client::processOptions` turns it on for `--query-fuzzer-runs`, `--create-query-fuzzer-runs`
and BuzzHouse, so that they tolerate unparseable input, and the success exit code of an
ignored batch was handed to those modes as well.
The AST fuzzer reports an early stop through that exit code and through nothing else. When
`tryToReconnect` fails in `processWithASTFuzzer`, the step returns `false` with `have_error`
and the exception still set - it skips the reset that every completed step does - and
`runNonInteractive" just returns, so `Client::main` is the only place left to report it.
Granting it success ends a fuzzer run that lost the server after a handful of queries with
`Fuzzer exited with success` in `ast_fuzzer_job`, which reads `fuzzer_exit_code` 0 as OK.
Exclude the fuzzing modes the same way BuzzHouse already was, so only a user-supplied
`--ignore-error` gets the success exit.
Co-authored-by: yakov <yakov@clickhouse.com>
`Client::processConfig` read `ignore-error` only in its non-interactive branch, and a
delayed-interactive run on a terminal (`--interactive` together with `--query` or
`--queries-file`) takes the other one. The option was dropped there, even though the given
queries are executed as a batch by `runNonInteractive` before the prompt is entered: the
prelude stopped at its first failing statement and `Client::main` returned the code of that
statement, ending the run before `runInteractive`.
`clickhouse-local` and the embedded client already load the option for
`delayed_interactive`; take the same condition.
The reported exception still ends such a run when `--ignore-error` is not given, which is
unchanged and outside of the option's contract.
Co-authored-by: yakov <yakov@clickhouse.com>
`Client::main` returned the code of the exception left behind by the last statement of the
batch. The per-statement state is reset in `processParsedSingleQuery`, so that was the code
of whichever error the final statement happened to hit and nothing else: the same batch
exited with a failure or a success depending only on the position of the failing statement
in it. `clickhouse-local --ignore-error` returned success in every one of those cases.
Align the client with it: `--ignore-error` has already reported every failed statement and
elected to carry on, so the run is not a failure. BuzzHouse keeps the error code, because
it stops the run on an error anyway - the same exception it already makes in
`ClientBase::executeMultiQuery`.
Co-authored-by: yakov <yakov@clickhouse.com>
A repo-wide scan for adjacent C++ and Python string literals that
concatenate without a separator, e.g.
"Virtual row does not cover sort column '{}'."
"Virtual row columns: {}, sort description [{}]",
which renders as `... sort column 'EventDate'.Virtual row columns: ...`.
The scan reports every pair of implicitly concatenated literals whose join
has no whitespace on either side, after decoding escapes; each of the 438
C++ and 531 Python candidates was reviewed by hand. Only the pairs where a
human reads the result are changed here: exception and log messages, the
`--host` help text of `clickhouse-client`, `system.*` column descriptions,
asynchronous-metric and trace-log descriptions, and two `argparse` help
strings. Two joins in the `--host` help text get `. ` rather than a bare
space, which is the separator the sentences were written for. The
multi-statement `wkb` documentation example gets `\n` between the
statements, matching `pointInPolygon`.
Left alone as intentional: quotes split across literals (`"... part '{}"`
`"' has inconsistent granularity"`), continuations that start with
punctuation, spliced identifiers (`"input_format_{}"` `"_skip_columns..."`),
`fmt` placeholders holding an optional fragment that carries its own leading
space (`MemoryTracker`), slash and bracket lists, and machine-consumed
strings where whitespace is not significant - SQL and regex fragments,
markup, connection strings, character sets and test fixtures.
The one exception is the `pg_attribute` query in
`fetchPostgreSQLTableStructure`, which produced `IN ('a','b')AND NOT
attisdropped`; the space moves from `") {}"` to `" AND NOT"` so that both
the empty and the non-empty `columns_part` render with exactly one space.
Six generated pages under `docs/reference/system-tables/` mirror the
`system.*` column descriptions changed here. They are inside
`AUTOGENERATED` regions, so they are deliberately not touched: the
description in the source registration is the source of truth and the docs
autogeneration workflow regenerates them.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Addresses the two remaining AI-review Majors.
The client sends a few hard-coded ClickHouse-SQL queries of its own: the
autocomplete query, the `system.documentation` lookup behind `help`, the AI
metadata query (all via `networkCompressionSettings`), and the `system.warnings`
probe in `Client::loadWarningMessages`. Both paths only covered the case where
the *session* had switched `dialect`: `networkCompressionSettings` dropped it
from the serialized settings, and `loadWarningMessages` overrode it only when the
local value differed from `clickhouse`.
That is not sufficient. `Connection::sendQuery` serializes only changed settings,
and the server takes the parser from the effective `dialect` of the authenticated
user - which a settings profile may itself default to Kusto or PRQL. Such a user
had these queries re-parsed under the foreign parser even though the session
never changed `dialect`; for `help` that is a visible "The help command failed",
and for the warnings probe it is silent, because `showWarnings` swallows the
exception and the server warnings simply stop being displayed.
Both places now set `dialect = 'clickhouse'` unconditionally, so the override
stays `changed` and is serialized - the same thing the interserver senders
(`MultiplexedConnections`, `HedgedConnections`, `RemoteInserter`) do for
secondary queries. Sending a value the user already has is a no-op for setting
constraints, so a profile that pins `dialect` read-only to `clickhouse` is not
tripped.
Both `04658_client_helper_queries_ignore_session_dialect` and
`05045_client_warnings_probe_with_session_dialect` gained a second half that logs
in as a user whose settings profile defaults to `dialect = 'kusto'` (plus a
positive control in `04658` proving the profile really is in effect). Verified
locally against a private server: both pass with the fix, and both fail in the
new half with the fix reverted and rebuilt, while their original halves and
`01945_show_debug_warning` keep passing.
Also corrected the `StreamingExchangeSink` comment: the exchange stream not
honoring `network_compression_method` is not a decision made there - the sink has
no query settings at hand, so the setting is simply not plumbed into it.
`01945_show_debug_warning` started failing on the previous head: the client no
longer displayed the `Obsolete setting` warning, and the `.expect` test waited
for it until the whole test was killed.
The cause is the previous commit `612e092bcd5`, which routed
`Client::loadWarningMessages` through `networkCompressionSettings` like `\h`,
autocomplete and the AI metadata query. Unlike those three, this probe is not
settings-agnostic: it reads `system.warnings`, and part of that table is derived
from the settings the *server* sees for the query - `Context::getCommonWarnings`
turns `settings->getChangedAndObsoleteNames()` into the `Obsolete setting ...
changed` warning. Stripping the session settings from the probe therefore
stripped the warnings that describe them, and because `showWarnings` swallows
any exception the loss was silent.
Send what an ordinary query sends instead - `settingsWithoutCompatibilityDerived`,
which also keeps a compatibility-derived value from being serialized as an
explicit change and thus from tripping a profile that pins it read-only, the
second half of the original review comment. The one setting that has to be
overridden is `dialect`, the reason the probe was changed in the first place: it
is ClickHouse SQL, so a session that switched to Kusto or PRQL could not parse
it.
New test `05045_client_warnings_probe_with_session_dialect` pins both directions
at once - a client started with `--dialect kusto` *and* an obsolete setting must
still print `Warnings:` and ` * Obsolete setting`. Verified against both negative
controls: with `networkCompressionSettings` the new test and `01945` both fail,
and with the plain session settings the new test fails while `01945` passes.
`Client::loadWarningMessages` still passed `&client_context->getSettingsRef()`
to `Connection::sendQuery`, so its plain SQL probe carried the full session
settings while `\h`, autocomplete and the AI metadata probe already went through
`networkCompressionSettings`. That probe has the same invariants as the others:
after `SET dialect = 'kusto'` it is not valid in the session dialect, and under
`compatibility` with a profile that pins the derived network settings read-only
the serialized derived values make the server reject it.
`showWarnings` swallows the exception, so the failure is silent - server
warnings simply stop being displayed. Route the probe through
`networkCompressionSettings` as well.
`clickhouse-client` printed `Connecting to ...` twice when it had to prompt
for a password:
ClickHouse client version 26.8.1.1.
Connecting to clickhouse.new:9440 as user alexey-milovidov.
Password for user (alexey-milovidov):
Connecting to clickhouse.new:9440 as user alexey-milovidov.
Connected to ClickHouse server version 26.8.1.
Neither `--password` nor `--ask-password` was given, so the client did not
know whether the user has a password at all and connected without one. The
server answered `REQUIRED_PASSWORD`, and the retry loop in `Client::main`
set `ask-password` and called `connect` again, which prompted for the
password and announced the very same endpoint a second time.
Announce an endpoint only when it differs from the one announced last, and
forget the announcement after a successful connection, so that a fallback to
another port and a reconnect later in the session are still announced.
`--ssh-key-file` can now be specified without a value. In that case the key
is looked up the same way as `ssh` does it:
- the identity files configured for this host in `~/.ssh/config` and in
`/etc/ssh/ssh_config`;
- the default identity files, such as `~/.ssh/id_ed25519`;
- the keys held by the `ssh-agent`.
A key held by the `ssh-agent` is preferred, because it does not need a
passphrase. It is also used when `--ssh-key-file` names a key file that the
agent holds. Signing with the agent is implemented in `SSHAgent`, a small
client of the agent protocol that produces the same `SSHSIG` signature as
`SSHKey::signString` does for a local key.
The passphrase is now asked only if the key file is really encrypted; before,
`--ssh-key-file` always asked for it. The message about a key that cannot be
imported now names the file.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The commands are equivalent to `SET dialect = '...'`, but are handled
on the client side before parsing, so they also work when the current
dialect cannot parse a `SET` query (for example, to leave `kusto` or
`promql`). Without an argument, `/dialect` prints the current dialect.
When the dialect is not `clickhouse`, it is displayed in the prompt
after the server display name in parentheses, with a separating space
if the display name is non-empty:
clickhouse-cloud (polyglot) :)
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The automatic choice between the plain and the secure port gave the secure port
a 100 milliseconds head start over a plain port that had already answered, so
that TLS would win whenever a server serves both. That delay bought nothing:
when both ports are reachable, connecting over either of them works, so the
port that answers first is the one to use.
`probePlainAndSecurePorts` now returns the single endpoint that answered
instead of the plain one alongside the secure one, and TLS wins only when both
answer at the same moment, which costs no waiting. The candidate the connection
falls back to when the chosen port turns out to be unusable is derived from the
address that answered, so `Candidate::answered` and the special case that let a
`SOCKET_TIMEOUT` retry a port already known to answer are gone as well: only a
protocol-level failure is retried now, never a timeout.
`test_an_unresponsive_secure_port_falls_back_to_the_plain_port` covered that
special case and is removed with it. The test of a server that listens on both
ports no longer asserts which protocol it is connected with, because either one
will do.
Racing `connectNB` against every address a host resolves to left a session that sends nothing on every
reachable backend but the one that won, on every automatically detected connect. A load balancer behind
one hostname is the normal case for that, and the server logs each of those as `Client has not sent any
data.` and counts it against `max_connections` - so handing the winning connection over fixed the
single-address case only, which is all the previous test covered.
The two ports are still probed concurrently, which is the whole point of the probing: serialising them
would stall the client for the entire connection timeout on a firewalled plain port before TLS is even
attempted. Within a port, the addresses are now attempted one at a time, the next one only after 250
milliseconds without an answer, or right away when the previous attempt has already failed. That is the
"Connection Attempt Delay" of RFC 8305 (Happy Eyeballs), for the same reason and with the same default,
and it keeps the common case - the first address answers - down to a single connection per port.
The fast paths are unaffected: a plain-only server still returns as soon as the secure port is refused,
without waiting out the preference window, and a firewalled plain port still lets the secure port win at
once. A host whose first address is a black hole now costs one attempt delay before the second address is
tried, which is far below the connect timeout it replaces.
Covered by `PortsProbe.AttemptsOneAddressAtATime`, which listens on the same port in both address
families so that `localhost` has two reachable addresses and asserts that exactly one of them has a
connection queued, and by `test_the_probe_leaves_no_extra_connection_on_the_other_addresses`, which
points one hostname at both plain-only servers and requires neither to be left with such a session.
Two problems with the fallback between the automatically chosen ports.
**The fallback could lose the backend the probe had already reached.** When the secure port won while
the plain probe was still pending, the plain fallback candidate had no address at all, so it walked
the addresses the host resolves to from the start - and `Connection::connect` has dropped the DNS
cache entries by then - paying a whole connection timeout for every unresponsive address in front of
the working one, which is exactly the delay the probing exists to avoid. It now starts from the plain
port of the address whose secure port answered, symmetrically to the `plain -> secure` branch, which
already derived its candidate that way.
**A fallback candidate must not reuse the connection the probe left on it.** The first candidate can
spend an unbounded amount of time failing - a TLS handshake waits out `handshake_timeout_ms` - while
the server drops a connection that has not finished its handshake within
`handshake_timeout_milliseconds`, so by the time the fallback runs, a connection the probe left idle
may be gone. With the defaults (10 s against 30 s) it survives, but a longer client handshake timeout
or a shorter server one turns the advertised fallback into a failure on a dead socket. Only the
candidate the probe chose takes over its connection now; a fallback keeps the address the probe
reached, which is what makes it fast, and opens a connection of its own.
The retry rule is restated in the same terms: a timeout is retriable towards a candidate the probe
actually reached, because connecting to it cannot hang, and not towards one whose address is merely
the best guess for the same backend - so a plain port that accepts the connection and then never
answers, with the secure port refused, is still reported without a second attempt.
Two tests: `test_an_unresponsive_secure_port_falls_back_to_the_plain_port` makes the secure port
accept and never answer and requires the query to come back over the plain port, and
`test_the_fallback_keeps_the_address_the_probe_reached` puts a black hole in front of the server in
the resolver order and requires the certificate-rejection fallback to reach it well inside the
connect timeout.
Three problems with the automatic choice between the plain and the secure port, in the shape
it took after TLS became the preferred protocol.
**A server whose secure port has an untrusted certificate stopped working.** TLS was not
requested by the user, it was chosen automatically, so a secure port that turns out to be
unusable must not make the client fail: the plain port is what it would have connected to if
there were no automatic choice at all, so falling back to it takes nothing away. The common
case is a server that serves both ports with a self-signed certificate, which every client
that does not pass `--accept-invalid-certificate` rejects - `Stateless tests` and
`Integration tests` are full of such servers, and that is how this surfaced. Specify
`--secure` to require TLS. The probe now reports both ports it found rather than a single
choice, so the fallback usually reuses a connection that is already established, and a
timeout is retriable in exactly that case: it costs no connection attempt, so it cannot
double the time before the failure is reported.
**Every automatically detected connection left a stray session on the server.** The probe
established a connection and discarded it, and the real connection was opened afterwards, so
the server accepted a client that sends nothing and logged `Client has not sent any data.`
for every single connect, on top of counting it against `max_connections`. The connection to
the chosen port is now handed over to `Connection`, which takes it over instead of opening a
second one (`Connection::setAdoptedSocket`); for a secure port, `SecureStreamSocket::attach`
adopts it, and the TLS handshake stays deferred to the first read or write the same way as
for a socket `Connection` connects itself. Nothing is left behind for a server that listens
on the plain port only, which is the common case.
**A build without TLS support turned a working connection into `SUPPORT_IS_DISABLED`.**
`probePlainAndSecurePorts` raced the secure port there as well, so a server answering on both
ports made the client pick TLS and then throw `tcp_secure protocol is disabled because poco
library was built without NetSSL support.` from `Connection::connect`, for a user who never
asked for TLS. There is nothing to choose between in such a build, so the detection is
skipped entirely and the plain port stays the default.
Per the review comment on the docs
(https://github.com/ClickHouse/ClickHouse/pull/110130#discussion_r3754530344,
"TLS should be preferred"), flip the automatic choice: the secure port now
wins whenever it answers, and the plain port is chosen only when the secure
port does not answer within 100 milliseconds after it (so that plain-only
servers, the most common setup, are not stalled for the whole connection
timeout of the unreachable secure port).
- `PortsProbeResult::Choice` becomes `PreferSecure`/`PlainOnly`/`Neither`,
and the preference window is renamed from `plain_preference_window` to
`secure_preference_window`.
- The `secure_address` field is gone: with TLS preferred, the plain port is
only chosen when no secure probe has answered, so there is never a probed
secure address to start the protocol-level fallback from (the fallback
still derives it from the address that answered on the plain port).
- The unit and integration tests are flipped accordingly; the integration
test for the secure-fallback address is removed (its scenario, a TLS
listener behind the plain port, is now resolved by the preference itself
and is covered by `test_secure_port_chosen_when_plain_serves_tls`), and
`test_the_remembered_address_is_refreshed_when_the_connection_falls_through`
pins the plain transport by rejecting the secure port, since it needs a
transport that both servers speak.
- The docs describe the new rule.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
When the plain probe won on a multi-address host but the native handshake
on the plain port failed (a proxy that accepts TCP on the plain port but
only serves TLS there), the retry over TLS on the secure port dropped the
`preferred_address` learned by the probe, because it carried the plain
port. The retry then walked the resolved addresses from the start and paid
a whole connection timeout on every unresponsive address in front of the
working one, reintroducing exactly the stall the remembered answering
address was added to avoid.
`probePlainAndSecurePorts` now also reports the address that answered on
the secure port (when a secure probe had connected by the time the plain
port won), and `Client::connect` starts the secure fallback candidate
from it; when the probe has no secure winner, the address that answered
on the plain port is reused with the secure port substituted, as the same
backend is the best guess for its secure port.
Adds the unit test `PortsProbe.ReportsTheSecureAddressForTheFallback` and
the integration regression
`test_the_probed_address_is_used_for_the_secure_fallback`
(the plain port serving TLS behind a multi-address hostname).
clickhouse-client could crash at process exit, after its work was already
done, returning 139 to the shell. Any script or CI harness keying on the
client's exit status sees a hard failure.
UntrackedMemoryRegistry::instance() is a function-local static holding a
boost::intrusive::list of safe_link hooks, so ~list_impl calls clear() and
nulls next_/prev_ on every node that is still linked. The registry documents
the resulting requirement in its own comment: every thread that owns a
ThreadStatus must be joined before main returns.
clickhouse-client was the only entry point that did not do that. Parsing
stdin lazily initializes the process-lifetime format-parsing pool
(FormatFactory.cpp, whose comment notes that the server and clickhouse-local
initialize their pools at startup instead), and that pool's workers detach
themselves on the shrink branch of ThreadPoolImpl::worker, so nothing will
ever join them. Such a worker can still be unwinding while static
destruction runs; its ~ThreadStatus then reaches
UntrackedMemoryRegistry::remove, which unlinks through the already destroyed
list head.
The core dump from Stateless tests (arm_binary, parallel) shows exactly that
chain: set_next(n=0x0, next=0x0) <- circular_list_algorithms::unlink <-
list_impl::erase_and_dispose <- UntrackedMemoryRegistry::remove <-
~UntrackedMemoryCounter <- ~ThreadStatus <- startThreadFromGlobalPool's
lambda <- ThreadFromThreadPool::worker. The crashing thread is a ThreadPool
worker, not main. Two unrelated PRs hit it, on 02290_client_insert_cancel and
on 00900_long_parquet. In a debug build the same state is caught one step
earlier by boost's safe_link invariant assert in list::iterator_to, so it
aborts with 134 instead of segfaulting with 139.
Report:
https://s3.amazonaws.com/clickhouse-test-reports/json.html?PR=112945&sha=5edbad2c740429c3262652b1171e80a254f6907e&name_0=PR&name_1=Stateless%20tests%20%28arm_binary%2C%20parallel%29
The fix copies the clickhouse-local precedent established by commit
b219eaf99e "Join threads from global thread pool before exiting from
main", including its include set and its comment. With it, all 9 entry
points that can create a ThreadStatus join their threads before main
returns. The other entry points need no guard: only a pool owned by a
process-lifetime static outlives main, and the one that this bug goes through
is created on the input path of FormatFactory alone.
src/Common/UntrackedMemoryRegistry.* is left untouched, since its documented
invariant is correct and this change is what makes it hold for the client.
Validation. Forcing the window, by holding a self-detached worker's
UntrackedMemoryCounter registered while main exits, reproduces the crash
15/15 before this change and 0/15 after, with the probe verifiably still
firing in both arms, 52 to 84 times per run before the change and 47 to 58
after; the oracle is a direct read of counters.size() inside
~UntrackedMemoryRegistry, which is non-zero on every crashing run and zero on
every clean one. Removing GlobalThreadPool::shutdown
from the guard restores the crash 10/10. Because that call is documented as
able to block when a local pool has been leaked, client exit was timed before
and after on four paths, a plain query, --queries-file, a piped stdin INSERT
and a kill -INT cancel: no hang, and at most 7.6 ms of added exit time on the
path that actually uses the pool. 02290_client_insert_cancel and
00900_long_parquet already assert this contract, so no new test is added.
Together with 04229_untracked_memory_async_metric and
02490_benchmark_max_consecutive_errors they were rerun with randomized
settings: the three that are not tagged long pass 150/150 over 50 runs each,
and 00900_long_parquet and 00900_long_parquet_decimal pass 5/5 over the 5 runs
that --long-test-runs-ratio gives a long test.
Note that a scope guard cannot run on _exit, so the --version and --help
early exits and the second-Ctrl+C safeExit path still bypass static
destruction. Both return before any pool worker exists, so they avoid this
window rather than entering it unsafely.
`--format` (and the `format` client-config key) was mirrored into the
bidirectional `format` setting, so `clickhouse-client --format Pretty -q
"INSERT INTO t FORMAT CSV"` tried to parse the CSV input data as `Pretty`
— a behavior regression, since `--format` is documented as output-only in
`clickhouse-client`. The same leak also happened through the config-driven
path in `Client.cpp`, which applies config keys matching unchanged setting
names to the client context.
Map `--format` / config `format` to `output_format` in `clickhouse-client`
(including the embedded client), keeping the historical bidirectional
meaning in `clickhouse-local` via `mappedFormatOptionSetting`, and skip
the `format` config key in the generic config-to-settings loop of
`Client.cpp`. Output behavior is unchanged in both binaries.
Addresses the two review threads on `ClientBase.cpp` about `--format` /
config `format` changing INSERT input semantics.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
When the transport is auto-detected, the address that answered is
remembered so that a reconnect does not walk the resolved addresses of
the host from the start. But it was only written when the ports were
probed: a reconnect that fell through from the dead remembered address
to another resolved address (`Connection::connect` tries them one by
one) kept the stale address, so every following reconnect waited out a
whole connection timeout on it again before falling through to the
working one.
Now the remembered address is refreshed from
`Connection::getResolvedAddress` after every successful connect while
the transport stays auto-detected. Covered by a new integration test
where the remembered address stops accepting new connections while
another resolved address of the same host keeps working.
A plain port that accepts the TCP connection and then never answers belongs to
an unresponsive server, not to a TLS listener, and the secure port of the same
server does not answer either. Retrying it doubled the time the client waited
before it reported the failure, which is exactly the delay the automatic choice
between the ports is supposed to avoid.
This is visible in `Stress test (amd_tsan)` of this pull request: the shutdown
loop of the stress test runs `clickhouse client -q 'SYSTEM STOP DISTRIBUTED
SENDS'` against a server that is stopped for a backtrace, and every iteration
took 10m22s (300 seconds of the handshake timeout on `9000` plus 300 seconds of
the send timeout of the TLS handshake on `9440`) instead of the 5 minutes it
takes without the automatic choice, so the job ran out of its time limit:
https://s3.amazonaws.com/clickhouse-test-reports/PRs/110130/3e6ccfaf1d7ace475a6030ce9392221c55ecd48d/stress_test_amd_tsan/job.log
`SOCKET_TIMEOUT` is therefore no longer a retriable connection error. The
documented case of a proxy that accepts TCP on the plain port but only serves
TLS there keeps working: it fails with `UNEXPECTED_PACKET_FROM_SERVER`,
`NETWORK_ERROR` or `ATTEMPT_TO_READ_AFTER_EOF`, which stay retriable.
The new test lets the TCP handshake through to both ports and drops everything
that is sent afterwards, so that both of them are accepted and never answered.
The automatic choice between the plain and the secure port remembered only
the port and the TLS mode for the address it was made for. A reconnect goes
through `Client::connect` again, and the ports are not probed the second time
(the port is no longer unspecified), so the new `Connection` was created
without a preferred address: a host resolving to an unresponsive address and a
healthy one connected quickly once and then paid a whole connection timeout on
the very next reconnect.
Remember the address of the established connection in `HostAndPort` along with
the port and the TLS mode, and forget it together with them after a failed
connection attempt. `Connection::getResolvedAddress` is made public for that.
The regression test breaks the session in the middle of the client's batch of
queries - the server rejects an `INSERT` while it receives the data, so the
client disconnects and reconnects for the next query - and requires the whole
batch, including the reconnect, to finish well within the connection timeout.
Also fix the cleanup of that test: `/etc/hosts` is bind-mounted into the
container, so `sed -i` cannot rename its temporary file over it and fails with
`Device or resource busy`.
A host can resolve to several addresses, and `Connection::connect` tries
them one by one, so every unresponsive address in front of the list costs
a whole connection timeout. The port probing already knows which address
has answered, but it was throwing that away: the connection started over
from the first resolved address, which could be exactly the address the
probe has just found unresponsive. The automatic choice then reintroduced
the very timeout it is supposed to avoid.
`probePlainAndSecurePorts` now reports the address that answered, and it
is passed to `Connection` as a preferred address, which is tried first.
It is only a preference: if the address is gone by the time of the
connection or of a reconnect, the remaining addresses are tried as usual.
Adds a unit test for the reported address and an integration test where
the host resolves to a black hole first and to a healthy server second.
Alexey Milovidov