Skip to content

Fix/cache disk thread pool size#101712

Merged
Michicosun merged 1 commit intoClickHouse:masterfrom
Milias:fix/cache-disk-thread-pool-size
Apr 15, 2026
Merged

Fix/cache disk thread pool size#101712
Michicosun merged 1 commit intoClickHouse:masterfrom
Milias:fix/cache-disk-thread-pool-size

Conversation

@Milias
Copy link
Copy Markdown
Contributor

@Milias Milias commented Apr 3, 2026

Description

Hello, I noticed that when using a S3 cached disk the setting thread_pool_size crashes ClickHouse on startup. This is a relatively minor change, just pass through that setting.

First time contributing to this repo so please let me know if you would like me to change something!

Thank you :)

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):

Fixes a crash when thread_pool_size is configured on a cache-wrapped disk. Previously, FileCacheSettings::loadFromConfig() rejected thread_pool_size as
an unknown setting, preventing the server from starting. The setting is a valid IDisk parameter that controls the number of threads used for disk-to-disk
copy operations during background part moves.

Documentation entry for user-facing changes

  • Documentation is written (mandatory for new features)

@Michicosun Michicosun added can be tested Allows running workflows for external contributors labels Apr 3, 2026
@Michicosun Michicosun self-assigned this Apr 3, 2026
@clickhouse-gh
Copy link
Copy Markdown
Contributor

clickhouse-gh Bot commented Apr 3, 2026

Workflow [PR], commit [4310b07]

Summary:

job_name test_name status info comment
Stateless tests (arm_asan_ubsan, targeted) failure
02789_filesystem_cache_alignment FAIL cidb
02789_filesystem_cache_alignment FAIL cidb
02789_filesystem_cache_alignment FAIL cidb
02789_filesystem_cache_alignment FAIL cidb

AI Review

Summary

This PR fixes cache-disk config parsing by excluding IDisk-level keys (notably thread_pool_size) from FileCacheSettings parsing in both loadFromConfig and loadFromCollection, and adds a stateless regression test. I did not find remaining correctness, safety, performance, or compatibility issues in the current diff, and the fix appears complete for both config paths.

ClickHouse Rules
Item Status Notes
Deletion logging
Serialization versioning
Core-area scrutiny
No test removal
Experimental gate
No magic constants
Backward compatibility
SettingsChangesHistory.cpp
PR metadata quality
Safe rollout
Compilation time
No large/binary files
Final Verdict
  • Status: ✅ Approve

@clickhouse-gh clickhouse-gh Bot added the pr-bugfix Pull request with bugfix, not backported by default label Apr 3, 2026
@Milias
Copy link
Copy Markdown
Contributor Author

Milias commented Apr 4, 2026

I'm not sure if the failed test is wobbly or related to this change, honestly. If it is somehow related I will try to fix it!

@Michicosun
Copy link
Copy Markdown
Member

Michicosun commented Apr 4, 2026

I'm not sure if the failed test is wobbly or related to this change, honestly. If it is somehow related I will try to fix it!

@Milias Hi! No worries - this isn’t related to your changes

@alexey-milovidov
Copy link
Copy Markdown
Member

The Stress test (arm_msan) failure is fixed by #101239, which should be merged first. After it is merged, please update the branch to include the fix.

Comment thread src/Interpreters/Cache/FileCacheSettings.cpp Outdated
@Milias
Copy link
Copy Markdown
Contributor Author

Milias commented Apr 7, 2026

Thanks @Michicosun @alexey-milovidov!

See reply for the last change. Let me know if you would rather just leave it as is was, without the setting changing anything.

Comment thread tests/queries/0_stateless/04077_cache_disk_thread_pool_size.sql Outdated
Comment thread src/Disks/DiskObjectStorage/RegisterDiskCache.cpp Outdated
@clickhouse-gh clickhouse-gh Bot added the manual approve Manual approve required to run CI label Apr 7, 2026
@alexey-milovidov
Copy link
Copy Markdown
Member

The MSan stress test failure (MemorySanitizer: use-of-uninitialized-value, STID 4179-5154 or 4148-3044) is a known pre-existing issue unrelated to this PR. Fix: #102158

…on cache-wrapped disks

Pass `thread_pool_size` through as a non-cache key in both
`loadFromConfig` and `loadFromCollection`, so it reaches the IDisk layer
instead of being rejected as UNKNOWN_SETTING.

Rebased onto master after the Cache → FileCache directory rename and
renumbered the test from 04077 to 04100.

PR: ClickHouse#101712

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@Milias Milias force-pushed the fix/cache-disk-thread-pool-size branch from c33439a to 4310b07 Compare April 15, 2026 12:17
@clickhouse-gh
Copy link
Copy Markdown
Contributor

clickhouse-gh Bot commented Apr 15, 2026

LLVM Coverage Report

Metric Baseline Current Δ
Lines 84.10% 84.00% -0.10%
Functions 90.90% 90.90% +0.00%
Branches 76.60% 76.50% -0.10%

Changed lines: 90.00% (9/10) · Uncovered code

Full report · Diff report

@Michicosun Michicosun added this pull request to the merge queue Apr 15, 2026
Merged via the queue into ClickHouse:master with commit 0164da0 Apr 15, 2026
159 of 161 checks passed
@robot-clickhouse robot-clickhouse added the pr-synced-to-cloud The PR is synced to the cloud repo label Apr 15, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

can be tested Allows running workflows for external contributors manual approve Manual approve required to run CI pr-bugfix Pull request with bugfix, not backported by default pr-synced-to-cloud The PR is synced to the cloud repo

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants