englefly commented on code in PR #68282:
URL: https://github.com/apache/doris/pull/68282#discussion_r4081348087
##########
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:
Addressed at `11b68bbb685`. Every record transition and the journal entry
which describes it are now one step under the same lock, and the transitions of
different operations are serialized by it.
- `AnalysisManager.resetTableStats(table, truncateInfo)` does the
create/reset of the record and `logTruncateTable(...)` inside the same
`synchronized (idToTblStats)` block, and
`truncateInfo.setTableStatsRecordCreated(...)` is set in that same critical
section, so the entry always describes the record as it is at the moment the
entry is written.
- `AnalysisManager.removeTableStatsAndLog(tableId)` (new) does the removal
and `logDeleteTableStats(...)` inside the same block. All the transitions
outside the replay paths use it (DROP TABLE, whole table DROP STATS from the
cleaner and the auto collector, ALTER, rename, schema change); the replay paths
keep the plain `removeTableStats(long)`, which must not journal.
- Because both pairs hold `idToTblStats`, the mutation order and the journal
order are the same order. The `OP_DELETE_TABLE_STATS` / `OP_TRUNCATE_TABLE`
inversion described in the thread cannot happen any more: whichever operation
takes the lock first is the one whose entry is written first, and
`replayResetTableStats(table, recordCreated)` only creates the record when the
trailing truncate entry records that the truncate created it, so a truncate
entry which ran before a deletion no longer resurrects the record on a follower.
- Deterministic tests:
`AnalysisManagerTest#testStatsTransitionAndItsJournalEntryStayOrdered` drives
the two transitions and asserts, from inside the journal write itself, that the
truncate entry is only written when the record is in the state the entry claims
(`recordCreated` agrees with the record existing) and that the deletion entry
is only written once the record is gone, over a create/remove/create cycle;
`AnalysisManagerTest#testReplayOfTruncateFollowsTheRecordedStatsTransition`
covers the replay consequences (deletion applied first plus a truncate entry
which did not create the record -> the record stays absent; a truncate entry
which created it -> `updatedRows = 0`; a truncate entry which did not create it
on a table without a record -> no record).
Honest remaining gap, not addressed here: the analyze completion path
(`AnalysisManager.updateTableStats` / `updateTableStatsForAlterStats`, which
mutate the record and then journal the snapshot with `logCreateTableStats`)
still journals outside `idToTblStats`. It behaves the same before and after
this change. I did not wrap it because the table has to be resolved first
(`StatisticsUtil.findTable` takes the catalog lock), while the truncate path
holds the table lock and then takes `idToTblStats`; taking them in the other
order would introduce the inverse lock order. If you want that path covered
too, I will do it as a separate change that resolves the table before entering
the critical section.
##########
fe/fe-core/src/main/java/org/apache/doris/statistics/analysis/TableStatsMeta.java:
##########
@@ -130,6 +137,59 @@ 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);
+ }
+
+ /**
+ * TRUNCATE TABLE removes all the data of the table. Reset this record
back to the state of an empty
+ * table instead of dropping it, so that the rows loaded after the
truncation can still be accumulated
+ * into {@link #updatedRows} and be reported as the row count of the table.
+ */
+ public void reset(OlapTable table) {
+ rowCount = 0;
+ updatedRows.set(0);
+ // Nothing has been collected for the emptied table, so none of the
rows loaded from now on is
+ // included in the collected row count. They are all delta rows.
+ updatedRowsBase.set(0);
+ partitionUpdateRows.clear();
+ // All the data is removed, so the base index is known to be empty.
Review Comment:
Addressed at `11b68bbb685`: the collected row count and the baseline are now
published and read as one snapshot.
- `TableStatsMeta.reset()` and `TableStatsMeta.update()` run under the
monitor of the record, and the planner no longer composes the two reads itself:
the new `TableStatsMeta.getRowCountWithDeltaRows(table, indexId)` takes the
same monitor and returns the collected row count of the index plus the delta
rows of one single state. `StatsCalculator.getOlapTableRowCount()` uses it, so
the `MTMVCache.from(..., needLock=false)` path you identified can no longer
pair the collected row count of one generation with the baseline of another one
and keep that cardinality for later rewrites. The rollup case is inside the
method (the delta rows belong to the base index), so the previous
`getRowCount(...) + computeDeltaRowCount(...)` composition is gone from that
site.
- The field publication order still matters for the readers which cannot
take that monitor, and it is documented at both transitions: `update()`
publishes the baseline before the collected counts, and `reset()` publishes the
emptied row count only after the baseline moved to the row count of the removed
data. That reorder was measurable: an analysis which collected 100 rows on a
table holding 150 rows, publishing its collected count before its baseline, is
read as `100 + (150 - 0) = 250` by a reader which reads the count first and the
delta second (that is exactly what the pre-change test run reproduced; it is
also why no lock-free ordering alone can fix this — the count reference may be
arbitrarily stale while the baseline is fresh — which is why the two reads had
to become one).
- Tests: `TableStatsMetaTest#testRowCountSnapshotIsCoherent` runs only real
transitions (TRUNCATE TABLE, then 150 rows loaded into the emptied table, then
an analysis which collected 100 of those 150 rows) and asserts that a reader of
`getRowCountWithDeltaRows()` observes either the 150 rows of the table or the
emptied table, never a value of neither state; the inflated 250 fails it.
- One residue I did not close, deliberately: an entry of the reusable MTMV
cache which was built before the truncation and re-inserted after the
invalidation. That is the bypass you named in the alternative —
`needLock=false` plans the definition without holding its base tables — and it
is not specific to this change: a load or an analysis completing during such a
build has the same effect. I did not take the base tables inside that path
because the statement already holds the MTMV, and the refresh path takes base
tables before the MTMV, so it would introduce the inverse lock order; making it
generation-aware (a build which started before the truncation must not be
published) is the shape I would use. Say the word if you want that in this PR
and I will add it as a separate commit.
--
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]
