Doris-Breakwater commented on issue #70281:
URL: https://github.com/apache/doris/issues/70281#issuecomment-6654243069

   **Assessment:** This warrants a data-correctness investigation for 
shared-nothing read-modify-write stream consumers. At 
`72e91896bbdd8760f8bd4e488e5672406b40ce52`, the reported timing window is 
supported by the code, and the existing offset-conflict check does not close 
it. Advancing an offset at COMMITTED is established transaction behavior; the 
additional concern here is consuming the next delta while reading stale target 
state. **I have not executed the reproduction, so the final lost-update outcome 
remains a code-supported hypothesis, not a reproduced result.** The issue is 
currently open with no labels.
   
   **Verified code path at the reported commit:**
   
   - 
[`InsertIntoTableCommand`](https://github.com/apache/doris/blob/72e91896bbdd8760f8bd4e488e5672406b40ce52/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/insert/InsertIntoTableCommand.java#L366-L389)
 attaches the stream update information to the consuming transaction. 
[`updateCatalogAfterCommitted()`](https://github.com/apache/doris/blob/72e91896bbdd8760f8bd4e488e5672406b40ce52/fe/fe-core/src/main/java/org/apache/doris/transaction/DatabaseTransactionMgr.java#L2433-L2446)
 applies it at commit. 
[`OlapTableStream.getStreamUpdate()`](https://github.com/apache/doris/blob/72e91896bbdd8760f8bd4e488e5672406b40ce52/fe/fe-core/src/main/java/org/apache/doris/catalog/stream/OlapTableStream.java#L292-L300)
 then exposes that offset as the next scan's starting point.
   - 
[`GlobalTransactionMgr.commitAndPublishTransaction()`](https://github.com/apache/doris/blob/72e91896bbdd8760f8bd4e488e5672406b40ce52/fe/fe-core/src/main/java/org/apache/doris/transaction/GlobalTransactionMgr.java#L310-L336)
 releases the target/stream write locks before waiting for visibility. Those 
locks therefore do not serialize the next consumer behind publish. 
[`OlapTableStreamUpdate.checkPartitionOffset()`](https://github.com/apache/doris/blob/72e91896bbdd8760f8bd4e488e5672406b40ce52/fe/fe-core/src/main/java/org/apache/doris/catalog/stream/OlapTableStreamUpdate.java#L62-L91)
 checks expected offsets, not the previous consumer transaction's visibility. A 
statement planned after N's offset advance can pass this check.
   - The ordinary target scan takes 
[`partition.getVisibleVersion()`](https://github.com/apache/doris/blob/72e91896bbdd8760f8bd4e488e5672406b40ce52/fe/fe-core/src/main/java/org/apache/doris/planner/OlapScanNode.java#L514-L525).
 Thus it can read the pre-N state even though its stream scan starts after N's 
consumed range. MoW's [duplicate-key 
handling](https://github.com/apache/doris/blob/72e91896bbdd8760f8bd4e488e5672406b40ce52/be/src/storage/tablet/base_tablet.cpp#L950-L1014)
 operates on materialized input rows; it does not rerun `s.amount + d.delta` 
against subsequently published state.
   - 
[`IvmDeltaRewriter`](https://github.com/apache/doris/blob/72e91896bbdd8760f8bd4e488e5672406b40ce52/fe/fe-core/src/main/java/org/apache/doris/mtmv/ivm/IvmDeltaRewriter.java#L76-L90)
 explicitly guards aggregate refreshes against this stale-state dependency. 
That guard is specific to IVM; it does not protect the ordinary INSERT shown 
here.
   
   **Reproduction refinement:** The debug point requires the target table name 
as its value. Follow the [existing regression 
test](https://github.com/apache/doris/blob/72e91896bbdd8760f8bd4e488e5672406b40ce52/regression-test/suites/mtmv_p0/ivm/test_ivm_agg_previous_commit_not_visible.groovy#L29-L46):
 enable `DatabaseTransactionMgr.finishTransaction.block_visible` on all FEs 
with `[value: "state"]`. Enabling it without a nonempty value does not block 
this path. Targeting `state` allows both detail-table batches to become visible 
normally.
   
   Use one key, a fully visible baseline of 100, and successive detail deltas 
of +10 and +20. Use a plain MoW target without a sequence-column override or 
another writer. Set a short `insert_visible_timeout_ms` and explicitly select 
`insert_visible_timeout_return_mode=committed`. After N returns, verify its 
transaction is COMMITTED, its stream offset advanced, and `state` still reads 
100. Make +20 visible in `detail`, then run N+1 and capture its 
transaction/result before disabling the point. After cleanup, wait until 
**both** consumer transactions are VISIBLE before comparing: the hypothesized 
result is 120 versus the correct 130. An immediate comparison while either 
transaction remains COMMITTED would demonstrate only delayed visibility. Always 
disable the point in `finally`.
   
   **Evidence still needed:** Complete table/stream DDL and inserts; relevant 
session settings; both INSERT responses including labels/transaction IDs; 
transaction status and target committed/visible versions at each boundary; 
stream consumption offsets before/after each round; and the final ordered 
target/source aggregate comparison after both transactions become VISIBLE. FE 
transaction/publish log excerpts correlated by transaction ID would establish 
the ordering. The exact revision is already supplied; no query profile is 
needed for this initial correctness check.
   
   **Next steps:** Add a focused shared-nothing regression with the above 
assertions. If it confirms the mismatch, preserve the invariant that a 
dependent consumer cannot start its target read from an advanced stream 
position until the preceding consumption's writes are visible. A wait/rejection 
needs to happen before fixing the read snapshot. Simply moving the existing 
offset update to VISIBLE also requires retaining commit-time offset 
reservation/conflict protection, otherwise two consumers could commit the same 
range; persistence/replay and concurrent-consumer tests are needed for that 
design.
   
   Until semantics are clarified, document this constraint and have 
orchestration wait for the preceding transaction to reach VISIBLE before 
starting the next dependent statement. Waiting only for statement completion, 
or increasing the publish timeout, does not establish that guarantee. 
`insert_visible_timeout_return_mode=error` changes the response but does not 
roll back the committed transaction. The reported FE COMMITTED-to-VISIBLE 
window is specific to the shared-nothing path; this investigation does not 
establish a blanket guarantee for every shared-storage failure mode. The 
hypothesized loss affects accumulated target state; retained detail rows can 
still support a controlled rebuild.
   
   Breakwater-GitHub-Analysis-Slot: slot_b79f743f618d
   


-- 
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]

Reply via email to