englefly commented on PR #68282:
URL: https://github.com/apache/doris/pull/68282#issuecomment-5806783862
New commit `f1359ea6196` fixes the two P1s of the last review (inline
`r4082465261` and `r4082465267`). Only the two replay-capable statistics
removals changed plus a new test class.
**What was wrong**
`removeTableStatsAndLog()` writes `OP_DELETE_TABLE_STATS`, and two of the
call sites I had converted are reached with `isReplay` set:
`InternalCatalog.unprotectDropTable()` (the shared body of live DROP and of
`replayDropTable` / `replayDropDb` / replay-time MTMV stream cleanup) and the
non-swap branch of `Alter.replaceTableInternal()` (`replayReplaceTable()`).
Writing to the edit log while replaying it is fatal on a BDB replica, and on
the live path the separate entry made the removal durable before the enclosing
DROP/REPLACE entry, so a crash in between restored a table without its
statistics.
**Fix**
Both sites use the plain `removeTableStats(...)` again — the enclosing
DROP/REPLACE entry owns the transition and its replay runs the same code on
every frontend. The pairing added for the transitions which journal on their
own (whole-table DROP STATS from the cleaner and the auto collector, rename,
schema change) is unchanged.
**Tests (new `StatisticsRecordReplayTest`, real dispatchers, edit log that
rejects writes)**
- `testDropTableReplayDoesNotJournalTheStatsDeletion`: seeds the record,
swaps in `Mockito.spy(env.getEditLog())` whose `logDeleteTableStats` throws,
calls `Env.getCurrentEnv().replayDropTable(db, tableId, true, 0L)`, asserts the
record is removed.
- `testReplaceTableReplayDoesNotJournalTheStatsDeletion`: same, through
`Env.getCurrentEnv().getAlterInstance().replayReplaceTable(...)` with
`swap=false`.
- Negative control: both tests fail with `a replay must not write
OP_DELETE_TABLE_STATS` when `removeTableStatsAndLog` is put back (that version
was actually built and run, not assumed).
**Verification on this commit**
- `StatisticsRecordReplayTest` 2/2, `TableStatsMetaTest` 9/9,
`AnalysisManagerTest` 18/18 (`mvn test -pl fe-core -am
-Dtest='StatisticsRecordReplayTest,TableStatsMetaTest,AnalysisManagerTest'`,
BUILD SUCCESS, checkstyle not skipped).
- `./build.sh --fe` SUCCESS, FE restarted from that build.
- End to end unchanged: `TRUNCATE TABLE t; INSERT INTO t VALUES
(1,1),(2,2),(3,3); explain select * from t;` reports `cardinality=3`.
- Smoke over the two changed paths on that cluster: `DROP TABLE` after
`ANALYZE` then re-CREATE shows no statistics of the dropped table; `ALTER TABLE
t_orig REPLACE WITH TABLE t_new PROPERTIES('swap'='false')` moves the 4 rows to
`t_orig` and leaves no record of the replaced table.
**Still open from earlier reviews (unchanged, stated for completeness)**
- `AnalysisManager.updateTableStats` / `updateTableStatsForAlterStats`
journal the snapshot after the mutation without holding `idToTblStats`
(pre-existing; wrapping it needs the table resolution moved out of the critical
section).
- A reusable MTMV cache entry built before a truncation and re-inserted
after the invalidation.
- Creating the statistics record at CREATE TABLE time is not part of this PR.
--
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]
