englefly commented on code in PR #68282:
URL: https://github.com/apache/doris/pull/68282#discussion_r4254643007
##########
fe/fe-core/src/main/java/org/apache/doris/alter/SchemaChangeHandler.java:
##########
@@ -3543,7 +3543,7 @@ public void modifyTableLightSchemaChange(String rawSql,
Database db, OlapTable o
if (!FeConstants.runningUnitTest) {
Env.getCurrentEnv().getEditLog().logModifyTableAddOrDropInvertedIndices(info);
// Drop table column stats after light schema change
finished.
-
Env.getCurrentEnv().getAnalysisManager().removeTableStats(olapTable.getId());
+
Env.getCurrentEnv().getAnalysisManager().removeTableStatsAndLog(olapTable.getId());
Review Comment:
You are right, and it is fixed in `ae01164abcd`.
`replayModifyTableAddOrDropInvertedIndices()` removes the record itself (before
it replays the entry), so the extra `OP_DELETE_TABLE_STATS` here was redundant:
the transition already belongs to the schema change entry.
I checked all the call sites of `removeTableStatsAndLog()` and found two
more of the same kind, reverted in the same commit:
- `SchemaChangeHandler` add/drop columns:
`replayModifyTableLightSchemaChange()` removes the record.
- `Env.renameColumn`: `replayRenameColumn()` removes the record.
The remaining call sites keep the paired entry, and the javadoc of
`removeTableStatsAndLog()` now spells out when each form is the right one:
- the schema change job (`SchemaChangeJobV2`) is *not* redundant: its replay
does not remove the record at all, so this entry is what propagates the
removal, and the code only runs on the live path (the replay of a job does not
enter `runRunningJob()`). If you would rather have that removal in the replay
like the light schema changes, I can move it.
- whole table `DROP STATS`, the statistics cleaner and the analyzer's
cleanup of an empty table: the removal *is* the operation, there is no
enclosing entry to own it.
Verified on a local FE and BE cluster: ADD COLUMN and RENAME COLUMN remove
the record with no error, and the truncate + immediate load case is unchanged
(cardinality 3).
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/InternalCatalog.java:
##########
@@ -3903,19 +3907,21 @@ public void truncateTable(String dbName, String
tableName, PartitionNamesInfo pa
long versionTimeMs = Config.isNotCloudMode() ?
System.currentTimeMillis() : 0L;
oldPartitions = truncateTableInternal(olapTable, newPartitions,
truncateEntireTable, recyclePartitionParamMap, forceDrop,
version, versionTimeMs);
- if (truncateEntireTable) {
-
Env.getCurrentEnv().getAnalysisManager().removeTableStats(olapTable.getId());
- } else {
- Env.getCurrentEnv().getAnalysisManager().updateUpdatedRows(
- updateRecords, db.getId(), olapTable.getId(), 0);
- }
-
// write edit log
TruncateTableInfo info =
new TruncateTableInfo(db.getId(), db.getFullName(),
olapTable.getId(), olapTable.getName(),
newPartitions, truncateEntireTable,
rawTruncateSql, oldPartitions, forceDrop,
updateRecords, version, versionTimeMs);
- Env.getCurrentEnv().getEditLog().logTruncateTable(info);
Review Comment:
Because the reset of the record and the entry which describes it have to be
one step.
Before, `logTruncateTable(info)` was called once after the if/else, so a
concurrent whole table `DROP STATS` - which removes the record and journals
`OP_DELETE_TABLE_STATS` under the `idToTblStats` monitor - could slip in
between the reset and the truncate entry: the master ended up without the
record, while a follower which applied the two entries in that order recreated
it (`r4078713416`). `AnalysisManager.resetTableStats()` therefore writes the
truncate entry inside the same `synchronized (idToTblStats)` block that
creates/resets the record, which is why the whole table branch no longer logs
at this level.
The `else` branch (partition truncation) keeps its call where it was, and
that is intentional: a partition truncation does not touch the record at all,
it only accumulates `updateRecords` into `updatedRows` through
`updateUpdatedRows()`, so its entry has nothing to be ordered against. I can
add a comment there saying so if you prefer.
--
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]
