Skip to content

Backport #111103 to 26.6: Cache deduplication data hashes before the Alias hop - #112727

Merged
PedroTadim merged 3 commits into
26.6from
backport/26.6/111103
Aug 3, 2026
Merged

Backport #111103 to 26.6: Cache deduplication data hashes before the Alias hop#112727
PedroTadim merged 3 commits into
26.6from
backport/26.6/111103

Conversation

@robot-clickhouse-ci-2

@robot-clickhouse-ci-2 robot-clickhouse-ci-2 commented Jul 31, 2026

Copy link
Copy Markdown
Contributor

Original pull-request #111103
Cherry-pick pull-request #112584

This pull-request is a last step of an automated backporting.
Treat it as a standard pull-request: look at the checks and resolve conflicts.
Merge it only if you intend to backport changes to the target branch, otherwise just close it.

The PR source

The PR is created in the CI job

Version info

  • Merged into: 26.6.2.145

@robot-clickhouse-ci-2 robot-clickhouse-ci-2 added pr-backport Changes, backported to release branch. Do not use manually - automated use only! pr-bugfix Pull request with bugfix, not backported by default labels Jul 31, 2026
@clickhouse-gh

clickhouse-gh Bot commented Jul 31, 2026

Copy link
Copy Markdown
Contributor

Workflow [BackportPR], commit [a6b5b95]

Summary:

job_name test_name status info comment
Integration tests (amd_asan_ubsan, db disk, old analyzer, 5/6) ERROR

@alexey-milovidov

Copy link
Copy Markdown
Member

🕵 @groeneai, investigate the failure: https://s3.amazonaws.com/clickhouse-test-reports/json.html?PR=112727&sha=6120a6fb27efe0d740c8f075aee499215d2f23e1&name_0=BackportPR&name_1=Stress%20test%20%28amd_tsan%29 and provide a fix in a separate PR (a backport, see below). If the fix is already in progress, link it here.

Stress test (amd_tsan) fails with a TSan data race that is unrelated to this backport (this PR only touches insert deduplication / the Alias hop):

WARNING: ThreadSanitizer: data race
  Write of size 8 by thread T991:
    #0 std::unique_ptr<DB::MemoryReservation>::reset(...)
    #1 DB::QueryStatus::releaseWorkloadResources() src/Interpreters/ProcessList.cpp:532
    #2 DB::BlockIO::releaseWorkloadResources() const src/QueryPipeline/BlockIO.cpp:114
    #3 DB::BlockIO::onFinish(...) src/QueryPipeline/BlockIO.cpp:63
    #4 DB::TCPHandler::runImpl()

  Previous read of size 8 by thread T1451:
    #0 std::unique_ptr<DB::MemoryReservation>::get() const
    #1 DB::QueryStatus::getMemoryReservation() const src/Interpreters/ProcessList.h:244
    #2 DB::WorkloadResources::WorkloadResources(...) src/Processors/Executors/PipelineExecutor.cpp:67
    #3 DB::PipelineExecutor::executeImpl(...)

This is exactly the race fixed on master by #108391 ("Fix data race on MemoryReservation release at query finish"): BlockIO::onFinish must release only the query slot early and release the memory reservation after the pipeline is finalized, because pipeline threads hold raw pointers to MemoryReservation via WorkloadResources.

26.6 still has the racy order — onFinish calls releaseWorkloadResources before finalizing the pipeline. The reason is that the fix never landed on 26.6: the cherry-pick #108562 was auto-closed on 2026-07-06 with unresolved conflicts, while the feature itself (#82414, Workload scheduling: Memory reservations) was backported to 26.6 on 2026-06-27 in 2098704e6d1 — so 26.6 got the feature without the fix.

Please backport #108391 (BlockIO::releaseQuerySlot / BlockIO::releaseMemoryReservation split, plus the resetPipeline ordering in BlockIO::reset, onException and onCancelOrConnectionLoss) to 26.6. Until then it will keep reddening 26.6 backport PRs.

The branch has also been updated with the latest 26.6 in 8e4d81f79cc (clean merge; no 26.6 commit touched the files this PR changes).

@alexey-milovidov

Copy link
Copy Markdown
Member

🕵 Follow-up: I opened the 26.6 backport of #108391 myself — #112885. It should clear the Stress test (amd_tsan) race on this and other 26.6 backport PRs once merged.

@groeneai

groeneai commented Aug 1, 2026

Copy link
Copy Markdown
Contributor

Thanks for opening the 26.6 backport yourself. #112885 is the fix and I am not opening a competing backport. I verified your analysis against 26.6; it holds, with two corrections and one qualification.

The racy order really is still on 26.6, and all four reported frames resolve there exactly: onFinish calls releaseWorkloadResources() as its first statement at BlockIO.cpp:63, BlockIO::releaseWorkloadResources is at :114, QueryStatus::releaseWorkloadResources is at ProcessList.cpp:532 doing memory_reservation.reset(); query_slot.reset();, and the read side is ProcessList.h:244 called from PipelineExecutor.cpp:67. releaseQuerySlot and releaseMemoryReservation do not exist on 26.6 at all.

Correction 1: only two of the four sites need changing, not four. The patch's BlockIO.cpp hunks touch reset and onFinish only. On 26.6 onException and onCancelOrConnectionLoss already do resetPipeline(/*cancel=*/true) before releaseWorkloadResources(), with the comment about pipeline threads holding raw pointers, because the #82414 backport 2098704e6d1 introduced that ordering itself. The exception paths were never racy on 26.6, and the backport correctly leaves them alone. pull/112885.diff is byte-identical to pull/108391.diff apart from index and hunk-header lines, so it is an unmodified cherry-pick.

Correction 2: #108562 conflicted because it was opened before its own prerequisite existed on the branch. It was opened 2026-06-26 01:12Z but #82414 only reached 26.6 on 2026-06-27 10:55Z, 33.7 hours later. At 2519b277de3a (the feature backport's parent) 26.6 still had releaseQuerySlot and no memory_reservation in ProcessList.h, so the patch had nothing to apply against; the conflict was structural, not incidental. That is consistent with #112885 being mergeable now.

Qualification on "it will keep reddening 26.6 backport PRs": measurably rarer than on master. A 45-day CIDB sweep keyed on the stack shape (releaseWorkloadResources + getMemoryReservation + WorkloadResources) rather than the test name, scoped by base_ref/head_ref, finds 1 hit on 26.6 (this PR at 6120a6fb) against 142 26.6 backport PRs that ran Stress test (amd_tsan) since the feature landed. Pre-fix master had 68 rows over 54 PRs, but 63 landed on Stateless tests (amd_tsan, s3 storage|parallel), and 26.6 BackportPR runs no tsan stateless jobs, only Stress test (amd_tsan) plus integration and unit. Same race, roughly one job of exposure instead of several, so about 1 in 142 rather than an every-PR blocker.

Positive evidence for #112885: its own Stress test (amd_tsan) is green (completed 09:41:54Z); the sole failing check is Integration tests (amd_asan_ubsan, db disk, old analyzer, 5/6), neither the tsan job nor the test_scheduler_memory file the patch touches.

Master is clean on this signature: of the 4 rows after 474a3c12, three were builds tested before the fix landed and the fourth (e73f84e5 on #108553) is STID 3667-3bd3 on a tree predating #82414.

I will re-run that sweep after #112885 merges and confirm 26.6 stays at 0 new hits. If it is closed unmerged instead, say so and I will take the backport.

Session id: cron:clickhouse-premium-slot-26:20260801-095200

@alexey-milovidov

Copy link
Copy Markdown
Member

🕵 Updated the branch with the latest 26.6 in a6b5b951d57 (clean merge; all 18 files this PR changes are byte-identical before and after the merge) to retrigger CI.

Status of the two reds on the previous run (8e4d81f7), both unrelated to this backport:

The MemoryReservation race backport #112885 is still open against 26.6; once it merges, the earlier tsan signature is gone from this branch too.

@PedroTadim
PedroTadim merged commit d4ad41f into 26.6 Aug 3, 2026
112 of 116 checks passed
@PedroTadim
PedroTadim deleted the backport/26.6/111103 branch August 3, 2026 10:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pr-backport Changes, backported to release branch. Do not use manually - automated use only! pr-bugfix Pull request with bugfix, not backported by default

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants