ClickHouse / ClickHouse/ClickHouse

ALTER TABLE ... DELETE on a Join-engine table is not crash-atomic: a server kill during the mutation silently loses acked rows the DELETE never matched

Open
#116,819 0 comments 0 reactions 1 assignee Claimed by @azat View on GitHub
comp-mutations comp-simple-engines minor potential bug
Dominant language
C++
Stars
49.9k
Forks
9k
Avg merge
21h 32m
Merged PRs (30d)
515

Description

### Describe what's wrong

`ALTER TABLE ... DELETE` on a table with `ENGINE = Join` (and `ENGINE = Set`-family via the same base) is not crash-atomic. `StorageJoin::mutate` unlinks **every** persisted `.bin` file *before* the replacement file is renamed into place, and the replacement is written under `tmp/mut.bin`, which the load path never reads. If the server dies (SIGKILL / OOM / segfault — no power loss required) at any point between the first unlink and the final rename, the next startup silently loads whatever `.bin` files happened to survive (possibly none), and the survivor data staged in `tmp/mut.bin` is orphaned and ignored. The result is silent loss of previously-acked `INSERT` rows that the `DELETE` never even matched — up to the entire table — with no error and nothing in the log beyond the normal "Loaded from backup file" lines.

The relevant code is `StorageJoin::mutate` (`src/Storages/StorageJoin.cpp`):

```
setJoin(new_data);
increment = 1;
if (persistent)
{
backup_stream.flush();
compressed_backup_buf.finalize();
backup_buf->finalize();

std::vector files;
disk->listFiles(path, files);
for (const auto & file_name : files)
{
if (file_name.ends_with(".bin"))
disk->removeFileIfExists(path + file_name); // <-- destroys the only durable copies
}

disk->replaceFile(path + tmp_backup_file_name, path + std::to_string(increment) + ".bin"); // <-- replacement appears only after the whole delete loop
}
```

The reload path `StorageSetOrJoinBase::restore` (`src/Storages/StorageSet.cpp`) iterates only the top-level files of the table directory and skips the `tmp/` subdirectory, so `tmp/mut.bin` is never a load candidate:

```
for (auto dir_it{disk->iterateDirectory(path)}; dir_it->isValid(); dir_it->next())
{
...
if (disk->existsFile(file_path) && endsWith(name, file_suffix) && disk->getFileSize(file_path) > 0)
backup_files.push({file_num, file_path});
}
```

This is not the "no fsync at default settings" durability class: the unlinks are namespace operations that take effect immediately in the live kernel, so a plain process kill (not a power cut) is sufficient. It is also strictly worse than `MergeTree`, whose mutations never unlink the old part before the new part is renamed into place (and additionally keep the old part for `old_parts_lifetime`). A related exception-only variant exists with no crash at all: if `mutate` throws after `increment = 1` but before the rename, `increment` is left at `1` while the old files `2.bin..N.bin` remain on disk, so subsequent acked `INSERT`s reuse those numbers and `replaceFile` silently overwrites previously-acked data.

### Does it reproduce on the most recent release?

Yes, reproduces on 26.9.

### How to reproduce

- ClickHouse version: 26.9.1.1 (and the same code is present on current `master`).
- Single node, default settings (`persistent = 1` is the default for `Join`).

```sql
CREATE TABLE j (k UInt64, v UInt64) ENGINE = Join(ANY, LEFT, k);
-- 3000 separate INSERT statements so the table has 3000 persisted .bin files
INSERT INTO j VALUES (1, 1);
INSERT INTO j VALUES (2, 2);
...
INSERT INTO j VALUES (3000, 3000);

SELECT count() FROM j; -- 3000 (all acked)
```

Then issue a `DELETE` that matches a single row and kill the server during the mutation (a watcher that `SIGKILL`s the process the instant the `.bin` count in the table directory drops below 3000 lands reliably inside the window, because the rename to `1.bin` happens only after the whole delete loop finishes):

```sql
ALTER TABLE j DELETE WHERE k = 7; -- kill -9 the server while this runs
```

After restarting the server:

```sql
SELECT count() FROM j; -- observed: 2910 (89 acked rows the DELETE never matched are gone); the 2999 survivors staged in tmp/mut.bin are orphaned
```

A self-contained differential harness confirms it with two controls:

- CTRL-1 (run the `ALTER DELETE` to completion, no kill, restart) → `count() = 2999` (clean semantics hold).
- CTRL-2 (`SIGKILL` at steady state with no `ALTER` running, restart) → `count() = 3000` (a plain kill loses nothing; the loss is specific to the mutation window, not to the kill or the page cache).
- FAULT (`SIGKILL` during the `ALTER DELETE`, restart) → observed `count() = 2910` (the watcher landed the kill at a `.bin` count of 2960; 2910 files survived the unlink loop), with `tmp/mut.bin` orphaned on disk. The loss grows with how late in the unlink loop the crash lands — a crash just before the rename leaves zero `.bin` files and an empty table.

### Expected behavior

A crash at any point during `ALTER TABLE ... DELETE` on a `Join`/`Set` table must leave either the complete pre-mutation state or the complete post-mutation state. Rows that the `DELETE` did not match must never be lost. The mutation should publish its result atomically — for example by renaming the new file into place before unlinking the old files (and under a number the load path prefers), or by keeping the old files until the replacement is durable — rather than unlinking every durable copy and leaving the replacement under a `tmp/` name the reader ignores.

### Error message and/or stacktrace

No error and no exception. The table silently loads with missing rows. The only log lines are the normal `StorageSetOrJoinBase: Loaded from backup file ...` messages for whatever `.bin` files survived.

### Additional context

Reachable at default settings on a supported, non-deprecated path (`ALTER TABLE ... DELETE` is the documented mutation for the `Join` engine). The related concurrency variant — an `INSERT` whose sink outlives the start of a concurrent `ALTER ... DELETE` — can lose the acked insert without any crash, because `mutate` builds its survivor set from a snapshot and then resets `increment = 1` and deletes all `.bin` files without draining in-flight sinks; the `mutate_mutex` only serializes sink *creation*, not the sink's file publish.

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.