Feature: Enable overlay databases for server.#86768
Conversation
|
@azat I believe this is ready for initial review, I don't know why the rest of the CI is not running |
|
@alexey-milovidov this test is failing with random error messages that doesn't happen in other runs or locally can you give me any pointers on why this might be failing ? https://github.com/ClickHouse/ClickHouse/actions/runs/17553376388/job/49853979804?pr=86768 |
|
@azat @alexey-milovidov I believe this is ready for review |
|
Pushed three commits addressing the remaining review items:
All five |
The previous commit set `interpreter.setIsRestoreFromBackup(true)` for every database created by `RestorerFromBackup::createDatabase`. That was meant only to let an `Overlay` facade be restored before its source databases exist (lazy resolution skips the creation-time source-existence check), but applying it to all databases also changed the loading-strictness level to `SECONDARY_CREATE` and flipped the `distributed_backup_restore` flag for `Replicated` databases, breaking their `RESTORE ON CLUSTER` (consistent `NO_ZOOKEEPER` failures across `test_backup_restore_on_cluster`, `test_backup_restore_on_cluster_with_checksum_data_file_name` and `test_refreshable_mv`). Restrict the restore-from-backup mode to the `Overlay` engine, resolving the engine name through `DatabaseFactory::resolveCanonicalEngineName` so it matches regardless of casing (the parser lowercases `Overlay` to `overlay` because it collides with the `OVERLAY` string function). Every other database engine reverts to the previous master behavior. CI: ClickHouse#86768 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
A server-side `Overlay` facade is itself a local database, so it stays in
`DatabaseCatalog`'s `databases_without_remote` set. Bulk enumerators that must
avoid implicit calls to a remote service — `system.tables`, `system.columns`
and the asynchronous metrics — fetch `getDatabases({.with_remote_databases = false})`,
which returns that set, and then iterate each database's tables. Because
`DatabaseOverlay::getTablesIterator` walks every underlying source
unconditionally, an `Overlay('mysql_db', 'local_db')` would let those enumerators
reach `DatabaseMySQL::getTablesIterator` and query `INFORMATION_SCHEMA` even when
`show_remote_databases_in_system_tables` is disabled — exactly the implicit
remote call the setting suppresses.
Reject a remote source (`MySQL`/`PostgreSQL`/`DataLake`, i.e. any database whose
`isRemoteDatabase` returns true) at `Overlay` CREATE time. This keeps the
contract simple: an `Overlay` over a remote database is never persisted, so the
lazy `ATTACH` path on startup never has to deal with one.
Add `04338_overlay_database_rejects_remote_source` covering the rejection
(regardless of the source's position) and that an `Overlay` over only local
databases is still accepted.
CI: ClickHouse#86768
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}
Supersedes the previous commit's CREATE-time rejection of remote sources.
Instead of forbidding a remote source, `DatabaseOverlay::isRemoteDatabase`
now returns the `readonly` flag: a server-side (read-only) facade is reported
as remote, while the `clickhouse-local` (non-read-only) `Overlay` stays
non-remote.
This excludes the server-side facade from the default catalog enumeration
`getDatabases({.with_remote_databases = false})` used by `system.tables`,
`system.columns` and the asynchronous metrics, so an `Overlay` over a remote
source (`MySQL`/`PostgreSQL`/`DataLake`) no longer lets those consumers reach
the remote service and issue implicit calls (e.g. query `INFORMATION_SCHEMA`)
when `show_remote_databases_in_system_tables` is disabled. Explicit
`SHOW TABLES`/`SHOW COLUMNS`/`SHOW INDEXES` on the facade still list its tables
(they override the setting for remote databases), `system.databases` still
lists it, and direct queries through the facade are unaffected.
Answering from the constant `readonly` flag — rather than by resolving the
sources — sidesteps two problems a source-resolving implementation would have:
it does not depend on the order in which databases are loaded at startup, and
it touches no other database, so it is safe to call from
`DatabaseCatalog::attachDatabase`, which evaluates `isRemoteDatabase` while
holding `databases_mutex` (resolving sources there would re-enter the same
non-recursive mutex and deadlock).
Replace test `04338` with one that asserts the new contract: a read-only
`Overlay` (including one over a remote source) is hidden from `system.tables`
by default but visible with `show_remote_databases_in_system_tables = 1`,
`SHOW TABLES` and direct queries still work, and the `clickhouse-local`
`Overlay` keeps showing its tables in `system.tables`.
CI: ClickHouse#86768
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
A read-only `Overlay` facade flattened a nested read-only `Overlay` source into its
leaf databases, dropping the middle facade from every runtime check. With
`top = Overlay('mid')` and `mid = Overlay('src')`, `SELECT ... FROM top.t` resolved
the storage to `src.t`, so the access and row-policy code -- which only sees the
written id (`top.t`) and the resolved storage id (`src.t`) -- never required the
grants or applied the row policies defined on `mid.t`. A user granted on `top.*` and
`src.*` but not `mid.*` could read through `top`, and the same shape applied to
synchronous and asynchronous `INSERT`.
Reject a read-only `Overlay` source instead of flattening it, both at CREATE time
(an immediate, friendly error) and at lazy resolution (the safety net that also
covers reference cycles formed after creation by dropping and re-creating a source).
This makes the nested-facade bypass impossible rather than relying on the analyzers
to re-derive and re-check the full resolution chain.
Reported by the AI review on ClickHouse#86768
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Its only user was the removed `resolveDatabasesImpl` declaration. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
I pushed a few changes on top of this PR: Fixed the access/row-policy bypass for nested
CI: the only red was Left for your call (your open threads, not mechanical fixes): creating a table through the facade in the first underlying database (the author noted a UUID-collision limitation), and the |
The test creates databases with fixed global names (`ov_nested_src`,
`ov_nested_mid`, `ov_nested_top`), so it cannot run concurrently with
copies of itself. The `Stateless tests (flaky check)` jobs run a test
many times in parallel, and a concurrent run dropped a source database
out from under another run between `CREATE DATABASE ov_nested_src` and
the facade `CREATE DATABASE ... ENGINE = Overlay('ov_nested_src')`,
producing `Overlay database requires existing underlying database
'ov_nested_src'` (and a later `Table ov_nested_mid.t does not exist`).
The regular stateless runs passed; only the parallel flaky check failed.
The sibling `03611_overlay_database_server_side.sql` and
`03611_overlay_database_restore_rename.sql` tests are already
`no-parallel` for the same reason.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ce check `DatabaseOverlay::createTable`/`attachTable` already throw `TABLE_IS_PERMANENTLY_READ_ONLY` for a read-only facade, but for a table name that already resolves through the facade to a source table the generic existence check in `InterpreterCreateQuery::doCreateTable` short-circuits first: `database->isTableExist` returns true, so `CREATE TABLE IF NOT EXISTS db_overlay.t` returned success and a plain `CREATE TABLE db_overlay.t` reported `TABLE_ALREADY_EXISTS`, neither of them ever reaching `DatabaseOverlay::createTable`. That violated the documented read-only facade contract and let a user with only a facade-scoped `CREATE TABLE` grant probe the existence of source tables. Reject creation/attach on a read-only `Overlay` up front, before the existence check, for both `CREATE` and `ATTACH` and regardless of `IF NOT EXISTS`, mirroring the guards already used in `InterpreterDropQuery`/`InterpreterAlterQuery`/etc. Test `03611_overlay_database_create_existing_source_rejected` covers the existing-source-table name (plain, `IF NOT EXISTS`, and `ATTACH`) as well as a brand-new name, and verifies the source table is untouched. CI: ClickHouse#86768 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Pushed AI-review blocker — read-only
|
|
Both red checks on the current head (
Everything else is green or still in progress: all builds (incl. |
|
Merged fresh Rationale: on the previous head the whole public CI was green (AI Review ✅ Approve, A fresh public CI run and a fresh private re-sync are now in flight on The PR is otherwise merge-ready; it remains blocked solely on the standing 2025-09-08 |
Address the `clickhouse-gh` AI review "Request changes": the new server-side
`Overlay` coverage never created the same table name in two sources or repeated
a source in `Overlay(...)`, so the "first listed source wins" and
"duplicate source names are collapsed, keeping the first occurrence" contract
was documented and implemented but not pinned by a test. An earlier regression
on this branch (sources resolved in alphabetical order instead of the listed
order) would have left the existing suite green.
New `03611_overlay_database_lookup_precedence` creates the same table `t` in two
sources and verifies that:
* `db_overlay_precedence.t` reads from the source listed first (the second
source's `t` is shadowed, not unioned);
* reversing the source order flips which source wins;
* `Overlay('db_src_first', 'db_src_second', 'db_src_first')` collapses the
duplicate, keeping the first occurrence, as shown by `SHOW CREATE DATABASE`
and by `t` still resolving to `db_src_first`.
Marked `no-parallel` like the sibling `03611_overlay_database_*` tests, since it
creates databases with fixed global names.
|
Pushed AI-review blocker — lookup-precedence contract had no test (
|
|
|
||
| ## Access control {#access-control} | ||
|
|
||
| Accessing a table through an `Overlay` database requires a grant on **both** the `Overlay` |
There was a problem hiding this comment.
This access-control contract is still wider than the implementation. The new dual-grant checks only cover SELECT and INSERT; metadata lookups through the facade still require only facade-side SHOW_* privileges.
For example, SHOW CREATE TABLE ov.t resolves ov.t, checks SHOW_COLUMNS on that facade name, and then asks DatabaseOverlay::getCreateTableQueryImpl for the definition, which walks the source databases and returns the underlying table metadata (src/Interpreters/Context.cpp:7596-7600, src/Interpreters/InterpreterShowCreateQuery.cpp:65-74, src/Databases/DatabaseOverlay.cpp:265-281). DESCRIBE ov.t, SHOW COLUMNS FROM ov.t, and EXISTS TABLE ov.t have the same shape (src/Interpreters/InterpreterDescribeQuery.cpp:181-190, src/Interpreters/InterpreterShowColumnsQuery.cpp:176-183 together with src/Storages/System/StorageSystemColumns.cpp:185-195, and src/Interpreters/InterpreterExistsQuery.cpp:54-75): they resolve or inspect the source table without ever re-checking the source database.
So a user with SHOW_COLUMNS / SHOW_TABLES on the Overlay but no privileges on the source can still enumerate source-table existence and schema through the facade. Please either enforce the source-side SHOW_* checks on these metadata paths or narrow the documented contract and tests to SELECT / INSERT only.
LLVM Coverage ReportChanged lines: Changed C/C++ lines covered: 532/645 (82.48%) · Uncovered code |
3 participants
Enables server-side
Overlaydatabases. AnOverlaydatabase is a read-only facade that exposes the union of the tables of several underlying databases, resolving each table name through the listed sources in order (the first source that has the table wins). DDL on the facade is rejected — it has no storage of its own — whileSELECTand pass-throughINSERTresolve to the underlying source table. Reading or writing through the facade requires the corresponding grant on both the facade database and the underlying source, and the facade's row policies are combined with the source's. Previously anOverlaydatabase existed only as the implicit default database ofclickhouse-local.Closes: #52764
Changelog category (leave one):
Changelog entry (a user-readable short description of the changes that goes into CHANGELOG.md):
Allow creating
Overlaydatabases.Documentation entry for user-facing changes