github-actions[bot] commented on code in PR #66307:
URL: https://github.com/apache/doris/pull/66307#discussion_r3872351725
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/InternalCatalog.java:
##########
@@ -616,50 +628,64 @@ public void recoverDatabase(String dbName, long dbId,
String newDbName) throws D
}
}
- Database db = Env.getCurrentRecycleBin().recoverDatabase(dbName, dbId);
-
// add db to catalog
if (!tryLock(false)) {
throw new DdlException("Failed to acquire catalog lock. Try
again");
}
- db.writeLock();
- List<Table> tableList = db.getTablesOnIdOrder();
- MetaLockUtils.writeLockTables(tableList);
+ ConstraintManager constraintManager =
Env.getCurrentEnv().getConstraintManager();
+ boolean frontendAdmissionAcquired = false;
try {
- if (!Strings.isNullOrEmpty(newDbName)) {
- if (fullNameToDb.containsKey(newDbName)) {
- throw new DdlException("Database[" + newDbName + "]
already exist.");
- // it's ok that we do not put db back to CatalogRecycleBin
- // cause this db cannot recover any more
+ frontendAdmissionAcquired =
constraintManager.acquireFrontendAdmissionFence();
+ Database db = Env.getCurrentRecycleBin().recoverDatabase(dbName,
dbId);
+ db.writeLock();
+ List<Table> tableList = db.getTablesOnIdOrder();
+ MetaLockUtils.writeLockTables(tableList);
+ try {
+ if (!Strings.isNullOrEmpty(newDbName)) {
+ if (fullNameToDb.containsKey(newDbName)) {
+ throw new DdlException("Database[" + newDbName + "]
already exist.");
+ // it's ok that we do not put db back to
CatalogRecycleBin
+ // cause this db cannot recover any more
+ }
+ } else {
+ if (fullNameToDb.containsKey(db.getFullName())) {
+ throw new DdlException("Database[" + db.getFullName()
+ "] already exist.");
+ // it's ok that we do not put db back to
CatalogRecycleBin
+ // cause this db cannot recover any more
+ }
}
- } else {
- if (fullNameToDb.containsKey(db.getFullName())) {
- throw new DdlException("Database[" + db.getFullName() + "]
already exist.");
- // it's ok that we do not put db back to CatalogRecycleBin
- // cause this db cannot recover any more
+ if (!Strings.isNullOrEmpty(newDbName)) {
+ db.setNameWithLock(newDbName);
}
+ fullNameToDb.put(db.getFullName(), db);
+ idToDb.put(db.getId(), db);
+ for (Table table : tableList) {
+ constraintManager.restoreTableConstraints(
+ TableNameInfoUtils.fromDb(db, table.getName()),
table);
+ }
+ // log
+ RecoverInfo recoverInfo = new RecoverInfo(db.getId(), -1L,
-1L, newDbName, "", "", "", "");
+ Env.getCurrentEnv().getEditLog().logRecoverDb(recoverInfo);
+ db.unmarkDropped();
+ registerDbFunctionsToNereids(db);
+ LOG.info("recover database[{}]", db.getId());
+ } finally {
+ MetaLockUtils.writeUnlockTables(tableList);
+ db.writeUnlock();
}
- if (!Strings.isNullOrEmpty(newDbName)) {
- db.setNameWithLock(newDbName);
- }
- fullNameToDb.put(db.getFullName(), db);
- idToDb.put(db.getId(), db);
- // log
- RecoverInfo recoverInfo = new RecoverInfo(db.getId(), -1L, -1L,
newDbName, "", "", "", "");
- Env.getCurrentEnv().getEditLog().logRecoverDb(recoverInfo);
- db.unmarkDropped();
- registerDbFunctionsToNereids(db);
} finally {
- MetaLockUtils.writeUnlockTables(tableList);
- db.writeUnlock();
+ if (frontendAdmissionAcquired) {
+ constraintManager.releaseFrontendAdmissionFence();
+ }
unlock();
}
- LOG.info("recover database[{}]", db.getId());
}
public void recoverTable(String dbName, String tableName, String
newTableName, long tableId) throws DdlException {
Database db = getDbOrDdlException(dbName);
db.writeLockOrDdlException();
+ ConstraintManager constraintManager =
Env.getCurrentEnv().getConstraintManager();
+ boolean frontendAdmissionAcquired =
constraintManager.acquireFrontendAdmissionFence();
Review Comment:
[P1] Release the database lock if admission fencing fails
The database write lock is acquired before this potentially throwing call,
but the `try/finally` that unlocks it starts only afterward. If another
mapping/admission transition holds the new fence long enough for
`acquireFrontendAdmissionFence()` to time out (or this thread is interrupted),
`RECOVER TABLE` exits while its worker still owns the DB lock, wedging later
metadata access to that database. Please move the fence acquisition inside an
outer `try` that always unlocks `db`, and add a failure-path test.
##########
fe/fe-core/src/main/java/org/apache/doris/catalog/RefreshManager.java:
##########
@@ -57,9 +57,18 @@ public class RefreshManager {
// Refresh catalog
public void handleRefreshCatalog(String catalogName, boolean invalidCache)
throws UserException {
CatalogIf catalog =
Env.getCurrentEnv().getCatalogMgr().getCatalogOrAnalysisException(catalogName);
- refreshCatalogInternal(catalog, invalidCache);
- CatalogLog log = CatalogLog.createForRefreshCatalog(catalog.getId(),
invalidCache);
-
Env.getCurrentEnv().getEditLog().logCatalogLog(OperationType.OP_REFRESH_CATALOG,
log);
+ if (catalog instanceof ExternalCatalog) {
+ try (ExternalCatalog.ConstraintMetadataMutationGuard ignored =
+ ((ExternalCatalog)
catalog).beginConstraintMetadataMutation()) {
+ refreshCatalogInternal(catalog, invalidCache);
Review Comment:
[P1] Reconcile constraints before publishing refreshed schemas
This guarded refresh invalidates connector/object caches but never
reconciles `ConstraintManager`. If a constrained external column or table
changed out of band, `REFRESH TABLE`/`DATABASE`/`CATALOG` can therefore expose
the new schema while retaining the old name-keyed PK/UK/FK proof; planning can
hit a missing column, and same-name reuse can reactivate an unsound rewrite
proof. The event-gap path already drops constraints before refreshing. Please
persist and replay an appropriate drop/revalidation (and invalidate dependent
MTMV/SQL rewrite caches) within these refresh scopes, with out-of-band
schema-change coverage.
##########
fe/fe-connector/fe-connector-hms/src/main/java/org/apache/doris/connector/hms/event/HmsEventParser.java:
##########
@@ -126,8 +128,8 @@ private static List<MetastoreChangeDescriptor>
doParse(HmsNotificationEvent even
before.getDbName(), before.getTableName(),
after.getDbName(), afterTable, eventId,
updateTime));
}
- return one(MetastoreChangeDescriptor.forTable(
- Op.REFRESH_TABLE, before.getDbName(),
before.getTableName(), null,
+ return one(MetastoreChangeDescriptor.forTableRefresh(
Review Comment:
[P1] Preserve removed columns across rename events
This refresh branch receives `removedColumnNames(before, after)`, but the
earlier rename/view return bypasses it. An HMS `alter_table` can rename a table
while changing its schema (and the same branch handles view replacement), so
the driver then only renames the centralized constraints. A constraint on a
removed column survives at the target identity, causing missing-column planning
failures or a false proof after name reuse. Please carry and apply both the
identity transition and removed-column cleanup atomically, with
rename-plus-column-drop and same-name replacement coverage.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/MetastoreEventSyncDriver.java:
##########
@@ -285,8 +363,28 @@ private void applyOne(PluginDrivenExternalCatalog catalog,
Connector connector,
catalogMgr.registerExternalTableFromEvent(after.localDbName,
after.remoteTableName, after.localTableName,
catalogName, descriptor.getUpdateTime());
+ TableNameInfo oldTable =
+ new TableNameInfo(catalogName, before.localDbName,
before.localTableName);
+ TableNameInfo newTable =
+ new TableNameInfo(catalogName, after.localDbName,
after.localTableName);
+ if (applyConstraintChanges) {
+ if (persistConstraintChanges) {
+ applyPersistedConstraintMutation(
+
ConstraintManager.MetastoreConstraintMutation.renameTable(oldTable, newTable),
Review Comment:
[P1] Invalidate dependent MTMV plans on rename
Persisting the constraint rename moves the proof, but it does not invalidate
MTMV rewrite caches built with that proof. External dependencies are
name-based, so if the old qualified name is reused, the relation becomes
eligible again while its cached plan can still contain the pre-rename
uniqueness/FK rewrite. The adjacent drop/removed-column paths already
invalidate affected MTMVs. Please fence dependents for both old and new
identities in every event/explicit/replay rename path.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/CatalogMgr.java:
##########
@@ -410,10 +461,27 @@ public void alterCatalogComment(String catalogName,
String comment) throws UserE
* Modify the catalog property and write the meta log.
*/
public void alterCatalogProps(String catalogName, Map<String, String>
newProperties) throws UserException {
+ CatalogIf<?> catalog = getCatalog(catalogName);
+ if (!(catalog instanceof ExternalCatalog)) {
+ alterCatalogPropsInternal(catalogName, newProperties, catalog);
+ return;
+ }
+ try (ExternalCatalog.ConstraintMetadataMutationGuard ignored =
Review Comment:
[P1] Reset constraint state when catalog identity changes
This guard serializes the property update, but identity-affecting changes
(for example a metastore endpoint or `meta_names_mapping`) retain the old
catalog constraints and event cursors. Switching from a source at event 1000 to
one at event 10 leaves polling after 1000 indefinitely, while same-named
objects inherit the old PK/UK/FK proofs; an old-source poll can also publish
after the reset because acquisition happens before this guard. Please make such
changes an exclusive catalog-generation transition that quarantines/reconciles
constraints, resets durable/local cursors, and rejects in-flight work from the
old connector.
##########
fe/fe-core/src/main/java/org/apache/doris/catalog/constraint/ConstraintManager.java:
##########
@@ -117,20 +241,125 @@ public void addConstraint(TableNameInfo tableNameInfo,
String constraintName,
tableNameInfo, (ForeignKeyConstraint) constraint);
}
tableConstraints.put(constraintName, constraint);
+ if (constraint instanceof DistributionMappingConstraint) {
+ putTableLocalConstraint(table, constraintName, constraint);
+ }
if (!replay) {
- logAddConstraint(tableNameInfo, constraint);
+ logItem = submitAddConstraint(tableNameInfo, constraint);
}
LOG.info("Added constraint {} on table {}", constraintName, key);
} finally {
writeUnlock();
+ if (acquireFrontendAdmission) {
+ frontendAdmissionLock.unlock();
+ }
+ }
+ return logItem;
+ }
+
+ public void acquireFrontendAdmission() throws DdlException {
+ try {
+ if (!frontendAdmissionLock.tryLock(
+ Config.catalog_try_lock_timeout_ms,
TimeUnit.MILLISECONDS)) {
+ throw new DdlException("Failed to acquire frontend admission
lock. Try again");
+ }
+ } catch (InterruptedException e) {
+ Thread.currentThread().interrupt();
+ throw new DdlException("Interrupted while acquiring frontend
admission lock", e);
+ }
+ boolean admitted = false;
+ try {
+ readLock();
+ try {
+ for (Map<String, Constraint> tableConstraints :
constraintsMap.values()) {
Review Comment:
[P1] Keep admission closed until mapping history is bootstrap-safe
These scans cover only current holders. Right after an ordinary mapping
DROP, they pass even though the latest image may still contain the ADD and
`OP_DROP_CONSTRAINT` itself serializes the unknown subtype; a newly admitted
old FE loads that image before replay and fails before the removal can help.
Force REPLACE and mapping-free atomic restore also unindex the mapped origin
before their terminal record is durable. Please gate admission on a
mapping-compatibility journal/image watermark (or a backward-readable encoding)
until a post-removal bootstrap image is available, and cover immediate
DROP/replace/restore followed by old-FE bootstrap.
--
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]
