morningman commented on code in PR #69566:
URL: https://github.com/apache/doris/pull/69566#discussion_r4366909793
##########
fe/fe-core/src/main/java/org/apache/doris/qe/MysqlConnectProcessor.java:
##########
@@ -281,6 +285,21 @@ protected void handleExecute(PrepareCommand
prepareCommand, long stmtId, Prepare
}
}
+ /**
+ * COM_STMT_EXECUTE has no per-statement close like the one COM_QUERY runs
in its finally. An execution that
+ * fails after its scans were planned, such as one a scan-level SQL block
rule rejects or a prepared
+ * INSERT ... SELECT whose transaction fails to begin, never reaches
unregisterQuery: the query-finish callbacks
+ * its scans registered (releasing a connector read transaction, closing
the connector scope) would stay in the
+ * registry for good, its connector scope would stay open until the
statement's next EXECUTE, and scan nodes left
+ * for the statement end would never stop. The context ExecuteCommand ran
this execution with is the prepared
+ * statement's current one (see
PreparedStatementContext#nextStatementContext). Both calls are no-ops for an
+ * execution that finished normally, which already unregistered its query
and closed its scope.
+ */
+ private void finishExecution(PreparedStatementContext prepCtx) {
+ QeProcessorImpl.INSTANCE.unregisterQuery(ctx.queryId());
Review 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 sets `cancelled` and does not wait for a run that is already past that
check, and `ConnectorStatementScopeImpl.computeIfAbsent` ignores `closed`, so a
value loaded after `closeAll()` is kept and never closed.
- `finishExecution` runs the sequence every other statement already runs:
`unregisterQuery` (the stop callback, then `QueryFinishCleanup`, in the order
`startSplit` registers them), then `StatementContext.close()`. That is
`finalizeQuery` followed by ConnectProcessor's per-statement finally for
COM_QUERY, and `TaskProcessor.closeTaskContext` for job tasks.
- In the case you describe, COM_QUERY is more exposed today. A first-split
timeout is a planning failure, which never reaches `unregisterQuery` on
COM_QUERY, so the producer is not cancelled at all while
`StatementContext.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.
--
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]
