RoanHeNaN opened a new issue, #70206: URL: https://github.com/apache/doris/issues/70206
## Search before asking - [X] I had searched in the [issues](https://github.com/apache/doris/issues?q=is%3Aissue) and found no similar issues. ## Description > **Note on confidence:** This report and the proposed fix are **derived from reading the BE source code**, not yet confirmed by a running regression test, and the proposed patch has not been compiled. It is filed as a **proposal / needs-verification** so maintainers can validate the root cause and discuss the fix direction. Code references are against `master` (around commit `173a0e2`); line numbers are approximate. ### Summary For a **Merge-On-Write (MOW) Unique Key table with a sequence column**, a deleted key can be **resurrected** after base/full compaction physically prunes its delete-sign tombstone, if a later **out-of-order write with a lower sequence value** then arrives. This is a correctness bug on a single copy (including cloud/compute-storage-separation mode), and additionally causes **permanent divergence between replicas** in the local (compute-storage-coupled) mode because replicas compact independently. ### Background In a MOW table with a sequence column, rows with the same key are resolved by **sequence value: the larger sequence wins**, independent of version/arrival order. A delete (delete sign) with sequence `S` is semantically expected to **permanently suppress** any later write of the same key whose sequence `< S`. A delete-sign row (tombstone) is a real stored row. Base compaction physically prunes these tombstones to reclaim space (`enable_prune_delete_sign_when_base_compaction`, default `true`). ### Reproduction scenario (reasoned from code) MOW table with a sequence column: 1. Write a delete for key `K1` with `sequence = 1000`. 2. Base compaction runs and **physically prunes** the tombstone. 3. A late write for `K1` arrives with `sequence = 900` (lower than the deleted `1000`). - **Expected:** `K1` stays deleted (because `900 < 1000`, the delete should suppress it). - **Actual:** `K1` is **resurrected** and becomes visible. ### Impact - **Correctness:** Even a single copy (including cloud mode with no replicas) returns deleted data again. - **Replica consistency (local mode):** Replicas compact independently. A replica that has pruned the tombstone resurrects `K1`; a replica that has not still suppresses it. The two replicas return **different results permanently** and do not self-heal, because the delete bitmap is computed against physical data at write time. ### Root cause The MOW delete bitmap is computed **at write time** against the physical data present at that moment. The resurrection chain: | Step | Code location | Behavior | |---|---|---| | ① Base compaction prunes the tombstone unconditionally | `be/src/storage/row_ttl.cpp` `build_row_visibility_filter` (filters every `delete_sign != 0` row); gated by `_delete_sign_available` in `be/src/storage/tablet/tablet_reader.cpp` (`_init_delete_condition`) | The tombstone (carrying `seq=1000`) is physically removed. **It does not check whether the table has a sequence column.** | | ② Late write looks up the historical key | `be/src/storage/tablet/base_tablet.cpp` `lookup_row_key` | `K1` not found → `KEY_NOT_FOUND`. | | ③ No sequence to compare against | delete bitmap calculation | Since the key is not found, the new `seq=900` row is not marked in the delete bitmap → visible. Resurrection complete. | **Contrast:** if the tombstone were still alive (not yet compacted), `lookup_row_key` would hit it, return `seq=1000`, compare `900 < 1000`, and mark the new row as deleted. **So the sole root cause is that the tombstone — and with it the `seq=1000` suppression watermark — is pruned too early.** **Why only sequence-column tables are affected:** without a sequence column, conflicts are resolved by version order; a later write always has a higher version and should win, so pruning the tombstone and making the new write visible is *correct*. No resurrection is possible. ### Proposed fix: grace watermark Do not change any delete/query/compaction resolution logic — only **defer the physical reclamation** of the tombstone until we can be confident no earlier (lower-sequence) write can still arrive. This mirrors Cassandra's `gc_grace_seconds` and Elasticsearch's `index.gc_deletes`. **Decision logic**, added in `_init_delete_condition` for tables **with a sequence column** only: ``` grace = config.mow_sequence_delete_sign_grace_seconds if grace < 0: keep tombstones (never prune) for sequence-column tables if grace == 0: legacy behavior, prune immediately (default, no behavior change) if grace > 0: newest = max(creation_time of input rowsets of this compaction) if now - newest <= grace: do not prune this round (data still too new) else: allow pruning ``` **Intuition:** if even the newest input rowset is already older than `grace`, then any late write whose lateness is bounded by `grace` has already arrived and been applied, so pruning the tombstone is safe. **Why local wall-clock + `creation_time` is tolerant of replica clock skew:** as long as `grace >> (max out-of-order lateness + clock skew)`, by the time *any* replica prunes, the grace window has passed and no earlier write can arrive to cause resurrection or divergence. The grace window absorbs the per-replica timing difference. **Config** | Config | Default | Meaning | |---|---|---| | `mow_sequence_delete_sign_grace_seconds` (BE, mutable, int64) | `0` | `>0` keep tombstones at least this long (recommend ≥ max expected out-of-order lateness of the sequence column); `0` prune immediately (current behavior); `<0` never prune | Default `0` means **no behavior change** for existing deployments; opt-in only. **Scope** - **Local and cloud covered by one change:** both paths funnel through `Merger::vmerge_rowsets` (`be/src/storage/merger.cpp`) → `TabletReader::_init_delete_condition`, where `tablet_schema` and `rs_splits` are always populated. - **Sequence-column tables only:** non-sequence MOW tables are unaffected (zero behavior change, zero extra space). ### Why the change is small Key insight: **as long as the tombstone is physically alive, the existing `lookup_row_key` + sequence comparison already produce the correct result.** So the fix only needs to "keep the tombstone a little longer" — the read path, delete-bitmap calculation, and queries are untouched. The gate point also has direct access to the input rowsets and schema, so no plumbing through the compaction class is needed. Change surface (proposed, not yet compiled/tested): - `be/src/common/config.h` / `config.cpp`: new config `mow_sequence_delete_sign_grace_seconds` + docs. - `be/src/storage/tablet/tablet_reader.cpp`: grace gate inside `_init_delete_condition` (~40 lines) + 2 includes. Total ~68 lines, additive only; no existing logic removed. It is **fail-safe**: even an aggressive config only keeps tombstones longer (space cost), it never changes a correctness decision. ### Assumptions / limitations 1. The sequence column must be roughly correlated with wall-clock time (e.g. a CDC event time). The grace window is a time window; it is only meaningful when the out-of-order lateness has a time bound. 2. If the sequence is unrelated to time (e.g. an arbitrary version number), the time window is meaningless; use `grace < 0` (never prune) as a fallback. 3. Cumulative compaction does not apply delete sign by default and is unaffected; if `enable_delete_when_cumu_compaction` is enabled, the same grace logic should be applied there too (follow-up). ### Alternatives considered - **Persist a per-key max-deleted-sequence watermark** and consult it on `KEY_NOT_FOUND`: unbounded per-key state, requires read-path changes, and the side structure needs its own reclamation policy (grace again). Higher complexity, only saves space. - **Row-level grace** (decide per winning row by its source rowset's `creation_time`): needs to propagate source time to the filter; the coarse rowset-level approach is already correct and only costs some extra space. - **Never prune** (`enable_prune_delete_sign_when_base_compaction=false` / `grace<0`): correct but unbounded space growth; kept as the fallback for arbitrary sequences, not as default. ### Test plan 1. Single-copy repro: write `K1` delete `seq=1000` → trigger base compaction (prune tombstone) → write `K1 seq=900` → assert `K1` resurrects with `grace=0` (exposes the bug) and stays deleted with `grace>0`. 2. Replica-consistency case (local, 2 replicas) with different compaction timing → assert consistent query results after enabling grace. 3. Boundary cases: `creation_time` right at the grace edge; sequence out-of-order crossing the edge. 4. Regression protection: reuse `enable_mow_compaction_correctness_check`; assert bit-for-bit identical behavior when `grace=0`. 5. Non-sequence tables: assert behavior identical to before. ### Open questions for maintainers - Is the root-cause analysis correct, in particular the `lookup_row_key` → `KEY_NOT_FOUND` → resurrection path? - Preferred fix location/direction: grace watermark (this proposal) vs. persisting a sequence watermark? - Should the default eventually be a conservative positive value ("fix out of the box") rather than `0`? - Should this be a table property rather than a BE config? I'm happy to provide the draft patch and regression cases if the direction is agreed. ## Are you willing to submit PR? - [X] Yes I am willing to submit a PR! ## Code of Conduct - [X] I agree to follow this project's [Code of Conduct](https://www.apache.org/foundation/policies/conduct) -- 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]
