ClickHouse / ClickHouse/ClickHouse

Persisted `loop(db, t)` table pins its source: `DROP TABLE t SYNC` never returns

Open Beginner friendly
#119,765 2 comments 0 reactions 0 assignees View on GitHub
comp-table-functions
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

Open the contributing 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.