englefly commented on PR #68282: URL: https://github.com/apache/doris/pull/68282#issuecomment-5776105226
Handled in 343cd946a4a: **the transaction id fence is withdrawn**, and the base-index scoping of the delta plus its regression case stay. Why withdrawn rather than patched: all nine inline findings target the fence, and the ones I confirmed (1, 2, 3, 4, 9 — read after the partition replacement / fail open / MetaService RPC and transaction id allocation while holding the table write lock / parent transaction id of a multi-table transaction / non-atomic check with the reset) are properties of the heuristic itself, not bugs I can patch one by one. Moving the watermark read before the replacement only moves the first error to the other side, and the cloud call is an RPC with a write side effect under a lock, which `fe/fe-core/AGENTS.md` rules out. An imprecise, fail-open fence which performs external IO under a table lock is worse than the bounded gap it closes, so the previous commit's fence is removed and the gap is documented instead. Kept: `StatsCalculator.computeDeltaRowCount()` returns the delta only for the base index (the delta is the rows loaded into the base index, a selected aggregate/rollup index has its own, usually much smaller, row count), with the aggregate rollup regression case you asked for. Accepted gap, unchanged by this PR and bounded: a load which was in flight when the table was truncated can still add the rows it loaded, which the truncation removed, to the retained record. The value is only used by the planner fallback until the backends report the new tablets (at most `tablet_stat_update_interval_second`), and it is replaced by the reported count or by the next analysis. Two of your points are exactly this gap seen from the replay side (journal order between the cloud update entry and the truncate entry) and from the reset side (a delayed update arriving during the reset). Not caused by the fence or this PR (with the numbers checked against the pre-PR implementation, they are identical): the stale analysis which completes after a truncation and publishes `updateRows = 0` with a collected row count of the old data (thread 4067844431 and 4067844439). Fencing it needs the truncate generation to travel with the analysis job — a compatible field in `AnalysisInfo` filled in `buildAnalysisJobInfo()` and compared in `updateTableStats()` — which I would rather do in one change together with the asynchronous producers of the open thread 4063317611. One correction (thread 4067844434): `TableStatsMeta.rowCount` is not part of the planner fallback. `getOlapTableRowCount()` uses `getRowCount(indexId)` (which reads `indexesRowCount`) plus the delta; `rowCount` is displayed by `SHOW TABLE STATS` and set for external tables, and `invalidateLocalStats()` never writes `indexesRowCount` or `updatedRows`, so the cleanup task cannot make the fallback report 6 instead of 3. The exact fix for the accepted gap is the one you suggested: carry a per table write generation or the partition incarnation in the row count event. Neither event carries it today (cloud: `tableId -> rows`; local: `tableId -> (tabletId -> rows)`), so I would rather land it as its own change than approximate it again. Tests on this revision (local FE + BE cluster): `TableStatsMetaTest` 7/7, `AnalysisManagerTest` 16/16, `test_row_count_after_truncate` (including the aggregate 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]
