[fix](external catalog) Handle incomplete table ownership [branch-4.0] - #66600
[fix](external catalog) Handle incomplete table ownership [branch-4.0]#66600Gabriel39 wants to merge 2 commits into
Conversation
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
run buildall |
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Request changes: this backport does not repair ownership on branch-4.0's actual recovery lifecycle.
Critical checkpoints:
- Recovery/lifecycle: blocking.
ExternalDatabase.registerTableis the HMS event-registration path, and its caller constructs a fresh table with the same catalog/database before entering it. Branch-4.0 image recovery instead rebuilds non-persistent external meta caches throughbuildDbForInit/buildTableForInit; it does not route a recovered table through these setters. - Publication, naming, and locking: verified for the HMS event path. The caller holds the database write lock and
MetaCache.updateCachepublishes the same already-owned object under the resolved local key. - MTMV fallback: the guard prevents
BaseTableInfofrom dereferencing a missing owner, but the review found no branch-4.0 production path that creates such an ownerless cached table. The conditional downstream concern was therefore dismissed rather than reported separately. - Compatibility/tests: the mixed test frameworks match module conventions and CheckStyle is green. FE UT and compile are still pending. No local build was run because this review runner forbids builds. The added tests manufacture the state and do not exercise checkpoint/follower recovery.
- User focus: no additional focus was supplied; the complete four-file diff was reviewed.
Review converged after two complete rounds, including separate risk-focused scans. One inline blocking issue remains; all other candidates were verified, deduplicated, or dismissed with evidence.
| String tableName = tableIf.getName(); | ||
| T table = (T) tableIf; | ||
| // Replayed metadata may lose these non-persistent owner references, so restore them before publication. | ||
| table.setCatalog(extCatalog); |
There was a problem hiding this comment.
registerTable is not the checkpoint/cache-recovery boundary described by this PR. Its only production external caller, CatalogMgr.registerExternalTableFromEvent, first calls buildTableForInit(..., hmsCatalog, db, false), whose constructor has already installed these exact owners (and nameMapping), so these assignments are no-ops there. Branch-4.0 recovery deliberately no longer persists external database/table maps: the external meta caches are rebuilt lazily through buildDbForInit/buildTableForInit, and no recovered table is routed through this method. A standalone deserialized ExternalTable would also have a null non-persistent nameMapping, which these two setters do not restore. Please put complete invariant reconstruction on the real recovery/cache-load path (and test that path), or demonstrate the production recovery caller that reaches this method; the current test only clears fields manually and invokes an event-only path.
FE UT Coverage ReportIncrement line coverage |
|
run buildall |
|
/review |
There was a problem hiding this comment.
Request changes: the defensive MTMV fallback is incomplete for the newly handled null-catalog state; see the inline issue.
Critical checkpoints:
- Planner correctness/fallback: blocking.
collectMTMVCandidatesreturns for a non-null database with a null catalog, but the same installed query/DML materialization hook later unconditionally collects partitions and dereferences that catalog throughStatementContext.getTableId/TableIf.getFullQualifiers. A surviving scan can therefore still abort instead of using the original plan. - Recovery/lifecycle: verified. Branch-4.0 images omit external database/table meta caches, and lazy
buildDbForInit/buildTableForInitreconstruct catalog, database, and name-mapping ownership. No production image, replay, cache reload, or event path was found that publishes a partially owned table. The supersededregisterTableconcern is absent from the current diff and was not duplicated. - State, locking, and concurrency: the null-database return occurs before candidate or related-table mutation; the original query table remains in normal statement state, and MTMV locks are released in
finally. No distinct state or lock issue survived review. - Compatibility/tests: the image round trip meaningfully proves fresh lazy-loaded owner reconstruction, and the reflection test covers the null-database branch. It does not exercise the full post-rewrite hook or the null-catalog arm, which is why the blocking downstream failure is missed. CheckStyle is green; FE UT and compile are still pending. No local build was run because this review runner forbids builds.
- Configuration, observability, and performance: no protocol/configuration change or material performance regression was found; the warning is limited to malformed metadata with an MV hook.
- User focus: no additional focus was supplied; the complete three-file diff was reviewed.
Review converged after two complete rounds, including separate risk-focused scans. One distinct inline blocking issue remains; every other candidate was verified, deduplicated, or dismissed with code evidence.
| if (shouldCollect) { | ||
| DatabaseIf database = table.getDatabase(); | ||
| // MTMV rewrite is optional, so malformed ownership metadata must not abort the query. | ||
| if (database == null || database.getCatalog() == null) { |
There was a problem hiding this comment.
The database.getCatalog() == null arm does not actually provide the promised fallback. With the MV hook installed, InitMaterializationContextHook.afterRewrite() later runs QueryPartitionCollector; that visitor only skips a null database and then calls StatementContext.getTableId(table), which keys through table.getFullQualifiers() and dereferences database.getCatalog().getName(). A table handled by this new arm can therefore still abort the query during post-rewrite partition collection (the DML hook inherits the same path). Please make the downstream MV-hook work skip incomplete catalog ownership too, and cover this with a full planner regression using a non-null database whose catalog is null.
FE Regression Coverage ReportIncrement line coverage |
2 participants
What problem does this PR solve?
This is the branch-4.0 backport of #66599.
Malformed external-table ownership metadata can cause optional materialized-view candidate collection to abort an otherwise valid query when
BaseTableInfois constructed.Current image serialization does not persist external object caches. After an image round trip, lazy loading rebuilds tables with their catalog and database owners. The fix therefore keeps the recovery lifecycle unchanged and makes MTMV candidate collection skip only malformed table metadata.
Related issue: #58441
Test
BaseTableInfoconstruction.ExternalDatabaseImageRestoreTest,ExternalEqualsTest, andCollectRelationTest: 3 tests passed, 0 failures.Release note
None.
Check List (For Author)