[fix](insert) reset skipAuth on all INSERT OVERWRITE exit paths - #66383
Merged
CalvinKirs merged 1 commit intoAug 4, 2026
Conversation
CalvinKirs
requested review from
924060929,
englefly,
morrySnow and
starocean999
as code owners
August 3, 2026 09:42
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
CalvinKirs
force-pushed
the
fix-insert-overwrite-skipauth
branch
2 times, most recently
from
August 3, 2026 10:04
57858ce to
373aacf
Compare
Member
Author
|
run buildall |
Member
Author
|
/review |
924060929
reviewed
Aug 3, 2026
| // Set the flag here, inside the try, so the finally below always pairs the reset even if | ||
| // an earlier step (e.g. the @branch guard) throws before we get here. | ||
| if (physicalTableSink instanceof PhysicalOlapTableSink && targetTable instanceof OlapTable) { | ||
| ConnectContext.get().setSkipAuth(true); |
Contributor
There was a problem hiding this comment.
use local variable ctx instead of
![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 additional blocking or correctness issue in this change.
Checkpoint conclusions:
- Goal and proof: the patch closes the connection-level
skipAuthleak by moving the OLAP-only enable into the existingtry, after the branch and running-table guards. The new JUnit 5 regression is a valid base-fails/head-passes oracle for the@branchfailure path: the base sets the flag before that exception, while the patched path reaches the same exception without touching it. - Scope and clarity: the change is focused to the flag boundary plus one regression test. The existing inline thread already asks to use the local
ctx; I did not duplicate it because production caller tracing did not establish a separate context-identity bug. - Authorization and lifecycle: source analysis and the explicit target
LOADcheck remain before the bypass. Task registration, inner insert planning, temp-partition add/replace, cancellation returns, failure cleanup, and rollback after enablement remain inside the pairedtry/finally; the reset is the firstfinallyaction. Known MySQL/proxy and MTMV callers install the same context thread-locally, and no production pre-existing-true caller into this command was found. - Concurrency and data writes: no new concurrency, locking, or transaction protocol is introduced. The existing
isRunning/task-manager synchronization and partition write/replace paths are unchanged; normal, auto-detect, cancellation, MTMV, remote, and connector paths were traced, with the bypass condition still limited to local OLAP targets. - Compatibility, persistence, configuration, and observability: no protocol/storage-format compatibility, EditLog/persistence, configuration, FE-BE variable propagation, or new observability requirement applies. The change adds no meaningful hot-path cost.
- Tests and validation: the regression fixture, parser reachability, thread-local context setup, and old/new control flow were reviewed statically. Per the review-only workflow I did not run builds or tests locally. The external CheckStyle job is green; FE UT and compile were still pending at final verification.
- Review focus and completion: no additional user-provided focus was supplied, so the full PR was reviewed. Two normal reviewers and a separate lifecycle-risk reviewer all returned
NO_NEW_VALUABLE_FINDINGSin round 1 against the same frozen head, and the final changed-file/unresolved-candidate sweep was clean.
CalvinKirs
force-pushed
the
fix-insert-overwrite-skipauth
branch
from
August 3, 2026 12:04
373aacf to
0cab8bf
Compare
Member
Author
|
run buildall |
INSERT OVERWRITE marks the connection with skipAuth=true so its internal partition-replacement work can run, and clears it in a finally block. The flag was set before that try block, so any early return or exception in between (for example the @branch-on-non-iceberg guard, or a running-table conflict) returned without the finally ever running and left skipAuth set for the rest of the connection. Set the flag as the first statement inside the try instead, under the same OLAP-only condition as before, so it is always paired with the reset in the finally. Add a regression test that runs a failing INSERT OVERWRITE ... @Branch and asserts skipAuth is reset afterwards.
CalvinKirs
force-pushed
the
fix-insert-overwrite-skipauth
branch
from
August 3, 2026 12:16
0cab8bf to
ceb4d2d
Compare
Member
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 28539 ms |
Contributor
TPC-DS: Total hot run time: 169350 ms |
Contributor
ClickBench: Total hot run time: 24.06 s |
Contributor
FE UT Coverage ReportIncrement line coverage |
Contributor
FE Regression Coverage ReportIncrement line coverage |
924060929
approved these changes
Aug 4, 2026
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.
What problem does this PR solve?
InsertOverwriteTableCommandruns its internal partition-replacement workwith
ConnectContext.skipAuth=truefor OLAP targets, and clears the flagin a
finallyblock. The flag was set before thattryblock, so if anystatement in between returned early or threw — for example the
@branch-on-non-iceberg guard, orrecordRunningTableOrExceptionwhen thesame table already has an overwrite running — the method exited without the
finallyever running, andskipAuthstayed set for the rest of thatconnection.
What changed
Move
setSkipAuth(true)to the first statement inside the existingtryblock (keeping the same OLAP-only condition), so it is always paired with
the
setSkipAuth(false)infinally. The guards that can throw before thatpoint now run while
skipAuthis still untouched, so there is nothing toleak. The
finallyis unchanged.Test
Added
InsertOverwriteSkipAuthResetTest: runs a failingINSERT OVERWRITE ... @branchagainst an OLAP table and assertsskipAuthis reset afterwards. It fails on the old code(
expected: <false> but was: <true>) and passes with this change.Checklist
mvn checkstyle:check -pl fe-corepasses