Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

Fix potential crash caused by concurrent mutation of underlying const PREWHERE columns #88605

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Oct 17, 2025
Merged

Fix potential crash caused by concurrent mutation of underlying const PREWHERE columns #88605

merged 1 commit into from
Oct 17, 2025
+18 −1

Conversation

azat
Copy link
Member

@azat azat commented Oct 15, 2025 •
edited
Loading

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 potential crash caused by concurrent mutation of underlying const PREWHERE columns

The issue was that a constant column in PREWHERE could be mutated concurrently by multiple threads (in MergeTreeReadTask::read() via shrinkToFit()), because for single-row columns it returns the same object without copying (e.g., via materialize()).

Fixes: #85404

@azat azat added the pr-must-backport Pull request should be backported intentionally. Use this label with great care! label Oct 15, 2025
@clickhouse-gh ClickHouse-GH
Copy link

clickhouse-gh bot commented Oct 15, 2025 •
edited
Loading

Workflow [PR], commit [25675b2]

Summary: ❌

job_name test_name status info comment
Stateless tests (amd_binary, old analyzer, s3 storage, DatabaseReplicated, parallel) failure
03237_insert_sparse_columns_mem FAIL cidb
Stateless tests (amd_msan, parallel, 2/2) failure
03602_alter_update_nullable_json FAIL cidb
Bugfix validation (functional tests) failure
Integration tests (amd_tsan, 6/6) failure
test_database_hms/test.py::test_hive_catalog_url_parsing FAIL cidb

@clickhouse-gh clickhouse-gh bot added the pr-bugfix Pull request with bugfix, not backported by default label Oct 15, 2025
@azat azat changed the title Fix mutating const columns (shrinkToFit()) from PREWHERE from multiple threads Fix potential crash caused by concurrent mutation of underlying const PREWHERE columns Oct 15, 2025
@azat
Copy link
Member Author

azat commented Oct 15, 2025

[2025-10-15 20:35:10] failure [Bugfix validation (functional tests)]
[2025-10-15 20:35:10] | Failed: 0, Passed: 1, Skipped: 0

Test requires sanitizer to reproduce the issue

@azat azat force-pushed the prewhere-const-shrink-fix branch from 9623805 to 954d0f4 Compare October 15, 2025 22:11
@davenger
Copy link
Member

It looks like the problem is caused by this dangerous call of assumeMutableRef() which is doing a const_cast
https://github.com/ClickHouse/ClickHouse/blob/master/src/Storages/MergeTree/MergeTreeReadTask.cpp#L354
Maybe it's cleaner to clone const columns when they are added to read_result.columns?

@davenger davenger self-assigned this Oct 16, 2025
@azat
Copy link
Member Author

azat commented Oct 16, 2025

Maybe it's cleaner to clone const columns when they are added to read_result.columns?

You mean here, right?

ClickHouse/src/Storages/MergeTree/MergeTreeRangeReader.cpp

Line 1491 in 9dea2ef

result.columns.emplace_back(std::move(col.column));
(or maybe even later)

But the problem is that here we do not have const column anymore, since i.e. materialize will simply return underlying column -

ClickHouse/src/Functions/materialize.h

Line 59 in 9dea2ef

auto res = arguments[0].column->convertToFullColumnIfConst();

@azat azat force-pushed the prewhere-const-shrink-fix branch from 954d0f4 to 9273b7f Compare October 16, 2025 12:42
@azat
Copy link
Member Author

azat commented Oct 16, 2025

Rewrote it to

if (column->use_count() == 1)
    column->assumeMutableRef().shrinkToFit();

@azat azat force-pushed the prewhere-const-shrink-fix branch from 9273b7f to 64833aa Compare October 16, 2025 16:25
@azat azat enabled auto-merge October 16, 2025 16:27
@azat
Fix potential crash caused by concurrent mutation of underlying const…
25675b2 
… PREWHERE columns

The issue was that in MergeTreeReadTask we may have column that has
other references, usually it is a constant column that created during
analysis (that is not a constant anymore where we call shrink, i.e.
after `materialize()`), and we do not need to mutate such column anyway.

v2: move code from MergeTreeSplitPrewhereIntoReadSteps.cpp::addClonedDAGToDAG() into MergeTreeReadTask
@azat azat force-pushed the prewhere-const-shrink-fix branch from 64833aa to 25675b2 Compare October 16, 2025 18:29
@azat
Copy link
Member Author

azat commented Oct 17, 2025 •
edited
Loading

CI is unrelated, something is temporary, something is known

@azat azat added this pull request to the merge queue Oct 17, 2025
Merged via the queue into ClickHouse:master with commit 8ad6289 Oct 17, 2025
118 of 123 checks passed
@azat azat deleted the prewhere-const-shrink-fix branch October 17, 2025 07:44
@robot-ch-test-poll3 robot-ch-test-poll3 added the pr-synced-to-cloud The PR is synced to the cloud repo label Oct 17, 2025
@robot-ch-test-poll robot-ch-test-poll mentioned this pull request Oct 17, 2025
Merged
robot-ch-test-poll added a commit that referenced this pull request Oct 17, 2025
@robot-ch-test-poll
Merge pull request #88711 from ClickHouse/cherrypick/25.3/88605
5e4e359 
Cherry pick #88605 to 25.3: Fix potential crash caused by concurrent mutation of underlying const PREWHERE columns
robot-clickhouse added a commit that referenced this pull request Oct 17, 2025
@robot-clickhouse
Backport #88605 to 25.3: Fix potential crash caused by concurrent mut…
973c382 
…ation of underlying const PREWHERE columns
This was referenced Oct 17, 2025
Open
Merged
robot-ch-test-poll added a commit that referenced this pull request Oct 17, 2025
@robot-ch-test-poll
Merge pull request #88713 from ClickHouse/cherrypick/25.7/88605
4bf7d89 
Cherry pick #88605 to 25.7: Fix potential crash caused by concurrent mutation of underlying const PREWHERE columns
robot-clickhouse added a commit that referenced this pull request Oct 17, 2025
@robot-clickhouse
Backport #88605 to 25.7: Fix potential crash caused by concurrent mut…
88a20e6 
…ation of underlying const PREWHERE columns
@robot-ch-test-poll robot-ch-test-poll mentioned this pull request Oct 17, 2025
Merged
@robot-ch-test-poll robot-ch-test-poll mentioned this pull request Oct 17, 2025
Merged
robot-ch-test-poll added a commit that referenced this pull request Oct 17, 2025
@robot-ch-test-poll
Merge pull request #88715 from ClickHouse/cherrypick/25.8/88605
f07a211 
Cherry pick #88605 to 25.8: Fix potential crash caused by concurrent mutation of underlying const PREWHERE columns
robot-clickhouse added a commit that referenced this pull request Oct 17, 2025
@robot-clickhouse
Backport #88605 to 25.8: Fix potential crash caused by concurrent mut…
5426bfc 
…ation of underlying const PREWHERE columns
This was referenced Oct 17, 2025
Merged
Merged
robot-ch-test-poll added a commit that referenced this pull request Oct 17, 2025
@robot-ch-test-poll
Merge pull request #88717 from ClickHouse/cherrypick/25.9/88605
055e9e8 
Cherry pick #88605 to 25.9: Fix potential crash caused by concurrent mutation of underlying const PREWHERE columns
robot-clickhouse added a commit that referenced this pull request Oct 17, 2025
@robot-clickhouse
Backport #88605 to 25.9: Fix potential crash caused by concurrent mut…
e47365f 
…ation of underlying const PREWHERE columns
@robot-ch-test-poll robot-ch-test-poll mentioned this pull request Oct 17, 2025
Open
clickhouse-gh bot added a commit that referenced this pull request Oct 17, 2025
@clickhouse-gh
Merge pull request #88714 from ClickHouse/backport/25.7/88605
8d3e880 
Backport #88605 to 25.7: Fix potential crash caused by concurrent mutation of underlying const PREWHERE columns
clickhouse-gh bot added a commit that referenced this pull request Oct 17, 2025
@clickhouse-gh
Merge pull request #88716 from ClickHouse/backport/25.8/88605
261935c 
Backport #88605 to 25.8: Fix potential crash caused by concurrent mutation of underlying const PREWHERE columns
@robot-ch-test-poll robot-ch-test-poll added pr-backports-created-cloud pr-must-backport-synced The `*-must-backport` labels are synced into the cloud Sync PR labels Oct 17, 2025
@robot-ch-test-poll2 robot-ch-test-poll2 added the pr-backports-created Backport PRs are successfully created, it won't be processed by CI script anymore label Oct 17, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@davenger davenger davenger approved these changes

Assignees

@davenger davenger

Labels

pr-backports-created Backport PRs are successfully created, it won't be processed by CI script anymore pr-backports-created-cloud pr-bugfix Pull request with bugfix, not backported by default 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-synced-to-cloud The PR is synced to the cloud repo

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

AddressSanitizer: attempting double-free in DB::ColumnVector<char8_t>::shrinkToFit

5 participants

@azat @davenger @robot-ch-test-poll @robot-ch-test-poll2 @robot-ch-test-poll3
Morty Proxy This is a proxified and sanitized view of the page, visit original site.