Repository navigation
[fix](fe) Finish a failed prepared EXECUTE like a failed COM_QUERY - #69566
Conversation
### What problem does this PR solve? Issue Number: None Related PR: apache#66473, apache#66530 Problem Summary: COM_QUERY closes each statement's StatementContext in a finally, and a coordinated query unregisters its query id in finalizeQuery, which runs the query-finish callbacks its scans registered. COM_STMT_EXECUTE (MysqlConnectProcessor.handleExecute) has neither. An execution that fails after its scans were planned, e.g. one a scan-level SQL block rule rejects (checkBlockRulesByScan runs after the plan, before handleQueryWithRetry) or a prepared INSERT ... SELECT whose transaction fails to begin, never reaches unregisterQuery. So: - the QueryFinishCleanup that PluginDrivenScanNode.getSplits registered under the execution's query id stays in the process-wide registry for good: neither the next EXECUTE (a new query id), COM_STMT_CLOSE nor a connection reset removes it, and it holds the execution's connector scope and scan provider. The connector read transaction it should release (Hive ACID reads open one with a shared read lock during planScan) is never committed by Doris; - the execution's connector scope, including a connector write transaction the executor never committed, stays open until the statement's next EXECUTE (PreparedStatementContext.nextStatementContext), and for good when the statement is not executed again; - scan nodes left for the end of the statement (a remote Doris scan's CloseSession) never stop: StatementContext.close() never runs for an EXECUTE. branch-4.x clears the execution's state in a finally of handleExecute. The execution now ends the same way: it unregisters its query id, which runs and drops the callbacks still registered, and closes the statement context ExecuteCommand ran it with, the prepared statement's current one. Both are no-ops for an execution that finished normally, which already unregistered its query and whose scope close is close-once. ### Release note None ### Check List (For Author) - Test: Unit Test - MysqlConnectProcessorExecuteCleanupTest (new): an EXECUTE rejected after planning runs its query-finish callback and closes its scope, one failing before any scan closes its scope, a successful one closes once; the first two fail without the change - MysqlConnectProcessorLongDataTest and MysqlConnectProcessorCursorFetchTest give the connection the query id its PREPARE left, as their mocked executors never execute - ExecuteCommandTest, ConnectProcessorForwardProtocolTest, ConnectProcessorRetryTest, StatementContextTest, QueryFinishCallbackRegistryTest, PluginDrivenScanNodeReadTxnReleaseTest, AuditLogWorkloadGroupTest, TaskProcessorTest - Behavior changed: No - Does this need documentation: No Co-Authored-By: Claude Opus 5.5 (1M context) <[email protected]>
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
run buildall |
|
/review |
There was a problem hiding this comment.
Static review of all four changed files at 854ee08 found one P2 resource-lifetime regression, reported inline. The new finally covers the basic failed-EXECUTE callback/context cleanup, but it can close a scope before an active split producer finishes using it. No existing inline findings were present to carry forward.
Critical checkpoint conclusions:
- Goal and correctness: terminal EXECUTE cleanup is added for success and failure; the inline finding identifies a first-split-timeout interleaving that can permanently leak a late-acquired Iceberg table lease.
- Scope and clarity: the production change is small and localized, reusing existing query-unregister and StatementContext cleanup mechanisms.
- Concurrency and locking: MySQL requests on one connection are serialized, but split producers run on the shared metadata scheduler. Cancellation does not join those producers; close-once scope state is insufficient for this cross-thread lifetime boundary. No new metadata lock order is introduced.
- Lifecycle and initialization: current prepared contexts and refresh rollback were traced. Ordinary repeated callback/scope cleanup is idempotent; the late-publication race is the exception. No static initialization change.
- Configuration: no new configuration.
- Compatibility: no public API, MySQL wire, or storage-format change.
- Thrift/protobuf: no definition changes; cross-branch field-ID comparison is not applicable.
- Parallel paths: checked COM_QUERY, forwarded/proxy EXECUTE, ordinary execution retries, cached point queries, group commit, empty cursor FETCH, and Arrow result ownership. MySQL does not use the Arrow deferred-result branch; cached plans and empty-cursor flags survive StatementContext.close.
- Conditions and errors: the explicit-query-ID branch is unchanged. Real execution assigns an ID before parsing/planning; added test IDs compensate for mocked executors. Ordinary callback and value-close exceptions are isolated by existing cleanup code.
- Tests: the three new JUnit tests exercise real registry/scope cleanup through mocked StmtExecutor instances, including failure and close-once success. They do not execute real SQL-block rejection, INSERT transaction-start failure, concurrent producer timeout, or multi-attempt retry/cache integration.
- Test results: no expected-output files changed. No builds or tests were run under this static-only contract.
- Observability: existing execution, callback, scope-close, and scan-stop logs cover ordinary failures; the inline resource leak lacks a corresponding lifecycle regression test.
- Transactions and persistence: existing scope teardown finalizes abandoned connector write transactions before metadata closes. No EditLog, persisted format, or failover protocol changes.
- Data writes and atomicity: no new commit/write path; the reported consequence concerns resource ownership during failed read planning.
- FE/BE variables: none added.
- Performance and memory: ordinary successful cleanup adds a bounded idempotent lookup/drain; the reported race can retain FileIO/catalog resources after scope retirement.
- Other issues: a callback/Hive-transaction leak for superseded IDs during pre-dispatch planner retries was traced to unchanged code and classified as preexisting, rather than posted as a new defect.
User focus: no additional focus was supplied. Coverage limits are static analysis and the mocked-test gaps above; the reported concurrency interleaving was established from the source, not reproduced at runtime. All candidate and risk dispositions are recorded in the shared ledger.
Review converged after 3 round(s).
| * execution that finished normally, which already unregistered its query and closed its scope. | ||
| */ | ||
| private void finishExecution(PreparedStatementContext prepCtx) { | ||
| QeProcessorImpl.INSTANCE.unregisterQuery(ctx.queryId()); |
There was a problem hiding this comment.
[P2] Wait for split producers before closing the execution scope
An Iceberg streaming scan can hit the first-split timeout while its producer is already running but has not reached the statement-scope lookup; for example, scheduler backlog can start it just before the 30-second deadline. unregisterQuery() runs the scan's stop callback and then QueryFinishCleanup, which calls statementScope.closeAll(), but SplitProducer.close() only requests cancellation and does not wait for that running producer. The producer can therefore resume after the scope was cleared and insert a new ScopedBorrow/tracked table through computeIfAbsent on the already-closed scope. Its split source closes only task iterators, while subsequent closeAll() calls (including the next EXECUTE's reset) return immediately because closed is already true, permanently retaining the table/FileIO/catalog lease. Previously, if the producer finished before the next EXECUTE, the still-open outgoing scope released that holder at reset. Quiesce producers before closing their scope, or defer scope teardown until they complete, and cover timeout versus in-flight scope lookup.
There was a problem hiding this comment.
Thanks, the race is real, but this PR neither introduces nor widens it.
- It comes from two things this PR does not touch:
SplitProducer.close()only setscancelledand does not wait for a run that is already past that check, andConnectorStatementScopeImpl.computeIfAbsentignoresclosed, so a value loaded aftercloseAll()is kept and never closed. finishExecutionruns the sequence every other statement already runs:unregisterQuery(the stop callback, thenQueryFinishCleanup, in the orderstartSplitregisters them), thenStatementContext.close(). That isfinalizeQueryfollowed by ConnectProcessor's per-statement finally for COM_QUERY, andTaskProcessor.closeTaskContextfor job tasks.- In the case you describe, COM_QUERY is more exposed today. A first-split timeout is a planning failure, which never reaches
unregisterQueryon COM_QUERY, so the producer is not cancelled at all whileStatementContext.close()closes the scope: even a producer still queued at the timeout later loads into the closed scope, and it keeps its split source and its registry entry. With this PR, EXECUTE cancels the producer first, so only a producer that is already past its cancellation check can still reach the scope. - Before this PR the same EXECUTE never ran its stop callback or
QueryFinishCleanup: the registry entry stayed for good, the producer was never cancelled, and the scope closed only if the statement was executed again. The sub-case where that later close released the lease is the only one that changes.
Closing the window needs the scope/producer contract fixed for every statement kind: a closed scope that rejects (and closes) late values, which first needs the cloud replan retry to stop reusing a closed scope, or a scope close that waits for the producers to quiesce. I'd rather do that as a separate change than in this PR.
TPC-H: Total hot run time: 28327 ms |
TPC-DS: Total hot run time: 150694 ms |
ClickBench: Total hot run time: 24.07 s |
FE Regression Coverage ReportIncrement line coverage |
…69718) ### What problem does this PR solve? Issue Number: None Related PR: #69298, #69566, #66530 Problem Summary: #### In short A statement's connector state could outlive the statement in two ways on master. A split producer still inside a read when the statement ends went on to load values into the already-closed connector statement scope, such as an Iceberg table borrowed from the table cache, and nothing ever closed them. `EXPLAIN REFRESH MATERIALIZED VIEW` plans on a context of its own that has no query id, so its scans registered their query-finish cleanup under `""` and its connector scope was never closed: each such EXPLAIN over an external base table left one registry entry and one open scope behind, without bound. This PR makes a closed scope refuse to load, and finishes the explain context's own statement when the EXPLAIN ends. This was found while checking master against branch-4.x's #66473 (scan reuse, ported as #66530). branch-4.x has neither the connector statement scope nor the query-finish registry, and its statement resources already refuse registration once closed. #### Background - Every statement has a connector statement scope (`ConnectorStatementScopeImpl`, hung on the `StatementContext`). Connectors keep what the statement shares in it: the `ConnectorMetadata` of each catalog, Iceberg table leases borrowed from the table cache (returning one is what lets the cache close the table's FileIO when it evicts it), Iceberg tracked tables, write transactions, and the query's task groups since #69298. `closeAll()` releases them when the statement ends. - A scan registers two query-finish callbacks under the query id when it starts planning splits: one stops its split producers, the other (`QueryFinishCleanup`) releases a read transaction and closes the scope. #69298 runs them at the end of every attempt, also for EXPLAIN and for plans that fail; #69566 does the same for prepared EXECUTE. - Batch and streaming scans plan on split producers. A producer calls into the connector off the statement thread, for example Iceberg's `streamSplits`, which resolves its table through the scope. #### The problem, and what it cost 1. **Late producers load into the closed scope.** Stopping a scan only flags its producers (`SplitProducer.close()`), and closing the query's task groups cancels queued work but deliberately does not interrupt metadata IO. A producer already inside `getNextBatch` keeps going after `closeAll()`. `closeAll()` had also cleared the scope, so the producer found nothing and loaded again: Iceberg borrowed a new table lease, Hive partition batches built a new `ConnectorMetadata`. Both stayed in the closed scope forever, so the lease was never returned. A value whose load was in flight while `closeAll()` ran was missed the same way. This happens whenever a statement ends while a producer is still reading: an EXPLAIN of a large table (EXPLAIN also starts batch scans), a LIMIT that finishes early, a KILL, a timeout. Nothing fails and no result is wrong; the FE just keeps the leases, metadata and FileIO alive. 2. **`EXPLAIN REFRESH MATERIALIZED VIEW` leaks its own statement.** It plans on an MTMV context of its own (`RefreshMTMVCommand.getExplainConnectContext`), and that context had no query id, so the connector sessions of its scans had `""` as their query id and registered their cleanup under `""`. The end of the outer statement finishes only the outer query id; nothing finishes `""`, and it could not be finished at the end of one statement anyway, since every context without a query id shares it. The explain context's `StatementContext` was never closed either. | Before this PR | Effect | |---|---| | A producer reading when the statement ends | Borrows a new table lease (or builds a new metadata) into the closed scope; never released | | `EXPLAIN REFRESH MATERIALIZED VIEW` over an external base table | One permanent registry entry under `""` and one open connector scope per execution | #### How this PR fixes it - `ConnectorStatementScopeImpl`: after `closeAll()` the statement is over, so `computeIfAbsent` (and with it `computeIfAbsentForPlanning` and `getOrCreateMetadata`) does not run the loader and throws `CancellationException`, as the query's task groups already do once closed; Iceberg's planning paths treat it as "the query stopped" and do not fall back to the SDK. A load that raced the close is released by its caller, because `closeAll()` could not see it. `closeAll()` now removes each entry before releasing its value, still finalizing transactions first, so every value is released exactly once. `getIfPresent` returns nothing once closed. - `StatementContext.getConnectorStatementScopeForNewSession()`: a session built on a statement whose scope is already closed (the lineage of an INSERT, after the insert finished its query, say) captures `ConnectorStatementScope.NONE` and the direct task scheduler, as offline work does, instead of failing in or loading into the closed scope. Sessions captured before the close keep the closed scope, so a producer holding one stops at its next load. - `ExplainCommand`: an explain context of its own gets a query id, and when the EXPLAIN ends, whether planning succeeded or not, it runs that query id's query-finish callbacks and closes the context's `StatementContext`, as `MTMVTask` does for a refresh. The thread-local context is still restored if that cleanup throws. - Comments that said EXPLAIN never registers a query-finish cleanup and that `closeAll()` runs after the producers have quiesced are corrected; the SPI javadoc of `computeIfAbsent` states the closed-scope behavior. #### Results | | Before | After | |---|---|---| | Producer resolving its table after the statement closed the scope | New lease borrowed, never returned | `CancellationException`, nothing borrowed | | Value whose load raced `closeAll()` | Kept in the closed scope, never closed | Closed once by its caller | | Session built after the scope closed | Loads task groups into the closed scope | Offline session (no scope, direct scheduler) | | `EXPLAIN REFRESH MATERIALIZED VIEW` | Entry under `""` and open scope left behind | Its callbacks run once, its scope closed | New tests: - `ConnectorStatementScopeTest`: `closedScopeRefusesToLoadAndHandsOutNothing`, `valueLoadedWhileTheScopeClosesIsReleasedOnceByItsCaller` (the loader blocks on another thread while `closeAll()` runs), `sessionBuiltAfterTheScopeClosesRunsAsOfflineWork`. - `SplitProducerTest.readThatOutlivesItsStatementLoadsNothingIntoTheClosedScope`: a producer is stopped inside `getNextBatch`, the scope closes, then the read resolves its table. - `ExplainRefreshMTMVCommandTest.testExplainCommandFinishesTheStatementOfItsOwnContext`: planning registers a callback under the explain context's query id and keeps a closeable in its scope; after the EXPLAIN, both succeeding and failing, the callback ran once and the value was closed. Each new test was checked against a mutation of the code it guards; every mutation turns its test red: | Mutation | Red test | |---|---| | Drop the closed check before loading | `closedScopeRefusesToLoadAndHandsOutNothing` (the loader ran twice), `readThatOutlivesItsStatementLoadsNothingIntoTheClosedScope` (the late read loaded once) | | Drop the closed check after loading | `valueLoadedWhileTheScopeClosesIsReleasedOnceByItsCaller` (the caller got the value) | | Let a new session capture the closed scope | `sessionBuiltAfterTheScopeClosesRunsAsOfflineWork` (building the session throws: the scope cannot load `engine:query-task-groups`) | | Do not finish the explain context | `testExplainCommandFinishesTheStatementOfItsOwnContext` (callback never ran) | | Give the explain context no query id | `testExplainCommandFinishesTheStatementOfItsOwnContext` (planned without a query id) |
What problem does this PR solve?
Issue Number: None
Related PR: #66473, #66530
Problem Summary:
In short. A prepared statement's
COM_STMT_EXECUTEthat 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 inhandleExecute.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 duringplanScan) 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 itsStatementContextinConnectProcessor'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.handleExecutehas neither. An execution that fails after planning never reachesfinalizeQuery: a scan-level SQL block rule (checkBlockRulesByScanruns afterplanner.planand beforehandleQueryWithRetry), or a preparedINSERT ... SELECTwhose transaction fails to begin.QueryFinishCleanupin the process-wide registryPreparedStatementContext.nextStatementContext), never if there is noneStatementContext.close()never runs for an EXECUTEHow this PR fixes it.
handleExecuteruns the execution in atrywhosefinallyunregisters the execution's query id and then closes the statement context thatExecuteCommandran 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:finalizeQueryalready unregistered its query id (runAndClearfinds nothing), and the scope'scloseAllis close-once. The master-side forwarded path already closes the statement context afterhandleExecute, and closing it twice is a no-op. A binary-protocol execution has sent its whole result whenexecutereturns (Arrow Flight does not use this path), so nothing still running is closed.Results.
MysqlConnectProcessorExecuteCleanupTest(new) driveshandleExecutewith an executor that does what planning leaves behind:finalizeQuery, runs its callback once and closes its scope value once.With
handleExecuteof master, the first two fail (expected: <1> but was: <0>for the callback and for the scope close) and the third passes.MysqlConnectProcessorLongDataTestandMysqlConnectProcessorCursorFetchTestmock 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 andExecuteCommandTest,ConnectProcessorForwardProtocolTest,ConnectProcessorRetryTest,StatementContextTest,QueryFinishCallbackRegistryTest,PluginDrivenScanNodeReadTxnReleaseTest,AuditLogWorkloadGroupTestandTaskProcessorTestpass (77 tests).Release note
None
Check List (For Author)
Test
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)