gavinchou commented on PR #67594:
URL: https://github.com/apache/doris/pull/67594#issuecomment-5595563154
As discussed offline, the following two major findings will be addressed in
follow-up fixes. Recording the details here for tracking. Reviewed commit:
`934b1e52f50e6a193c77cb24389262f9749ca0ba`.
### 1. [major] Classic-mode incremental reads can wait for their own
uncommitted transaction
With `enable_eventual_consistent_change=false`, an explicit transaction that
writes to a table and then reads that table through `@incr` can time out while
planning the second statement. For example, with row binlog enabled on `t` and
compatible tables in the same database:
```sql
BEGIN;
INSERT INTO t SELECT id FROM seed;
INSERT INTO dst
SELECT id FROM t@incr('endTimestamp'='2026-01-01 00:00:00');
COMMIT;
```
The first insert registers transaction T and leaves it in `PREPARE`. When
planning the second insert, the new transaction watermark includes T, and
`isPreviousTransactionsFinished` treats T as unfinished. The statement waits
for T, but the same connection cannot execute `COMMIT` until that statement
finishes. This ends in a timeout (about 10 seconds with the default setting),
even when the requested end timestamp is far in the past. The query only needs
already committed history; it does not require reading its own uncommitted
writes.
This is a regression in the classic path: the previous waiter only selected
`COMMITTED` transactions with a matching commit TSO and skipped T. A minimal
Java probe using the actual base/head methods confirmed that the old predicate
skips the same `PREPARE` transaction while the new waiter times out. This was
not a full-cluster SQL test.
Relevant code: [new waiter
call](https://github.com/apache/doris/blob/934b1e52f50e6a193c77cb24389262f9749ca0ba/fe/fe-core/src/main/java/org/apache/doris/qe/TimeBasedChangeVisibleWaiter.java#L163-L175),
[unfinished-transaction
check](https://github.com/apache/doris/blob/934b1e52f50e6a193c77cb24389262f9749ca0ba/fe/fe-core/src/main/java/org/apache/doris/transaction/DatabaseTransactionMgr.java#L2663-L2680).
Suggested follow-up: handle the current session's uncommitted transaction
explicitly without weakening the fence for other transactions that already
obtained an in-range commit TSO, and add a multi-statement transaction
regression test. Simply skipping every `PREPARE` transaction would need a
separate correctness argument for the TSO-allocation/state-transition window.
### 2. [major] INCR reads inside a CTE still bypass the fence and future-end
validation
```sql
WITH c AS (
SELECT id FROM t@incr('endTimestamp'='2999-01-01 00:00:00')
)
SELECT * FROM c;
```
`collectChangeReadInfo` uses `plan.foreach`, which only traverses
`children()`. A `LogicalCTE` stores its CTE definitions in `aliasQueries`,
exposed through `extraPlans()`:
```text
LogicalCTE
children: Project -> UnboundRelation(c)
extraPlans: SubQueryAlias(c) -> Project -> UnboundRelation(t@incr)
```
Although `CollectRelation` collects the underlying table separately, the
waiter finds no incremental relation here and returns early. The later
timestamp conversion in `BindRelation` does not repeat the future-end
validation. Consequently, this shape skips both the new validation and the read
fence. The actual `foreach` method was checked with a minimal matching tree;
the SQL-level conclusion is based on the complete static call chain, not a
cluster execution.
The traversal omission predates this PR. This finding is an existing gap
left uncovered by the fix, not a newly introduced traversal regression.
Relevant code: [change-read
collection](https://github.com/apache/doris/blob/934b1e52f50e6a193c77cb24389262f9749ca0ba/fe/fe-core/src/main/java/org/apache/doris/qe/TimeBasedChangeVisibleWaiter.java#L180-L212),
[foreach
traversal](https://github.com/apache/doris/blob/934b1e52f50e6a193c77cb24389262f9749ca0ba/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/TreeNode.java#L218-L223),
[CTE plan
storage](https://github.com/apache/doris/blob/934b1e52f50e6a193c77cb24389262f9749ca0ba/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/logical/LogicalCTE.java#L42-L70).
Suggested follow-up: accumulate incremental-read information during complete
relation collection, or explicitly traverse CTE definitions, expression
subqueries, and expanded view plans. Add a CTE future-end rejection test and a
test showing that nested reads actually wait for relevant unfinished
transactions.
--
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]
