branch:4.2: [refactor](constraint): centralize constraint management in ConstraintManager #61118 - #68294
Draft
morrySnow wants to merge 2 commits into
Draft
branch:4.2: [refactor](constraint): centralize constraint management in ConstraintManager #61118#68294morrySnow wants to merge 2 commits into
morrySnow wants to merge 2 commits into
Conversation
…lized ConstraintManager - Create ConstraintManager class with ConcurrentHashMap storage keyed by fully qualified table name (catalog.db.table) - Move all constraint CRUD operations from TableIf to ConstraintManager - Add own persistence module (image + editlog replay) for constraints - Support cleanup hooks: table drop, database drop, catalog drop, rename - Maintain FK-PK bidirectional references via foreignTableNames/referencedTableName - Backward compatibility: migrate old table-based constraints via GsonPostProcessable and migrateConstraintsFromTables() - Remove all constraint methods from TableIf interface (233 lines) - Update optimizer (ForeignKeyContext), commands, and catalog relations - Use TableNameInfo in AlterConstraintLog for name-based editlog persistence with backward compat migration from old TableIdentifier format - Clear old table constraints after migration to prevent duplicate migration - Deprecate getTableAttributes() on Table/ExternalTable and getConstraintsMap() on TableAttributes - Fix review findings: null-guard in EditLog replay, idempotent replay, volatile referencedTableName, cross-catalog FK cleanup, migration FK ref rebuild, deprecated getForeignTables() Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> refactor: Replace String qualifiedTableName with TableNameInfo in constraint APIs - ForeignKeyConstraint: Add referencedTableInfo (TableNameInfo) field alongside referencedTableNameStr for backward compat. Constructor now takes TableNameInfo instead of String. getReferencedTableName() returns TableNameInfo. - PrimaryKeyConstraint: Add foreignTableInfos (List<TableNameInfo>) alongside foreignTableNameStrs for backward compat. addForeignTable/removeForeignTable/ getForeignTableInfos/renameForeignTable now use TableNameInfo. - ConstraintManager: All public methods take TableNameInfo instead of String. Added toKey() helper to convert TableNameInfo to map key string internally. - Updated all callers: EditLog, AddConstraintCommand, DropConstraintCommand, ShowConstraintsCommand, InternalCatalog, Env, LogicalCatalogRelation, PhysicalCatalogRelation, ForeignKeyContext. - Updated tests: ConstraintPersistTest, ConstraintTest. - gsonPostProcess() handles migration from old serialized formats. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> fix(constraints): Add thread safety and DDL constraint checks - ConstraintManager.addConstraint() now validates table/column existence atomically under write lock to prevent TOCTOU race conditions - Drop table: rejects if PK is referenced by FK (unless FORCE) - Schema change: rejects DROP COLUMN if column is in any constraint - Replace table: handles constraint swap/rename/drop properly - Add 4 new test cases covering all safety scenarios Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> fix(constraints): Allow DropConstraintCommand when table no longer exists External tables can be deleted by other systems outside Doris. When this happens, DropConstraintCommand would fail because it tries to resolve the table via the planner. This change adds a fallback path: if table resolution fails, extract the table name from the UnboundRelation's name parts and fill in catalog/db from the ConnectContext. Also handles 1-part, 2-part, and 3-part table name specifications correctly in the fallback path. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> fix(constraints): Handle tables without database in constraint lookups TableNameInfo(TableIf) throws AnalysisException when the table has no database (e.g., standalone OlapTable objects in unit tests created via PlanConstructor). This broke many rewrite/analysis tests that use such tables in LogicalCatalogRelation.computeUnique/computeFdItems, PhysicalCatalogRelation.computeUnique, and ForeignKeyContext methods. Added TableNameInfo.createOrNull(TableIf) factory method that returns null instead of throwing when the table lacks a database or catalog. All four call sites now use this method and skip constraint lookups when the table has no database context. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> fix(constraints): Address code review findings - Fix TableNameInfo hashCode()/equals() contract violation: hashCode() now uses toString().hashCode() to be consistent with equals() which compares toString() results. Previously hashCode() used Objects.hash(tbl, db, ctl) which included the raw ctl field, while equals()/toString() skipped the 'internal' catalog name. - Add LOG.warn in DropConstraintCommand fallback path to aid debugging when table resolution fails and name-based lookup is used instead. - Fix ShowConstraintsCommand Javadoc: was 'add constraint command', corrected to 'show constraints command'. - Document TOCTOU vs deadlock tradeoff in ConstraintManager.addConstraint Javadoc: validation is kept inside write lock for correctness. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> fix(constraints): Integrate renameTable into all rename paths ConstraintManager.renameTable() existed but was never called from any rename code path, leaving constraints keyed under the old table name after rename (becoming unreachable). Added renameTable() calls to: - Env.renameTable() — internal catalog master path - Env.replayRenameTable() — internal catalog replay path - ExternalCatalog.renameTable() — external catalog master path - RefreshManager.replayRefreshTable() — external catalog replay path Added renameTableUpdatesConstraintsTest to verify constraints are correctly migrated when a table is renamed. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> fix(constraints): Eliminate TOCTOU gap in drop table constraint check checkNoReferencingForeignKeys (readLock) and dropTableConstraints (writeLock) were called separately, creating a TOCTOU window where a new FK could be added between the check and the drop. Added checkAndDropTableConstraints() which holds the write lock for both the FK reference check and the constraint drop, making the operation atomic. Updated InternalCatalog.unprotectDropTable to use the new method. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Add comprehensive ConstraintManager unit tests Add ~50 direct API tests for ConstraintManager covering: - Basic CRUD (add/get/drop constraints) - Type-specific getters (PK/FK/UNIQUE) - FK bidirectional reference management - Cascade drop (PK drops referencing FKs) - checkAndDropTableConstraints (atomic check + drop) - findConstraintWithColumn - dropCatalogConstraints - renameTable (moves constraints + updates FK refs) - swapTableConstraints - dropAndRenameConstraints - migrateFromTable - Serialization round-trip (write/read) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Fix ConstraintManagerTest review findings - Fix dropCatalogConstraintsCascadesFKsAcrossCatalogs: assert FK on T1 IS cascade-dropped when referenced PK's catalog is dropped (was incorrectly documented as not cascade-dropped) - Add before-assertion in rebuildForeignKeyReferencesWiresFKToPK to verify PK doesn't know about FK table before rebuild - Add swapTableConstraintsUpdatesFKReferences: verify FK cross-references are updated when tables are swapped - Add dropAndRenameUpdatesFKReferences: verify FK cross-references are updated when table is replaced without swap Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Bug 1: PrimaryKeyConstraint.addForeignTable() now deduplicates via foreignTableNameStrs (HashSet) before appending to foreignTableInfos (ArrayList). Prevents duplicate entries from rebuildForeignKeyReferences or duplicate editlog replay. Bug 2: Add ConstraintManager.dropDatabaseConstraints() to pre-clear all constraints for a database before iterating tables in unprotectDropDb(). This avoids non-deterministic FK check failures when table B has an FK referencing table A's PK and B happens to be iterated before A. Also refactored dropCatalogConstraints to share dropConstraintsByPrefix helper with the new dropDatabaseConstraints method. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Contributor
Author
|
run buildall |
morrySnow
marked this pull request as draft
September 21, 2026 00:20
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
picked from #61118