github-actions[bot] commented on code in PR #61118:
URL: https://github.com/apache/doris/pull/61118#discussion_r2917345585
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/InternalCatalog.java:
##########
@@ -551,7 +552,8 @@ public void dropDb(String dbName, boolean ifExists, boolean
force) throws DdlExc
LOG.info("finish drop database[{}], is force : {}", dbName, force);
}
- public void unprotectDropDb(Database db, boolean isForeDrop, boolean
isReplay, long recycleTime) {
+ public void unprotectDropDb(Database db, boolean isForeDrop, boolean
isReplay, long recycleTime)
+ throws DdlException {
for (Table table : db.getTables()) {
Review Comment:
**Bug: `DROP DATABASE` (without FORCE) can fail due to FK constraint
ordering**
`unprotectDropDb` iterates `db.getTables()` in arbitrary order and calls
`unprotectDropTable` for each. `unprotectDropTable` calls
`checkAndDropTableConstraints(table, !isForceDrop && !isReplay)`. When
`isForceDrop=false` and `isReplay=false`, this checks whether the table's PK is
referenced by any FK from other tables.
If within the same database, table A has an FK referencing table B's PK, and
`db.getTables()` returns B before A, then when dropping B, the FK on A still
exists in `ConstraintManager`, so `checkAndDropTableConstraints` throws:
> `"Cannot drop table B because its primary key is referenced by foreign key
constraints from table(s): A"`
This makes `DROP DATABASE mydb` (without FORCE) fail non-deterministically
based on table iteration order when the DB contains cross-table FK
relationships.
Suggested fix: Either:
1. Pre-drop all FK constraints in the DB before iterating tables (similar to
how `cleanupConstraintReferencesOutsideCatalog` works for catalog drop), or
2. Pass `checkForeignKeys=false` when dropping all tables within a `DROP
DATABASE` operation (since the entire DB is being dropped, FK references within
the same DB are irrelevant). Only check FKs referencing tables *outside* the DB
being dropped.
##########
fe/fe-core/src/main/java/org/apache/doris/catalog/constraint/PrimaryKeyConstraint.java:
##########
@@ -50,10 +62,16 @@ public Set<Column> getPrimaryKeys(TableIf table) {
return
columns.stream().map(table::getColumn).collect(ImmutableSet.toImmutableSet());
}
- public void addForeignTable(TableIf table) {
- foreignTables.add(new TableIdentifier(table));
+ public void addForeignTable(TableNameInfo tni) {
+ foreignTableInfos.add(tni);
Review Comment:
**Bug: `addForeignTable` lacks deduplication on `foreignTableInfos`
(ArrayList)**
`foreignTableNameStrs` is a `HashSet` which naturally deduplicates, but
`foreignTableInfos` is an `ArrayList` that unconditionally appends. This means
duplicate `TableNameInfo` entries can accumulate in `foreignTableInfos`.
Scenario: After image deserialization, `gsonPostProcess()` (line 116)
populates `foreignTableInfos` from `foreignTableNameStrs` or `foreignTables`.
Then if `rebuildForeignKeyReferences()` is called (line 411 in
`ConstraintManager`), it calls `addForeignTable` again for every FK
relationship, adding duplicates to the list. While
`rebuildForeignKeyReferences` is only called after
`migrateConstraintsFromTables` when `migratedCount > 0`, the issue also exists
in the normal `addConstraint` → `registerForeignKeyReference` path if the same
FK constraint is somehow added twice (e.g., duplicate EditLog replay edge case).
The duplicates would cause `getForeignTableInfos()` to return a list with
redundant entries, which affects `checkAndDropTableConstraints` error messages
and `checkNoReferencingForeignKeys` behavior. More importantly,
`removeForeignTable(TableNameInfo)` uses `removeIf` which removes all matches,
but `renameForeignTable` only renames the first match (line 81-87), leaving
stale entries.
Suggested fix: Add a dedup check before adding:
```java
public void addForeignTable(TableNameInfo tni) {
String key = tni.getCtl() + "." + tni.getDb() + "." + tni.getTbl();
if (foreignTableNameStrs.add(key)) {
foreignTableInfos.add(tni);
}
}
```
##########
fe/fe-core/src/main/java/org/apache/doris/catalog/constraint/ConstraintManager.java:
##########
@@ -0,0 +1,953 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+package org.apache.doris.catalog.constraint;
+
+import org.apache.doris.catalog.DatabaseIf;
+import org.apache.doris.catalog.Env;
+import org.apache.doris.catalog.TableIf;
+import org.apache.doris.common.DdlException;
+import org.apache.doris.common.io.Text;
+import org.apache.doris.common.io.Writable;
+import org.apache.doris.datasource.CatalogIf;
+import org.apache.doris.info.TableNameInfo;
+import org.apache.doris.nereids.exceptions.AnalysisException;
+import org.apache.doris.persist.AlterConstraintLog;
+import org.apache.doris.persist.gson.GsonPostProcessable;
+import org.apache.doris.persist.gson.GsonUtils;
+
+import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableMap;
+import com.google.gson.annotations.SerializedName;
+import org.apache.logging.log4j.LogManager;
+import org.apache.logging.log4j.Logger;
+
+import java.io.DataInput;
+import java.io.DataOutput;
+import java.io.IOException;
+import java.util.Collection;
+import java.util.HashMap;
+import java.util.Iterator;
+import java.util.List;
+import java.util.Map;
+import java.util.Map.Entry;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.locks.ReentrantReadWriteLock;
+import java.util.stream.Collectors;
+
+/**
+ * Centralized manager for all table constraints (PK, FK, UNIQUE).
+ * Constraints are indexed by fully qualified table name (catalog.db.table).
+ */
+public class ConstraintManager implements Writable, GsonPostProcessable {
+
+ private static final Logger LOG =
LogManager.getLogger(ConstraintManager.class);
+
+ @SerializedName("cm")
+ private final ConcurrentHashMap<String, Map<String, Constraint>>
constraintsMap
+ = new ConcurrentHashMap<>();
+
+ private final ReentrantReadWriteLock lock = new ReentrantReadWriteLock();
+
+ public ConstraintManager() {
+ }
+
+ private static String toKey(TableNameInfo tni) {
+ return tni.getCtl() + "." + tni.getDb() + "." + tni.getTbl();
+ }
+
+ /** Returns true if no constraints are stored. */
+ public boolean isEmpty() {
+ return constraintsMap.isEmpty();
+ }
+
+ private void readLock() {
+ lock.readLock().lock();
+ }
+
+ private void readUnlock() {
+ lock.readLock().unlock();
+ }
+
+ private void writeLock() {
+ lock.writeLock().lock();
+ }
+
+ private void writeUnlock() {
+ lock.writeLock().unlock();
+ }
+
+ /**
+ * Add a constraint to the specified table.
+ * For FK constraints, validates that the referenced PK exists
+ * and registers bidirectional reference via foreignTableInfos.
+ *
+ * <p>Note: validation is performed inside the write lock to prevent
TOCTOU races
+ * (e.g., table dropped between validation and registration). For external
catalogs,
+ * this could cause longer lock hold times if catalog initialization is
slow.
+ * This tradeoff is intentional — correctness over performance.</p>
+ */
+ public void addConstraint(TableNameInfo tableNameInfo, String
constraintName,
+ Constraint constraint, boolean replay) {
+ String key = toKey(tableNameInfo);
+ writeLock();
+ try {
+ if (!replay) {
+ validateTableAndColumns(tableNameInfo, constraint);
+ }
+ Map<String, Constraint> tableConstraints =
constraintsMap.computeIfAbsent(
+ key, k -> new HashMap<>());
+ checkConstraintNotExistence(constraintName, constraint,
tableConstraints);
+ if (constraint instanceof ForeignKeyConstraint) {
+ registerForeignKeyReference(
+ tableNameInfo, (ForeignKeyConstraint) constraint);
+ }
+ tableConstraints.put(constraintName, constraint);
+ if (!replay) {
+ logAddConstraint(tableNameInfo, constraint);
+ }
+ LOG.info("Added constraint {} on table {}", constraintName, key);
+ } finally {
+ writeUnlock();
+ }
+ }
+
+ /**
+ * Drop a constraint from the specified table.
+ * For PK constraints, cascade-drops all referencing FKs.
+ * For FK constraints, updates the referenced PK's foreign table set.
+ */
+ public void dropConstraint(TableNameInfo tableNameInfo, String
constraintName,
+ boolean replay) {
+ String key = toKey(tableNameInfo);
+ writeLock();
+ try {
+ Map<String, Constraint> tableConstraints = constraintsMap.get(key);
+ if (tableConstraints == null ||
!tableConstraints.containsKey(constraintName)) {
+ if (replay) {
+ LOG.warn("Constraint {} not found on table {} during
replay, skipping",
+ constraintName, key);
+ return;
+ }
+ throw new AnalysisException(String.format(
+ "Unknown constraint %s on table %s.",
+ constraintName, key));
+ }
+ Constraint constraint = tableConstraints.remove(constraintName);
+ cleanupConstraintReferences(tableNameInfo, constraint);
+ if (tableConstraints.isEmpty()) {
+ constraintsMap.remove(key);
+ }
+ if (!replay) {
+ logDropConstraint(tableNameInfo, constraint);
+ }
+ LOG.info("Dropped constraint {} from table {}",
+ constraintName, key);
+ } finally {
+ writeUnlock();
+ }
+ }
+
+ /** Returns an immutable copy of all constraints for the given table. */
+ public Map<String, Constraint> getConstraints(TableNameInfo tableNameInfo)
{
+ String key = toKey(tableNameInfo);
+ readLock();
+ try {
+ Map<String, Constraint> tableConstraints
+ = constraintsMap.get(key);
+ if (tableConstraints == null) {
+ return ImmutableMap.of();
+ }
+ return ImmutableMap.copyOf(tableConstraints);
+ } finally {
+ readUnlock();
+ }
+ }
+
+ /** Get a single constraint by name, or null if not found. */
+ public Constraint getConstraint(TableNameInfo tableNameInfo,
+ String constraintName) {
+ String key = toKey(tableNameInfo);
+ readLock();
+ try {
+ Map<String, Constraint> tableConstraints
+ = constraintsMap.get(key);
+ if (tableConstraints == null) {
+ return null;
+ }
+ return tableConstraints.get(constraintName);
+ } finally {
+ readUnlock();
+ }
+ }
+
+ /** Returns all PrimaryKeyConstraints for the given table. */
+ public ImmutableList<PrimaryKeyConstraint> getPrimaryKeyConstraints(
+ TableNameInfo tableNameInfo) {
+ return getConstraintsByType(toKey(tableNameInfo),
+ PrimaryKeyConstraint.class);
+ }
+
+ /** Returns all ForeignKeyConstraints for the given table. */
+ public ImmutableList<ForeignKeyConstraint> getForeignKeyConstraints(
+ TableNameInfo tableNameInfo) {
+ return getConstraintsByType(toKey(tableNameInfo),
+ ForeignKeyConstraint.class);
+ }
+
+ /** Returns all UniqueConstraints for the given table. */
+ public ImmutableList<UniqueConstraint> getUniqueConstraints(
+ TableNameInfo tableNameInfo) {
+ return getConstraintsByType(toKey(tableNameInfo),
+ UniqueConstraint.class);
+ }
+
+ /**
+ * Remove all constraints for a table and clean up bidirectional
references.
+ * Called when a table is dropped.
+ */
+ /**
+ * Atomically check for referencing foreign keys and then drop all
constraints
+ * for the given table. Holds the write lock for both operations to prevent
+ * TOCTOU races where a new FK could be added between the check and the
drop.
+ *
+ * @param tableNameInfo the table whose constraints are to be dropped
+ * @param checkForeignKeys if true, throw DdlException if any PK is
FK-referenced
+ */
+ public void checkAndDropTableConstraints(TableNameInfo tableNameInfo,
+ boolean checkForeignKeys) throws DdlException {
+ String key = toKey(tableNameInfo);
+ writeLock();
+ try {
+ Map<String, Constraint> tableConstraints = constraintsMap.get(key);
+ if (tableConstraints == null) {
+ return;
+ }
+ if (checkForeignKeys) {
+ for (Constraint c : tableConstraints.values()) {
+ if (c instanceof PrimaryKeyConstraint) {
+ PrimaryKeyConstraint pk = (PrimaryKeyConstraint) c;
+ List<TableNameInfo> fkTables =
pk.getForeignTableInfos();
+ if (fkTables != null && !fkTables.isEmpty()) {
+ String fkTableNames = fkTables.stream()
+ .map(t -> toKey(t))
+ .collect(Collectors.joining(", "));
+ throw new DdlException(String.format(
+ "Cannot drop table %s because its primary"
+ + " key is referenced by foreign
key"
+ + " constraints from table(s): %s."
+ + " Drop the foreign key
constraints"
+ + " first.",
+ key, fkTableNames));
+ }
+ }
+ }
+ }
+ constraintsMap.remove(key);
+ for (Constraint constraint : tableConstraints.values()) {
+ cleanupConstraintReferences(tableNameInfo, constraint);
+ }
+ LOG.info("Dropped all constraints for table {}", key);
+ } finally {
+ writeUnlock();
+ }
+ }
+
+ public void dropTableConstraints(TableNameInfo tableNameInfo) {
+ String key = toKey(tableNameInfo);
+ writeLock();
+ try {
+ Map<String, Constraint> tableConstraints
+ = constraintsMap.remove(key);
+ if (tableConstraints == null) {
+ return;
+ }
+ for (Constraint constraint : tableConstraints.values()) {
+ cleanupConstraintReferences(tableNameInfo, constraint);
+ }
+ LOG.info("Dropped all constraints for table {}", key);
+ } finally {
+ writeUnlock();
+ }
+ }
+
+ /**
+ * Remove all constraints whose qualified table name starts with
+ * the given catalog prefix. Called when a catalog is dropped.
+ */
+ public void dropCatalogConstraints(String catalogName) {
+ writeLock();
+ try {
+ String prefix = catalogName + ".";
+ List<String> tablesToRemove = constraintsMap.keySet().stream()
+ .filter(k -> k.startsWith(prefix))
+ .collect(Collectors.toList());
+ for (String tableName : tablesToRemove) {
+ Map<String, Constraint> tableConstraints
+ = constraintsMap.remove(tableName);
+ if (tableConstraints != null) {
+ for (Constraint constraint : tableConstraints.values()) {
+ cleanupConstraintReferencesOutsideCatalog(
+ tableName, constraint, prefix);
+ }
+ }
+ }
+ LOG.info("Dropped all constraints for catalog {}", catalogName);
+ } finally {
+ writeUnlock();
+ }
+ }
+
+ /**
+ * Move constraints from oldTableInfo to newTableInfo
+ * and update all FK/PK references. Called when a table is renamed.
+ */
+ public void renameTable(TableNameInfo oldTableInfo,
+ TableNameInfo newTableInfo) {
+ String oldKey = toKey(oldTableInfo);
+ String newKey = toKey(newTableInfo);
+ writeLock();
+ try {
+ // Move this table's own constraints
+ Map<String, Constraint> tableConstraints
+ = constraintsMap.remove(oldKey);
+ if (tableConstraints != null) {
+ constraintsMap.put(newKey, tableConstraints);
+ }
+ // Update FK/PK references in OTHER tables
+ for (Map.Entry<String, Map<String, Constraint>> entry
+ : constraintsMap.entrySet()) {
+ if (entry.getKey().equals(newKey)) {
+ continue;
+ }
+ for (Constraint c : entry.getValue().values()) {
+ if (c instanceof ForeignKeyConstraint) {
+ ForeignKeyConstraint fk = (ForeignKeyConstraint) c;
+ TableNameInfo refInfo = fk.getReferencedTableName();
+ if (refInfo != null && oldTableInfo.equals(refInfo)) {
+ fk.setReferencedTableInfo(newTableInfo);
+ }
+ } else if (c instanceof PrimaryKeyConstraint) {
+ ((PrimaryKeyConstraint) c).renameForeignTable(
+ oldTableInfo, newTableInfo);
+ }
+ }
+ }
+ LOG.info("Renamed table constraints from {} to {}",
+ oldKey, newKey);
+ } finally {
+ writeUnlock();
+ }
+ }
+
+ /**
+ * Migrate constraints from old table-based storage into this manager.
+ */
+ public void migrateFromTable(TableNameInfo tableNameInfo,
+ Map<String, Constraint> existingConstraints) {
+ if (existingConstraints == null || existingConstraints.isEmpty()) {
+ return;
+ }
+ String key = toKey(tableNameInfo);
+ writeLock();
+ try {
+ Map<String, Constraint> tableConstraints
+ = constraintsMap.computeIfAbsent(
+ key, k -> new HashMap<>());
+ tableConstraints.putAll(existingConstraints);
+ LOG.info("Migrated {} constraints for table {}",
+ existingConstraints.size(), key);
+ } finally {
+ writeUnlock();
+ }
+ }
+
+ /**
+ * After all tables have been migrated, wire up FK→PK bidirectional
+ * references that could not be established during per-table migration
+ * (because the referenced PK table may not have been migrated yet).
+ */
+ public void rebuildForeignKeyReferences() {
+ writeLock();
+ try {
+ for (Map.Entry<String, Map<String, Constraint>> entry
+ : constraintsMap.entrySet()) {
+ String fkTableKey = entry.getKey();
+ TableNameInfo fkTableInfo = new TableNameInfo(fkTableKey);
+ for (Constraint c : entry.getValue().values()) {
+ if (!(c instanceof ForeignKeyConstraint)) {
+ continue;
+ }
+ ForeignKeyConstraint fk = (ForeignKeyConstraint) c;
+ TableNameInfo refTableInfo = fk.getReferencedTableName();
+ if (refTableInfo == null) {
+ continue;
+ }
+ String refTableKey = toKey(refTableInfo);
+ Map<String, Constraint> refTableConstraints
+ = constraintsMap.get(refTableKey);
+ if (refTableConstraints == null) {
+ continue;
+ }
+ for (Constraint rc : refTableConstraints.values()) {
+ if (rc instanceof PrimaryKeyConstraint) {
+ PrimaryKeyConstraint pk = (PrimaryKeyConstraint)
rc;
+ if (pk.getPrimaryKeyNames().equals(
+ fk.getReferencedColumnNames())) {
+ pk.addForeignTable(fkTableInfo);
Review Comment:
**Note**: This `addForeignTable` call can produce duplicates in
`foreignTableInfos` if the PK constraint was deserialized with pre-existing
foreign table references (populated via `gsonPostProcess`). See inline comment
on `PrimaryKeyConstraint.addForeignTable` for details and suggested fix.
--
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]
