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. @azat You might be interested in this one.

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 [94e523f]

[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.