Skip to content

Mark task as done without parent ProcessList task#95535

Merged
antonio2368 merged 1 commit intomasterfrom
less-locks-for-cancelling
Jan 30, 2026
Merged

Mark task as done without parent ProcessList task#95535
antonio2368 merged 1 commit intomasterfrom
less-locks-for-cancelling

Conversation

@antonio2368
Copy link
Copy Markdown
Member

Changelog category (leave one):

  • Not for changelog (changelog entry is not required)

Should reduce lock contention.
cc @yariks5s any reason not to do it like this?

Documentation entry for user-facing changes

  • Documentation is written (mandatory for new features)

@clickhouse-gh
Copy link
Copy Markdown
Contributor

clickhouse-gh bot commented Jan 29, 2026

Workflow [PR], commit [26a7291]

Summary:

job_name test_name status info comment
Stateless tests (amd_debug, AsyncInsert, s3 storage, parallel) failure
02012_changed_enum_type_non_replicated FAIL cidb
Logical error: There was an error on A: B (probably it's a bug) (STID: 6337-3cad) FAIL cidb, issue

@clickhouse-gh clickhouse-gh bot added the pr-not-for-changelog This PR should not be mentioned in the changelog label Jan 29, 2026
@yariks5s yariks5s self-assigned this Jan 29, 2026
@antonio2368 antonio2368 requested a review from Copilot January 30, 2026 08:57
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 optimizes the process list cleanup path by moving the cancellation checker notification outside of the parent process list lock, reducing lock contention during query destruction.

Changes:

  • Move CancellationChecker::getInstance().appendDoneTasks(*it) call before acquiring the ProcessList mutex
  • Modernize mutex lock syntax in appendDoneTasks method

Reviewed changes

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

File Description
src/Interpreters/ProcessList.cpp Moved appendDoneTasks call outside the parent mutex lock to reduce contention
src/Interpreters/CancellationChecker.cpp Updated mutex lock to use CTAD (Class Template Argument Deduction)

Comment thread src/Interpreters/ProcessList.cpp
Copy link
Copy Markdown
Member

@yariks5s yariks5s left a comment

Choose a reason for hiding this comment

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

Looks correct, thank you

@antonio2368 antonio2368 added this pull request to the merge queue Jan 30, 2026
Merged via the queue into master with commit c0ef1f1 Jan 30, 2026
132 of 134 checks passed
@antonio2368 antonio2368 deleted the less-locks-for-cancelling branch January 30, 2026 14:40
@robot-ch-test-poll2 robot-ch-test-poll2 added the pr-synced-to-cloud The PR is synced to the cloud repo label Jan 30, 2026
@serxa serxa added the pr-must-backport Pull request should be backported intentionally. Use this label with great care! label Feb 2, 2026
@robot-clickhouse robot-clickhouse added the pr-must-backport-synced The `*-must-backport` labels are synced into the cloud Sync PR label Feb 2, 2026
@robot-ch-test-poll3 robot-ch-test-poll3 added the pr-backports-created Backport PRs are successfully created, it won't be processed by CI script anymore label Feb 2, 2026
clickhouse-gh bot added a commit that referenced this pull request Feb 2, 2026
Backport #95535 to 26.1: Mark task as done without parent ProcessList task
clickhouse-gh bot added a commit that referenced this pull request Feb 2, 2026
Backport #95535 to 25.12: Mark task as done without parent ProcessList task
clickhouse-gh bot added a commit that referenced this pull request Feb 2, 2026
Backport #95535 to 25.11: Mark task as done without parent ProcessList task
serxa added a commit that referenced this pull request Feb 2, 2026
Backport #95535 to 25.8: Mark task as done without parent ProcessList task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pr-backports-created Backport PRs are successfully created, it won't be processed by CI script anymore pr-must-backport Pull request should be backported intentionally. Use this label with great care! pr-must-backport-synced The `*-must-backport` labels are synced into the cloud Sync PR 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.

8 participants