github-actions[bot] commented on code in PR #66307:
URL: https://github.com/apache/doris/pull/66307#discussion_r3709371323
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/DropConstraintCommand.java:
##########
@@ -81,7 +84,18 @@ public void run(ConnectContext ctx, StmtExecutor executor)
throws Exception {
String.format("Unknown constraint %s on table %s.", name,
tableNameInfo));
}
List<MTMV> dependentMtmvs =
MTMVUtil.getDependentMtmvsByConstraint(tableNameInfo, constraint);
-
Env.getCurrentEnv().getConstraintManager().dropConstraint(tableNameInfo, name,
false);
+ if (constraint instanceof DistributionMappingConstraint && table
instanceof OlapTable) {
+ OlapTable olapTable = (OlapTable) table;
+ olapTable.writeLockOrDdlException();
Review Comment:
[P1] Re-resolve the table before locking mapping DROP
This command captures both the table object and its qualified name before
taking this lock. If `REPLACE TABLE A WITH B PROPERTIES ("swap"="true")`
completes in that window, the captured object A is now named B, while object B
is the current A. DROP then locks captured A-now-B, but
`dropConstraint(tableNameInfo=A, ...)` resolves and removes the mapping from
B-now-A; the active object is unprotected. `invalidateAboutTable(table)`
likewise targets A-now-B by ID/name, so a query on B-now-A can plan/cache
across the removal and its mapping-dependent result survives. This is distinct
from the existing ADD/schema-change and ordinary invalidation threads because
the swap makes the lock/invalidation target differ from the manager mutation
target. Please serialize with rename/replace or re-resolve and verify the table
identity under the appropriate catalog/table locks, with a coordinated
swap-vs-DROP test.
##########
fe/fe-core/src/main/java/org/apache/doris/persist/EditLog.java:
##########
@@ -1240,6 +1241,7 @@ public static void loadJournal(Env env, Long logId,
JournalEntity journal) {
List<MTMV> dependentMtmvs =
MTMVUtil.getDependentMtmvsByConstraint(tni, constraint);
env.getConstraintManager().dropConstraint(
tni, constraint.getName(), true);
+ env.getSqlCacheManager().invalidateAboutTable(tni);
Review Comment:
[P1] Re-read this cache entry after journal synchronization
On a lagging follower with strong-consistency reads, `tryParseSql` fetches
its `SqlCacheContext` before calling `syncJournalIfNeeded`. Synchronization can
replay this DROP and this line removes the map entry, but the query still holds
the old context locally. Its later checks compare table identity and visible
data version, neither of which changes for mapping DDL, so it can still return
the cached no-shuffle result after the DROP is synchronized. This is a distinct
follow-on to the earlier missing-invalidation thread: the new invalidation runs
but cannot revoke the value already fetched by the same lookup. Please sync
before the initial cache lookup or re-read/identity-check the entry afterward,
and add a lookup-before-replay regression.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/DropConstraintCommand.java:
##########
@@ -81,7 +84,18 @@ public void run(ConnectContext ctx, StmtExecutor executor)
throws Exception {
String.format("Unknown constraint %s on table %s.", name,
tableNameInfo));
}
List<MTMV> dependentMtmvs =
MTMVUtil.getDependentMtmvsByConstraint(tableNameInfo, constraint);
-
Env.getCurrentEnv().getConstraintManager().dropConstraint(tableNameInfo, name,
false);
+ if (constraint instanceof DistributionMappingConstraint && table
instanceof OlapTable) {
+ OlapTable olapTable = (OlapTable) table;
+ olapTable.writeLockOrDdlException();
+ try {
+
Env.getCurrentEnv().getConstraintManager().dropConstraint(tableNameInfo, name,
false);
+
Env.getCurrentEnv().getSqlCacheManager().invalidateAboutTable(table);
Review Comment:
[P1] Fence cache publication against mapping DROP
This invalidation only removes entries that already exist. A query can
finish planning under the mapping and release its table read lock while
distributed execution continues; DROP then obtains the write lock, removes the
mapping, runs this invalidation, and unlocks. When the earlier query reaches
EOS, `tryAddBeCache` publishes its pre-DROP `SqlCacheContext` without
re-locking/revalidating the table or checking an invalidation epoch. A later
identical lookup accepts it because mapping DDL changes neither the table ID
nor visible data version. This is a distinct follow-on to the earlier
invalidation thread and to the follower lookup race: here the stale entry is
inserted after a correctly targeted invalidation has already completed. Please
fence cache publication with a mapping/table metadata epoch (or equivalent
atomic revalidation), and add a latch-based invalidate-before-insert 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]
