github-actions[bot] commented on code in PR #69028:
URL: https://github.com/apache/doris/pull/69028#discussion_r4336013846
##########
fe/fe-core/src/main/java/org/apache/doris/statistics/analysis/AnalysisManager.java:
##########
@@ -1276,6 +1276,45 @@ public TableStatsMeta findTableStatsStatus(long tblId) {
return idToTblStats.get(tblId);
}
+ /**
+ * The record of the table, created in the state of a table whose row
count has never been collected when
+ * it doesn't exist yet. A truncation, a whole table DROP STATS and a
schema change remove the record, so
+ * the rows which are loaded afterwards can only be turned into the row
count of the table while the
+ * record exists: the planner falls back to it exactly while the backends
have not reported the row count
+ * of the new tablets yet, which is the interval right after those
operations.
+ *
+ * <p>Loads run this from the transaction visibility of the load, both on
the master
+ * ({@code DatabaseTransactionMgr.updateCatalogAfterVisible}) and while a
follower replays the transaction
+ * state, so the record exists on every frontend without a journal entry
of its own.
+ *
+ * @return the existing or the created record, or null when the table
cannot be resolved any more
+ */
+ private TableStatsMeta findOrCreateTableStats(InternalCatalog catalog,
long dbId, long tableId) {
+ TableStatsMeta tableStats = idToTblStats.get(tableId);
+ if (tableStats != null) {
+ return tableStats;
+ }
+ Optional<Database> dbOption = catalog.getDb(dbId);
+ if (!dbOption.isPresent()) {
+ return null;
+ }
+ Optional<Table> tableOption = dbOption.get().getTable(tableId);
+ if (!tableOption.isPresent() || !(tableOption.get() instanceof
OlapTable)) {
+ return null;
+ }
+ return findOrCreateTableStats((OlapTable) tableOption.get());
+ }
+
+ private TableStatsMeta findOrCreateTableStats(OlapTable table) {
+ TableStatsMeta tableStats = idToTblStats.get(table.getId());
+ if (tableStats != null) {
+ return tableStats;
+ }
+ TableStatsMeta created = new TableStatsMeta(table);
+ TableStatsMeta existing = idToTblStats.putIfAbsent(table.getId(),
created);
Review Comment:
[P2] Preserve the stats-map lock when inserting load records
`writeTableStats()` holds the `idToTblStats` monitor while serializing its
size and entries, but this insertion bypasses that monitor. During a live
metadata dump (`MetaService.dump()` -> serving `Env.dumpImage()`), a cloud BE's
`reportCommitTxnResult()` can reach this helper without a table lock, despite
the dump's table read locks. If it inserts a first-load record after the size
is written but before the iterator is created, the dump declares N records and
emits N+1. `readIdToTblStats()` then leaves the extra JSON in the stream and
the next module reader consumes it, making the image unreadable. This concerns
live dumps; ordinary checkpoints use an isolated Env. Use the same map monitor
as the existing publication/removal helpers, or serialize a consistent snapshot.
##########
fe/fe-core/src/main/java/org/apache/doris/statistics/analysis/TableStatsMeta.java:
##########
@@ -268,6 +382,29 @@ public long getBaseIndexDeltaRowCount(OlapTable table) {
return updatedRows.get() - maxUpdateRows;
}
+ /**
+ * The row count of the index together with the rows loaded since it was
collected, i.e. the row count of
+ * the table. Both are read by the planner without the table lock, for
instance while it plans a direct
+ * scan of a materialized view, so they have to come from the same state
of this record: a collected row
+ * count paired with the baseline of another analysis would count rows
twice or miss them. The rows loaded
+ * while this call runs are not part of the snapshot, whichever state
reads them accumulates them in
+ * {@link #updatedRows} and reports them as delta rows.
+ */
+ public synchronized long getRowCountWithDeltaRows(OlapTable table, long
indexId) {
+ long rowCount = getRowCount(indexId);
+ if (!keepsOneRowPerBaseRow(table, indexId)) {
+ // The index aggregates, or it merges the rows of a unique key
table: it has its own, smaller row
+ // count, and the rows loaded into the base index would overstate
it.
+ return rowCount;
+ }
+ if (rowCount == TableIf.UNKNOWN_ROW_COUNT) {
Review Comment:
[P2] Keep a usable estimate for a newly added projection rollup
On a DUP table `t(k1,k2,v)`, load 5 rows and ANALYZE, then add `r_dup(k1,v)`
and load 100 more rows before the new rollup tablets report their counts. ADD
ROLLUP retains the existing `TableStatsMeta`, so it has a collected base count
and delta=100 but no `r_dup` entry. The plan `Project(k1,v) -> OlapScan(t INDEX
r_dup)` passes `keepsOneRowPerBaseRow()`, yet this branch returns -1 and
`computeOlapScan()` clamps it to 1 although the rollup holds 105 rows.
Previously the fallback returned -1+100=99. For a row-preserving index without
its own snapshot, use the known base count and delta, or preserve the usable
load estimate. Add coverage for ANALYZE -> ADD ROLLUP -> LOAD before the first
new-index report. This needs neither injected stats nor DROP STATS.
##########
fe/fe-core/src/main/java/org/apache/doris/statistics/analysis/AnalysisManager.java:
##########
@@ -1366,52 +1405,52 @@ public void replayUpdateRowsRecord(UpdateRowsEvent
event) {
// Record : TableId -> (TabletId -> update rows)
if (event.getTabletRecords() != null) {
for (Entry<Long, Map<Long, Long>> record :
event.getTabletRecords().entrySet()) {
- TableStatsMeta statsStatus = idToTblStats.get(record.getKey());
- if (statsStatus != null) {
- Optional<Database> dbOption =
catalog.getDb(event.getDbId());
- if (!dbOption.isPresent()) {
- LOG.warn("Database {} does not exist.",
event.getDbId());
- continue;
- }
- Database database = dbOption.get();
- Optional<Table> tableOption =
database.getTable(record.getKey());
- if (!tableOption.isPresent()) {
- LOG.warn("Table {} does not exist in DB {}.",
record.getKey(), event.getDbId());
- continue;
- }
- Table table = tableOption.get();
- if (!(table instanceof OlapTable)) {
- continue;
- }
- OlapTable olapTable = (OlapTable) table;
- short replicaNum =
olapTable.getTableProperty().getReplicaAllocation().getTotalReplicaNum();
- Map<Long, Long> tabletRows = record.getValue();
- if (tabletRows == null || tabletRows.isEmpty()) {
- LOG.info("Tablet row count map is empty");
- continue;
- }
- long rowsForAllReplica = 0;
- for (Entry<Long, Long> entry : tabletRows.entrySet()) {
- rowsForAllReplica += entry.getValue();
- if (LOG.isDebugEnabled()) {
- LOG.debug("Table id {}, tablet id {}, row count
{}",
- record.getKey(), entry.getKey(),
entry.getValue());
- }
- }
- long tableUpdateRows = rowsForAllReplica / replicaNum;
- LOG.info("Update rows for table {} is {}, replicaNum is
{}, "
- + "rows for all replica {}, tablets count {}",
- olapTable.getName(), tableUpdateRows, replicaNum,
rowsForAllReplica, tabletRows.size());
- statsStatus.updatedRows.addAndGet(tableUpdateRows);
- if (StatisticsUtil.enablePartitionAnalyze()) {
- updatePartitionRows(olapTable, tabletRows,
statsStatus, replicaNum);
+ Optional<Database> dbOption = catalog.getDb(event.getDbId());
+ if (!dbOption.isPresent()) {
+ LOG.warn("Database {} does not exist.", event.getDbId());
+ continue;
+ }
+ Database database = dbOption.get();
+ Optional<Table> tableOption =
database.getTable(record.getKey());
+ if (!tableOption.isPresent()) {
+ LOG.warn("Table {} does not exist in DB {}.",
record.getKey(), event.getDbId());
+ continue;
+ }
+ Table table = tableOption.get();
+ if (!(table instanceof OlapTable)) {
+ continue;
+ }
+ OlapTable olapTable = (OlapTable) table;
+ TableStatsMeta statsStatus = findOrCreateTableStats(olapTable);
Review Comment:
[P2] Order record recreation with DROP STATS journaling
The `VISIBLE` transaction is durable before
`DatabaseTransactionMgr.finishTransaction()` calls
`updateCatalogAfterVisible()`. A concurrent whole-table `DROP STATS t` takes no
target-table lock, so it can remove the record, journal `DELETE_TABLE_STATS`,
and finish remote invalidation in that gap. Resuming here creates a record
containing the load's N rows on the master. Followers and the checkpoint
instead replay `VISIBLE` (create N) followed by `DELETE_TABLE_STATS` (remove
it), leaving no record. Before tablet reporting, `Project(*) -> OlapScan(t)`
consequently estimates N on the master but 1 after replay or failover. The
previous get-only path could not resurrect the deleted record. Order
creation/removal consistently with the transaction and deletion journals, and
cover this interleaving with deterministic barriers.
--
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]
