englefly commented on code in PR #68282:
URL: https://github.com/apache/doris/pull/68282#discussion_r4071360669
##########
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:
Withdrawn in 343cd946a4a together with the fence, and the point is one of
the reasons for it.
Confirmed: the watermark was read after `truncateTableInternal()` had
replaced the partitions
(`InternalCatalog`, the read was right after the call), and transaction
admission does not take the table
write lock, so a load admitted between the replacement and the read is
inside the fence while its rows
belong to the new partitions. Moving the read before the replacement only
moves the error to the other
side: a transaction which started before the replacement and was removed
would then be counted. The two
moments (the transaction admission point and the partition replacement
point) cannot be ordered by
reading a watermark on either side of the replacement, which is why I
removed the fence instead of
retrying the ordering. Agreed that the boundary has to be carried by the
data itself (partition
incarnation or a per table write generation).
##########
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:
Withdrawn in 343cd946a4a, together with the fail-open path.
Confirmed and it is the reason I did not try to make the failure "closed":
failing a user `TRUNCATE
TABLE` because the statistics bookkeeping cannot read a watermark is not
acceptable either, and
persisting a fail-closed generation state needs the generation to exist in
the first place. The
`try/catch` with `truncateTxnId = -1` is gone with the fence, so there is no
longer a state where the
code claims to be fenced but is not.
##########
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:
Withdrawn in 343cd946a4a. Confirmed, and this is the strongest of the nine
objections.
In cloud mode `CloudGlobalTransactionMgr.getTransactionIdWatermark()` (line
2681) is
`getNextTransactionId() + 1`, i.e. it allocates a transaction id through the
MetaService, so the fence did
external I/O, with a write side effect, while `olapTable.writeLock()` was
held by `truncateTable`
(`fe/fe-core/AGENTS.md` asks metadata-locking paths to avoid RPC and
external IO under catalog/database/
table locks). Moving the call outside the lock would not fix the boundary
problem of the first comment,
so the fence is removed rather than relocated.
##########
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:
Withdrawn in 343cd946a4a. Confirmed.
The event carried the parent transaction id and `isUpdateOfTruncatedRows()`
compared it with the
watermark, so any transaction which was admitted before the truncation but
wrote this table afterwards,
for instance a multi-statement transaction which does `TRUNCATE`-like work
between its statements, or a
transaction which adds this table later, had its valid rows discarded. The
id of the transaction is not
the right identity for "which incarnation of the table did these rows go
to", which is exactly what the
partition/tablet information in the event would give.
##########
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:
Withdrawn in 343cd946a4a, but the ordering hazard is worth separating from
the fence.
The master applies the cloud update in memory (`replayUpdateRowsRecord`) and
then journals it
(`logUpdateRowsRecord`), while the truncate applies its reset under the
table write lock and journals the
truncate entry — the two are not mutually exclusive, so the journal order
can be the opposite of the
apply order and a follower then replays reset-then-update or
update-then-reset. The fence did not address
that at all. Removing the fence does not introduce it either: it exists as
soon as the record survives
the truncation, which is the point of this PR, and it is the same "the
update describes rows which the
truncation removed" gap that I have accepted and documented in the commit
message of 343cd946a4a. A
generation carried by the event would also have to be part of the journal
entry to make replay
commutative, which is the follow-up I proposed.
##########
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:
This one is not caused by the fence or by this PR; the numbers are the same
without it.
A job created after an analysis and before any load has `updateRows = 0`,
and it records that value for
every column it collected, so the baseline it publishes is 0 under both
implementations:
* before this PR: `getBaseIndexDeltaRowCount()` took the maximum
`updatedRows` of the base index column
statistics, and the stale job sets `colStatsMeta.updatedRows =
analyzedJob.updateRows = 0`, so the
baseline is 0 as well, and the fallback is `100 + (5 - 0) = 105`;
* with the baseline field: `updatedRowsBase.set(0)` and the fallback is `100
+ (5 - 0) = 105`.
So a stale snapshot which completes after the table was truncated
over-estimates in the same way before
and after this change. Fencing it needs the truncate generation to travel
with the analysis job: a field
in `AnalysisInfo` (it is journaled and replayed, so it has to be compatible)
filled in
`buildAnalysisJobInfo()`, compared in
`updateTableStats()`/`updateTableStatsForAlterStats()` against the
generation in the record, and the job dropped when it is older. I am happy
to do that as a follow-up
together with the other asynchronous producers (the `DropStatsTask`/analyze
thread 4063317611), but it is
a separate change from this PR.
##########
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:
I looked at this one and I believe the fallback cannot be affected by that
assignment.
`invalidateLocalStats()` (the method `DropStatsTask` ends with) writes only
these to the record:
```java
if (allPartition) { ... tableStats.removeColumn(indexName, column); } //
the column statistics metas
...
tableStats.userInjected = false;
tableStats.rowCount = table.getRowCount();
```
`TableStatsMeta.rowCount` (the field) is not read by the planner fallback:
it is displayed by
`SHOW TABLE STATS` (`ShowTableStatsCommand` line ~244) and set for external
tables, while
`StatsCalculator.getOlapTableRowCount()` uses
`tableStats.getRowCount(indexId)` (which reads
`indexesRowCount`) plus the delta, and `invalidateLocalStats()` never writes
`indexesRowCount` or
`updatedRows`. So the fallback cannot become 6 through this path.
The concern behind it, a late asynchronous cleanup applying to a record
which changed in between, is the
open thread 4063317611 and I agree it should be made generation-aware; with
the fence withdrawn there is
no `truncateTxnId` left to compare, so it needs the same generation
mechanism as the previous comment
rather than a change here.
--
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]
