morningman opened a new pull request, #69718:
URL: https://github.com/apache/doris/pull/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) |
### Release note
None
### Check List (For Author)
- Test: Unit Test
- FE UT: 51 related test classes in fe-core, 457 tests, all pass with
`TZ=Asia/Shanghai`: `ConnectorStatementScopeTest`, `SplitProducerTest`,
`ExplainRefreshMTMVCommandTest`, `StmtExecutorTest`,
`ConnectorSessionImplTest`, `QueryTaskGroupsTest`,
`QueryFinishCallbackRegistryTest`, `MysqlConnectProcessorExecuteCleanupTest`,
`ExecuteCommandTest`, `StatementContextTest`, `TaskProcessorTest`,
`ConnectProcessorRetryTest`, `PluginDrivenScanNode*Test` (36 classes),
`PluginDrivenMetadataTest`, `PluginDrivenMissingTableTest`,
`NereidsPlannerConnectorPlanningTest`, `SplitAssignmentTest`,
`SplitStreamingConcurrencyTest`, `CloudInsertProfileRetryTest`; fe-core
Checkstyle passes
- Behavior changed: Yes. A connector statement scope refuses to load once
the statement has closed it (`CancellationException`); a connector session
built after that runs without a statement scope.
- Does this need documentation: No
--
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]
