Enable setting max_threads for freeze queries#287
Open
MikhailBurdukov wants to merge 4 commits intomainfrom
Open
Enable setting max_threads for freeze queries#287MikhailBurdukov wants to merge 4 commits intomainfrom
MikhailBurdukov wants to merge 4 commits intomainfrom
Conversation
Reviewer's GuideThis PR extends the freeze workflow to optionally parallelize Sequence diagram for freeze workflow with partition and ClickHouse-level parallelizationsequenceDiagram
participant BackupLogic as BackupLogic
participant FreezePool as FreezeThreadPool
participant Context as BackupContext
participant ChCtl as ClickhouseControl
participant ClickHouse as ClickHouseServer
BackupLogic->>FreezePool: submit _freeze_table(table, backup_name, schema_only, parallelize_freeze_in_ch, freeze_partition_threads, freeze_table_query_max_threads)
FreezePool->>BackupLogic: run _freeze_table
BackupLogic->>Context: get_ch_ctl
BackupLogic->>ChCtl: freeze_table(backup_name, table, parallelize_freeze_in_ch, freeze_partition_threads, freeze_table_query_max_threads)
alt parallelize_freeze_in_ch is false and freeze_partition_threads > 0 and table_has_PARTITION_BY
ChCtl->>ChCtl: list_partitions(table)
ChCtl->>FreezePool: ThreadExecPool(freeze_partition_threads)
loop for_each_partition
ChCtl->>ClickHouse: ALTER TABLE FREEZE PARTITION settings max_threads=1, optional_max_execution_time
end
ChCtl->>FreezePool: wait_all
else use ClickHouse internal parallelization
ChCtl->>ChCtl: build FREEZE_TABLE query
opt ch_version_ge_25_11
ChCtl->>ChCtl: set_query_setting_max_threads=freeze_table_query_max_threads
end
ChCtl->>ClickHouse: ALTER TABLE FREEZE settings max_threads, optional_max_execution_time
end
ClickHouse-->>ChCtl: freeze_completed
ChCtl-->>BackupLogic: freeze_table_return
BackupLogic-->>FreezePool: _freeze_table_completed
Updated class diagram for freeze configuration and controlclassDiagram
class ConfigDefaults {
+int freeze_threads
+bool parallelize_freeze_in_clickhouse
+int freeze_partition_threads
+int freeze_table_query_max_threads
}
class Table {
+string database
+string name
+string create_statement
+bool is_merge_tree()
}
class ClickhouseControl {
-Dict ch_ctl_config
-int _freeze_timeout
+bool ch_version_ge(version)
+kill_old_freeze_queries()
+freeze_table(backup_name, table, parallelize_freeze_in_ch, freeze_partition_threads, clickhouse_query_max_threads) void
+list_partitions(table) List~string~
}
class BackupContext {
+ClickhouseControl ch_ctl
}
class BackupLogic {
+_backup(context, tables, multiprocessing_config) void
+_freeze_table(context, table, backup_name, schema_only, parallelize_freeze_in_ch, freeze_partitions_threads, freeze_table_query_max_threads) Table
}
ConfigDefaults <.. BackupLogic : uses_config
BackupLogic o-- BackupContext : has_context
BackupContext o-- ClickhouseControl : has
BackupLogic --> Table : freezes
ClickhouseControl --> Table : operates_on
ClickhouseControl --> ConfigDefaults : reads_user
Flow diagram for freeze_table parallelization decisionflowchart TD
A[start_freeze_table] --> B{parallelize_freeze_in_ch}
%% Branch: do not parallelize in ClickHouse, use partition threads
B -- false --> C{freeze_partition_threads > 0
and has_PARTITION_BY}
C -- true --> D[set_freeze_by_partitions_true]
C -- false --> E[set_freeze_by_partitions_false]
D --> F[init_query_settings_with_max_threads_1]
F --> G[if ch_version_ge_25_2
then_add_max_execution_time]
G --> H[list_partitions]
H --> I[start_ThreadExecPool_with_freeze_partition_threads]
I --> J[submit_FREEZE_PARTITION_queries]
J --> K[wait_all]
K --> L[return_from_freeze_table]
E --> M[init_query_settings_with_max_threads_1]
%% Branch: parallelize in ClickHouse
B -- true --> N[init_query_settings_with_max_threads_1]
N --> O{ch_version_ge_25_2}
O -- true --> P[add_max_execution_time]
O -- false --> Q[keep_query_settings_as_is]
P --> R{ch_version_ge_25_11}
Q --> R
R -- true --> S[set_max_threads_to_clickhouse_query_max_threads]
R -- false --> T[log_warning_parallel_freeze_requires_25_11]
S --> U[build_FREEZE_TABLE_query]
T --> U
%% Common table-freeze path (no partition parallelization)
M --> V{ch_version_ge_25_2}
V -- true --> W[add_max_execution_time]
V -- false --> X[keep_query_settings_as_is]
W --> U
X --> U
U --> Y[execute_FREEZE_TABLE_via_ch_client]
Y --> Z[end_freeze_table]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 3 issues, and left some high level feedback:
- In
freeze_table,query_settingsis first initialized with{"max_threads": 1}and then overwritten to{"max_execution_time": ...}for CH >= 25.2, which drops themax_threadsdefault—consider merging settings instead of overwriting so both constraints apply. - The
_freeze_tablefunction uses the parameter namefreeze_partitions_threadswhile the upstream call site and config key usefreeze_partition_threads, which is easy to confuse—align the naming to a single, consistent form. - There are a few typos in new comments (e.g. 'clould' and minor wording around the freeze parallelization explanation); tightening this up will make the configuration intent clearer for future readers.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `freeze_table`, `query_settings` is first initialized with `{"max_threads": 1}` and then overwritten to `{"max_execution_time": ...}` for CH >= 25.2, which drops the `max_threads` default—consider merging settings instead of overwriting so both constraints apply.
- The `_freeze_table` function uses the parameter name `freeze_partitions_threads` while the upstream call site and config key use `freeze_partition_threads`, which is easy to confuse—align the naming to a single, consistent form.
- There are a few typos in new comments (e.g. 'clould' and minor wording around the freeze parallelization explanation); tightening this up will make the configuration intent clearer for future readers.
## Individual Comments
### Comment 1
<location> `ch_backup/clickhouse/control.py:628-631` </location>
<code_context>
+ and "PARTITION BY" in table.create_statement
+ )
+
+ query_settings: Dict[str, Any] = {"max_threads": 1}
- query_settings = None
# Since https://github.com/ClickHouse/ClickHouse/pull/75016
if self.ch_version_ge("25.2"):
query_settings = {"max_execution_time": self._freeze_timeout}
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Avoid overwriting `max_threads` when adding `max_execution_time` for newer ClickHouse versions.
Because `query_settings` is first set to `{"max_threads": 1}` and then replaced with `{"max_execution_time": ...}` when `ch_version_ge("25.2")` is true, `max_threads` is silently dropped for newer versions, despite the later logic that expects a default. Instead, keep the initial dict and add `max_execution_time` to it so both settings are preserved unless explicitly changed.
</issue_to_address>
### Comment 2
<location> `ch_backup/logic/table.py:195-197` </location>
<code_context>
backup_name: str,
table: Table,
- threads: int,
+ parallelize_freeze_in_ch: bool,
+ freeze_partition_threads: int,
+ clickhouse_query_max_threads: int,
</code_context>
<issue_to_address>
**suggestion:** Align naming of `freeze_partitions_threads` parameter with the existing `freeze_partition_threads` configuration key.
The `_freeze_table` parameter is named `freeze_partitions_threads` (plural), but both the config key and the `_backup` call site use `freeze_partition_threads` (singular). Even though it works now, this mismatch makes future refactors more error-prone. Please rename the parameter to `freeze_partition_threads` for consistency with the config and existing API.
</issue_to_address>
### Comment 3
<location> `ch_backup/config.py:170` </location>
<code_context>
"cloud_storage_restore_workers": 4,
# The number of threads for parallel freeze of tables
"freeze_threads": 4,
+ # To execute freeze efficiently we should parallelize freeze operations. But we got options where we clould parallelize:
+ # 1) Inside ch-backup: We can perform several `ALTER TABLE FREEZE PARTITION` queries. Then freeze_partition_threads are used.
+ # 2) Inside clickhouse(preferable): Since 25.11 clickhouse can parallelize `ALTER TABLE FREEZE` queries. Then freeze_table_query_max_threads are used.
+ # https://github.com/ClickHouse/ClickHouse/pull/71743
+ "parallelize_freeze_in_clickhouse": False,
# The number of threads for parallel freeze of partitions. If set to 0, will freeze table in one query.
</code_context>
<issue_to_address>
**nitpick (typo):** Fix small typos in the configuration comment for clarity and professionalism.
In particular, update "we clould parallelize" to fix the typo and adjust the sentence so it reads more smoothly (for example, "we can parallelize" or "where we could parallelize").
```suggestion
# To execute freeze efficiently, we should parallelize freeze operations. We have two options where we can parallelize:
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
aalexfvk
reviewed
Feb 2, 2026
ch_backup/logic/table.py
Outdated
| schema_only: bool, | ||
| freeze_partition_threads: int, | ||
| parallelize_freeze_in_ch: bool, | ||
| freeze_partitions_threads: int, |
Contributor
There was a problem hiding this comment.
Suggested change
| freeze_partitions_threads: int, | |
| freeze_partition_threads: int, |
| # Table has no partitions or created with deprecated syntax. | ||
| # FREEZE PARTITION ID with deprecated syntax throws segmentation fault in CH. | ||
| freeze_by_partitions = threads > 0 and "PARTITION BY" in table.create_statement | ||
| freeze_by_partitions = ( |
Contributor
There was a problem hiding this comment.
How about if freeze_partition_threads > 0 set it to max_threads for >=25.11 otherwise fallback to freezing by partitions. Without special parallelize_freeze_in_ch setting.
Contributor
Author
There was a problem hiding this comment.
I don't really like it because:
- We will lose the ability to disable the new behaviour. We haven't use it in prod environment yet. Therefore, in my opinion, it is better to keep the possibility of enabling freezes in ch-backup.
- Knobs naming problems: maybe it nitpicking but... AFAIK freezes in the ch got part granularity, but the name is
freeze_partition_threads, so renaming or aliases required
Maybe it is overthinking, but I like current solution more.
Contributor
There was a problem hiding this comment.
Ok, it makes sense. Let's make parallelize_freeze_in_clickhouse: True by default
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by Sourcery
Allow configuring how ClickHouse table freeze operations are parallelized, including support for ClickHouse-level parallel freeze threads.
New Features:
Enhancements:
Tests: