englefly commented on PR #68282: URL: https://github.com/apache/doris/pull/68282#issuecomment-5769848688
Review follow-up committed in bc9a5e70e3 (a second commit on top of 643c00b3ce0, so the two rounds can be reviewed separately). Fixed: 1. **Cloud row-count updates can repopulate the retained record** — confirmed the path (`CloudGlobalTransactionMgr.afterCommitTxnResp()` builds the map and `AnalysisManager.updateUpdatedRows()` applies and journals it, outside the table write lock). `TableStatsMeta` now records the transaction id watermark of the truncation (`GlobalTransactionMgr.getTransactionIdWatermark()`, carried in `TruncateTableInfo` so every frontend resets to the same value), the event carries the id of the load transaction (`UpdateRowsEvent.txnId`, journaled so replay decides identically) and `replayUpdateRowsRecord()` discards the updates of transactions which started not later than the watermark. This is exact for a whole-table truncation (the transactions which started before it cannot have loaded rows which are still there) and it also closes the outstanding shared-nothing thread 4063317605, which is the same shape. 2. **The reset zeroed every index while the fallback added a base-index delta** — `StatsCalculator.computeDeltaRowCount()` now returns the delta only for the base index, so a selected aggregate/rollup index no longer reports the base rows as its own. Coverage added as asked: the regression suite now builds an `AGGREGATE KEY` table with an `r1(k1, v)` rollup, truncates it, loads 100 base rows and asserts the rollup scan reports 1 row while the base index scan reports 100; both hold independently of the backend report timing, so the case is deterministic. Explained, with the trade-off you offered (marker vs gate), in the thread for 4064954133: 3. **Mixed-version replay** — the old FE behavior is unchanged by this PR (an old FE removed the record on both the DDL and the replay path before this change too), so nothing is corrupted and the only difference is that the improvement is not available there. A compatible marker has to be written after the truncate entry (otherwise the old `Env.replayTruncateTable()` removal deletes it again) and carries a snapshot of the record, which can roll back row updates journaled in between — the same non-atomicity the previous round asked me to remove. The reverse direction (old master, new followers, which is the documented upgrade order) cannot be fixed by a marker at all. Options: (a) keep the current behavior and note the requirement, (b) add the trailing marker and accept the snapshot caveat, (c) gate the new behavior behind a config defaulting to on. I recommend (a) and will implement (b) or (c) on your word. Tests on this revision (local FE + BE cluster): `TableStatsMetaTest` 8/8, `AnalysisManagerTest` 17/17, `test_row_count_after_truncate` (with the new rollup case) and `test_drop_stats_and_truncate` pass, and the end to end case still reports 3 rows after `TRUNCATE TABLE` + immediate load + immediate query instead of 1. -- 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]
