[fix](fe) Deduplicate recursive CTE fragment reset requests - #67738
Open
Mryange wants to merge 1 commit into
Open
[fix](fe) Deduplicate recursive CTE fragment reset requests#67738Mryange wants to merge 1 commit into
Mryange wants to merge 1 commit into
Conversation
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Contributor
Author
|
run buildall |
Contributor
FE UT Coverage ReportIncrement line coverage |
Contributor
TPC-H: Total hot run time: 16807 ms |
Contributor
TPC-DS: Total hot run time: 81873 ms |
Contributor
ClickBench: Total hot run time: 14.83 s |
Contributor
FE Regression Coverage ReportIncrement line coverage |
Contributor
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Review status: complete after two review rounds converged. One blocking lifecycle issue remains.
Critical checkpoint conclusions:
- Goal and proof: The change correctly removes repeated reset entries within a single recursive CTE controller. The new regression deterministically materializes the shared CTE, executes multiple recursion rounds, and exercises the duplicate-suppression branch; exact-head P0, FE, compile, and CheckStyle checks pass, and FE regression coverage reports all five changed Java lines covered.
- Scope and clarity: The patch is small and clear for the covered topology, but its per-node set lifetime is narrower than the BE lifecycle ownership domain described in the inline finding.
- Concurrency and lifecycle: FE construction is local/single-threaded. BE lifecycle state is singular per
(query_id, fragment_id)on each addressed BE and assumes one serial controller; independent recursive sibling controllers can share that state without an owner, refcount, or round barrier. This is the blocking issue. - Configuration and compatibility: No configuration, Thrift/protocol field, storage format, persistence schema, or rolling-upgrade contract changes. Same-BE local instances are correctly collapsed and distinct BE addresses remain distinct.
- Parallel paths and conditions: Targets are already consumed once per scan fragment and notify-close uses a fragment-id set. Multi-BE and local-shuffle behavior is preserved. The missing parallel path is two independent recursive controllers sharing one transitive materialized fragment.
- Tests and results: The ordered golden output is correct and the suite is included in P0. A two-controller regression is still needed for the blocking topology. No local build was run because this review environment prohibits builds; the cited exact-head CI and coverage results were inspected.
- Error handling, observability, transactions, writes, FE/BE variables, memory, and performance: Status propagation and existing query/fragment identifiers remain adequate. The PR adds no transaction/data-write path, transmitted variable, static lifecycle, or significant allocation. No additional issue was found.
- User focus: No additional user-provided focus was specified; the complete PR was reviewed.
Because the valid multi-controller topology can still reproduce Fragment context ... not found or mix recursion rounds, I am requesting changes.
| List<TRecCTETarget> targets = new ArrayList<>(); | ||
| // reset infos for all instances of child fragments (used to reset state) | ||
| List<TRecCTEResetInfo> fragmentsToReset = new ArrayList<>(); | ||
| Set<String> resetFragmentKeys = new HashSet<>(); |
Contributor
There was a problem hiding this comment.
[P1] Keep one owner for a shared fragment's recursive lifecycle
This set is recreated for every RecursiveCteNode, so it removes duplicates only within one controller. A valid reduced plan is:
CTEProducer(base) -> Fbase
Join
|-- RecCTE r1
| `-- recursive side -> inlined edges -> CTEConsumer(base) x2
`-- RecCTE r2
`-- recursive side -> inlined edges -> CTEConsumer(base) x2
BindRelation marks the directly referenced edges CTE must-inline, but not its nested base; because base has two consumers and the default threshold is one, it remains materialized. Deep-copying edges retains the lower CTE id, and physical translation returns the same MultiCastPlanFragment, so both fresh sets retain the same (Fbase, BE). Each source then independently sends WAIT/REBUILD/SUBMIT/FINAL_CLOSE, while FragmentMgr owns only one context for that key. For example, R1 can WAIT/REBUILD PFC1, R2 can WAIT and remove PFC1, and R1's SUBMIT then returns NotFound; full sequential execution still fails after the first controller's FINAL_CLOSE removes state. Please clone/inline the transitive fragment per controller or add explicit shared lifecycle ownership/round coordination (a query-wide drop from one list is insufficient), and add a two-controller regression.
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.
Recursive CTE queries could fail with
Fragment context ... not foundduring recursive fragment cleanup. The FE collected recursive child fragments through a shared plan tree and could generate duplicate reset entries for the same fragment on the same BE. The firstWAIT_FOR_DESTROYrequest removed the BE fragment context, while the duplicate request then failed to find it. This change deduplicates reset entries by fragment ID and BE address while preserving entries for different BEs.Release note
None
Check List (For Author)
Test
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)