Skip to content

Fix Logical error: data->allocated_size != debug_allocated_size#100147

Closed
nickitat wants to merge 5 commits intomasterfrom
fix_allocated_memory_accounting_in_hj_for_dynamic_columns
Closed

Fix Logical error: data->allocated_size != debug_allocated_size#100147
nickitat wants to merge 5 commits intomasterfrom
fix_allocated_memory_accounting_in_hj_for_dynamic_columns

Conversation

@nickitat
Copy link
Copy Markdown
Member

@nickitat nickitat commented Mar 19, 2026

Changelog category (leave one):

  • Not for changelog (changelog entry is not required)

Closes: #99920

@nickitat
Copy link
Copy Markdown
Member Author

nickitat commented Mar 19, 2026

First, another bug should be fixed: #100148

@clickhouse-gh
Copy link
Copy Markdown
Contributor

clickhouse-gh bot commented Mar 19, 2026 •
edited
Loading

Workflow [PR], commit [a5bba59]

Summary: ❌

job_name test_name status info comment
Stateless tests (amd_llvm_coverage, old analyzer, s3 storage, DatabaseReplicated, WasmEdge, parallel) failure
04042_hash_join_allocated_size_tracking FAIL cidb
04043_column_variant_compress_shared_subcolumns FAIL cidb

AI Review

Summary

This PR fixes a real correctness issue in Set::appendSetElements by forcing independent ownership with IColumn::mutate before later mutable operations, preventing COW-shared sub-column races in concurrent pipelines. The added stateless regressions are aligned with the reported failure mode (#99920) and exercise the affected path. I did not find blocker/major issues in the patch itself.

Missing context

  • ⚠️ Full CI signal is not available yet (PR and several required checks are still pending), so this review is based on code and tests in the diff.

ClickHouse Rules

Item Status Notes
Deletion logging ➖
Serialization versioning ➖
Core-area scrutiny ✅
No test removal ✅
Experimental gate ➖
No magic constants ✅
Backward compatibility ✅
SettingsChangesHistory.cpp ➖
PR metadata quality ✅
Safe rollout ✅
Compilation time ✅

Final Verdict

  • Status: ✅ Approve

@clickhouse-gh clickhouse-gh bot added the pr-not-for-changelog This PR should not be mentioned in the changelog label Mar 19, 2026
@Avogar Avogar self-assigned this Mar 20, 2026
@nickitat nickitat force-pushed the fix_allocated_memory_accounting_in_hj_for_dynamic_columns branch from 44d89cc to 68b0c68 Compare March 20, 2026 15:59
@nickitat nickitat marked this pull request as ready for review March 20, 2026 19:32
@clickhouse-gh
Copy link
Copy Markdown
Contributor

clickhouse-gh bot commented Mar 20, 2026

LLVM Coverage Report

Metric Baseline Current Δ
Lines 83.80% 83.80% +0.00%
Functions 24.00% 24.00% +0.00%
Branches 76.40% 76.40% +0.00%

PR changed lines: PR changed-lines coverage: 100.00% (7/7, 0 noise lines excluded)
Diff coverage report
Uncovered code

Avogar added a commit that referenced this pull request Mar 20, 2026
@Avogar
Add test from #100147
c7d33df
@Avogar Avogar mentioned this pull request Mar 20, 2026
Merged
1 task
@nickitat nickitat closed this Mar 21, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

@Avogar Avogar

Labels

pr-not-for-changelog This PR should not be mentioned in the changelog

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Logical error: data->allocated_size != debug_allocated_size (A != B)

2 participants

@nickitat @Avogar