25.8 Stable backport of #91574: Add cleanup thread to MergeTree to avoid cleanup starvation

[ilejn]

This is just a try to backport this PR.

Changelog category (leave one):

• Improvement

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

Add a dedicated cleanup thread for MergeTree to prevent cleanup delays under heavy merge load. This resolves ClickHouse#86181.

The MergeTree cleanup logic was tightly coupled with the merge scheduler, which caused cleanup tasks to be delayed or completely blocked when the table was under heavy merge load. ReplicatedMergeTree already had a separate cleanup thread, so this change aligns MergeTree with that design.
(ClickHouse#91574 by amosbird)

CI/CD Options

Exclude tests:

• Fast test
• Integration Tests
• Stateless tests
• Stateful tests
• Performance tests
• All with ASAN
• All with TSAN
• All with MSAN
• All with UBSAN
• All with Coverage
• All with Aarch64
• All Regression
• Disable CI Cache

Regression jobs to run:

• Fast suites (mostly <1h)
• Aggregate Functions (2h)
• Alter (1.5h)
• Benchmark (30m)
• ClickHouse Keeper (1h)
• Iceberg (2h)
• LDAP (1h)
• Parquet (1.5h)
• RBAC (1.5h)
• SSL Server (1h)
• S3 (2h)
• S3 Export (2h)
• Swarms (30m)
• Tiered Storage (2h)

[github-actions]

Workflow [PR], commit [c793dbe]

[ilejn]

Review notes

commit b0e1bc6
look_behind_lines in test_merge_tree_load_parts

In upstream the equivalent change is done by ea67e82 from ClickHouse#87719 , but it is not directly applicable to 25.8

commit 9f01fd8
Hardening tests

Fixes fluky check. Actually not related to the backport: if test files are altered artificially, e.g. via adding fake comments, we have same issue.

commit 79107eb
Author: Mikhail Artemenko mikhail.artemenko@clickhouse.com
fix integration tests

Some good test fixes

commit 792180a
Call clearCaches

The trickiest change. Fixes failures in e.g. 03174_exact_rows_before_aggregation.
Inspired by ClickHouse#97505
Master does not have this code because QueryPlanResourceHolder keeps storage_holders alive until the query pipeline is destroyed and it is Ok to have late clearCaches().
I did not dare to backport this logic to 25.8

commit 02fecfe
Author: Azat Khuzhin a3at.mail@gmail.com
tests: make test test_merge_tree_load_parts retriable

Some required test fixes

commit 2643af8
Author: Azat Khuzhin a3at.mail@gmail.com
Merge pull request ClickHouse#91574 from amosbird/merge-tree-clean-up-thread

Actual backport with some rather significant changes. In particular in ReplicatedMergeTreeCleanupThread area, e.g. NodeCacheEntry.

[ilejn]

Same test failures are observed in #1817 , although there were no failures before rebase.

[mkmkme]

Overall LGTM! Please fix the comments I left on the backport commit and I'll approve :)

[CarlosFelipeOR]

AI audit note: This review comment was generated by AI (gpt-5.3-codex).

Audit update for PR #1589 (cleanup-thread backport for MergeTree / ReplicatedMergeTree):

Confirmed defects

• Low: storage_merge_tree_background_schedule_merge_fail does not model schedule failure correctly
  Impact: failpoint-based tests can report a scheduling failure while merge/mutation tasks were already enqueued and may execute, reducing test fidelity and making starvation simulations unreliable.
  Anchor: src/Storages/StorageMergeTree.cpp / StorageMergeTree::scheduleDataProcessingJob
  Trigger: enable storage_merge_tree_background_schedule_merge_fail when assignee.scheduleMergeMutateTask returns true.
  Why defect: the failpoint flips scheduled = false after task submission; it changes return value only, not actual scheduling side effects.
  Fix direction (short): inject failpoint before enqueue (or cancel/remove queued task when failpoint fires).
  Regression test direction (short): assert no merge task starts under failpoint, not just returned boolean.

• Low: incorrect points per minute diagnostic calculation
  Impact: cleanup throughput telemetry is severely underreported, making runtime tuning/debugging of cleanup cadence misleading.
  Anchor: src/Storages/MergeTree/IMergeTreeCleanupThread.cpp / IMergeTreeCleanupThread::run
  Trigger: any cleanup iteration with nonzero interval_ms logs points per minute.
  Why defect: expression changed to cleanup_points / (interval_ms * 60000) instead of (cleanup_points / interval_ms) * 60000.
  Fix direction (short): restore unit conversion order to compute per-minute rate correctly.
  Regression test direction (short): add unit test for throughput formula with fixed cleanup_points/interval_ms.

Coverage summary

• Scope reviewed: core C++ changes in cleanup-thread refactor and integration points (IMergeTreeCleanupThread, MergeTreeCleanupThread, ReplicatedMergeTreeCleanupThread, StorageMergeTree), plus related part-cache lifetime changes.
• Categories failed: failpoint fault-injection contract, diagnostics/math consistency.
• Categories passed: shared-state synchronization review, lock-order/deadlock pass over new paths, exception/rollback flow in cleanup loop, cache/lifetime hardening around outdated-part transitions.
• Assumptions/limits: static audit only (no runtime execution); review focused on changed C++ behavior, not full integration-test script semantics.

[ilejn]

@CarlosFelipeOR

Both mentioned defects seem

1. valid
2. not specific for backport, IOW exist in original PR.

So,

1. I need some extra time to double check
2. If you can run the check against original PR - would be great.

Presumably I don't think that we have anything to do here.

[CarlosFelipeOR]

✅ QA Verification: PASS — no regression attributable to this PR

Backport of upstream #91574 (MergeTree dedicated cleanup thread) to stable-25.8. Verified on the PR binary and CI run 28942353268 (sha 94e523f).

Functional verification (manual, PR release binary):

• Failpoint storage_merge_tree_background_schedule_merge_fail present; SYSTEM STOP/START CLEANUP works on plain MergeTree.
• Differential — with SYSTEM STOP CLEANUP active, outdated parts are held (2) on the fixed binary vs cleaned (0) without the fix; START CLEANUP releases them → 0. Confirms cleanup is decoupled from the merge scheduler and controllable.
• Clean shutdown with cleanup thread active and with SYSTEM STOP CLEANUP active: both shut down cleanly in ~1s, no hang in cleanup_thread.stop(), no crash/Fatal.

Added/edited tests — all passing:

• New 03742_clean_up_thread_for_merge_tree: green on all build types including all sanitizers (asan/tsan/msan/ubsan) and the flaky check.
• Edited stateless (00652, 01560, 02421, 03008, 03198, 03305_mutations_counters, 03305_rename_mutations_counter, 03357) and integration (test_merge_tree_s3, test_disk_configuration, test_merge_tree_load_parts, test_alter_moving_garbage, test_join_set_family_s3, test_log_family_s3, test_replicated_s3_zero_copy_drop_partition, test_s3_plain_rewritable, test_s3_with_https): all OK, zero FAIL/BROKEN.

Diff-check vs upstream ClickHouse#91574:

• Faithful port; all divergences documented by the author. The extra clearCaches change (25.8-specific UAF prevention, since 25.8 lacks the master-side QueryPlanResourceHolder lifetime guarantee) is the trickiest part — covered by the green sanitizer runs above.
• One upstream test (03745_system_background_schedule_pool) not backported: depends on the system.background_schedule_pool table absent in 25.8; introspection-only, not the fix's validation (03742 is).

CI failure triage:

• All 4 Integration job failures are solely test_mysql_protocol/test.py::test_mysql_dotnet_client — a known issue fixed upstream by #105636, backported to 25.8.25.7 (also 26.6.1.96, 26.5.2.3, 26.4.4.8, 26.3.13.4). Not related to this PR.
• Remaining stateless failures are pre-existing flaky (each failed a single job and passed on 10–11 other jobs in the same run) or known-BROKEN tests.

Build (amd_release): In the analyzed run it failed because it used the wrong runner. #2026 fixed the runner for release build jobs; the latest run includes that fix and the release build passes. This build feeds only packaging/compat/docker jobs — no stateless/integration/regression test depends on it, so it did not affect functional coverage.

Verdict

No regression introduced. The fix works and is controllable, shutdown is clean, all new/edited tests pass across build types (incl. sanitizers), and all CI failures are either an already-fixed upstream issue or pre-existing flaky/broken tests. Approved from QA side.

[mkmkme]

Antalya 25.8 is covered in #2059