Fix KeeperMap DELETE processing only the first block of matched rows

[alexey-milovidov]

Closes: #112244
Related: #112438

In the DELETE branch of StorageKeeperMap::mutate, the per-block loop exited with return as soon as the multi-remove for the first block succeeded. As a result, a DELETE matching more than max_block_size rows (default 65409) deleted only the first block of matched rows, while the remaining matched rows were silently left in the table and system.mutations showed the mutation as done with no error. Both ALTER TABLE ... DELETE and the lightweight DELETE FROM are affected. The bug existed since DELETE/UPDATE support was added to KeeperMap (commit 454705b, v23.3).

The fix replaces return with continue, so the loop consumes every block pulled from the mutation pipeline — the same way the UPDATE branch below does.

The new test 04660_keeper_map_delete_multiple_blocks forces multiple blocks with SETTINGS max_block_size = 100 and covers ALTER TABLE ... DELETE, lightweight DELETE FROM, and keeper_map_strict_mode = 1. Verified locally: on an unfixed build, ALTER TABLE ... DELETE WHERE key < 750 on 1000 rows left 926 rows instead of 250, and DELETE FROM ... WHERE 1 left 826 rows instead of 0; with the fix, the results match the reference.

This reimplements #112438 (kudos to @waterWang for the fix) together with the regression test requested in its review; that pull request was closed without the test.

Additionally, the same DELETE branch broke the keeper_map_strict_mode contract. When the version-checked multi request built for a block failed with ZNONODE, the code fell back to removing every key of the block one by one with version -1, dropping the version checks captured by the mutation scan. If the block was read as k1@v0, k2@v0, another session removed k1 and updated k2 to v1, and the multi failed on k1, the fallback deleted the updated k2 from a stale snapshot while the mutation reported success — contradicting the documented guarantee that a strict-mode delete succeeds only if it can be executed atomically. In strict mode the conflict is now surfaced instead; the non-strict fallback is unchanged.

Strict mode was also not atomic across mutation blocks: each block was committed on its own, so a conflict detected in a later block failed the mutation after the earlier blocks had already been removed. In strict mode the remove requests of every block are now accumulated and sent as a single version-checked multi request once the scan is over, so the whole delete either applies or leaves the table untouched. Non-strict deletes keep the previous block-by-block behaviour and its one-by-one fallback.

Note that a strict-mode delete is now bounded by the maximum Keeper request size. Exceeding it fails the mutation without deleting anything, which is what the strict mode contract asks for.

The new test 04661_keeper_map_delete_strict_mode_conflict covers all of this. It uses a new keeper_map_delete_pause_before_multi failpoint to deterministically interleave a concurrent removal (and a concurrent update) between the block scan and the multi request, and it checks that a multi-block strict delete issues exactly one write multi request while the non-strict one issues more than one. Verified locally against builds with each fix disabled: without the strict ZNONODE check both race scenarios silently deleted the surviving row and reported success; with per-block strict commits the multi-block delete issued two write multi requests instead of one. All are caught with the fixes in place.

Changelog category (leave one):

• Bug Fix (user-visible misbehavior in an official stable release)

Changelog entry (a user-readable short description of the changes that goes into CHANGELOG.md):

Fix DELETE on KeeperMap tables deleting only the first max_block_size matched rows while reporting the mutation as successfully completed. Also fix DELETE with keeper_map_strict_mode = 1 not being atomic: it could fall back to unversioned removals and delete rows that were concurrently updated, and it applied the matched rows block by block so a conflict could leave the delete partially applied.

Version info

• Merged into: 26.8.1.657 (included in 26.8 and later)

[clickhouse-gh]

Workflow [PR], commit [fe57e3c]

Summary: ✅

• Performance Comparison: Performance dashboard

---

AI Review

Summary

This PR fixes KeeperMap DELETE mutations to process every matched block and restores the documented keeper_map_strict_mode atomicity guarantee by rejecting the unversioned fallback and issuing one version-checked multi request for the full strict delete. The current code matches the earlier discussion, the regression tests cover both the multi-block deletion bug and the strict-mode conflict cases, and I did not find a remaining contract violation in the changed surface.

Final Verdict

• Status: ✅ Approve

LLVM Coverage Report

Metric	Baseline	Current	Δ
Lines	86.40%	86.40%	+0.00%
Functions	91.90%	91.90%	+0.00%
Branches	78.70%	78.70%	+0.00%

Changed lines: Changed C/C++ lines covered: 46/48 (95.83%) · Uncovered code

Full report · Diff report

[clickhouse-gh]

LLVM Coverage Report

Metric	Baseline	Current	Δ
Lines	86.50%	86.50%	+0.00%
Functions	91.90%	91.90%	+0.00%
Branches	78.70%	78.70%	+0.00%

Changed lines: Changed C/C++ lines covered: 3/5 (60.00%) · Uncovered code

Full report · Diff report

[alexey-milovidov]

🕵 Addressed the multi-block atomicity blocker in a8a52e1.

In keeper_map_strict_mode the remove requests of every block are now accumulated and sent as a single version-checked multi request once the mutation scan is over, so the whole delete either applies or leaves the table untouched. Any failure of that request, ZNONODE included, is surfaced as is — retrying key by key would drop the version checks and skipping the failed keys would apply the delete partially. Non-strict deletes keep the previous block-by-block behaviour and its one-by-one fallback.

One consequence worth flagging: a strict-mode delete is now bounded by the maximum Keeper request size, and exceeding it fails the mutation without deleting anything. That is the fail-closed reading of "succeeds only if it can be executed atomically", and strict mode is opt-in and off by default, so I went with it rather than silently falling back to a non-atomic path.

On the test request: a failpoint pause before a later block does not actually discriminate, because PAUSEABLE_ONCE fires on the first multi under either design, and after this change strict mode issues only one multi at all. I verified that empirically — the earlier version of the later-block scenario passed against a per-block build. So 04661_keeper_map_delete_strict_mode_conflict now asserts the property directly instead: a multi-block strict delete (max_block_size = 100 over 200 rows) must issue exactly one write multi request, while the non-strict one issues more than one. Verified against a build with per-block strict commits restored: the strict count is 2 there instead of 1, so the check does catch the regression.

[alexey-milovidov]

🕵 Merged the latest master into the branch and re-ran CI.

The previous red was not caused by this PR:

• Stateless tests (arm_binary, parallel) — the whole job aborted with StopTesting: test run was stopped, and the earlier cause in the job log is the server disappearing, not a test assertion:

  [14:06:12] Hung check failed: server is not responding
  [14:06:12] Unable to locate any ClickHouse server process. It must have crashed or exited prematurely!
  [14:06:12] Stopping tests, terminating all processes...
  

  The only tests still in flight at that moment were 02835_drop_user_during_session.sh and 02481_async_insert_dedup.sh; no coredump, sanitizer report or Logical error was captured, and the job then sat until the 9000 s praktika timeout. This PR's own tests were unaffected — 04660_keeper_map_delete_multiple_blocks ran 3/3 [ OK ] under --repeat-newly-modified-tests, and 04661_keeper_map_delete_strict_mode_conflict never got to start before the abort.
• Finish Workflow / new_tests_check.py — it failed only because every per-arch Bugfix validation job came back DROPPED/SKIPPED, which is fallout of the aborted stateless job above rather than a verdict on the regression tests.

Both tests were verified locally against the freshly master-merged build (release, aarch64, embedded Keeper, keeper_map_path_prefix = /test_keeper_map):

[1 / 2] 04660_keeper_map_delete_multiple_blocks:                [ OK ] 2.06 sec.
[2 / 2] 04661_keeper_map_delete_strict_mode_conflict:           [ OK ] 4.25 sec.

@groeneai, investigate the failure: https://s3.amazonaws.com/clickhouse-test-reports/PRs/112777/a8a52e170d5655e7965e14167d0c886df00bc570/stateless_tests_arm_binary_parallel/job.log and provide a fix in a separate PR. If the fix is already in progress, link it here.

[groeneai]

I investigated that job. It is not caused by this PR, and it is not the Bad cast trunk bug from #112459 either (zero rows carrying that text for a8a52e17). The abort mechanics already have fixes open; the reason the server exited is not recoverable from what the job published.

The verdict is correct, the server was gone. "Unable to locate any ClickHouse server process" is printed on the branch where get_all_server_pids() returns empty, so the server had exited rather than stopped answering. Last test completion 04:05:03, verdict 04:06:12, a 69 s gap, and check_server_liveness burns 65 s of back-off before returning False.

The cause is unreadable, and that is the part worth fixing. Every server-side artifact for that job 403s: clickhouse-server.err.log and its .zst, clickhouse-server.log.zst, fatal.log, test_result.txt, dmesg.log, the lldb and SQL stacktrace logs. CIDB holds 1 row for the check where the passing run of the same check on this PR wrote 12189, and that row carries no Logical error, no Received signal and no sanitizer text. I checked all six aborts of this check in the last 7 days including both on master (d12ae720fe31, d0fc7f568ea7): not one publishes a server log. Host memory is ruled out by the job's own metrics (peak 46.2% of 61.65 GB, server RSS falling from 17.5 GB to 2.6 GB across the death).

Two open PRs cover the mechanics:

• Do not let an orphaned test process wedge the functional-test pipeline #112265 explains the 2 h 19 m of silence between StopTesting (14:06:18) and praktika's kill (16:25:54), and why the job then publishes results: [] with every log unreachable. Your log holds the direct evidence: at the verdict the wrapper shells of 02835_drop_user_during_session.sh (pgid 504738) and 02481_async_insert_dedup.sh (pgid 510246) are still alive in their own process groups, holding the runner's stdout, so ts and tee never see EOF. The 139.6 min here sits inside the 71.8 to 142.4 min band measured there.
• Throttle the clickhouse-test hung-check liveness probe so it stops starving the parent loop #112393 covers the probe cost above (65 s budget against the 69 s gap). It does not make the verdict more tolerant, and this run is one of the cases where there genuinely is no server left.

I would rather not open a third PR guessing at the server exit itself: with no crash text, no core and no server log anywhere in this family, anything I changed would be speculative. Once #112265 lands, the next occurrence keeps its logs and I will take the cause from there.

Report: https://s3.amazonaws.com/clickhouse-test-reports/json.html?PR=112777&sha=a8a52e170d5655e7965e14167d0c886df00bc570&name_0=PR&name_1=Stateless%20tests%20%28arm_binary%2C%20parallel%29