Doris-Breakwater commented on issue #70206: URL: https://github.com/apache/doris/issues/70206#issuecomment-6628168378
Breakwater-GitHub-Analysis-Slot: slot_cb8c6ccd6051 **Initial assessment:** This is a credible MOW sequence/tombstone correctness report that warrants a deterministic regression test. The described lookup/bitmap mechanism is supported by source inspection, but the issue supplies no executed reproduction, so resurrection and replica divergence should remain **needs verification**, rather than being described as an already reproduced regression. The issue is currently open with no labels. Suggested triage area: BE storage / MOW / compaction / correctness. I inspected the configured local Doris repository at the exact cited commit, `173a0e2064e529bfa6f017608c40ff003c2f1066`, without changing its branch or source. No build, running-cluster reproduction, or proposed-patch validation was performed. **What the source establishes** 1. Base/full compaction enables delete-sign filtering when `enable_prune_delete_sign_when_base_compaction=true`; this decision has no sequence-column guard. The row filter then removes rows whose delete sign is nonzero. This supports loss of the tombstone's sequence information from the compacted output for the ordinary unique-key merge path. See [the reader gate](https://github.com/apache/doris/blob/173a0e2064e529bfa6f017608c40ff003c2f1066/be/src/storage/tablet/tablet_reader.cpp#L454-L480) and [the filter](https://github.com/apache/doris/blob/173a0e2064e529bfa6f017608c40ff003c2f1066/be/src/storage/row_ttl.cpp#L116-L137). 2. When a matching stored key exists, the segment lookup compares sequence values and returns `KEY_ALREADY_EXISTS` for a lower incoming sequence. For a full-row upsert, bitmap calculation marks that incoming row deleted. When lookup instead returns `KEY_NOT_FOUND`, bitmap calculation continues without marking that row as a conflict. Thus, **if compaction has removed the last relevant stored key/sequence and no other conflict exists**, the proposed `1000` delete → compaction → `900` upsert chain follows from the code. See [sequence comparison](https://github.com/apache/doris/blob/173a0e2064e529bfa6f017608c40ff003c2f1066/be/src/storage/segment/segment.cpp#L1325-L1341), [rowset lookup](https://github.com/apache/doris/blob/173a0e2064e529bfa6f017608c40ff003c2f1066/be/src/storage/tablet/base_tablet.cpp#L627-L729), and [bitmap handling](https://github.com/apache/doris/blob/173a0e2064e529bfa6f017608c40ff003c2f1066/be/src/storage/tablet/base_tablet.cpp#L907-L950). 3. The shared-reader fix location is plausible for local and cloud execution, but the claim that everything funnels through `Merger::vmerge_rowsets` needs correction: compaction selects either horizontal or vertical merging. Both use `TabletReader::init`, and both block readers apply the visibility filter. Cloud compaction calls the shared `merge_input_rowsets`. See [merge selection](https://github.com/apache/doris/blob/173a0e2064e529bfa6f017608c40ff003c2f1066/be/src/storage/compaction/compaction.cpp#L530-L560), [cloud execution](https://github.com/apache/doris/blob/173a0e2064e529bfa6f017608c40ff003c2f1066/be/src/storage/compaction/compaction.cpp#L2403-L2408), and [vertical filtering](https://github.com/apache/doris/blob/173a0e2064e529bfa6f017608c40ff003c2f1066/be/src/storage/iterator/vertical_block_reader.cpp#L94-L112). This does not establish identical behavior for every schema variant, such as cluster keys or sequence mapping. **Corrections and limits of the proposed fix** - At this revision, `enable_delete_when_cumu_compaction` enables **delete predicates**, not delete-sign pruning. `_delete_sign_available` remains false for cumulative compaction even when that option is true; the source explicitly documents this distinction. The proposed cumulative-compaction follow-up is therefore not required on that premise. See [the same reader gate](https://github.com/apache/doris/blob/173a0e2064e529bfa6f017608c40ff003c2f1066/be/src/storage/tablet/tablet_reader.cpp#L454-L476). - A positive grace period is a **conditional retention policy**, not an unconditional sequence watermark. Once it expires, a sufficiently late lower-sequence replay can still resurrect the key. Safety requires an enforceable bound covering ingestion delays, retries, replay/backfill, and clock error. Sequence/time correlation alone does not prove that all earlier writes have been applied. Default `0` preserves the reported exposure. - Rowset `creation_time` is storage lifecycle metadata: local publication resets it, and rowset building also sets it to the current time. It is not the original event timestamp or a persisted per-key deletion watermark. Repeated compaction can refresh the age of retained tombstones, so the coarse policy can retain them much longer than the configured interval. Validate its local/cloud lifecycle semantics explicitly. See [publication](https://github.com/apache/doris/blob/173a0e2064e529bfa6f017608c40ff003c2f1066/be/src/storage/rowset/rowset.cpp#L87-L94) and [rowset building](https://github.com/apache/doris/blob/173a0e2064e529bfa6f017608c40ff003c2f1066/be/src/storage/rowset/beta_rowset_writer.cpp#L1173-L1176). - If the intended scope is MOW+sequence only, the guard must check both conditions. Preserve checksum/query filtering semantics, and review other physical reclamation modes separately. A mutable BE-wide setting also needs consistent deployment across replicas; changing it cannot recover tombstones already discarded or repair existing divergence. - The existing pruning switch is explicitly annotated **“only for test”** in [config.cpp](https://github.com/apache/doris/blob/173a0e2064e529bfa6f017608c40ff003c2f1066/be/src/common/config.cpp#L2068-L2070). It is useful as a controlled diagnostic comparison, but should not be presented as a supported production workaround without maintainer confirmation. **Missing evidence and recommended next step** Please provide a runnable SQL/load regression with exact FE/BE build hashes, `SHOW CREATE TABLE`, sequence mapping/type, the delete ingestion method and payload, load labels/transaction IDs, and relevant compaction settings. An explicit delete-sign load and a predicate-based SQL DELETE should not be assumed equivalent without checking the actual generated sequence. Start with one replica and one key: optionally insert an older live row, load the `seq=1000` tombstone, verify the key is logically absent, run base/full compaction **to successful completion**, verify that its selected inputs included the tombstone and that the output no longer contains it, then load the full-row `seq=900` upsert. Supply normal query results and diagnostic hidden-column/rowset/bitmap evidence before and after each stage, plus the corresponding compaction log. Add unrelated rowsets if necessary to satisfy compaction selection; merely submitting a compaction request does not prove pruning occurred. Run an identical control with tombstone pruning disabled. Also compare a lower-sequence write arriving **before** compaction with one arriving **after** it. For the replica claim, hold all replicas at the same published version, deliberately vary pruning timing, and capture each replica's result and bitmap state before and after subsequent compaction. The source supports a risk of persistent divergence; it does not by itself prove that divergence is permanent under all repair or later-write paths. Before accepting a grace implementation, cover inside/outside-window arrivals, the exact boundary, negative/zero settings, higher/equal sequence values, repeated compaction, horizontal/vertical paths, local/cloud modes, and non-sequence/MOR controls. Use query-result assertions: the existing `enable_mow_compaction_correctness_check_core` / `_fail` checks concern compaction bitmap/row accounting and do not prove future late-write semantics. Maintainers should first decide the intended suppression lifetime; a bounded grace policy cannot satisfy the report's stated **permanent** suppression contract for arbitrary future replays. -- 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]
