Repository navigation
planner: use local CTE counts for MPP shared CTE - #68848
Conversation
|
@winoros I've received your pull request and will start the review. I'll conduct a thorough review covering code quality, potential issues, and implementation details. ⏳ This process typically takes 10-30 minutes depending on the complexity of the changes. ℹ️ Learn more details on Pantheon AI. |
📝 WalkthroughWalkthroughThe PR replaces global CTE executor counting with local per-address counting to fix a deadlock risk in MPP shared CTE execution when producers contain UNION ALL branches scheduled across different TiFlash workers. The implementation introduces grouping structures, traversal logic, counting validation, and a comprehensive unit test. ChangesLocal CTE Count Calculation for MPP Shared CTEs
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Tools execution failed with the following error: Failed to run tools: 13 INTERNAL: Received RST_STREAM with code 2 (Internal server error) Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/planner/core/operator/physicalop/fragment_test.go (1)
56-96: ⚡ Quick winConsider adding a negative-path test for the local-count mismatch validation.
This test only covers the uniform colocated case. Per the PR objectives,
fillLocalCTECountsis expected to return an error when a CTE node's local per-address counts cannot be represented uniformly (thegetUniformCTELocalCountvalidation path). Adding a case where sink/source self-tasks yield differing per-address counts would guard the error branch against regressions.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/planner/core/operator/physicalop/fragment_test.go` around lines 56 - 96, Add a negative-path unit test in fragment_test.go that exercises mppTaskGenerator.fillLocalCTECounts when per-address local counts are non-uniform so getUniformCTELocalCount returns an error: create CTE sinks/sources (PhysicalCTESink, PhysicalCTESource) and ExchangeSenders with self-tasks arranged such that one address has a different number of local tasks than another (e.g., sink0 has tasks ["tiflash0"], sink1 has tasks ["tiflash0","tiflash1"] or similar), call (&mppTaskGenerator{}).fillLocalCTECounts(frags) and assert it returns a non-nil error, and optionally assert that the error message indicates non-uniform local CTE counts.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@pkg/planner/core/operator/physicalop/fragment_test.go`:
- Around line 56-96: Add a negative-path unit test in fragment_test.go that
exercises mppTaskGenerator.fillLocalCTECounts when per-address local counts are
non-uniform so getUniformCTELocalCount returns an error: create CTE
sinks/sources (PhysicalCTESink, PhysicalCTESource) and ExchangeSenders with
self-tasks arranged such that one address has a different number of local tasks
than another (e.g., sink0 has tasks ["tiflash0"], sink1 has tasks
["tiflash0","tiflash1"] or similar), call
(&mppTaskGenerator{}).fillLocalCTECounts(frags) and assert it returns a non-nil
error, and optionally assert that the error message indicates non-uniform local
CTE counts.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 26fe0d37-93c5-4051-8feb-d0bc2895654c
📒 Files selected for processing (4)
pkg/planner/core/operator/physicalop/BUILD.bazelpkg/planner/core/operator/physicalop/fragment.gopkg/planner/core/operator/physicalop/fragment_test.gopkg/planner/core/operator/physicalop/physical_cte.go
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #68848 +/- ##
================================================
- Coverage 76.3085% 73.9176% -2.3910%
================================================
Files 2041 2042 +1
Lines 563262 597537 +34275
================================================
+ Hits 429817 441685 +11868
- Misses 132529 153915 +21386
- Partials 916 1937 +1021
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
|
/retest |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: time-and-fate, xzhangxian1008 The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
[LGTM Timeline notifier]Timeline:
|
What problem does this PR solve?
Issue Number: close #68837, ref #61241
Problem Summary:
MPP shared CTE currently fills
CteSinkNumandCteSourceNumby global CTE id counts. This is unsafe for TiFlash because the runtimeCTEManageris node-local. When a producer-sideUNION ALLcreates sink fragments on different TiFlash nodes, a source running on one node can only observe the local sinks on that node, but the plan may still tell it to wait for the global sink count.What changed and how does it work?
This PR changes MPP shared CTE sink/source count filling to use TiFlash-address-local counts instead of global CTE-id counts. It records the fragment self tasks for each CTE sink/source plan node, computes the local sink/source instance count per TiFlash address, and writes those local counts back to the CTE operators.
If one CTE plan node spans tasks that would require different local counts, the planner now returns an error instead of sending counts that cannot be represented in the current TiPB fields.
The comments and helper naming are also updated to explain why local executor-instance counts are required.
Check List
Tests
Commands:
./tools/check/failpoint-go-test.sh pkg/planner/core/operator/physicalop -run TestFillLocalCTECountsUsesLocalTaskCounts -count=1 go test -run TestMPPSharedCTEScan -tags=intest,deadlock ./pkg/planner/core/casetest/enforcempp -count=1 GOFLAGS=-buildvcs=false make lint git diff --check e53c50c1c5ae0bd926d75d278ed6fa958670b648..HEADSide effects
Documentation
Release note
Please refer to Release Notes Language Style Guide to write a quality release note.
Summary by CodeRabbit
Release Notes
Bug Fixes
Tests
Chores