ClickHouse / ClickHouse/ClickHouse
Persisted `loop(db, t)` table pins its source: `DROP TABLE t SYNC` never returns
- Dominant language
- C++
- Stars
- 49.9k
- Forks
- 9k
- Avg merge
- 21h 32m
- Merged PRs (30d)
- 515
Description
### Describe what's wrong
**`CREATE TABLE q AS loop(db, 't')` is accepted. Once `q` has been read, `DROP TABLE t SYNC` blocks forever (`t` sits in `system.dropped_tables`, the query stays in `system.processes`) and returns the instant `q` is dropped. This is the exact failure mode this PR closes for the six MergeTree introspection names, in a function the PR leaves persistable.**
- **Root cause:** `TableFunctionLoop` keeps the inherited `canBeUsedToCreateTable() == true` (`[`src/TableFunctions/TableFunctionLoop.cpp:27-30`](https://github.com/ClickHouse/ClickHouse/blob/00260f9813a75/src/TableFunctions/TableFunctionLoop.cpp#L27-L30)`) although `StorageLoop` holds the source table's `StoragePtr` as a data member - the same property for which this PR refuses persistence in the five introspection classes. `ENGINE = Loop` is already refused at `StorageFactory.cpp:132` ('use Loop as a table function only'), so `AS loop(...)` is the one remaining door to a persisted Loop-backed table.
Analysis details (evidence, affected locations, impact)
**Why we believe this is a bug:** `InterpreterCreateQuery.cpp:2606` -> `throwIfTableFunctionCannotBeUsedToCreateTable` (`InterpreterCreateQuery.cpp:882`) asks only the outermost table function, and `TableFunctionLoop` (`TableFunctionLoop.cpp:27`) never overrides `canBeUsedToCreateTable`, so the table is created. The first read memoises `StorageLoop` inside the `StorageTableFunctionProxy` (`StorageTableFunction.h:38-48`), and `StorageLoop::inner_storage` (`StorageLoop.h:35`) is the source table's `StoragePtr`, so the drop worker's uniqueness test (`DatabaseCatalog.cpp:1661`) never selects the source and `waitTableFinallyDropped` (`DatabaseCatalog.cpp:1843`) polls forever.
**Affected locations:**
- [`src/TableFunctions/TableFunctionLoop.cpp:27`](https://github.com/ClickHouse/ClickHouse/blob/00260f9813a75/src/TableFunctions/TableFunctionLoop.cpp#L27) — class TableFunctionLoop - no canBeUsedToCreateTable override, unlike the five classes this PR changes
- [`src/Storages/StorageLoop.h:35`](https://github.com/ClickHouse/ClickHouse/blob/00260f9813a75/src/Storages/StorageLoop.h#L35) — StorageLoop::inner_storage holds the source table's StoragePtr
- [`src/Interpreters/InterpreterCreateQuery.cpp:882`](https://github.com/ClickHouse/ClickHouse/blob/00260f9813a75/src/Interpreters/InterpreterCreateQuery.cpp#L882) — the veto this PR wires up, consulted for the outermost function only
- [`src/Interpreters/DatabaseCatalog.cpp:1661`](https://github.com/ClickHouse/ClickHouse/blob/00260f9813a75/src/Interpreters/DatabaseCatalog.cpp#L1661) — drop worker skips a marked-dropped table whose StoragePtr is not unique
- [`src/Storages/StorageFactory.cpp:132`](https://github.com/ClickHouse/ClickHouse/blob/00260f9813a75/src/Storages/StorageFactory.cpp#L132) — ENGINE = Loop already refused for the same 'table function only' reason
**Impact:** A `DROP TABLE` on the source never completes and holds a server thread; the table stays in `system.dropped_tables` indefinitely. Recovery requires dropping the persisted `loop` table, `KILL QUERY`, or a restart. Not a regression of this PR - the acceptance predates it - but it leaves the hazard the PR is closing reachable through one more registered name.
### Does it reproduce on most recent release?
Yes — confirmed on current `master` (commit `00260f9813a75`).
### How to reproduce
[▶ Run on ClickHouse Fiddle](https://fiddle.clickhouse.com/73c88d2b-3dac-4aee-ac91-ebbfe97bd088)
Reproducer
```sql
DROP TABLE IF EXISTS tab;
CREATE TABLE tab (col String) ENGINE=Loop; -- { serverError INCORRECT_QUERY }
DROP TABLE IF EXISTS src_03307;
DROP TABLE IF EXISTS tab_loop_03307;
CREATE TABLE src_03307 (id UInt32) ENGINE = MergeTree ORDER BY id;
-- A persistent table over `loop` keeps the source table's storage object alive, so `DROP TABLE src_03307 SYNC` never returns.
CREATE TABLE tab_loop_03307 AS loop(currentDatabase(), 'src_03307'); -- { serverError BAD_ARGUMENTS }
DROP TABLE IF EXISTS tab_loop_03307;
DROP TABLE IF EXISTS src_03307;
```
### Expected behavior
Expected output of the reproducer above:
```
empty .reference - `CREATE TABLE ... AS loop(...)` fails with `BAD_ARGUMENTS` the way the six names in 05138 do, and `DROP TABLE SYNC` returns immediately whether or not a persisted loop table exists
```
### Error message and/or stacktrace
Actual output of the reproducer above on `master` (`00260f9813a75`):
```
[1 / 1] 05210_loop_as_table_function_persisted: [ FAIL ] 0.57 sec.
Reason: having stderror:
The query succeeded but the server error '36' was expected (query: CREATE TABLE tab_loop_03307 AS loop(currentDatabase(), 'src_03307'); -- { serverError BAD_ARGUMENTS })
Manual pin repro on the same build (client on 127.0.0.1:19010):
T+0 DROP TABLE db_118969_e.s SYNC -- started 09:12:50
T+10 SELEC
```
Suggested fix
Give `TableFunctionLoop` the same one-line override the five classes get here: `bool canBeUsedToCreateTable() const override { return false; }`. Trade-off: that also refuses replay of `CREATE TABLE ... AS loop(...)` definitions stored by earlier versions - the same backward incompatibility this PR already accepts. The alternative, keeping persistence legal and having `StorageLoop` hold a `StorageID` resolved per read, keeps such tables working but touches `StorageLoop`/`ReadFromLoopStep`.
Additional context
**Open risks:**
- `CREATE TABLE p AS loop(mergeTreeIndex(db, t))` is accepted too but measured NOT to pin (the drop returned at once) - `ReadFromLoopStep` re-resolves the inner table function, so only the plain `loop(db, table)` form holds the source.
- `DROP DATABASE ... SYNC` over the same database was not measured; it drops member tables in catalog order, so whether it blocks depends on whether the persisted table is dropped before the source.
Found during automated review of [PR #118969](https://github.com/ClickHouse/ClickHouse/pull/118969). Severity P2 · Finding `h_pr118969_001`
cc @groeneai @alexey-milovidov (from #118969)
Contributor guide
Research direction
Start with src/TableFunctions/TableFunctionLoop.cpp:27 and compare its table-creation capability with the five introspection classes changed by PR #118969. Run the 05210_loop_as_table_function_persisted test and review the listed InterpreterCreateQuery.cpp veto path. Done means persisted CREATE TABLE ... AS loop(...) is rejected with BAD_ARGUMENTS and DROP TABLE SYNC returns promptly.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- cpp, sql
- Domain
- backend, databases
- Issue type
- Bug
- Difficulty
- 2/5
- Estimated time
- 1-3 hours
- Activity status
- Active
- Clarity
- Clearly specified
- Newbie friendliness
- 82/100