Fix max_execution_time not being applied for backup/restore by kssenii · Pull Request #99205 · ClickHouse/ClickHouse

kssenii · 2026-03-10T15:41:53Z

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

Fix max_execution_time not being applied for backup/restore.

Documentation entry for user-facing changes

Documentation is written (mandatory for new features)

Note

Medium Risk
Changes when/where BACKUP/RESTORE query settings are applied, which can affect timeouts/limits and cancellation behavior for long-running backup/restore operations. Adds new failpoints used by integration tests; low runtime impact unless explicitly enabled.

Overview
Fixes BACKUP/RESTORE query SETTINGS handling so core execution limits (e.g. max_execution_time=0) are applied to the query context before the ProcessList entry and cancellation checker are created, preventing async backups/restores from being cancelled using profile-level timeouts.

Adds pauseable failpoints (backup_pause_on_start, restore_pause_on_start) and an integration regression test that stalls async backup/restore on a node with a short profile timeout to verify the timeout-disable setting is honored.

^{Written by Cursor Bugbot for commit f36922f. This will update automatically on new commits. Configure here.}

cursor

Cursor Bugbot has reviewed your changes and found 1 potential issue.

^{Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.}

clickhouse-gh · 2026-03-10T15:46:37Z

Workflow [PR], commit [f36922f]

Summary: ✅

AI Review

1) Summary

This PR fixes BACKUP/RESTORE query-setting propagation so core settings (notably max_execution_time) are applied before ProcessList registration, and removes duplicate late application in BackupsWorker; it also adds targeted integration coverage with failpoints. High-level verdict: change is directionally correct and matches the reported root cause, with no clear correctness/safety regressions found in touched code.

2) Missing context (if any)

No full Praktika artifact logs were reviewed from this environment (only current check statuses).
No performance benchmark artifacts were provided for backup/restore startup path (expected impact appears negligible).

3) Findings (by severity)

No Blockers or Majors identified.
PR looks good from a correctness/safety standpoint for the touched paths.

4) Tests & Evidence

Positives: new integration test test_async_backup_restore_with_max_execution_time_zero validates both async BACKUP and async RESTORE under profile timeout pressure, directly exercising the original failure mode.
Negative/error-handling coverage: the new test validates non-failure under prior cancellation conditions; explicit invalid-setting/constraint-failure cases are not added in this PR.
Optional follow-up tests: add one case with constrained user profile to verify constraint rejection still occurs when backup/restore SETTINGS contains forbidden core settings; add one ON CLUSTER variant to confirm identical early-setting behavior on distributed path.

5) ClickHouse Compliance Checklist (Yes/No + short note)

Data deletions logged? Yes (no new deletion path added).
Serialization formats versioned? Yes (no serialization changes).
Experimental setting gate present? N/A (bug fix, not new feature).
Settings exposed for constants/thresholds? N/A (no new magic thresholds in server code).
Backward compatibility preserved? Yes (behavior aligns with explicit query settings semantics).
SettingsHistory.cpp updated for new/changed settings? N/A (no settings definitions changed).
Existing tests untouched (only additions)? Yes (adds test/config; no test relaxation).
Docs/user-facing notes updated? N/A for this targeted bug fix.
Core-area change got extra scrutiny? Yes (InterpreterSetQuery + backup worker lifecycle reviewed).

6) Performance & Safety Notes

Hot-path impact is minimal: one additional settings extraction/apply step during query setup for ASTBackupQuery, while duplicate application in worker constructors was removed.
Memory/concurrency: no new shared mutable state or lock-order changes introduced; failpoints are test-oriented and gated by explicit enable commands.
Benchmarks: none provided; if needed, run a minimal benchmark comparing async backup startup latency before/after with and without SETTINGS max_execution_time=0.

7) User-Lens Review

Behavior becomes more intuitive: explicit query-level SETTINGS for BACKUP/RESTORE now affect timeout/cancellation from query start, consistent with user expectation.
Error/actionability remains adequate; no regression in user-visible diagnostics observed in touched code.

8) Final Verdict

Status: Approve
Minimum required actions: none.

pamarcos

I left some minor comment here of a tiny improvement we could make, but I'm approving the PR as it is cause I don't think it's worth blocking just because of that

pamarcos · 2026-03-10T16:47:45Z

Thanks for fixing this, @kssenii 🫂

vitlibar · 2026-03-10T17:35:49Z

+            if (!core_settings.empty())
+            {
+                context_->checkSettingsConstraints(core_settings, SettingSource::QUERY);
+                context_->applySettingsChanges(core_settings);


Do we need the lines

ClickHouse/src/Backups/BackupsWorker.cpp

Lines 389 to 390 in 2fb96d4

backup_context->checkSettingsConstraints(backup_settings.core_settings, SettingSource::QUERY);

backup_context->applySettingsChanges(backup_settings.core_settings);

now?

Good point, I think no as we do copy existing query context backup_context(Context::createCopy(query_context)), will remove

Please remove the same code also from RestoreStarter then

vitlibar · 2026-03-10T19:10:50Z

+namespace FailPoints
+{
+    extern const char backup_pause_before_main_loop[];
+    extern const char restore_pause_before_main_loop[];


One small thing about naming - backup and restore processes are not loops, they're linear processes from start to finish

backup_pause_on_start?

clickhouse-gh · 2026-03-11T16:28:12Z

LLVM Coverage Report

Metric	Baseline	Current	Δ
Lines	83.80%	83.80%	+0.00%
Functions	23.90%	23.90%	+0.00%
Branches	76.30%	76.30%	+0.00%

PR changed lines: PR changed-lines coverage: 100.00% (48/48)
Diff coverage report
Uncovered code

…r backup/restore

… backup/restore

Backport #99205 to 26.2: Fix max_execution_time not being applied for backup/restore

Backport #99205 to 26.1: Fix max_execution_time not being applied for backup/restore

Issue: ClickHouse#103324 `BACKUP TABLE t TO S3('uri', 'k', 's') SETTINGS base_backup = S3({backup_name:String}, ...) ASYNC` threw `Code: 36. BAD_ARGUMENTS Expected literal, got {backup_name:String}` since PR ClickHouse#99205 (commit `e6721ef7b16`, "Apply settings changes from backup/restore earlier"). The regression hit users on `26.1.5`, `25.12`, `25.10`, `25.8`, `26.2`, `26.3`, `26.4` and `master`. Root cause: PR ClickHouse#99205 added a call to `BackupSettings::fromBackupQuery` (and `RestoreSettings::fromRestoreQuery`) inside `InterpreterSetQuery::applySettingsFromQuery` to surface core settings (e.g. `max_execution_time`) before `ProcessList::insert`. But `fromBackupQuery` unconditionally calls `BackupInfo::fromAST(*query.base_backup_name)`, which only accepts `ASTLiteral` and throws `BAD_ARGUMENTS` on `ASTQueryParameter`. `applySettingsFromQuery` is invoked on the client side from `ClientBase::processOrdinaryQuery` BEFORE `ReplaceQueryParameterVisitor` substitutes parameters, so any `{name:Type}` placeholder inside `base_backup` triggered the throw. The destination URI was unaffected because `fromBackupQuery` does not call `fromAST` on `query.backup_name`. Fix: introduce two lightweight helpers, `BackupSettings::extractCoreSettingsFromQuery` and `RestoreSettings::extractCoreSettingsFromQuery`, that iterate over `query.settings` and return only the non-backup/restore-specific entries. They never look at `base_backup_name`, so they are safe to call before parameter substitution. `InterpreterSetQuery::applySettingsFromQuery` now uses these helpers. The original PR ClickHouse#99205 motivation is preserved -- core settings such as `max_execution_time` still reach the context before `ProcessList::insert`. The existing `BackupsWorker::doBackup` / `BackupsWorker::doRestore` callers continue to use `fromBackupQuery` / `fromRestoreQuery`; by the time they run, parameters have been substituted and `BackupInfo::fromAST` works as before. Bisect credit and proposed fix sketch from `@fm4v`. Regression test: `tests/queries/0_stateless/04213_base_backup_with_query_parameter.sh` covers both `BACKUP` and `RESTORE` paths plus a sanity check that `max_execution_time` can still be passed alongside the parameterized `base_backup` (verifying PR ClickHouse#99205 behaviour). The test fails with `BAD_ARGUMENTS` against unfixed master and passes against this fix.

Apply settings changes from backup/restore earlier

e6721ef

kssenii requested a review from vitlibar March 10, 2026 15:44

cursor Bot reviewed Mar 10, 2026

View reviewed changes

Comment thread src/Interpreters/InterpreterSetQuery.cpp

clickhouse-gh Bot added the pr-bugfix Pull request with bugfix, not backported by default label Mar 10, 2026

pamarcos self-assigned this Mar 10, 2026

pamarcos self-requested a review March 10, 2026 16:19

pamarcos approved these changes Mar 10, 2026

View reviewed changes

Add check for settings constraints

b97dd17

kssenii added pr-must-backport Pull request should be backported intentionally. Use this label with great care! v25.12-must-backport v25.10-must-backport labels Mar 10, 2026

vitlibar reviewed Mar 10, 2026

View reviewed changes

vitlibar self-assigned this Mar 10, 2026

vitlibar reviewed Mar 10, 2026

View reviewed changes

Rename failpoint; Remove redundant applySettings

f36922f

vitlibar approved these changes Mar 11, 2026

View reviewed changes

kssenii added this pull request to the merge queue Mar 11, 2026

Merged via the queue into master with commit 4374426 Mar 11, 2026
322 of 323 checks passed

kssenii deleted the backup-restore-propogate-query-limit branch March 11, 2026 22:07

robot-ch-test-poll4 added the pr-must-backport-synced The `*-must-backport` labels are synced into the cloud Sync PR label Mar 11, 2026

robot-clickhouse-ci-1 added the pr-synced-to-cloud The PR is synced to the cloud repo label Mar 11, 2026

robot-clickhouse added a commit that referenced this pull request Mar 11, 2026

Backport #99205 to 25.12: Fix max_execution_time not being applied fo…

8082851

…r backup/restore

This was referenced Mar 11, 2026

Backport #99205 to 25.12: Fix max_execution_time not being applied for backup/restore #99290

Closed

Cherry pick #99205 to 26.1: Fix max_execution_time not being applied for backup/restore #99291

Merged

robot-clickhouse added a commit that referenced this pull request Mar 11, 2026

Backport #99205 to 26.1: Fix max_execution_time not being applied for…

51321de

… backup/restore

This was referenced Mar 11, 2026

Backport #99205 to 26.1: Fix max_execution_time not being applied for backup/restore #99292

Merged

Cherry pick #99205 to 26.2: Fix max_execution_time not being applied for backup/restore #99293

Merged

robot-clickhouse added a commit that referenced this pull request Mar 11, 2026

Backport #99205 to 26.2: Fix max_execution_time not being applied for…

7d4be9c

… backup/restore

robot-clickhouse-ci-1 mentioned this pull request Mar 11, 2026

Backport #99205 to 26.2: Fix max_execution_time not being applied for backup/restore #99294

Merged

kssenii added a commit that referenced this pull request Mar 12, 2026

Merge pull request #99294 from ClickHouse/backport/26.2/99205

43b5aec

Backport #99205 to 26.2: Fix max_execution_time not being applied for backup/restore

kssenii added a commit that referenced this pull request Mar 12, 2026

Merge pull request #99292 from ClickHouse/backport/26.1/99205

5596576

Backport #99205 to 26.1: Fix max_execution_time not being applied for backup/restore

robot-ch-test-poll2 added the pr-backports-created Backport PRs are successfully created, it won't be processed by CI script anymore label Mar 12, 2026

alexey-milovidov mentioned this pull request Apr 8, 2026

CI: fix targeted AST fuzzer not resolving any queries #99320

Merged

1 task

fm4v mentioned this pull request May 8, 2026

Regression: base_backup setting now must have literal S3 URI #103324

Closed

groeneai mentioned this pull request May 8, 2026

Fix base_backup with query parameter throwing BAD_ARGUMENTS #104413

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix max_execution_time not being applied for backup/restore#99205

Fix max_execution_time not being applied for backup/restore#99205
kssenii merged 3 commits into
masterfrom
backup-restore-propogate-query-limit

kssenii commented Mar 10, 2026 •

edited by cursor Bot

Loading

Uh oh!

cursor Bot left a comment

Uh oh!

Uh oh!

clickhouse-gh Bot commented Mar 10, 2026 •

edited

Loading

Uh oh!

pamarcos left a comment

Uh oh!

pamarcos commented Mar 10, 2026

Uh oh!

vitlibar Mar 10, 2026

Uh oh!

kssenii Mar 10, 2026

Uh oh!

vitlibar Mar 10, 2026

Uh oh!

vitlibar Mar 10, 2026

Uh oh!

vitlibar Mar 10, 2026

Uh oh!

clickhouse-gh Bot commented Mar 11, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

	backup_context->checkSettingsConstraints(backup_settings.core_settings, SettingSource::QUERY);
	backup_context->applySettingsChanges(backup_settings.core_settings);

Conversation

kssenii commented Mar 10, 2026 • edited by cursor Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changelog category (leave one):

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

Documentation entry for user-facing changes

Uh oh!

cursor Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

clickhouse-gh Bot commented Mar 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

AI Review

1) Summary

2) Missing context (if any)

3) Findings (by severity)

4) Tests & Evidence

5) ClickHouse Compliance Checklist (Yes/No + short note)

6) Performance & Safety Notes

7) User-Lens Review

8) Final Verdict

Uh oh!

pamarcos left a comment

Choose a reason for hiding this comment

Uh oh!

pamarcos commented Mar 10, 2026

Uh oh!

vitlibar Mar 10, 2026

Choose a reason for hiding this comment

Uh oh!

kssenii Mar 10, 2026

Choose a reason for hiding this comment

Uh oh!

vitlibar Mar 10, 2026

Choose a reason for hiding this comment

Uh oh!

vitlibar Mar 10, 2026

Choose a reason for hiding this comment

Uh oh!

vitlibar Mar 10, 2026

Choose a reason for hiding this comment

Uh oh!

clickhouse-gh Bot commented Mar 11, 2026

LLVM Coverage Report

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

kssenii commented Mar 10, 2026 •

edited by cursor Bot

Loading

clickhouse-gh Bot commented Mar 10, 2026 •

edited

Loading