morningman opened a new pull request, #69566:
URL: https://github.com/apache/doris/pull/69566
### What problem does this PR solve?
Issue Number: None
Related PR: #66473, #66530
Problem Summary:
**In short.** A prepared statement's `COM_STMT_EXECUTE` that fails after its
scans were planned leaves its query-finish callbacks registered for good, keeps
its connector scope open until the statement's next EXECUTE (or forever), and
never stops the scan nodes left for the end of the statement. This PR ends
every EXECUTE the way a COM_QUERY statement ends, which is what branch-4.x
(#66473) does in `handleExecute`.
**Background.** A plugin scan registers a query-finish callback while it
plans its splits (`PluginDrivenScanNode.getSplits` → `QueryFinishCleanup`,
keyed by the query id). The callback releases the connector's per-query read
transaction (Hive full-ACID and insert-only reads open a metastore read
transaction with a shared read lock during `planScan`) and closes the
statement's connector scope, which holds connector metadata and any connector
write transaction the executor has not finished. It runs when the query is
unregistered: `StmtExecutor.finalizeQuery` → `QeProcessorImpl.unregisterQuery`
→ `QueryFinishCallbackRegistry.runAndClear`. A COM_QUERY statement also closes
its `StatementContext` in `ConnectProcessor`'s per-statement finally, which
closes the scope of statements that never registered a callback and stops the
scan nodes registered to stop at the statement end (a remote Doris scan sends
its CloseSession there).
**The problem, and what it cost.** `MysqlConnectProcessor.handleExecute` has
neither. An execution that fails after planning never reaches `finalizeQuery`:
a scan-level SQL block rule (`checkBlockRulesByScan` runs after `planner.plan`
and before `handleQueryWithRetry`), or a prepared `INSERT ... SELECT` whose
transaction fails to begin.
| What the failed EXECUTE leaves behind | Master today | After this PR |
|---|---|---|
| Its `QueryFinishCleanup` in the process-wide registry | kept for good: the
next EXECUTE has a new query id, and COM_STMT_CLOSE and a connection reset do
not touch the registry | run and removed when the EXECUTE ends |
| A connector read transaction / shared read lock opened while planning |
not released by Doris | released when the EXECUTE ends |
| Its connector scope (metadata, a connector write transaction never
committed) | closed at the statement's next EXECUTE
(`PreparedStatementContext.nextStatementContext`), never if there is none |
closed when the EXECUTE ends |
| Scan nodes registered to stop at the statement end | never stopped:
`StatementContext.close()` never runs for an EXECUTE | stopped when the EXECUTE
ends |
**How this PR fixes it.** `handleExecute` runs the execution in a `try`
whose `finally` unregisters the execution's query id and then closes the
statement context that `ExecuteCommand` ran it with, which is the prepared
statement's current context (`PreparedStatementContext.nextStatementContext`).
Unregistering runs and drops the callbacks still registered for the query id,
so they run in the same order as for a finished query: read-transaction release
first, then the scope. Both calls are no-ops for an execution that finished
normally: `finalizeQuery` already unregistered its query id (`runAndClear`
finds nothing), and the scope's `closeAll` is close-once. The master-side
forwarded path already closes the statement context after `handleExecute`, and
closing it twice is a no-op. A binary-protocol execution has sent its whole
result when `execute` returns (Arrow Flight does not use this path), so nothing
still running is closed.
**Results.** `MysqlConnectProcessorExecuteCleanupTest` (new) drives
`handleExecute` with an executor that does what planning leaves behind:
- an EXECUTE that registers a query-finish callback and is then rejected (a
SQL block rule) runs that callback once and closes its scope;
- an EXECUTE that fails before any scan registered closes its scope;
- a successful EXECUTE, whose query is unregistered by `finalizeQuery`, runs
its callback once and closes its scope value once.
With `handleExecute` of master, the first two fail (`expected: <1> but was:
<0>` for the callback and for the scope close) and the third passes.
`MysqlConnectProcessorLongDataTest` and `MysqlConnectProcessorCursorFetchTest`
mock the executor so that it never executes; their connections now carry the
query id the statement's PREPARE left, as a real connection does. These 3
classes and `ExecuteCommandTest`, `ConnectProcessorForwardProtocolTest`,
`ConnectProcessorRetryTest`, `StatementContextTest`,
`QueryFinishCallbackRegistryTest`, `PluginDrivenScanNodeReadTxnReleaseTest`,
`AuditLogWorkloadGroupTest` and `TaskProcessorTest` pass (77 tests).
### Release note
None
### Check List (For Author)
- Test <!-- At least one of them must be included. -->
- [ ] Regression test
- [x] Unit Test
- [ ] Manual test (add detailed scripts or steps below)
- [ ] No need to test or manual test. Explain why:
- [ ] This is a refactor/code format and no logic has been changed.
- [ ] Previous test can cover this change.
- [ ] No code files have been changed.
- [ ] Other reason <!-- Add your reason? -->
- Behavior changed:
- [x] No.
- [ ] Yes. <!-- Explain the behavior change -->
- Does this need documentation?
- [x] No.
- [ ] Yes. <!-- Add document PR link here. eg:
https://github.com/apache/doris-website/pull/1214 -->
### Check List (For Reviewer who merge this PR)
- [ ] Confirm the release note
- [ ] Confirm test cases
- [ ] Confirm document
- [ ] Add branch pick label <!-- Add branch pick label that this PR should
merge into -->
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
