A manifest walk over an Iceberg table with many manifests was bounded by
`iceberg_manifest_decode_concurrency`: fetching a manifest (a remote round trip)
and parsing it (CPU, about 2 ms per manifest) were one task on a dedicated pool,
so only that many round trips overlapped (issue #127061: 671 manifests, 9.6 s at
4, 4.0 s at 32).
Manifest files are now read ahead on the IO thread pool, 64 at a time and at most
64 MiB of manifests not in the metadata files cache, and nothing is parsed there.
The read goes through `getOrSetManifestFile` as before, so concurrent queries
still share one read per manifest and `SYSTEM CLEAR ICEBERG METADATA CACHE` keeps
its semantics; the cached cell holds the bytes and parses them on the first
`getDeserializer()` call (`std::call_once`).
The threads that call `IcebergIterator::next` parse the manifests: each one
continues an idle partially read manifest or parses the next one, so parsing
keeps the parallelism of the reading threads; with one reading thread the files
come in the manifests' order as before. Delete manifests are parsed by the
threads that need them, and the last one sorts the deletes. `totalRows` reads
ahead and parses on the calling thread.
Removed: the manifest decode thread pool and its metrics, the producer thread,
the bounded queue and the stop flag of `ManifestFileIterator`,
`ParallelManifestDecode.h`. The query settings `iceberg_manifest_decode_concurrency`
(and its old name `iceberg_delete_manifest_decode_concurrency`) and
`iceberg_file_entries_queue_size`, and the server settings
`max_iceberg_manifest_decode_thread_pool_size`,
`max_iceberg_manifest_decode_thread_pool_free_size` and
`iceberg_manifest_decode_thread_pool_queue_size` are obsolete: accepted and ignored.
The integration tests of the decode concurrency now vary `max_threads` instead;
the test of the removed pool and the timing test of the removed setting are
deleted.
Closes#127061
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Resolve conflicts in the `dlopen` guard in `programs/main.cpp` and
`programs/keeper/keeper_main.cpp`: keep the guards identical (as master now
requires) and keep `USE_GPU` from this branch in both.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
`tokenizeSync` decoded each token with a default `TextDecoder`, which strips a
leading `U+FEFF`. The token lengths then no longer matched the original text,
so the statement offsets used by the multi-statement "Format" button were off
by one and a BOM-prefixed `select 1;` was sent as `select `. Decode with
`ignoreBOM: true` and add a regression case to the harness.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
`loadMetadataSystem` attaches the persisted tables of the `system` database
before the other databases, so a table in `system` with a projection using
`dictGet` on an XML dictionary was still attached while the dictionary was
unknown. Register the dictionaries right after the temporary database is
loaded, and run the regression test for a table in `system` too.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Stopping the DDL worker joins the task it runs, including ones that are not
backups or restores. Before the connection wait, a `TRUNCATE ... ON CLUSTER`
waiting in `lockExclusively` for a lock held by a native `INSERT` that waits for
client data kept the shutdown waiting until `lock_acquire_timeout`, instead of
exiting forcefully after `shutdown_wait_unfinished` as master does (66.0 s vs
5.5 s with `shutdown_wait_unfinished = 5` and `lock_acquire_timeout = 60`).
The first kill stays where master has it and still spares the DDL worker's
parts, which keep running during the connection wait. After the forced exit
check, the remaining queries are cancelled again, as `ContextSharedPart::shutdown`
does before it stops the DDL worker, then the DDL worker is stopped and all
backups and restores are waited for, still before the AsyncLoader stops. Both
`shutdown_wait_unfinished_queries` modes now use the same place.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Basically two minor changes
1. Make the setting `collect_connection_metrics` reloadable at the
runtime (via SYSTEM RELOAD CONFIG for example). To make this
possible, now we check for registeration before each query during
HTTP. Common how it's done in other places
2. Make connection_Id UUID instead of Uint64. To avoid complexities and
conflict during cluster mode.
Signed-off-by: Kaviraj <kavirajkanagaraj@gmail.com>
The parts of ON CLUSTER backups and restores that the DDL worker registers after
the shutdown wait were spared by both shutdown kills and left to
`BackupsWorker::shutdown`. That runs in `Context::shutdown`, after the scope
guard in `Server.cpp` has already shut down the AsyncLoader. A late
`RESTORE DATABASE ... ON CLUSTER` of a dropped database then fails on the host
being restarted: `InterpreterCreateQuery::createDatabase` schedules the
database's startup job, and `AsyncLoader::schedule` throws
`ASYNC_LOAD_CANCELED: AsyncLoader was shut down: While creating database mydb`.
Drain those parts in `Server.cpp`, before the AsyncLoader stops: kill the other
queries as before, stop the DDL worker (which makes every part it delivered
registered), and wait for all backups and restores again. By default this
happens right after the existing kill. With `shutdown_wait_unfinished_queries`
there is no kill there, so the same kill and drain run after the queries were
waited for. Late initiators are still cancelled before the DDL worker stops,
because an initiator's own part needs it.
A DDL worker whose startup job has not run yet must not be started after it is
stopped, so the drain first removes that job from the AsyncLoader (cancelling
it if pending, waiting for it if running).
Since no such part outlives the drain, the `ContextSharedPart::shutdown` reorder
and its sparing kill are reverted to master. The only path that skips the drain
is a startup failure, where the DDL worker is not running.
The restore shutdown test now restores a dropped database, so it covers the
database and table creation, and it runs twice: restarting node2 (default
settings) and restarting node3, which has `shutdown_wait_unfinished_queries`.
- master shutdown code: RESTORE_CANCELLED (both cases)
- previous head (541c81bf): RESTORE_FAILED, ASYNC_LOAD_CANCELED (both cases)
- this commit without the drain after the query wait: RESTORE_CANCELLED in the
`shutdown_wait_unfinished_queries` case
- this commit (9120585a): RESTORED (both cases, 3/3 each)
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
- `splitAllQueries`: the lexer stops at the first error (e.g. an unterminated
string literal or comment), so the unlexed remainder was dropped and Format
would edit only the valid prefix. The remainder now belongs to the last
statement, so the server reports the error and the editor is unchanged.
- Format: re-check the connection, tab and text after awaiting the splitter
(which loads the lexer on first use), so the text is never sent with the
credentials of a connection selected after the click.
- Update the Format fixture of `test_play_reconcile_startup` for the new `f<N>`
aliases and cover multiple statements, all-or-nothing errors, lexer errors and
a connection change during the lexer load.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The definitions of the dictionaries from the configuration files were added to
`ExternalDictionariesLoader` only after `loadMetadata`, so a table referring to
such a dictionary was attached while the dictionary was unknown: the dependency
was dropped by `TablesLoader::removeUnresolvableDependencies` as nonexistent,
and a projection using `dictGet` could not be analyzed and was skipped.
Now the configuration repository is registered before `loadMetadata`, while
automatic loading is disabled in the loader until `loadOrReloadDictionaries`,
so no dictionary starts loading earlier than before.
Closes: https://github.com/ClickHouse/ClickHouse/issues/67521
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A lookup that misses no longer repeats the node search of every XML layer: `XMLConfiguration`
already reports a node that has children, so `LayeredConfiguration::getRaw` probes only the flat
`MapConfiguration` layers, and `MapConfiguration::enumerate` starts at `lower_bound(prefix)`
instead of scanning the whole map. On a server-shaped configuration (programs/server/config.xml,
the command line of CI's replica server, the application and system layers; debug build) a
missing key now costs 1036 ns, against 907 ns on master and 1934 ns before this change; a key
that exists is unchanged (318 ns).
A section given only on the command line reads as "" through `getString(section, default)`, and
`getInt(section, default)` throws; the comment in `getRaw` states this.
`remote_servers` stays file-only. `Cluster` takes the server's own `tcp_port` from that
configuration to find its local replicas, so with the command line on top, a server started with
`-- --tcp_port` treats the replicas on the file's port as remote: measured `is_local` 1 -> 0 and
`cluster()` failing with `ALL_CONNECTION_TRIES_FAILED`, which is the shape the performance job
uses (servers on 9001/19001, `test_cluster_two_shard_three_replicas_localhost` on 9000).
https://github.com/ClickHouse/ClickHouse/pull/124915#issuecomment-6192247516
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Split the editor's text into statements with `splitAllQueries`, format all of
them in one request (`SELECT formatQuery({q0:String}) AS f0, ...`), and replace
each statement in place. The `;` delimiters, whitespace, and comments between
statements are kept as is.
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Conflicts were only in the AI function integration tests: the cancellation
tests from master run alongside this branch's concurrency tests, pinned to one
request per thread.
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Conflicts with the query id change: the executor path now sets `query_id` and
`initial_query_id` and submits `AIChatRequest`; `sanitizeForLog` is `sanitize`.
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
`config()` has a writable layer at `PRIO_APPLICATION` where `Poco::Util::Application` puts
`application.name`, `application.dir` and the other `application.*` properties. The reload
snapshot of the file, the command line and `SystemConfiguration` did not have it, so
`${application.name}` stayed literal on reload: a masking rule written with it masked at
startup and silently stopped masking after the first `SYSTEM RELOAD CONFIG`, for a rule in
the file as well as for one on the command line.
Copy the `application.*` properties at the start of `Server::initialize`, before
`BaseDaemon::initialize` adds the command line and the file, and add that copy to every
reload snapshot at `PRIO_APPLICATION`. Copying them from `config()` at reload time instead
would also capture an `application.*` key of the previous file (a file can have one only with
`skip_check_for_incorrect_settings`) above the reloaded file. The snapshot now has the same
sources as `config()`: the command line, the application properties, the file and the system
properties.
The masking test now uses `${application.name}_${system.env.MASK_PATTERN}`, so it covers both
property sources: it fails on the previous head after the reload and passes here.
Reported by the review bot on #124915:
https://github.com/ClickHouse/ClickHouse/pull/124915#discussion_r4270445050
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
At startup the query masking rules, ZooKeeper and the listeners read `config()`,
which has the `SystemConfiguration` layer that Poco's `${system.env.NAME}`
expansion resolves against. On reload the main config reloader callback passes
them a snapshot of the file and the command line instead, which had no system
layer, so `${system.env.NAME}` stayed literal: a masking rule written that way
masked at startup and silently stopped masking after the first
`SYSTEM RELOAD CONFIG`, leaking the value into `system.query_log` and the text log.
Add a `SystemConfiguration` at `PRIO_SYSTEM` to the snapshot, owned by it
(`shared = false`, as `Application::setup` does for `config()`). The validation of
the reloaded file keeps reading the file alone, so key enumeration there does not
see a `system` node.
Reported by the review bot on #124915:
https://github.com/ClickHouse/ClickHouse/pull/124915#discussion_r4263740871
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Conflicts:
- `src/Storages/MergeTree/MergeTreeData.cpp`, `MergeTreeData.h`: master changed the old statistics
refresh task (`startStatisticsCache` / `stopStatisticsCache` / `refreshStatistics`, #125165) so that
shutdown does not wait for a running refresh. This PR removes that task entirely in favor of the
per-part cache, so the removal is kept. Its test `05361_statistics_refresh_does_not_block_drop`
exercises only the removed task and `refresh_statistics_interval` (now obsolete), so it is deleted.
- `src/Core/ServerSettings.cpp`: kept the `statistics_cache_*` settings and took master's new
description of `iceberg_metadata_files_cache_policy`; the description of `statistics_cache_policy`
now lists the possible values like the other cache policies after #123435.
`renderWithVisibleSelection` kept `state.selectedKey` when it pointed at a node missing from the
freshly loaded graph, so toggling a category or changing the database filter dimmed every node and
left the stale sidebar open. Treat a missing node the same as a hidden one.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Azat Khuzhin