Skip to content

Enhance estimation reliability for max_estimated_execution_time#94668

Merged
serxa merged 5 commits intomasterfrom
fix-01287_max_execution_speed
Jan 21, 2026
Merged

Enhance estimation reliability for max_estimated_execution_time#94668
serxa merged 5 commits intomasterfrom
fix-01287_max_execution_speed

Conversation

@serxa
Copy link
Copy Markdown
Member

@serxa serxa commented Jan 20, 2026

Changelog category (leave one):

  • Not for changelog (changelog entry is not required)

Fixes #69624

@serxa serxa requested a review from Copilot January 20, 2026 14:46
@clickhouse-gh
Copy link
Copy Markdown
Contributor

clickhouse-gh bot commented Jan 20, 2026

Workflow [PR], commit [b12b692]

Summary:

job_name test_name status info comment
Stateless tests (amd_debug, distributed plan, s3 storage, parallel) failure
03780_failed_ssh FAIL cidb, issue ISSUE EXISTS
BuzzHouse (amd_debug) failure
Logical error: 'Inconsistent AST formatting: the query: (STID: 1941-1bfa) FAIL cidb, issue ISSUE EXISTS

@clickhouse-gh clickhouse-gh bot added the pr-improvement Pull request with some product improvements label Jan 20, 2026
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR improves the reliability of the max_estimated_execution_time setting by adding constraints to ensure estimation accuracy before throwing timeout errors. The change addresses an issue where queries could be incorrectly terminated too early due to unreliable estimates based on insufficient data.

Changes:

  • Added a check requiring either 1% of rows to be read OR 1% of estimated time to elapse before enforcing max_estimated_execution_time
  • Restructured the conditional logic to group timeout enforcement checks together
  • Only applies the stricter estimation when timeout_overflow_mode is set to THROW

serxa and others added 2 commits January 20, 2026 14:47
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
@serxa serxa requested a review from Copilot January 20, 2026 14:49
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated 2 comments.

@pamarcos pamarcos self-assigned this Jan 20, 2026
@pamarcos pamarcos self-requested a review January 20, 2026 15:24
Copy link
Copy Markdown
Member

@pamarcos pamarcos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. I haven't been able to reproduce the flaky test even after thousands of runs.
Theoretically it failed ~0.07% of times, so it's quite unlikely to fail anyway.

@serxa
Copy link
Copy Markdown
Member Author

serxa commented Jan 20, 2026

Yes, but they are still failing pretty often

@clickhouse-gh clickhouse-gh bot added pr-not-for-changelog This PR should not be mentioned in the changelog and removed pr-improvement Pull request with some product improvements labels Jan 20, 2026
@serxa serxa added this pull request to the merge queue Jan 21, 2026
Merged via the queue into master with commit 52831c2 Jan 21, 2026
129 of 132 checks passed
@serxa serxa deleted the fix-01287_max_execution_speed branch January 21, 2026 17:17
@robot-clickhouse-ci-1 robot-clickhouse-ci-1 added the pr-synced-to-cloud The PR is synced to the cloud repo label Jan 21, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pr-not-for-changelog This PR should not be mentioned in the changelog 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.

01287_max_execution_speed is a little flaky

4 participants