Workflow [PR], commit [c16bef9]

Summary: ❌

• Performance Comparison: Performance dashboard

AI Review

Summary

This PR batches clearBlocksInPartition ZooKeeper removals so TRUNCATE TABLE / DROP PARTITION no longer build one oversized Keeper multi request, and it adds a regression test around the dedup-block cleanup. The batching logic looks reasonable, but the current test still does not isolate the specific TRUNCATE it is trying to validate, so the evidence for the bug fix is still unreliable.

Findings

⚠️ Majors

• [tests/queries/0_stateless/4061_truncate_many_dedup_blocks.sh:59] [dismissed in thread -- https://github.com/ClickHouse/ClickHouse/pull/105991#discussion_r3512391157] The final system.zookeeper_log assertion is still global to this table path. If a dirty rerun makes the opening DROP TABLE IF EXISTS clean up leftover truncate_many_dedup* tables, those earlier Remove requests hit the same regex and contaminate count() > 1 / max(removals_per_multi) <= 100. The test can then pass or fail based on unrelated cleanup instead of the TRUNCATE executed by this test run.
  Suggested fix: pin the TRUNCATE to a fixed query_id (or capture a cutoff timestamp immediately before it) and filter system.zookeeper_log to that query_id / time window in the assertion.

Final Verdict

• Status: ⚠️ Request changes
• Scope the system.zookeeper_log check to the exact TRUNCATE under test so the regression test proves this PR rather than leftover cleanup from a rerun.

@Michicosun thanks for the review — both points are addressed:

1. Deterministic batching assertion. The test is now 4061_truncate_many_dedup_blocks.sh, issuing 120 separate INSERTs (one dedup block node each, > MULTI_BATCH_SIZE = 100) and asserting via system.zookeeper_log that the Remove sub-requests were split across more than one multi, with no multi exceeding 100 ops. Pre-fix prints 0 0 (fails), fixed prints 1 1, independent of jute.maxbuffer. (commits 71d9ebe, 43dd935)
2. Parallelism. clearBlocksInPartition now keeps up to 16 batches in flight via asyncTryMultiNoThrow instead of running them strictly sequentially. (commit 71d9ebe)

I also restored the original failure contract after the async switch (re-throw on non-user/hardware Keeper errors) in 52dd789.

Could you take another look when you have a chance?

On the failing CI

• Stateless tests (arm_asan_ubsan, azure, sequential) — unrelated flaky crash. The segfault is in the object-storage metadata write path (MetadataStorageFromDiskTransactionOperations → WriteBufferFromFile dtor → ASan free) on a background INSERT/merge thread ((no query)), on the azure+ASan build. The new test 4061_truncate_many_dedup_blocks does not appear anywhere in that shard's report or server log, and it isn't scheduled in the sequential shard at all. This looks like infra/flaky and should clear on a re-run.
• AI Review — other dedup-cleanup callers. Acknowledged in the inline thread; proposing to keep this PR scoped to TRUNCATE/DROP PARTITION and handle dropPartImpl / shard-move paths in a follow-up, since those commit removals atomically with their log entry and need a separate design.

@Michicosun thanks for the review — all three points addressed in 7a69e5e:

1. Log only the first failed op. Now uses zkutil::getFailedOpIndex(response.error, response.responses) and logs just that op's path/error, instead of iterating all sub-responses (the rest carry ZRUNTIMEINCONSISTENCY).
2. Removed the failure-contract comment.
3. Trimmed the batching comment to a single line.

The failure contract is unchanged: non-user (hardware) errors still propagate via KeeperException; expected per-op user errors (e.g. ZNONODE) are logged and ignored. Could you take another look when you have a chance?

LLVM Coverage Report

Changed lines: Changed C/C++ lines covered: 27/33 (81.82%) · Uncovered code

Full report · Diff report

The test does not reproduce the bug with ClickHouse Keeper.
It looks only relevant to ZooKeeper. I will change the category to "Improvement".

Found via ClickGap automated review. Please close or comment if this is incorrect or needs adjustment.

TL;DR

Batch ZooKeeper multi-requests in clearBlocksInPartition

ClickGap verified: reproduced it with the script below on 26.5; tested 6 version(s) — 5 affected, 1 clean (see Affects).

Reproduction

On a Keeper/ZooKeeper-backed ClickHouse server:
CREATE TABLE t (k UInt64) ENGINE = ReplicatedMergeTree('/clickhouse/tables/t','r1') ORDER BY k SETTINGS replicated_deduplication_window = 10000;
-- Run >100 SEPARATE inserts so >100 deduplication-block nodes exist in ZooKeeper:
INSERT INTO t VALUES (1); INSERT INTO t VALUES (2); /* ... repeat to >=150 ... */
TRUNCATE TABLE t SETTINGS replication_alter_partitions_sync = 2;
SYSTEM FLUSH LOGS zookeeper_log;
SELECT count() AS multis, max(c) AS max_removes_per_multi
FROM (SELECT count() AS c FROM system.zookeeper_log
      WHERE type='Request' AND op_num='Remove'
        AND match(path, '/clickhouse/tables/t/(blocks|async_blocks|deduplication_hashes)/')
      GROUP BY session_id, xid);
-- Pre-fix: 1 multi with >100 Remove ops -> can exceed jute.maxbuffer (1MB) and TRUNCATE/DROP PARTITION fails.
-- Fixed:   removals split into several multis of <=100 ops each.

Bisect

• Bisected on master in several steps (confidence: MEDIUM); a single introducing PR could not be pinned (culprit confirmation was inconclusive).

Affects

• ✓ Fix is on master — this PR's change fixes the bug on master. Branches below still have the bug and need the backport.
• Reproduces on: 26.5, 26.4, 26.3, 26.2, 26.1
• Does NOT reproduce on: master
• Backport needed: 26.5, 26.4, 26.3, 26.2, 26.1

Suggested next step

The fix has landed on master; the affected release branches above still need the backport.

Component: comp-replication
Severity: P2

cc fix PR reviewer: @Michicosun

ClickGap · Finding: phase_d_pr105991

Triage metadata

• Primary component: comp-replication
• Secondary components: comp-testing
• Component owners (comp-replication): @alexey-milovidov @CheSema