[fix](stream) Mark streams stale when base tables are dropped - #66287
[fix](stream) Mark streams stale when base tables are dropped#66287seawinde wants to merge 7 commits into
Conversation
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
run buildall |
|
/review |
TPC-H: Total hot run time: 29693 ms |
TPC-DS: Total hot run time: 177743 ms |
ClickBench: Total hot run time: 24.87 s |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Request changes: the drop/recovery behavior is largely implemented, but two correctness issues remain: concurrent readers can null-dereference the volatile cached base table, and available streams report stale qualifiers after supported same-ID rename operations.
Review checkpoints
- Goal and proof: derived disabled/stale state fixes ordinary recoverable drop, original-ID recovery, force drop, and same-name/new-ID recreation. The added unit and IVM regression coverage exercise those paths, but rename coverage would expose MAIN-001.
- Scope and clarity: the change is focused on stream lifecycle state and metadata; no unrelated source changes were found.
- Concurrency: drop/recovery publication and all getter callers were traced. MAIN-002 is a reachable shared-reader race; no separate persistent cleanup/planner race was substantiated.
- Lifecycle: the runtime cache,
Table.isDropped, database registration, recycle-bin recovery, checkpoint load, and journal replay were reviewed. ID-based recovery is sound apart from the accepted issues. - Configuration and compatibility: no configuration, storage-format, function-symbol, schema-column, or FE-BE protocol change is introduced.
- Parallel paths and conditions: table drop, force drop, database drop/recovery, table/database rename, renamed recovery, planner binding, cleanup, metadata, and
SHOW CREATE STREAMwere checked. Rename paths produce MAIN-001. - Tests and results: test SQL ordering/output conventions are correct and the new result file is marked generated. This runner is review-only, so I did not execute builds or tests.
- Observability: the missing-table state is visible through
table_streamsand planner errors; lowering repeated missing-resolution logging to debug is acceptable. - Persistence, transactions, and data writes: explicit flags and base-table IDs remain serialized; no new transaction, EditLog schema, data-write, or master-failover issue was found.
- Performance and other concerns: repeated missing-table resolution is bounded to metadata/cleanup lookups; no additional material performance, memory, or static-initialization issue was found.
- User focus: no additional user-provided review focus was supplied; the full PR was reviewed.
Review completion: complete for the frozen head after the required bounded subagent convergence and final live-state verification.
|
|
||
| public TableIf getBaseTableNullable() { | ||
| if (baseTable instanceof Table && ((Table) baseTable).isDropped) { | ||
| baseTable = null; |
There was a problem hiding this comment.
[P1] Snapshot the volatile base table before dereferencing it
The two baseTable occurrences in this condition are separate volatile reads. After a drop marks the cached table dropped, two concurrent readers can both pass the instanceof; one can then execute baseTable = null before the other evaluates ((Table) baseTable).isDropped, causing that reader to dereference null. Concurrent table_streams requests are sufficient because the stream read lock admits multiple readers. Copy the field to one local snapshot, inspect that snapshot, and resolve/publish the replacement once.
There was a problem hiding this comment.
Fixed in 6baebd9e78a. getBaseTableNullable() now snapshots the volatile field into cachedBaseTable and uses that local for the dropped check and return path. Clearing the shared cache can no longer change the value being dereferenced by another reader.
| // STREAM_COMMENT | ||
| trow.addToColumnValue(new TCell().setStringVal(stream.getComment())); | ||
| List<String> baseTableQualifiers = stream.getBaseTableFullQualifiers(); | ||
| // BASE_TABLE_NAME |
There was a problem hiding this comment.
[P1] Use live qualifiers while the base table is available
TableStreamBaseTableInfo keeps the creation-time names but resolves internal tables by ID. After a supported same-ID rename (ALTER TABLE old RENAME new or RECOVER TABLE old AS new), this line still emits old even though the stream resolves successfully and is reported enabled; SHOW CREATE STREAM emits new, and callers filtering table_streams by the current base name miss the stream. Resolve baseTable first and use baseTable.getFullQualifiers() when it is non-null, falling back to the stored qualifiers only for the unavailable-table case.
There was a problem hiding this comment.
Fixed in 6baebd9e78a. table_streams now resolves the base table first and uses baseTable.getFullQualifiers() while it is available. Stored qualifiers are only the final fallback when neither the live table nor a same-ID recycle-bin entry is available. The unit test covers ALTER TABLE ... RENAME and RECOVER TABLE ... AS.
FE UT Coverage ReportIncrement line coverage |
FE Regression Coverage ReportIncrement line coverage |
2d14096 to
bf0a7de
Compare
|
run buildall |
TPC-H: Total hot run time: 28939 ms |
TPC-DS: Total hot run time: 178423 ms |
bf0a7de to
8a8eec2
Compare
|
run buildall |
ClickBench: Total hot run time: 24.73 s |
|
/review |
TPC-H: Total hot run time: 29310 ms |
TPC-DS: Total hot run time: 177733 ms |
ClickBench: Total hot run time: 24.69 s |
|
Codex automated review failed and did not complete. Error: You've hit your usage limit. Visit https://chatgpt.com/codex/settings/usage to purchase more credits or try again at Aug 5th, 2026 4:32 AM. Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
FE Regression Coverage ReportIncrement line coverage |
|
/review |
There was a problem hiding this comment.
Request changes: the sequential drop/recovery path works, but the current head still has the two existing P1 blockers and two additional metadata/DDL consistency issues: one table_streams row can mix availability snapshots during concurrent DDL, and SHOW CREATE STREAM loses the latest same-ID base identity while the table is recoverably absent.
Review checkpoints
- Goal and proof: ordinary drop, same-name recovery, force drop, and same-name/new-ID recreation derive the intended state. The added unit/regression tests and green FE/P0 CI prove those sequential paths, but not the accepted concurrent-row or
SHOW CREATEcases. - Scope and clarity: the source change is focused on stream availability, metadata, logging, and tests.
- Concurrency: the stream cache, database/table publication, metadata readers, planner binding, and cleanup locks were traced. Existing thread
r3682055286covers the shared-reader NPE;MAIN-001covers the distinct single-row snapshot race. No deadlock or further lock-order issue was found. - Lifecycle: dropped-cache invalidation, force drop, original-ID recovery, rename/recover-as and subsequent re-drop, image load, and replay were reviewed.
MAIN-002is the newly exposed unavailable-state DDL regression; the remaining ID-based reattachment is sound. - Configuration and compatibility: no new config, storage format, function symbol, schema column, or FE-BE protocol is introduced; no rolling-upgrade incompatibility was found.
- Parallel paths and conditions: metadata,
SHOW CREATE, Nereids collection/binding, cleanup, leader/replay, force-drop, and rename paths were checked. Existing threadr3682055289covers the live-qualifier path; the pre-existing Nereids creation-name lookup was not charged to this diff. - Tests and results: ordering/output conventions are correct. Current CI reports FE UT, P0 regression, checkstyle, compile, and the other completed checks green. This review runner forbids local builds/tests, so none were run here; latch-controlled concurrency plus unavailable
SHOW CREATEcoverage for simple drop and rename/recover-as followed by re-drop are still needed. - Observability: lowering expected missing-table resolution to debug and exposing stale state are appropriate; no additional metric is needed.
- Persistence, transactions, and data writes: IDs and explicit flags remain serialized, transient availability is correctly derived, and no EditLog schema, data-write atomicity, master-failover, or transaction-path issue was found.
- Performance and remaining concerns: repeated missing-table lookups are limited to metadata/cleanup paths; no material CPU, memory, static-initialization, or FE-BE propagation issue was found beyond the reported findings.
- User focus: no additional user-provided focus was supplied; the whole PR was reviewed.
Review completion: complete for the frozen head after required bounded subagent convergence and final live-state verification.
| // BASE_TABLE_TYPE | ||
| trow.addToColumnValue(new TCell().setStringVal(baseTable.getType().name())); | ||
| } | ||
| trow.addToColumnValue(new TCell().setStringVal( |
There was a problem hiding this comment.
[P2] Derive the row from one availability snapshot
baseTable is reused only for BASE_TABLE_TYPE; isDisabled(), isStale(), and getStaleReason() each resolve it again. Because base-table drop/recovery uses different locks from this stream read lock, a recovery after the type lookup can emit N/A with enabled/non-stale state, while a drop can emit OLAP with disabled/stale state. Resolve availability once per row and derive all four fields from that snapshot while still combining the persisted flags; a latch-controlled DDL test would cover the interleaving.
There was a problem hiding this comment.
Fixed in 6baebd9e78a. fillTableStreamValuesMetadataResult() resolves baseTable once per TRow and passes that snapshot to the qualifier, disabled, stale, and stale-reason calculations. The overloads still combine the snapshot with the persisted disabled/stale flags, so one row can no longer mix pre-drop/post-drop or pre-recovery/post-recovery availability.
| } | ||
|
|
||
| public TableIf getBaseTableNullable() { | ||
| if (baseTable instanceof Table && ((Table) baseTable).isDropped) { |
There was a problem hiding this comment.
[P2] Preserve the latest base identity before clearing it
Once this branch clears the dropped table, getBaseTableNullable() returns null and Env.getDdlStmt() emits ON TABLE UNKNOWN, making SHOW CREATE STREAM non-round-trippable during a recoverable drop. Falling back directly to TableStreamBaseTableInfo is not enough after old is renamed/recovered as new and dropped again: that descriptor still says old, while the recycle bin can recover only new. Preserve the latest same-ID live qualifiers in replay-equivalent state before clearing them (or resolve that recycle entry by ID), and cover both a simple drop and rename/recover-as followed by re-drop.
There was a problem hiding this comment.
Fixed in 6baebd9e78a. For an unavailable internal base table, display qualifiers are resolved from the recycle bin by (dbId, tableId) before falling back to creation-time qualifiers. SHOW CREATE STREAM now uses those qualifiers instead of UNKNOWN. Coverage includes a simple recoverable drop, rename then drop, Gson deserialization as a replay/restart-equivalent path, RECOVER TABLE ... AS, and a second drop retaining the recovered name.
| trow.addToColumnValue(new TCell().setStringVal(stream.getScanTypeString())); | ||
| // STREAM_COMMENT | ||
| trow.addToColumnValue(new TCell().setStringVal(stream.getComment())); | ||
| List<String> baseTableQualifiers = stream.getBaseTableFullQualifiers(); |
There was a problem hiding this comment.
[P1] 基表同 ID 重命名后，table_streams 仍返回旧名称
getBaseTableFullQualifiers() 永远返回创建 stream 时保存的名称(基于basetable info)。
There was a problem hiding this comment.
Fixed in 6baebd9e78a. getBaseTableFullQualifiers() no longer always returns the creation-time qualifiers from baseTableInfo: when the same-ID base table is available, it returns the live table qualifiers, so table_streams reflects ALTER TABLE ... RENAME; when the table is recoverably dropped, it resolves the same-ID recycle-bin entry; only if neither can be resolved does it fall back to the creation-time qualifiers. table_streams also resolves the base table once per row and passes that snapshot into the qualifier calculation. Unit coverage includes ALTER TABLE ... RENAME and RECOVER TABLE ... AS.
|
run buildall |
|
/review |
TPC-H: Total hot run time: 29100 ms |
TPC-DS: Total hot run time: 166374 ms |
ClickBench: Total hot run time: 23.87 s |
|
/review |
There was a problem hiding this comment.
Request changes: the stable-ID drop and recovery lifecycle is largely correct, but planner collection has one P1 correctness bug: an implicit stream-base lock dependency can overwrite an explicitly resolved relation in the shared name cache.
Review checkpoints
- Goal and proof: The patch dynamically reports the stream disabled/stale while its stable-ID base is unavailable and resumes the same base object after recovery. The FE tests cover the sequential lifecycle and the regression proves a real post-recovery scan. The concurrent planner fence described inline remains incorrect.
- Scope and clarity: The production changes are focused, except that the qualifier-to-relation resolver map is also used as an implicit lock-resource set, which causes the accepted finding.
- Concurrency and locking: I traced query collection/binding against table and database drop, rename, and recovery, plus metadata, recycle, and cleanup actors. Post-lock ID-ordered acquisition has no new deadlock or reverse lock order; the pre-lock unconditional cache overwrite is the P1 issue.
- Lifecycle, persistence, and compatibility: Stable-ID invalidation, same-object recovery, Gson/edit-log/replay, cleanup, and consumption paths are consistent on ordinary schedules. The patch adds no configuration, FE/BE protocol, storage-format, serialized availability field, or rolling-upgrade concern.
- Parallel and conditional paths: QUERY and INSERT_TARGET paths, view/CTE/subquery recursion, wrapper construction, metadata, SHOW CREATE, consumption, and cleanup were checked. A detached-base qualifier dereference is another manifestation of the accepted cache/resource abstraction bug; other name-durability concerns duplicate existing review threads.
- Tests and results: The changed expected output is deterministic, and the current head's FE unit, P0 regression, compile, and style checks are green. Per the review-only instructions, I did not run local builds or tests. A latch-controlled rename/collision test is still needed for the inline finding.
- Observability, performance, and data writes: Metadata exposes state and reason, expected absence logging avoids warning spam, and the added ordinary-path work is constant time. No distinct transaction, data-write, master-failover, or material performance issue survived review.
- User focus: No additional focus was supplied, so the full PR was reviewed.
- Completion: Converged after two complete rounds, each with two normal full-scope reviews and one separate risk-focused review. One new distinct inline finding survived live duplicate suppression.
| default: | ||
| throw new AnalysisException("Unknown table from " + tableFrom); | ||
| } | ||
| tables.put(baseTable.getFullQualifiers(), baseTable); |
There was a problem hiding this comment.
[P1] Keep stream lock dependencies out of the relation cache
Reduced plan:
Join
UnboundRelation(db.b) // table B
UnboundRelation(db.s) // stream base A, currently named a
If collection first caches db.b -> B, another session can rename B: b -> c and A: a -> b before the collector visits s. This unconditional put then replaces the already-resolved db.b entry with A. StatementContext.lock() omits B, and BindRelation.getAndCacheTable(db.b, ...) returns A, so the bound tree scans A for both branches and can silently return the wrong rows. This is distinct from the existing replacement-table thread: the stream base is resolved by the correct stable ID here, but the new write corrupts an explicit relation already fenced in the cache. Please keep implicit base lock resources in a separate ID/object-keyed collection consumed by lock(), and cover this pre-lock rename interleaving with a latch-controlled test.
There was a problem hiding this comment.
Addressed in a7850adc43a.
CollectRelation now only validates that the stream base resolves by its persisted stable ID; it no longer writes the implicit base object into the qualifier-to-relation cache. StatementContext.lock() expands each collected BaseTableStream into its stable-ID base table only in the existing local ID-ordered lock queue. This keeps tables[db.b] -> B intact while still locking both the explicit table B and stream base A. No additional statement-level dependency collection was introduced. The external-metadata preload guard also recognizes the implicit stream-base plan lock.
Added coverage for:
- a latch-controlled pre-lock interleaving that collects B, renames
B: b -> candA: a -> b, then verifiestables[db.b]remains B while the stream still resolves A by stable ID; - locking both the explicit relation and stream base without locking the stream object or replacing the relation cache entry;
- recognizing the stream base lock in external metadata preload.
Rebased onto the latest master and verified with:
./run-fe-ut.sh --run org.apache.doris.catalog.DropTableStreamTest,org.apache.doris.nereids.StatementContextTest,org.apache.doris.nereids.trees.plans.ExplainTableStreamPlanTest
Result: 40 tests run, 0 failures, 0 errors, and the FE reactor finished with BUILD SUCCESS.
Issue Number: N/A Related PR: yujun777#29 Problem Summary: Dropping a stream base table left the stream enabled and non-stale because its runtime cache continued returning the dropped table. Derive stream availability from the persisted base table identity, preserve diagnostic qualifiers when the table is unavailable, and allow recovery only when the original table ID returns. Add unit and regression coverage for drop, recover, and force-drop behavior. Streams whose base tables are unavailable are now reported disabled and stale. - Test: Unit Test - ./run-fe-ut.sh --run org.apache.doris.catalog.DropTableStreamTest (4 tests) - env DISABLE_BUILD_UI=ON ./build.sh --fe - Behavior changed: Yes. Streams become disabled and stale while their base tables are unavailable. - Does this need documentation: No
### What problem does this PR solve? Issue Number: N/A Related PR: apache#61382 Problem Summary: Stream metadata could dereference a concurrently cleared base table, mix availability states within one table_streams row, and keep creation-time base table names after same-ID rename or recovery. Resolve the base table once per metadata row, use live qualifiers when available, and recover the latest dropped-table identity from the recycle bin for SHOW CREATE without adding persistent state. ### Release note Stream metadata and SHOW CREATE STREAM now preserve consistent, current base table identity across rename, drop, and recovery. ### Check List (For Author) - Test: Unit Test - ./run-fe-ut.sh --run org.apache.doris.catalog.DropTableStreamTest (5 tests passed before the final SHOW CREATE assertion; the final source and test compilation passed, but rerunning was blocked by an external IntelliJ JPS error-stub class in target/test-classes) - env DISABLE_BUILD_UI=ON ./build.sh --fe --clean (passed with 0 checkstyle violations before the external JPS process began overwriting target classes) - Behavior changed: Yes. Stream metadata uses one base-table availability snapshot and preserves the latest same-ID base table qualifiers. - Does this need documentation: No
### What problem does this PR solve? Issue Number: N/A Related PR: apache#66287 Problem Summary: A recycled base table retained its pre-rename database name. Resolving full qualifiers through that detached table could dereference a missing database after the database was renamed. Resolve internal database qualifiers by stable database ID and read only the table name from live or recycled table metadata, with persisted names as fallback. ### Release note Streams with recycled base tables continue to expose valid qualifiers after database rename. ### Check List (For Author) - Test: Unit Test - ./run-fe-ut.sh --run org.apache.doris.catalog.DropTableStreamTest - Behavior changed: Yes. Stream metadata uses the current internal database name after database rename. - Does this need documentation: No
### What problem does this PR solve? Issue Number: None Related PR: apache#66287 Problem Summary: Stream relation collection previously re-resolved a renamed or recovered base table by display name, which could cache and lock a same-name replacement while binding later resolved the original table by stable ID. Cache the ID-resolved base table directly so the planner locks the object used during stream binding, while preserving the existing missing-table error message. ### Release note None ### Check List (For Author) - Test: FE unit test - ./run-fe-ut.sh --run org.apache.doris.catalog.DropTableStreamTest - Behavior changed: No - Does this need documentation: No
### What problem does this PR solve? Issue Number: close apache#65418 Related PR: apache#66287 Problem Summary: The previous stable-ID locking fix cached a stream base table under its current name qualifier. A pre-lock rename could therefore overwrite an explicit relation already resolved under the same qualifier and make binding scan the wrong table. Keep the relation cache limited to SQL relations and expand each stream base table by stable ID only when constructing the existing ordered lock queue. ### Release note None ### Check List (For Author) - Test: FE unit test and FE build - ./run-fe-ut.sh --run org.apache.doris.nereids.StatementContextTest,org.apache.doris.nereids.trees.plans.ExplainTableStreamPlanTest - ./run-fe-ut.sh --run org.apache.doris.catalog.DropTableStreamTest - env DISABLE_BUILD_UI=ON ./build.sh --fe - Behavior changed: Yes. Explicit relation bindings are preserved while stream base tables are locked by stable ID. - Does this need documentation: No
a0238b2 to
a7850ad
Compare
|
run buildall |
|
/review |
### What problem does this PR solve? Issue Number: close apache#65418 Related PR: apache#66287 Problem Summary: Document why stream base tables are validated by stable ID without entering the qualifier relation cache, and why implicit base dependencies are expanded only in the local ID-ordered planner lock queue. The comments preserve the concurrency invariant behind the relation-cache collision fix. ### Release note None ### Check List (For Author) - Test: FE unit test - ./run-fe-ut.sh --run org.apache.doris.nereids.StatementContextTest,org.apache.doris.nereids.trees.plans.ExplainTableStreamPlanTest - Behavior changed: No - Does this need documentation: No
|
run buildall |
|
/review |
|
Codex automated review failed and did not complete. Error: Codex completed, but no new pull request review was submitted for the current head SHA. Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
There was a problem hiding this comment.
Requesting changes: the sequential drop/recover behavior works, but three correctness and concurrency gaps remain. Recovery can expose a still-dropped table as available, MTMVs over streams lose the actual OLAP dependency and can remain stale, and a drop/recover race can bypass external-metadata preload and perform remote work under an internal table lock.
Critical checkpoint conclusions
- Goal and proof: the code makes streams disabled/stale while their stable-ID base is unavailable and restores them when that ID returns; the unit and regression additions prove the sequential path, but not the three inline scenarios.
- Scope: the 12-file FE/test change is focused on stream availability, qualifiers, planner dependencies, and coverage; no unrelated production change was found.
- Concurrency: metadata readers, recovery DDL, and planner threads access the volatile base cache under different database/table/recycle/stream locks. ID-ordered planner acquisition and release accounting are otherwise sound, but the publication and preload phase races remain.
- Lifecycle: normal/force/replay drop, recovery/replay recovery, rename, recycle erasure, Gson reload, and wrapper binding were traced. Stable IDs fence same-name replacements, and no lock-order cycle, static-initialization issue, or leaked lifecycle was found.
- Configuration: no configuration item was added or changed.
- Compatibility: no FE-BE symbol, thrift field, storage format, or new persisted field was introduced; existing image and replay state remains compatible.
- Parallel paths: query, insert-target, and MTMV maps all reach the ID-ordered lock expansion. Non-lock dependency consumers do not receive the implicit base, producing the MTMV issue called out inline; audit/view-dependency effects share that root cause.
- Conditions and error handling: missing bases fail planning and expected missing-base logging is appropriately DEBUG, but a cache-miss lookup does not apply the same dropped-state condition as the cached path.
- Test coverage: JUnit 5 and ordered regression coverage exercise sequential drop/recover, rename, force drop, reload, metadata, and lock-cache behavior. The accepted findings need an MTMV refresh case and latch-controlled recovery/preload races.
- Test results: the added
.outrows are ordered and consistent with the SQL. This review runner performed no build or test, as required by the review instructions, so author/CI results were not independently reproduced here. - Observability:
table_streamsexposes the new state and DEBUG logging avoids expected warning noise; no additional metric is required for this metadata path. - Transaction and persistence: no transaction protocol or EditLog schema changed; persisted stream identity and replay-equivalent paths were checked.
- Data writes: the production change does not add a data-write path; the MTMV finding affects refresh/freshness correctness after writes to the base.
- FE-BE variables and nullable handling: no new FE-BE variable or BE nullable path is involved.
- Performance: no general hot-path regression was found; the accepted preload race can move connector metadata I/O back under an internal table lock.
- Other issues: existing qualifier and relation-cache threads were duplicate-fenced, including the SHOW CREATE residual. No security-sensitive behavior was requested or reviewed, and no further non-duplicate issue remained.
User focus: review_focus.txt supplied no additional focus, so the entire PR and all required checkpoints were reviewed.
Review completion: converged after Round 2; both normal full-review subagents and the independent risk challenger returned NO_NEW_VALUABLE_FINDINGS, and every ledger candidate is accepted, dismissed, or duplicate-fenced.
| return null; | ||
| } | ||
| if (cachedBaseTable == null) { | ||
| cachedBaseTable = baseTableInfo.getTableNullable(); |
There was a problem hiding this comment.
[P2] Recheck dropped state after resolving a cache miss
During RECOVER TABLE, innerRecoverTable() holds A's write lock, but Database.registerTable() publishes A in the ID/name maps before calling unmarkDropped(). An unlocked table_streams request can therefore reach this cache-miss branch while A.isDropped is still true. Because the looked-up value is cached and returned without the dropped check applied to the initial snapshot, that row reports OLAP/enabled/non-stale before recovery has made the table available. Please make publication atomic to unlocked readers or apply the same dropped-state check to the resolved candidate, and add a latch test paused between map publication and unmarkDropped().
| // we need to collect stream table's base table as well | ||
| if (table instanceof BaseTableStream) { | ||
| collectFromTableStream((BaseTableStream) table, cascadesContext, tableFrom, unboundRelation); | ||
| collectFromTableStream((BaseTableStream) table); |
There was a problem hiding this comment.
[P1] Preserve the stream base in MTMV dependency tracking
Reduced plan:
MTMV M
UnboundRelation(S) // stream over OLAP table T
This change validates T but leaves StatementContext.tables containing only S; T is reintroduced only in the local lock queue. Both MTMV creation and every refresh build MTMVRelation from getTables().values(), so T is absent. Since S is not MTMVRelatedTableIf, isSyncWithBaseTable() treats it as synchronized and snapshot generation skips it, allowing a commit to T to leave M considered fresh and absent from T's invalidation relation. Keep the stable base in a separate ID/object-keyed dependency collection consumed by locking and MTMV relation/snapshot tracking (without putting it back in the qualifier cache), and add an async-MV-over-stream refresh test.
| } | ||
| if (tableIf instanceof BaseTableStream) { | ||
| // Mirror addTablesToLock(): a stream needs no plan lock itself, but its stable-ID base may need one. | ||
| TableIf baseTable = ((BaseTableStream) tableIf).getBaseTableNullable(); |
There was a problem hiding this comment.
[P2] Use one stream-base snapshot for preload and locking
For Join(S on internal A, external E), collection can validate A, A can be dropped before this preload gate (so the preload result is memoized as skipped), and A can recover before lock() resolves it again and acquires A's read lock. Binding E then still loads its snapshot/schema while that internal lock is held, defeating the preload boundary that keeps remote metadata work outside table locks. Freeze the stable stream-base dependency during collection and use the same set for both preload gating and ID-ordered locking (or conservatively treat a collected stream as a possible lock dependency), with a latch-controlled drop/gate/recover/lock test.
FE UT Coverage ReportIncrement line coverage |
TPC-H: Total hot run time: 28844 ms |
TPC-DS: Total hot run time: 158345 ms |
ClickBench: Total hot run time: 23.73 s |
FE Regression Coverage ReportIncrement line coverage |
FE Regression Coverage ReportIncrement line coverage |
### What problem does this PR solve? Issue Number: close apache#65418 Related PR: apache#66287 Problem Summary: Stream base tables are implicit planning dependencies resolved by stable table ID. Keeping them in the qualifier-keyed relation cache can overwrite an explicitly resolved relation after concurrent renames, while resolving them again during preload or locking can select a different object snapshot. MTMV relation generation also omitted the underlying stream base after it was removed from the relation cache. Store exact implicit dependency objects separately, reuse them for preload and locking, include them in all-level MTMV dependencies, and reject dropped tables during cache-miss recovery resolution. ### Release note Streams continue to track and lock their original base tables by stable identity without corrupting explicit relation bindings. ### Check List (For Author) - Test: Unit Test - 43 FE tests passed across DropTableStreamTest, StatementContextTest, ExplainTableStreamPlanTest, and MTMVRelationTest - StatementContextTest rerun: 11 tests passed - Java compilation and Checkstyle passed - Behavior changed: Yes. Stream base dependencies use a stable per-statement object snapshot for planning locks and MTMV dependency tracking. - Does this need documentation: No
|
run buildall |
|
/review |
There was a problem hiding this comment.
Requesting changes for two correctness gaps that remain on 25d0c27f52c20efa8496732fcb0ab457e21e9377:
- P1 — persisted MTMV compatibility: newly analyzed creation/refresh now tracks the stable stream base, but load compatibility does not repair an already persisted malformed relation. The base can remain absent from commit routing and freshness snapshots, allowing stale rewrite results after upgrade.
- P2 — database recovery lifecycle: the cached-base check observes only
Table.isDropped.RECOVER DATABASEclears that flag before the owning database is published, so a cross-database stream can expose the detached base during recovery and potentially after a failed publication attempt.
Critical-checkpoint conclusions:
- Goal and proof: the patch correctly makes direct table drop/recover affect stream state and repairs stable dependency capture for current planning. The unit/regression additions cover direct table lifecycle, identity capture, locking/preload reuse, and current MTMV relation construction, but they do not prove cross-database recovery or old-image migration; those are the two blocking gaps above.
- Focus: production changes are otherwise focused on stream availability, stable planner dependencies, and their metadata consumers. No extra user focus was supplied.
- Concurrency: collection, preload, and locking now reuse one identity-stable dependency snapshot and table locks remain ID-ordered. The remaining race is the database-recovery publication ordering described inline; no additional lock-order cycle was found.
- Lifecycle: cached stream bases, recycle state, drop/recover-as, force-drop/replacement, replay/load, and erase paths were traced. The table-only lifecycle fence is incomplete for an unpublished owning database.
- Configuration: no production configuration or dynamic-update contract is added. Test-only flags are isolated by FE Surefire's per-class JVM behavior.
- Compatibility: there is no FE/BE protocol, symbol, or storage-format change. Catalog rolling-upgrade compatibility is incomplete for an existing MTMV relation that omits the stable stream base.
- Parallel paths and conditions: both
RECOVER TABLEandRECOVER DATABASE, cache-hit/cache-miss resolution, creation/refresh, nested views, preload/locking, invalidation, freshness, and rewrite discovery were checked. The changed dropped-table condition fixes the reported table-recovery path but misses database recovery. - Tests and results: the changed tests and ordered
.outexpectations were inspected for fidelity and determinism. No builds or tests were run because the authoritative review task prohibited execution. Missing async-MTMV coverage already requested in an existing thread was not duplicated. - Observability: existing identifiers and log levels are sufficient for the new normal paths; no separate metrics issue was found. The recycle lookup's downgrade to DEBUG did not hide a distinct actionable failure.
- Persistence and transactions: no data-write transaction path changes. MTMV relation/snapshot persistence and registration were reviewed; the old-relation migration gap is the persistence finding above. Edit-log/replay changes otherwise remain equivalent.
- Data writes and FE/BE variables: no new data-write atomicity surface or FE-to-BE variable is introduced.
- Performance: identity deduplication avoids double locking, and no material new CPU, memory, or redundant-work issue was found.
- Other issues: all 14 changed files, direct consumers, and existing review threads were rechecked. A second full-review round with a separate risk pass converged with no additional valuable findings.
| baseTable = baseTableInfo.getTableNullable(); | ||
| TableIf cachedBaseTable = baseTable; | ||
| if (cachedBaseTable != null) { | ||
| if (cachedBaseTable instanceof Table && ((Table) cachedBaseTable).isDropped) { |
There was a problem hiding this comment.
[P2] Keep cached bases unavailable until their database recovers
A stream can live in ds while its base A lives in db. During RECOVER DATABASE db, recoverAllTables() calls Database.registerTable(A) and clears A.isDropped before InternalCatalog.recoverDatabase() republishes and unmarks db. If the stream retained its pre-drop cached pointer (because it was not read while db was dropped), this branch returns A in that gap, so metadata and planning expose it while its owning database is unavailable; if the later catalog lock/name check fails, that exposure can outlive the recovery attempt because the recycle entries were already removed. This is distinct from the existing RECOVER TABLE cache-miss thread because here the table flag already passes. Please fence on the owning database lifecycle as well (or defer clearing member-table dropped flags), and cover cross-database recovery with a latch test.
| return Pair.of(Sets.newHashSet(ctx.getStatementContext().getTables().values()), | ||
| Set<TableIf> baseTables = Sets.newHashSet(ctx.getStatementContext().getTables().values()); | ||
| // Implicit dependencies are all-level tables, not relations written at the first query level. | ||
| baseTables.addAll(ctx.getStatementContext().getImplicitTableDependencies()); |
There was a problem hiding this comment.
[P1] Migrate persisted stream dependencies on upgrade
These additions repair newly analyzed creation and refresh, but an MTMV image can already contain stream S without stable base T: old dependency collection could name-resolve replacement B while the stream wrapper still scanned its cached T, then persist a successful refresh as {S, B} with no T snapshot. On load, MTMVRelation.compatible() only normalizes stored entries and MTMV.compatible() re-registers them unchanged. T therefore has no commit-event edge or recorded snapshot, while freshness treats S as synchronous; the MTMV can remain rewrite-eligible with stale rows until a later refresh succeeds. Please expand persisted stream dependencies before registration (or conservatively invalidate them) and add an old-image upgrade test.
FE UT Coverage ReportIncrement line coverage |
FE Regression Coverage ReportIncrement line coverage |
4 participants
What problem does this PR solve?
Issue Number: #64518
Related PR: #61382
Problem Summary:
Dropping a stream base table left the stream enabled and non-stale in
information_schema.table_streams. Recoverable drops could also keep usingthe cached dropped table instead of resolving the persisted base table
identity.
Root cause:
BaseTableStream.getBaseTableNullable()retained a cachedTableafter it was marked dropped, whileisDisabled(),isStale(), andgetStaleReason()only returned their persisted flags.Change Summary:
BaseTableStream.javaTableStreamManager.javaN/A.TableStreamBaseTableInfo.javaDropTableStreamTest.javastateDiagram-v2 [*] --> Available Available --> Stale: drop base table Stale --> Available: recover original table ID Stale --> Stale: create same-name table with new IDRelease note
Streams whose base tables are unavailable are now reported disabled and
stale.
Check List (For Author)
Test
Local verification:
./run-fe-ut.sh --run org.apache.doris.catalog.DropTableStreamTest(4 tests)env DISABLE_BUILD_UI=ON ./build.sh --feBehavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)