Do not mark move operations that allocate through memory-tracking containers as noexcept#106096
Merged
Merged
Conversation
…ppends through memory-tracking containers and can throw MEMORY_LIMIT_EXCEEDED, terminating the server
…on move ops and ActionsDAG::decorrelate; they allocate and can throw
…-free by defaulting them (steal impl instead of make_unique)
Contributor
|
Workflow [PR], commit [632873c] Summary: ❌
AI ReviewSummaryThis PR removes incorrect Final VerdictStatus: ✅ Approve |
clickhouse-gh
Bot
added
the
pr-not-for-changelog
![clickhouse-gh[bot]](https://avatars.githubusercontent.com/in/928874?s=60&v=4){kind=small}
…had the same noexcept-but-allocating issue
…; an in-place *impl = std::move copies SettingFieldString (which has no move op) and can throw under the noexcept wrapper
nickitat
approved these changes
May 29, 2026
Member
nickitat
left a comment
nickitat
left a comment
There was a problem hiding this comment.
If there is a focused reproducer, let's add it as a test.
…s throw (std::bad_alloc), not only via the memory-tracking containers
…ans of a UNION query so a fault lands in QueryPlanResourceHolder::append; the server must not terminate
…ryPlanResourceHolder, which allocates and can throw (move ctor stays noexcept, it steals the holder)
…onally can throw; marking them noexcept would reintroduce the std::terminate (same NOLINT as StorageInMemoryMetadata)
3 tasks
Member
Author
|
Found another, different, issue in the CI preventing the merge: Fixing it here |
…SSL callback exception-safe; otherwise memory_tracker_fault_probability injects a fault that unwinds through C and terminates
Open
1 task
Contributor
LLVM Coverage Report
Changed lines: Changed C/C++ lines covered by tests: 84/114 (73.68%) | Lost baseline coverage (was covered on master, now uncovered in this PR): 2 line(s) · Uncovered code |
Merged
via the queue into
ClickHouse:master
with commit Jun 1, 2026
ecb53ca
164 of 166 checks passed
robot-ch-test-poll1
added
the
pr-synced-to-cloud
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
3 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.
A recent change (
d45e6cd6d6bc, "Convert std containers insrc/QueryPipelineto*WithMemoryTrackingaliases") madeQueryPlanResourceHolder's move-assignment able to throw: it appends intoVectorWithMemoryTrackingmembers, which allocate through the throwing memory tracker and raiseMEMORY_LIMIT_EXCEEDED. The function (and theQueryPipeline/BlockIOmove-assignments that call it) are markednoexcept, so under a memory limit the throw hits thenoexceptboundary and callsstd::terminate. It reproduces with aSELECT ... SETTINGS extremes = 1, max_memory_usage = '4Mi', which terminates the server while the resultBlockIOis move-assigned inTCPHandler.This removes
noexceptfrom that move-assignment chain (QueryPlanResourceHolder,QueryPipeline,BlockIO).While auditing other
noexceptspecifications it also fixes:ConstraintsDescription/ColumnDescriptionmove operations andActionsDAG::decorrelate, which allocate (rebuild derived data, clone ASTs, append to vectors) and can throw.noexceptmove constructors and move assignments either allocated a new impl viamake_unique, or move-assigned the impl in place (*impl = std::move(*settings.impl)). The latter is also unsafe:BaseSettings/Datamove walks per-fieldSettingFields, andSettingFieldString(and friends) declare a copy assignment but no move, so the rvalue assignment falls back to copying aString, which allocates and can throw. Both move ops are now= default, so they simply steal theunique_ptr(noexceptand allocation-free); the impl/BaseSettingsis never moved.Closes ClickHouse/ClickHouse#106079
References ClickHouse/ClickHouse#105580
Changelog category (leave one):
Changelog entry (a user-readable short description of the changes that goes into CHANGELOG.md):
...