github-actions[bot] commented on code in PR #66307:
URL: https://github.com/apache/doris/pull/66307#discussion_r3843898029
##########
fe/fe-core/src/main/java/org/apache/doris/catalog/CatalogRecycleBin.java:
##########
@@ -233,6 +234,19 @@ public boolean recycleTable(long dbId, Table table,
boolean isReplay,
}
}
+ @SuppressWarnings("deprecation")
+ public boolean containsDistributionMappingConstraint() {
+ readLock();
+ try {
+ return idToTable.values().stream()
Review Comment:
[P1] Fence the live-to-recycle mapping transition
Scanning the two stable endpoints still leaves a gap during non-swap,
non-force `REPLACE TABLE`. That path removes the original table's global
constraints first and only later inserts the still-mapped table object into
`idToTable`, without holding the frontend-admission fence. An `ADD FRONTEND` in
between sees neither a live nor recycled mapping, admits an older FE, and the
replace then publishes the unsupported subtype into recycle/image state. Please
hold the admission fence across that whole transition (and test the race),
rather than relying only on endpoint scans.
##########
fe/fe-core/src/main/java/org/apache/doris/persist/EditLog.java:
##########
@@ -1541,6 +1556,41 @@ public static void loadJournal(Env env, Long logId,
JournalEntity journal) {
}
}
+ private static void replayConstraint(
+ Env env, TableNameInfo tableNameInfo, Constraint constraint,
boolean add) {
+ TableIf table = null;
+ if (constraint instanceof DistributionMappingConstraint) {
+ CatalogIf<?> catalog =
env.getCatalogMgr().getCatalog(tableNameInfo.getCtl());
+ DatabaseIf<?> database = catalog == null ? null :
catalog.getDbNullable(tableNameInfo.getDb());
+ table = database == null ? null :
database.getTableNullable(tableNameInfo.getTbl());
+ if (table != null && !table.tryWriteLock(
Review Comment:
[P1] Do not turn replay lock contention into a fatal error
A serving follower can hold this table's read lock while planning a query.
If that ordinary planning work lasts beyond `catalog_try_lock_timeout_ms`, this
throws into the top-level replay handler, which calls `System.exit(-1)`, so
committing a mapping DDL can kill an otherwise healthy follower. Replay needs
to block or retry until the lock is available (with a replay-specific policy),
not inherit the bounded user-DDL timeout. Please add a latch test that releases
a long-held read lock and verifies replay subsequently succeeds.
##########
fe/fe-core/src/main/java/org/apache/doris/backup/RestoreJob.java:
##########
@@ -2586,154 +2651,184 @@ protected void cleanMetaObjects(boolean isReplay) {
}
private Status atomicReplaceOlapTables(Database db, boolean isReplay) {
+ Preconditions.checkState(db.isWriteLockHeldByCurrentThread(),
+ "atomic replacement must hold the database write lock");
+ Status validationStatus = prevalidateAtomicRestoreTargets(db);
+ if (!validationStatus.ok()) {
+ return validationStatus;
+ }
+ try {
+
Env.getCurrentEnv().getConstraintManager().checkAndDropTableConstraints(
Review Comment:
[P1] Invalidate dependent MTMV plans with the constraint batch drop
This removes the targets' PK/UK/FK metadata, but the replacement loop only
fences the SQL-result cache. A dependent MTMV remains keyed to the same
qualified base-table name, so its existing `MTMVCache` can still be returned
even though the uniqueness/FK proofs used to build that plan were just deleted.
Explicit `DROP CONSTRAINT` now fences these generations inside the metadata
transition; atomic restore bypasses that path. Please collect and invalidate
all dependent MTMV caches before publishing the replacements, with equivalent
replay coverage.
##########
fe/fe-core/src/main/java/org/apache/doris/backup/RestoreJob.java:
##########
@@ -1200,6 +1205,8 @@ protected void allReplicasCreated() {
+ " already exist in db: " + db.getFullName());
return;
}
+
Env.getCurrentEnv().getConstraintManager().restoreTableConstraints(
Review Comment:
[P1] Fence mapping-bearing restores against older frontends
This restores the mapping stored in the table object without the
frontend-version/admission protocol used by ordinary mapping ADD (the replay
and atomic-restore calls have the same gap). For example, after taking a backup
with a mapping, dropping the live mapping allows an older FE to be added;
restoring that backup then serializes `backupMeta`/`restoredTbls` with the
unknown `DistributionMappingConstraint` subtype and can break that follower's
replay/image loading. The existing ADD/recycle checks do not cover this import
path. Please validate mapping-bearing restores under the same admission fence
before any such job/table is journaled or published, and cover mixed-version
restore plus concurrent ADD FRONTEND.
##########
fe/fe-core/src/main/java/org/apache/doris/backup/RestoreJob.java:
##########
@@ -2586,154 +2651,184 @@ protected void cleanMetaObjects(boolean isReplay) {
}
private Status atomicReplaceOlapTables(Database db, boolean isReplay) {
+ Preconditions.checkState(db.isWriteLockHeldByCurrentThread(),
+ "atomic replacement must hold the database write lock");
+ Status validationStatus = prevalidateAtomicRestoreTargets(db);
+ if (!validationStatus.ok()) {
+ return validationStatus;
+ }
+ try {
+
Env.getCurrentEnv().getConstraintManager().checkAndDropTableConstraints(
+ getAtomicRestoreConstraintDropTargets(db),
!isForceReplace);
+ } catch (DdlException e) {
+ return new Status(ErrCode.COMMON_ERROR,
+ "replace table failed, reason=" + e.getMessage());
+ }
for (String tableName : jobInfo.backupOlapTableObjects.keySet()) {
- String originName = jobInfo.getAliasByOriginNameIfSet(tableName);
- if (GlobalVariable.isStoredTableNamesLowerCase()) {
- originName = originName.toLowerCase();
- }
+ String originName = restoreTargetName(tableName);
String aliasName = tableAliasWithAtomicRestore(originName);
- if (!db.writeLockIfExist()) {
- return Status.OK;
- }
+ Table newTbl = db.getTableNullable(aliasName);
+ Preconditions.checkNotNull(newTbl);
+ Preconditions.checkState(newTbl.getType() == TableType.OLAP);
+ Table originTbl = db.getTableNullable(originName);
+ Preconditions.checkState(originTbl == null
+ || originTbl.getType() == TableType.OLAP);
+ OlapTable originOlapTbl = (OlapTable) originTbl;
+
+ // replace the table.
+ OlapTable newOlapTbl = (OlapTable) newTbl;
+ newOlapTbl.writeLock();
try {
- Table newTbl = db.getTableNullable(aliasName);
- if (newTbl == null) {
- LOG.warn("replace table from {} to {}, but the temp table
is not found" + " isAtomicRestore: {}",
- aliasName, originName, isAtomicRestore);
- return new Status(ErrCode.COMMON_ERROR, "replace table
failed, the temp table "
- + aliasName + " is not found");
- }
- if (newTbl.getType() != TableType.OLAP) {
- LOG.warn(
- "replace table from {} to {}, but the temp table
is not OLAP, it type is {}"
- + " isAtomicRestore: {}",
- aliasName, originName, newTbl.getType(),
isAtomicRestore);
- return new Status(ErrCode.COMMON_ERROR, "replace table
failed, the temp table " + aliasName
- + " is not OLAP table, it is " + newTbl.getType());
- }
-
- OlapTable originOlapTbl = null;
- Table originTbl = db.getTableNullable(originName);
- if (originTbl != null) {
- if (originTbl.getType() != TableType.OLAP) {
- LOG.warn(
- "replace table from {} to {}, but the origin
table is not OLAP, it type is {}"
- + " isAtomicRestore: {}",
- aliasName, originName, originTbl.getType(),
isAtomicRestore);
- return new Status(ErrCode.COMMON_ERROR, "replace table
failed, the origin table "
- + originName + " is not OLAP table, it is " +
originTbl.getType());
- }
- originOlapTbl = (OlapTable) originTbl; // save the origin
olap table, then drop it.
- }
+ TableNameInfo originTableInfo = new TableNameInfo(
+ InternalCatalog.INTERNAL_CATALOG_NAME,
db.getFullName(), originName);
+ // rename new table name to origin table name and add the new
table to database.
+ db.unregisterTable(aliasName);
+ newOlapTbl.setName(originName);
+ db.unregisterTable(originName);
+ db.registerTable(newOlapTbl);
+
Env.getCurrentEnv().getConstraintManager().restoreTableConstraints(
+ originTableInfo, newOlapTbl);
+ Env.getCurrentEnv().getSqlCacheManager()
+
.invalidateAboutTableAndFencePublication(originTableInfo);
+
+ // set the olap table state to normal immediately for querying
+ newOlapTbl.setState(OlapTableState.NORMAL);
+ LOG.info(
+ "restore with replace table {} name to {}, and set
state to normal, origin table={}"
+ + " isAtomicRestore: {}",
+ newOlapTbl.getId(), originName,
+ originOlapTbl == null ? -1L : originOlapTbl.getId(),
+ isAtomicRestore);
+ } finally {
+ newOlapTbl.writeUnlock();
+ }
- // replace the table.
- OlapTable newOlapTbl = (OlapTable) newTbl;
- newOlapTbl.writeLock();
+ if (originOlapTbl != null) {
+ // The origin table is not used anymore, need to drop all its
tablets.
+ originOlapTbl.writeLock();
try {
- // rename new table name to origin table name and add the
new table to database.
- db.unregisterTable(aliasName);
- newOlapTbl.checkAndSetName(originName, false);
- db.unregisterTable(originName);
- db.registerTable(newOlapTbl);
-
- // set the olap table state to normal immediately for
querying
- newOlapTbl.setState(OlapTableState.NORMAL);
- LOG.info(
- "restore with replace table {} name to {}, and set
state to normal, origin table={}"
- + " isAtomicRestore: {}",
- newOlapTbl.getId(), originName, originOlapTbl ==
null ? -1L : originOlapTbl.getId(),
- isAtomicRestore);
- } catch (DdlException e) {
- LOG.warn("restore with replace table {} name from {} to
{}, isAtomicRestore: {}",
- newOlapTbl.getId(), aliasName, originName,
isAtomicRestore, e);
- return new Status(ErrCode.COMMON_ERROR, "replace table
from " + aliasName + " to " + originName
- + " failed, reason=" + e.getMessage());
+ LOG.info("drop the origin olap table {}. table={}" + "
isAtomicRestore: {}",
+ originOlapTbl.getName(), originOlapTbl.getId(),
isAtomicRestore);
+ Env.getCurrentEnv().onEraseOlapTable(db.getId(),
originOlapTbl, isReplay);
} finally {
- newOlapTbl.writeUnlock();
+ originOlapTbl.writeUnlock();
}
-
- if (originOlapTbl != null) {
- // The origin table is not used anymore, need to drop all
its tablets.
- originOlapTbl.writeLock();
- try {
- LOG.info("drop the origin olap table {}. table={}" + "
isAtomicRestore: {}",
- originOlapTbl.getName(),
originOlapTbl.getId(), isAtomicRestore);
- Env.getCurrentEnv().onEraseOlapTable(db.getId(),
originOlapTbl, isReplay);
- } finally {
- originOlapTbl.writeUnlock();
- }
- }
- } finally {
- db.writeUnlock();
}
}
for (BackupJobInfo.BackupViewInfo backupViewInfo :
jobInfo.newBackupObjects.views) {
- String originName =
jobInfo.getAliasByOriginNameIfSet(backupViewInfo.name);
- if (GlobalVariable.isStoredTableNamesLowerCase()) {
- originName = originName.toLowerCase();
- }
+ String originName = restoreTargetName(backupViewInfo.name);
String aliasName = tableAliasWithAtomicRestore(originName);
- if (!db.writeLockIfExist()) {
- return Status.OK;
- }
+ Table newTbl = db.getTableNullable(aliasName);
+ Preconditions.checkNotNull(newTbl);
+ Preconditions.checkState(newTbl.getType() == TableType.VIEW);
+ Table originTbl = db.getTableNullable(originName);
+ Preconditions.checkState(originTbl == null
+ || originTbl.getType() == TableType.VIEW);
+ View originViewTbl = (View) originTbl;
+
+ // replace the view.
+ View newViewTbl = (View) newTbl;
+ newViewTbl.writeLock();
try {
- Table newTbl = db.getTableNullable(aliasName);
- if (newTbl == null) {
- LOG.warn("replace view from {} to {}, but the temp view is
not found" + " isAtomicRestore: {}",
- aliasName, originName, isAtomicRestore);
- return new Status(ErrCode.COMMON_ERROR, "replace view
failed, the temp view "
- + aliasName + " is not found");
- }
- if (newTbl.getType() != TableType.VIEW) {
- LOG.warn(
- "replace view from {} to {}, but the temp view is
not VIEW, it type is {}"
- + " isAtomicRestore: {}",
- aliasName, originName, newTbl.getType(),
isAtomicRestore);
- return new Status(ErrCode.COMMON_ERROR, "replace view
failed, the temp view " + aliasName
- + " is not OLAP, it is " + newTbl.getType());
- }
+ // rename new view name to origin view name and add the new
view to database.
+ db.unregisterTable(aliasName);
+ db.unregisterTable(originName);
+ newViewTbl.setName(originName);
+ db.registerTable(newViewTbl);
+
+ LOG.info(
+ "restore with replace view {} name to {}, origin
view={}"
+ + " isAtomicRestore: {}",
+ newViewTbl.getId(), originName,
+ originViewTbl == null ? -1L : originViewTbl.getId(),
+ isAtomicRestore);
+ } finally {
+ newViewTbl.writeUnlock();
+ }
+ }
- View originViewTbl = null;
- Table originTbl = db.getTableNullable(originName);
- if (originTbl != null) {
- if (originTbl.getType() != TableType.VIEW) {
- LOG.warn(
- "replace view from {} to {}, but the origin
view is not VIEW, it type is {}"
- + " isAtomicRestore: {}",
- aliasName, originName, originTbl.getType(),
isAtomicRestore);
- return new Status(ErrCode.COMMON_ERROR, "replace view
failed, the origin view "
- + originName + " is not VIEW, it is " +
originTbl.getType());
- }
- originViewTbl = (View) originTbl; // save the origin view,
then drop it.
- }
+ return Status.OK;
+ }
- // replace the view.
- View newViewTbl = (View) newTbl;
- newViewTbl.writeLock();
- try {
- // rename new view name to origin view name and add the
new view to database.
- db.unregisterTable(aliasName);
- db.unregisterTable(originName);
- newViewTbl.setName(originName);
- db.registerTable(newViewTbl);
-
- LOG.info(
- "restore with replace view {} name to {}, origin
view={}"
- + " isAtomicRestore: {}",
- newViewTbl.getId(), originName,
- originViewTbl == null ? -1L :
originViewTbl.getId(),
- isAtomicRestore);
- } finally {
- newViewTbl.writeUnlock();
+ private Status prevalidateAtomicRestoreTargets(Database db) {
+ for (String tableName : jobInfo.backupOlapTableObjects.keySet()) {
+ String originName = restoreTargetName(tableName);
+ String aliasName = tableAliasWithAtomicRestore(originName);
+ Table newTable = db.getTableNullable(aliasName);
+ if (newTable == null) {
+ return new Status(ErrCode.COMMON_ERROR,
+ "replace table failed, the temp table " + aliasName +
" is not found");
+ }
+ if (newTable.getType() != TableType.OLAP) {
+ return new Status(ErrCode.COMMON_ERROR, "replace table failed,
the temp table "
+ + aliasName + " is not OLAP table, it is " +
newTable.getType());
+ }
+ try {
+ ((OlapTable) newTable).checkAndSetName(originName, true);
+ } catch (DdlException e) {
+ return new Status(ErrCode.COMMON_ERROR, "replace table failed,
the temp table "
+ + aliasName + " cannot be renamed to " + originName
+ + ", reason=" + e.getMessage());
+ }
+ Table originTable = db.getTableNullable(originName);
+ if (originTable != null && originTable.getType() !=
TableType.OLAP) {
+ return new Status(ErrCode.COMMON_ERROR, "replace table failed,
the origin table "
+ + originName + " is not OLAP table, it is " +
originTable.getType());
+ }
+ }
+ for (BackupJobInfo.BackupViewInfo backupViewInfo :
jobInfo.newBackupObjects.views) {
+ String originName = restoreTargetName(backupViewInfo.name);
+ String aliasName = tableAliasWithAtomicRestore(originName);
+ Table newView = db.getTableNullable(aliasName);
+ if (newView == null) {
+ return new Status(ErrCode.COMMON_ERROR,
+ "replace view failed, the temp view " + aliasName + "
is not found");
+ }
+ if (newView.getType() != TableType.VIEW) {
+ return new Status(ErrCode.COMMON_ERROR, "replace view failed,
the temp view "
+ + aliasName + " is not VIEW, it is " +
newView.getType());
+ }
+ Table originView = db.getTableNullable(originName);
+ if (originView != null && originView.getType() != TableType.VIEW) {
+ return new Status(ErrCode.COMMON_ERROR, "replace view failed,
the origin view "
+ + originName + " is not VIEW, it is " +
originView.getType());
+ }
+ }
+ return Status.OK;
+ }
+
+ private List<TableNameInfo> getAtomicRestoreConstraintDropTargets(Database
db) {
+ Set<String> tableNames = Sets.newLinkedHashSet();
+ for (String tableName : jobInfo.backupOlapTableObjects.keySet()) {
+ String originName = restoreTargetName(tableName);
+ tableNames.add(originName);
+ tableNames.add(tableAliasWithAtomicRestore(originName));
+ }
+ for (BackupJobInfo.BackupViewInfo view :
jobInfo.newBackupObjects.views) {
+ String originName = restoreTargetName(view.name);
+ tableNames.add(originName);
+ tableNames.add(tableAliasWithAtomicRestore(originName));
+ }
+ if (isCleanTables) {
Review Comment:
[P1] Keep clean-table mappings covered by frontend admission
With `clean_tables=true`, this adds every live table to the earlier batch
constraint drop. That removes a non-restored table's global mapping while its
concrete table object still retains the mapping and remains live until the
later drop/recycle loop. `ADD FRONTEND` does not take the database lock, so it
can pass both the live-manager and recycle-bin scans in that interval; the
later per-table fence only serializes after the old FE was admitted and does
not revalidate it. Please hold the admission fence across the batch-to-recycle
transition, or defer each clean table's mapping removal to its already-fenced
drop, and add a concurrent clean-restore/admission test.
##########
fe/fe-core/src/main/java/org/apache/doris/catalog/constraint/ConstraintManager.java:
##########
@@ -117,20 +157,111 @@ public void addConstraint(TableNameInfo tableNameInfo,
String constraintName,
tableNameInfo, (ForeignKeyConstraint) constraint);
}
tableConstraints.put(constraintName, constraint);
+ if (constraint instanceof DistributionMappingConstraint) {
+ putTableLocalConstraint(table, constraintName, constraint);
Review Comment:
[P1] Include active backup copies in frontend compatibility admission
Publishing the mapping into `TableAttributes` also makes every active
`BackupJob.backupMeta` deep-copy carry the new Gson subtype. That job can
outlive a later live `DROP CONSTRAINT`: after a checkpoint compacts the
ADD/DROP history, the manager and recycle-bin scans are empty, but
`BackupHandler.write` still serializes the mapping-bearing active job. An older
FE can then be admitted and fail loading that image even though no restore
occurs, so the restore and recycle fixes do not cover this holder. Please keep
mapping-bearing backup-job state inside the compatibility/admission protocol
until it is no longer journaled or checkpointed (or use a backward-readable
representation), with a paused-backup/drop/checkpoint/admission test.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/MetastoreEventSyncDriver.java:
##########
@@ -276,6 +293,8 @@ private void applyOne(PluginDrivenExternalCatalog catalog,
Connector connector,
case UNREGISTER_TABLE:
catalogMgr.unregisterExternalTableFromEvent(
before.localDbName, before.localTableName,
catalogName);
+
Env.getCurrentEnv().getConstraintManager().dropTableConstraints(
Review Comment:
[P1] Invalidate MTMV plans on external structural drops
This new manager transition removes the table's PK/UK/FK proofs, but neither
the event path nor the parallel explicit/replay plugin drops advance dependent
MTMV rewrite generations. After the external table is recreated under the same
qualified name, its generated ID and `BaseTableInfo` identity are name-stable,
and `getOrGenerateCache` can return the old plan immediately even though the
proof used to build it is gone. The existing structural-event thread required
removing the manager entries; it does not make this new cache transition
happen. Please collect and invalidate dependent MTMVs inside the same
metadata-mutation interval for table/database drops, with a pre-populated-cache
drop/recreate test.
--
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]
