github-actions[bot] commented on code in PR #61118:
URL: https://github.com/apache/doris/pull/61118#discussion_r2909768325
##########
fe/fe-core/src/main/java/org/apache/doris/catalog/constraint/PrimaryKeyConstraint.java:
##########
@@ -19,24 +19,36 @@
import org.apache.doris.catalog.Column;
import org.apache.doris.catalog.TableIf;
+import org.apache.doris.info.TableNameInfo;
+import org.apache.doris.persist.gson.GsonPostProcessable;
import com.google.common.base.Objects;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableSet;
import com.google.gson.annotations.SerializedName;
+import java.io.IOException;
+import java.util.ArrayList;
+import java.util.Collections;
import java.util.HashSet;
import java.util.List;
import java.util.Set;
-public class PrimaryKeyConstraint extends Constraint {
+public class PrimaryKeyConstraint extends Constraint implements
GsonPostProcessable {
@SerializedName(value = "cols")
private final Set<String> columns;
// record the foreign table which references the primary key
@SerializedName(value = "ft")
private final Set<TableIdentifier> foreignTables = new HashSet<>();
+ // qualified name strings kept for backward-compatible deserialization
+ @SerializedName(value = "ftn")
+ private final Set<String> foreignTableNameStrs = new HashSet<>();
+
+ @SerializedName(value = "ftni")
+ private final List<TableNameInfo> foreignTableInfos = new ArrayList<>();
Review Comment:
**Low: `foreignTables` (old Set<TableIdentifier> at line 43) is never
updated by new code paths**
`addForeignTable(TableNameInfo)` at line 65 only adds to `foreignTableInfos`
and `foreignTableNameStrs`, not to `foreignTables`. The deprecated
`getForeignTables()` still reads from `foreignTables`, which returns
empty/stale results for constraints created via the new `ConstraintManager`.
Please verify no remaining callers of `getForeignTables()` exist. If they
do, they will silently get wrong results. Consider making `getForeignTables()`
resolve from `foreignTableInfos` as a safety net, or remove the deprecated
method entirely if no callers remain.
##########
fe/fe-core/src/main/java/org/apache/doris/catalog/constraint/ConstraintManager.java:
##########
@@ -0,0 +1,901 @@
+// 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.
+ */
+ public void addConstraint(TableNameInfo tableNameInfo, String
constraintName,
+ Constraint constraint, boolean replay) {
+ String key = toKey(tableNameInfo);
+ writeLock();
+ try {
+ if (!replay) {
+ validateTableAndColumns(tableNameInfo, constraint);
Review Comment:
**Medium: Validation under write lock may cause long hold times or deadlock**
`validateTableAndColumns()` resolves tables via
`Env.getCurrentEnv().getCatalogMgr().getCatalog()` → `getDbNullable()` →
`getTableNullable()`. For external catalogs, this may trigger
`makeSureInitialized()` which acquires catalog-level synchronized locks and
potentially makes RPCs.
Holding the `ConstraintManager` write lock while doing catalog resolution
risks:
1. Long lock hold times if external catalog initialization is slow
2. Deadlock if any catalog initialization callback reads constraints from
`ConstraintManager`
Since validation is read-only and only runs on the non-replay path, consider
performing it **before** acquiring the write lock:
```java
public void addConstraint(..., boolean replay) {
if (!replay) {
validateTableAndColumns(tableNameInfo, constraint);
}
writeLock();
try {
// ... mutation only ...
} finally {
writeUnlock();
}
}
```
Note: `checkConstraintNotExistence` must remain under the lock since it's a
read-check-write sequence.
##########
fe/fe-core/src/main/java/org/apache/doris/catalog/constraint/ConstraintManager.java:
##########
@@ -0,0 +1,901 @@
+// 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();
Review Comment:
**High: `TableNameInfo` `equals()`/`hashCode()` contract violation impacts
constraint identity**
`toKey()` always includes the catalog name (`tni.getCtl() + "." +
tni.getDb() + "." + tni.getTbl()`), but `TableNameInfo.equals()` uses
`toString()` which omits the `"internal"` catalog prefix. Meanwhile,
`TableNameInfo.hashCode()` uses `Objects.hash(tbl, db, ctl)` which **includes**
the raw `ctl` field.
This means two `TableNameInfo` objects that are `equals()` can have
different `hashCode()` values:
```java
new TableNameInfo("internal", "test", "t1") // toString()="test.t1", hash
includes "internal"
new TableNameInfo(null, "test", "t1") // toString()="test.t1", hash
includes null
// equals() = true, hashCode() = DIFFERENT → violates Java contract
```
This is pre-existing in `TableNameInfo`, but this PR now critically depends
on it for:
- `PrimaryKeyConstraint.removeForeignTable(TableNameInfo)` at line 85 — uses
`equals()` for matching
- `ForeignKeyConstraint.isReferringPK()` — uses `equals()` on
`referencedTableInfo`
- `swapPrimaryKeyForeignTables()` at line 876 — uses `equals()` via stream
`anyMatch`
If any code path creates a `TableNameInfo` with `ctl=null` (e.g., from the
`TableNameInfo(String alias)` constructor with a 2-part name like
`"db.table"`), lookups in collections that use `hashCode()` will silently fail.
**Recommendation:** Fix `TableNameInfo.hashCode()` to be consistent with
`equals()` — either `return toString().hashCode()` or make `equals()` compare
fields directly including `ctl`.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/DropConstraintCommand.java:
##########
@@ -58,35 +57,39 @@ public DropConstraintCommand(String name, LogicalPlan plan)
{
this.plan = plan;
}
- private static List<TableIf> getConstraintRelatedTables(Constraint
constraint) {
- List<TableIf> tables = Lists.newArrayList();
- if (constraint instanceof PrimaryKeyConstraint) {
- tables.addAll(((PrimaryKeyConstraint)
constraint).getForeignTables());
- } else if (constraint instanceof ForeignKeyConstraint) {
- tables.add(((ForeignKeyConstraint)
constraint).getReferencedTable());
- }
- return tables;
- }
-
@Override
public void run(ConnectContext ctx, StmtExecutor executor) throws
Exception {
- TableIf table = extractTable(ctx, plan);
- table.readLock();
+ TableNameInfo tableNameInfo;
try {
- Constraint constraint = table.getConstraintsMapUnsafe().get(name);
- if (constraint == null) {
- throw new AnalysisException(
- String.format("Unknown constraint %s on table %s.",
name, table.getName()));
- }
- } finally {
- table.readUnlock();
+ TableIf table = extractTable(ctx, plan);
+ tableNameInfo = new TableNameInfo(table);
+ } catch (Exception e) {
+ // Table may no longer exist (e.g., external table deleted by
another system).
+ // Fall back to extracting the table name from the unresolved plan.
+ tableNameInfo = extractTableNameFromPlan(ctx);
}
- table.writeLock();
- try {
- table.dropConstraint(name, false);
- } finally {
- table.writeUnlock();
+ Env.getCurrentEnv().getConstraintManager().dropConstraint(
+ tableNameInfo, name, false);
+ }
+
+ private TableNameInfo extractTableNameFromPlan(ConnectContext ctx) {
+ if (!(plan instanceof UnboundRelation)) {
+ throw new AnalysisException(
+ "Cannot resolve table for dropping constraint " + name);
+ }
+ UnboundRelation unbound = (UnboundRelation) plan;
+ List<String> parts = unbound.getNameParts();
+ String ctl = ctx.getCurrentCatalog() != null
Review Comment:
**Low: Fallback may construct incorrect `TableNameInfo` from connect context
defaults**
`ctx.getCurrentCatalog()` and `ctx.getDatabase()` reflect the current
session state, which may differ from the original session that created the
constraint. During forwarded execution (this command implements
`ForwardWithSync`), the master's connect context may have different defaults.
Since this fallback only triggers when the table no longer exists (line
66-69 comment), the practical risk is limited. Consider adding a `LOG.warn`
when falling back to this path to aid debugging unexpected constraint-not-found
errors.
--
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]
