github-actions[bot] commented on code in PR #61118:
URL: https://github.com/apache/doris/pull/61118#discussion_r2910579133
##########
fe/fe-core/src/main/java/org/apache/doris/persist/EditLog.java:
##########
@@ -1187,18 +1189,36 @@ public static void loadJournal(Env env, Long logId,
JournalEntity journal) {
case OperationType.OP_ADD_CONSTRAINT: {
final AlterConstraintLog log = (AlterConstraintLog)
journal.getData();
try {
-
log.getTableIf().replayAddConstraint(log.getConstraint());
+ TableNameInfo tni = log.getTableNameInfo();
+ Constraint constraint = log.getConstraint();
+ if (tni == null) {
+ LOG.warn("Failed to replay add constraint {}: "
+ + "table name could not be resolved",
+ constraint.getName());
+ break;
+ }
+ env.getConstraintManager().addConstraint(
+ tni, constraint.getName(), constraint, true);
} catch (Exception e) {
- LOG.error("Failed to replay add constraint", e);
+ LOG.warn("Failed to replay add constraint", e);
}
break;
}
case OperationType.OP_DROP_CONSTRAINT: {
final AlterConstraintLog log = (AlterConstraintLog)
journal.getData();
try {
-
log.getTableIf().replayDropConstraint(log.getConstraint().getName());
+ TableNameInfo tni = log.getTableNameInfo();
+ Constraint constraint = log.getConstraint();
+ if (tni == null) {
+ LOG.warn("Failed to replay drop constraint {}: "
+ + "table name could not be resolved",
+ constraint.getName());
+ break;
+ }
+ env.getConstraintManager().dropConstraint(
+ tni, constraint.getName(), true);
} catch (Exception e) {
- LOG.error("Failed to replay drop constraint", e);
+ LOG.warn("Failed to replay drop constraint", e);
}
Review Comment:
Same issue as the `OP_ADD_CONSTRAINT` handler above — overly broad `catch
(Exception e)` silently swallows all replay errors including programming bugs.
Should be narrowed or removed to maintain consistency with other replay
handlers.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/InternalCatalog.java:
##########
@@ -1022,7 +1024,10 @@ public boolean unprotectDropTable(Database db, Table
table, boolean isForceDrop,
Env.getCurrentEnv().getAnalysisManager().removeTableStats(table.getId());
Env.getCurrentEnv().getDictionaryManager().dropTableDictionaries(db.getName(),
table.getName());
Env.getCurrentEnv().getQueryStats().clear(Env.getCurrentInternalCatalog().getId(),
db.getId(), table.getId());
- table.removeTableIdentifierFromPrimaryTable();
+ if (!isForceDrop && !isReplay) {
+
Env.getCurrentEnv().getConstraintManager().checkNoReferencingForeignKeys(new
TableNameInfo(table));
+ }
+ Env.getCurrentEnv().getConstraintManager().dropTableConstraints(new
TableNameInfo(table));
Review Comment:
**Issue (Low-Medium): TOCTOU gap between check and drop**
`checkNoReferencingForeignKeys` (line 1028) acquires+releases the
ConstraintManager `readLock`, then `dropTableConstraints` (line 1030)
acquires+releases the `writeLock`. Between these two calls, another thread
could theoretically `addConstraint` with a new FK referencing this table's PK,
bypassing the check.
In practice this is mitigated by the DB write lock held at the call site,
which makes concurrent DDL on the same database unlikely. But for defense in
depth, consider combining these into a single atomic operation:
```java
// e.g., constraintManager.checkAndDropTableConstraints(tni, !isForceDrop &&
!isReplay);
```
This would hold the writeLock for both the check and the drop, eliminating
the gap entirely.
##########
fe/fe-core/src/main/java/org/apache/doris/persist/EditLog.java:
##########
@@ -1187,18 +1189,36 @@ public static void loadJournal(Env env, Long logId,
JournalEntity journal) {
case OperationType.OP_ADD_CONSTRAINT: {
final AlterConstraintLog log = (AlterConstraintLog)
journal.getData();
try {
-
log.getTableIf().replayAddConstraint(log.getConstraint());
+ TableNameInfo tni = log.getTableNameInfo();
+ Constraint constraint = log.getConstraint();
+ if (tni == null) {
+ LOG.warn("Failed to replay add constraint {}: "
+ + "table name could not be resolved",
+ constraint.getName());
+ break;
+ }
+ env.getConstraintManager().addConstraint(
+ tni, constraint.getName(), constraint, true);
} catch (Exception e) {
- LOG.error("Failed to replay add constraint", e);
+ LOG.warn("Failed to replay add constraint", e);
}
Review Comment:
**Issue (High): Overly broad exception catching silently swallows replay
errors**
The `catch (Exception e)` block here catches ALL exceptions (including
`NullPointerException`, `ClassCastException`, programming bugs, etc.) and only
logs a warning. This is inconsistent with the standard EditLog replay pattern —
most other replay operations (e.g., `OP_DROP_CATALOG`, `OP_INDEX_CHANGE_JOB`)
let exceptions propagate to the outer handler at line ~1497, which calls
`System.exit(-1)` to prevent split-brain.
If a programming bug causes constraint replay to fail, the FE will silently
continue with incorrect constraint metadata, leading to leader-follower
inconsistency.
**Suggestion**: Narrow the catch to only expected exceptions, or remove the
inner try-catch entirely and let the outer handler deal with unexpected
failures:
```java
// Keep the tni == null check (line 1194-1199) for graceful handling of
dropped tables,
// but remove the broad catch(Exception) block so unexpected errors fail
fast.
```
The same issue applies to `OP_DROP_CONSTRAINT` at line 1220-1222.
--
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]
