github-actions[bot] commented on code in PR #68282:
URL: https://github.com/apache/doris/pull/68282#discussion_r4067844413
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/InternalCatalog.java:
##########
@@ -3903,8 +3903,18 @@ public void truncateTable(String dbName, String
tableName, PartitionNamesInfo pa
long versionTimeMs = Config.isNotCloudMode() ?
System.currentTimeMillis() : 0L;
oldPartitions = truncateTableInternal(olapTable, newPartitions,
truncateEntireTable, recyclePartitionParamMap, forceDrop,
version, versionTimeMs);
+ // Transactions which started not later than this watermark were
removed by the truncation, their
+ // row count updates may still arrive and must not be counted as
rows loaded after it. Reading the
+ // watermark only protects against those stale updates, so it must
not fail the truncation.
+ long truncateTxnId = -1;
+ try {
+ truncateTxnId =
Env.getCurrentEnv().getGlobalTransactionMgr().getTransactionIdWatermark();
+ } catch (UserException e) {
Review Comment:
The watermark is read only after `truncateTableInternal` has replaced the
partitions, while transaction admission does not take this table lock. A load
can begin on the new partition incarnation before this read; its ID is included
in `truncateTxnId`, so `isUpdateOfTruncatedRows(txnId)` drops its valid
post-truncate delta. Please establish the transaction/generation boundary
atomically with replacement (or carry partition incarnation into the update
event).
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/InternalCatalog.java:
##########
@@ -3903,8 +3903,18 @@ public void truncateTable(String dbName, String
tableName, PartitionNamesInfo pa
long versionTimeMs = Config.isNotCloudMode() ?
System.currentTimeMillis() : 0L;
oldPartitions = truncateTableInternal(olapTable, newPartitions,
truncateEntireTable, recyclePartitionParamMap, forceDrop,
version, versionTimeMs);
+ // Transactions which started not later than this watermark were
removed by the truncation, their
+ // row count updates may still arrive and must not be counted as
rows loaded after it. Reading the
+ // watermark only protects against those stale updates, so it must
not fail the truncation.
+ long truncateTxnId = -1;
+ try {
+ truncateTxnId =
Env.getCurrentEnv().getGlobalTransactionMgr().getTransactionIdWatermark();
+ } catch (UserException e) {
+ LOG.warn("Failed to get the transaction id watermark of
truncate table {}.{}",
+ db.getFullName(), olapTable.getName(), e);
Review Comment:
If MetaService cannot provide the watermark, this catch leaves
`truncateTxnId = -1` but still commits the retained stats record. That sentinel
makes every delayed transaction update pass the fence, so a pre-truncate load
can repopulate removed rows. Please fail/defer the whole-table truncate or
persist a fail-closed generation state; do not proceed with an unfenced reset.
##########
fe/fe-core/src/main/java/org/apache/doris/statistics/analysis/TableStatsMeta.java:
##########
@@ -195,6 +273,15 @@ public void update(AnalysisInfo analyzedJob, TableIf
tableIf) {
if (tableIf instanceof OlapTable) {
OlapTable olapTable = (OlapTable) tableIf;
indexesRowCount.putAll(analyzedJob.indexesRowCount);
+ // The collected row count above already includes the rows
which had been loaded when the
+ // job was built, remember how many they were, they are not
delta rows. The baseline may
+ // only advance together with the collected base index row
count, an analysis of another
+ // index (a materialized view) doesn't touch it.
+ // Statistics supplied by the user are not collected from the
table, they carry no baseline.
+ if (!analyzedJob.userInject
+ &&
analyzedJob.indexesRowCount.containsKey(olapTable.getBaseIndexId())) {
+ updatedRowsBase.set(analyzedJob.updateRows);
+ }
Review Comment:
A job created after a prior analysis but before any load carries `updateRows
= 0` while its collected row count still describes the old 100 rows. If it
completes after reset, `updatedRowsBase` becomes 0 and the old 100 is restored;
with five new rows, fallback returns 105 instead of 5. Please carry the
truncate generation into analysis jobs and discard pre-reset completions.
##########
fe/fe-core/src/main/java/org/apache/doris/statistics/analysis/AnalysisManager.java:
##########
@@ -1356,7 +1356,7 @@ public void replayUpdateRowsRecord(UpdateRowsEvent event)
{
if (event.getRecords() != null) {
for (Entry<Long, Long> record : event.getRecords().entrySet()) {
TableStatsMeta statsStatus = idToTblStats.get(record.getKey());
- if (statsStatus != null) {
+ if (statsStatus != null &&
!statsStatus.isUpdateOfTruncatedRows(event.getTxnId())) {
statsStatus.updatedRows.addAndGet(record.getValue());
Review Comment:
`event.getTxnId()` is the parent transaction ID, but a multi-table
transaction can add this table after its truncate. Those rows target the new
partition incarnation yet `txnId <= truncateTxnId` causes them to be discarded.
Please carry the per-table subtransaction/write generation or partition
incarnation into the event instead of using only the parent ID.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/InternalCatalog.java:
##########
@@ -3903,8 +3903,18 @@ public void truncateTable(String dbName, String
tableName, PartitionNamesInfo pa
long versionTimeMs = Config.isNotCloudMode() ?
System.currentTimeMillis() : 0L;
oldPartitions = truncateTableInternal(olapTable, newPartitions,
truncateEntireTable, recyclePartitionParamMap, forceDrop,
version, versionTimeMs);
+ // Transactions which started not later than this watermark were
removed by the truncation, their
+ // row count updates may still arrive and must not be counted as
rows loaded after it. Reading the
+ // watermark only protects against those stale updates, so it must
not fail the truncation.
+ long truncateTxnId = -1;
+ try {
+ truncateTxnId =
Env.getCurrentEnv().getGlobalTransactionMgr().getTransactionIdWatermark();
+ } catch (UserException e) {
Review Comment:
This call reaches MetaService synchronously while the table write lock
acquired above remains held. A slow/unavailable MetaService can stall readers,
loads, and DDL for the RPC timeout/retry duration. Please obtain a transaction
barrier outside the table lock and coordinate it with replacement instead of
performing external I/O in this critical section.
##########
fe/fe-core/src/main/java/org/apache/doris/statistics/analysis/AnalysisManager.java:
##########
@@ -1487,6 +1487,29 @@ public void removeTableStats(long tableId) {
}
}
+ /**
+ * TRUNCATE TABLE removes all the data of the table, but the table itself
stays and can be loaded
+ * immediately. The stats record must be kept, otherwise the row count of
the newly loaded data can
+ * never be reported: the backends report the row count of the new empty
tablets with a delay of up to
+ * {@code tablet_stat_update_interval_second}, and without a record there
is nothing to accumulate the
+ * loaded rows into. So reset the record to the state of an empty table
instead of removing it.
+ *
+ * <p>The transition is journaled by the truncate itself: the master DDL
path and
+ * {@link InternalCatalog#replayTruncateTable} both apply it. It must not
be journaled here, a separate
+ * entry is not atomic with the truncate entry, and a crash in between
would replay a zeroed record
+ * onto the data which was never truncated.
+ */
+ public void resetTableStats(OlapTable table, long truncateTxnId) {
+ synchronized (idToTblStats) {
+ TableStatsMeta tableStats = idToTblStats.get(table.getId());
+ if (tableStats == null) {
+ tableStats = new TableStatsMeta(table);
+ idToTblStats.put(table.getId(), tableStats);
Review Comment:
`updateTableStats` can observe no record before TRUNCATE, then publish `new
TableStatsMeta(...)` after `resetTableStats` has created the fenced record.
That replacement defaults `truncateTxnId` to `-1`, so delayed
removed-transaction updates pass the fence and stale state replaces the reset.
Please make first-record publication atomic with reset/generation checks and
preserve the current fence.
##########
fe/fe-core/src/main/java/org/apache/doris/cloud/transaction/CloudGlobalTransactionMgr.java:
##########
@@ -545,7 +545,7 @@ public void afterCommitTxnResp(CommitTxnResponse
commitTxnResponse, List<TabletC
}
}
Env env = Env.getCurrentEnv();
- env.getAnalysisManager().updateUpdatedRows(updatedRows);
+ env.getAnalysisManager().updateUpdatedRows(updatedRows, txnId);
// 2. update table and partition version
Review Comment:
This callback can enqueue `OP_LOG_UPDATE_ROWS` concurrently with truncate.
If a valid post-truncate event runs after the in-memory reset but before
`logTruncateTable`, its journal entry may precede `OP_TRUNCATE_TABLE`; a
follower then replays event-then-reset and loses rows the master retained.
Please serialize these journal transitions or make replay
generation-aware/commutative.
##########
fe/fe-core/src/main/java/org/apache/doris/statistics/analysis/TableStatsMeta.java:
##########
@@ -130,6 +142,72 @@ 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, long truncateTxnId) {
+ 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.
Review Comment:
`reset()` clears `updatedRows` before it writes `truncateTxnId`, but
`replayUpdateRowsRecord()` does not take the reset/map lock. A delayed
pre-truncate event can read the old `-1` in this window, add rows after the
clear, and then survive because reset does not clear again. Publish the
generation before clearing or synchronize/reset atomically so no update can
pass during the transition.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/InternalCatalog.java:
##########
@@ -3903,8 +3903,18 @@ public void truncateTable(String dbName, String
tableName, PartitionNamesInfo pa
long versionTimeMs = Config.isNotCloudMode() ?
System.currentTimeMillis() : 0L;
oldPartitions = truncateTableInternal(olapTable, newPartitions,
truncateEntireTable, recyclePartitionParamMap, forceDrop,
version, versionTimeMs);
+ // Transactions which started not later than this watermark were
removed by the truncation, their
+ // row count updates may still arrive and must not be counted as
rows loaded after it. Reading the
+ // watermark only protects against those stale updates, so it must
not fail the truncation.
+ long truncateTxnId = -1;
+ try {
+ truncateTxnId =
Env.getCurrentEnv().getGlobalTransactionMgr().getTransactionIdWatermark();
+ } catch (UserException e) {
+ LOG.warn("Failed to get the transaction id watermark of
truncate table {}.{}",
+ db.getFullName(), olapTable.getName(), e);
+ }
if (truncateEntireTable) {
-
Env.getCurrentEnv().getAnalysisManager().removeTableStats(olapTable.getId());
+
Env.getCurrentEnv().getAnalysisManager().resetTableStats(olapTable,
truncateTxnId);
} else {
Review Comment:
The task holds the reset object and ends by assigning `tableStats.rowCount =
table.getRowCount()`. That value is non-strict (unknown partitions contribute
zero), while post-truncate rows remain in `updatedRows` with baseline 0. With
one new partition reported at 3 and another unknown, fallback becomes 6 instead
of 3. Please fence this cleanup by reset generation and avoid replacing the
empty baseline with a partial count.
--
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]
