[fix](fe) Return transaction insert execution errors - #67386
Merged
Conversation
mymeiyi
requested review from
924060929,
englefly,
morrySnow and
starocean999
as code owners
September 1, 2026 11:27
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Contributor
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Pull request overview
Fixes FE behavior so transaction INSERT execution failures correctly surface to clients by setting INSERT error state when appropriate, while keeping already-reported errors intact.
Changes:
- Add error-state setting in
OlapTxnInsertExecutor.onFail()when query state has not already been marked as error. - Refactor common error-state construction into
OlapInsertExecutor.setErrorState()and reuse it from other executors. - Extend regression coverage to inject a fragment execution failure and validate the error is returned during transactional INSERT.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| regression-test/suites/insert_p0/transaction/txn_insert_inject_case.groovy | Adds a new injected fragment failure case and refactors debugpoint helpers for reuse. |
| fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/insert/RemoteOlapInsertExecutor.java | Switches failure handling to shared setErrorState() logic. |
| fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/insert/OlapTxnInsertExecutor.java | Sets MySQL error state on txn insert failure when state was previously left successful. |
| fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/insert/OlapInsertExecutor.java | Extracts shared error-state construction into setErrorState() and keeps load-job recording in onFail(). |
| fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/insert/OlapGroupCommitInsertExecutor.java | Reuses setErrorState() to standardize final error formatting. |
Suppressed comments (1)
regression-test/suites/insert_p0/transaction/txn_insert_inject_case.groovy:1
- After the second injected failure, the test no longer validates that a subsequent INSERT succeeds in the same transaction after the debugpoint is disabled (previously there was another INSERT before COMMIT). This reduces coverage of the 'recover and continue' path. Consider re-adding a post-disable successful INSERT (and/or an assertion on resulting row count) before
committo ensure the transaction remains usable after an injected execution failure.
// Licensed to the Apache Software Foundation (ASF) under one
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Contributor
Author
|
/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 opinion: approve. I found no new distinct blocking issue on head 13ae215cb40ec5c35fdd25be8aa763026a53d903.
Critical checkpoint conclusions:
- Goal and proof: The change closes the client-response gap for explicit-transaction INSERT failures that are caught by
executeSingleInsert()beforeErrorReportsetsQueryState.OlapTxnInsertExecutor.onFail()now supplies a fallback error only when no earlierERRexists. The regression injects the direct fragment-dispatch failure, requires the client error, then performs and commits a later INSERT in the same transaction; the unchanged ordered six-row result proves the later commit occurred and the failed INSERT did not contribute rows. - Scope and parallel paths: The production change is focused. The extracted error-state helper preserves the previous normal OLAP, group-commit, and remote OLAP message construction and abort/load-recording order.
- Concurrency and lifecycle: No new shared state, lock, or lock ordering is introduced. Failure handling runs on the statement execution path after the coordinator failure is observed. Failed-subtransaction cleanup precedes response-state mutation, successful subtransaction commit information remains intact, and the next statement resets
QueryStatewhile retaining the explicit transaction. - Transaction, persistence, and data correctness: Both shared-nothing and cloud subtransaction paths retain their existing abort/commit contracts. There is no EditLog, storage-format, visibility-version, delete-bitmap, or FE-BE protocol change.
- Conditions and error handling: The
stateType != ERRguard is necessary and correct: it preserves specific diagnostics already installed byErrorReport, while direct dispatch exceptions that leave stateOKreceive the missing client-visible error. Null throwable messages remain normalized tounknown reason. - Configuration, compatibility, observability, and performance: No configuration or compatibility surface changes. Existing logs retain query/label/transaction or remote-catalog context. The helper adds no RPC, scan, lock contention, or material hot-path work.
- Test coverage: The new negative/recovery case complements the existing flush-error path, which continues to exercise preservation of an existing
ErrorReportdiagnostic. Tests/builds were not run in this review-only runner, as required by the review instructions. - Existing review context: The mutable-
errMsghelper contract and debug-point cleanup symmetry are already covered by the existing inline threads, so I did not duplicate them. - User focus: No additional user-provided review focus was supplied.
Review completion: Round 1 converged. Both complete-review agents and the separate risk-focused agent returned NO_NEW_VALUABLE_FINDINGS; all initial risks were independently resolved, and there are no unresolved candidates or new inline comments.
Contributor
Author
|
run buildall |
### What problem does this PR solve? Issue Number: None Related PR: None Problem Summary: Transaction INSERT execution failures raised directly while dispatching fragments bypass ErrorReport. OlapTxnInsertExecutor catches these failures and aborts the subtransaction, but previously left QueryState successful, causing the client to receive Query OK. In cloud mode, abortSubTxn also populated AbortSubTxnRequest.sub_txn_num with the subtransaction ID instead of the subtransaction count, causing meta-service cleanup to reject the request. Set the INSERT error state using the same final error formatting as other insert executors, and pass the correct subtransaction count during cloud cleanup. Add regression coverage using the BE FragmentMgr.exec_plan_fragment.failed debug point and verify the transaction can still commit after the failed statement. ### Release note Transaction INSERT execution failures are now returned to clients instead of reporting success. ### Check List (For Author) - Test: Regression test added; not run per request - Behavior changed: Yes, transaction INSERT execution failures are returned to the client and cloud cleanup uses the correct subtransaction count - Does this need documentation: No
Contributor
Author
|
run buildall |
Contributor
FE UT Coverage ReportIncrement line coverage |
Contributor
TPC-H: Total hot run time: 16797 ms |
Contributor
TPC-DS: Total hot run time: 81938 ms |
Contributor
ClickBench: Total hot run time: 14.61 s |
gavinchou
approved these changes
Sep 2, 2026
Contributor
Author
|
run nonConcurrent |
Contributor
Author
|
run feut |
Contributor
FE UT Coverage ReportIncrement line coverage |
Contributor
FE Regression Coverage ReportIncrement line coverage |
924060929
approved these changes
Sep 2, 2026
Contributor
|
PR approved by anyone and no changes requested. |
github-actions
Bot
added
the
approved
Contributor
|
PR approved by at least one committer and no changes requested. |
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
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.
Transaction INSERT execution failures raised directly while dispatching fragments bypass ErrorReport. OlapTxnInsertExecutor catches these failures and aborts the subtransaction, but previously left QueryState successful, causing the client to receive Query OK. Set the INSERT error state when no earlier error exists so the failure reaches the client while preserving errors already reported by ErrorReport.