englefly commented on code in PR #68282:
URL: https://github.com/apache/doris/pull/68282#discussion_r4067719348
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/InternalCatalog.java:
##########
@@ -3904,7 +3904,7 @@ public void truncateTable(String dbName, String
tableName, PartitionNamesInfo pa
oldPartitions = truncateTableInternal(olapTable, newPartitions,
truncateEntireTable, recyclePartitionParamMap, forceDrop,
version, versionTimeMs);
if (truncateEntireTable) {
-
Env.getCurrentEnv().getAnalysisManager().removeTableStats(olapTable.getId());
+
Env.getCurrentEnv().getAnalysisManager().resetTableStats(olapTable);
Review Comment:
Confirmed and fixed in bc9a5e70e3, and the fix also closes the outstanding
shared-nothing thread (4063317605), which is the same shape.
The mechanism is exactly as described:
`CloudGlobalTransactionMgr.afterCommitTxnResp()` (lines 539-548) builds the
`tableId -> updated_row_count` map from the commit response and
`AnalysisManager.updateUpdatedRows(map, txnId)` applies it by table id and
journals it, none of it under the table write lock. Keeping the record is what
makes those updates land, where they used to be dropped by the
`idToTblStats.get() == null` check.
The fix carries a generation through the update instead of trying to
serialize it:
* `TableStatsMeta` records the transaction id watermark of the truncation
(`GlobalTransactionMgr.getTransactionIdWatermark()`, no id is allocated). It is
carried in `TruncateTableInfo`, so every frontend resets to the same value and
the decision is deterministic on replay.
* The load transaction id is carried by the event (`UpdateRowsEvent.txnId`),
which is journaled for the cloud path, so a follower applies exactly the same
fence.
* `AnalysisManager.replayUpdateRowsRecord()` discards the updates of the
transactions which started not later than the watermark. Transaction ids are
handed out when a transaction starts and increase monotonically, so this is
exact: a transaction which started before the truncation cannot have loaded
rows which are still in the table, the whole table was replaced, and a
transaction which started afterwards always has a newer id.
* Updates which carry no transaction id (the partition truncation reporting
the rows it removed) and entries written before the watermark existed are
applied as before, so nothing else changes behavior.
Covered by
`AnalysisManagerTest.testUpdateRowsRemovedByTruncateAreNotCounted` (a
transaction at the watermark is discarded, a newer one and an update without a
transaction id are applied) and
`TableStatsMetaTest.testFenceOfTransactionsRemovedByTruncate`.
##########
fe/fe-core/src/main/java/org/apache/doris/statistics/analysis/TableStatsMeta.java:
##########
@@ -130,6 +137,58 @@ public TableStatsMeta(long rowCount, AnalysisInfo
analyzedJob, TableIf table) {
update(analyzedJob, table);
}
+ /**
+ * Create a record for a table which doesn't have one yet, in the state of
an empty table. The rows
+ * loaded into the table are accumulated by {@link
AnalysisManager#replayUpdateRowsRecord}, so a record
+ * has to exist before the first load, otherwise these rows can never be
turned into a row count.
+ */
+ public TableStatsMeta(OlapTable table) {
+ this.ctlId = table.getDatabase().getCatalog().getId();
+ this.ctlName = table.getDatabase().getCatalog().getName();
+ this.dbId = table.getDatabase().getId();
+ this.dbName = table.getDatabase().getFullName();
+ this.tblId = table.getId();
+ this.tblName = table.getName();
+ this.idxId = -1;
+ this.indexesRowCount = buildEmptyIndexRowCount(table);
+ this.updatedRowsBase.set(0);
+ }
+
Review Comment:
Fixed in bc9a5e70e3 — this was a real defect of the reset, thank you.
`StatsCalculator.computeDeltaRowCount()` now returns 0 unless the selected
index is the base index:
```java
if (olapScan.getSelectedIndexId() != olapTable.getBaseIndexId()) {
return 0;
}
```
The delta is the rows loaded into the base index which the collected *base
index* row count doesn't include yet
(`TableStatsMeta.getBaseIndexDeltaRowCount()` computes it from
`table.getBaseIndexId()`), so it must not be added to the collected row count
of another index. Scoping it in `computeDeltaRowCount()` keeps both consumers
consistent (`getOlapTableRowCount()` and the `deltaRowCount` of the scan
statistics read by `FilterEstimation`).
Coverage added, as you asked: `test_row_count_after_truncate.groovy` now
also creates an `AGGREGATE KEY` table with an `r1(k1, v)` rollup, waits for the
schema change, truncates it and loads 100 rows into the base index, then
asserts that the plan which selects the rollup (`TABLE: ...(r1)`) reports
`cardinality=1`, its own row count, and that the base index scan reports
`cardinality=100`, the delta rows. Both assertions hold whether or not the
backends have reported the new tablets yet, so the case is deterministic. I
verified the same by hand on a local cluster before adding it.
##########
fe/fe-core/src/main/java/org/apache/doris/catalog/Env.java:
##########
@@ -6767,9 +6767,10 @@ public void replayTruncateTable(TruncateTableInfo info)
throws MetaNotFoundExcep
// In previous versions(before 2.1.8), there is no catalog info in
TruncateTableInfo,
// So if the catalog info is empty, we assume it's internal table.
getInternalCatalog().replayTruncateTable(info);
- if (info.isEntireTable()) {
-
Env.getCurrentEnv().getAnalysisManager().removeTableStats(info.getTblId());
- } else {
+ // The stats record of a wholly truncated table is reset by
InternalCatalog.replayTruncateTable()
+ // instead of being removed, so that the rows loaded after the
truncation are still accounted
+ // for. Only the rows removed by a partition truncation are
accounted here.
+ if (!info.isEntireTable()) {
Review Comment:
The behavior of an old FE in the mixed-version window is unchanged by this
PR, and no state is corrupted; what differs is that the improvement is not
available there. Before this change the record was removed on a whole-table
truncation on both paths of an old FE: the DDL path
(`InternalCatalog.truncateTable()` called `removeTableStats()`) and the replay
path (`Env.replayTruncateTable()`, which still contains that removal for old
binaries). An old FE therefore ends up in exactly the state it always ended up
in after replaying the same truncate entry, and it drops the later row count
events for the same reason it did before.
I looked at the two ways to change that, and neither is free:
* A compatible marker journal (`OP_UPDATE_TABLE_STATS` written after the
truncate entry, so the old FE re-creates the record instead of only removing
it) works for an old follower, but the marker carries a *snapshot* of the
record. Re-applying a snapshot on a new frontend can roll back row updates
which were journaled in between — the cloud row count path journals its delta
outside any table lock, so it can be journaled between the truncate entry and
the marker. That is the same class of non-atomicity the previous review round
asked me to remove, so I would rather not put it back without your agreement.
* The reverse direction cannot be solved by a marker at all: with the
documented upgrade order (followers first, master last) the window is an OLD
master with NEW followers. The old master removes the record in its DDL path
and journals only the truncate entry, while a new follower resets and keeps the
record while replaying that same entry.
So the honest options are: (a) keep the current behavior — old FEs behave
exactly as before, new ones keep the record, and the window closes when the
last FE is upgraded; (b) I add the trailing marker and accept the snapshot
caveat; (c) I gate the new behavior behind a config (default on) so an operator
can keep the old behavior during a rolling upgrade and turn it on afterwards.
My recommendation is (a), with the requirement spelled out in the release note;
I am happy to implement (b) or (c) instead if you prefer — please say which. A
rolling-version replay test is possible for (b)/(c), for (a) there is nothing
new to test on the old side beyond the unchanged behavior.
--
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]
