asdf2014 opened a new issue, #66192: URL: https://github.com/apache/doris/issues/66192
### Search before asking - [x] I had searched in the [issues](https://github.com/apache/doris/issues?q=is%3Aissue) and found no similar issues. ### Version master (line numbers below are as of e7b7f1d1359), cloud mode only, merge-on-write UNIQUE tables only. ### What's Wrong? On a cloud merge-on-write table, a plain `SELECT` can return rows that were already overwritten, so the same UNIQUE key appears more than once in the result. No query-side feature is involved; an ordinary scan is enough to hit it. The wrong result is also not a brief window. It does not end when the compaction that caused it commits: in the reproduction below the query kept returning the resurrected rows after the compaction had finished, and only became correct once the tablet left that BE's cache. The persisted data is not damaged, though. The meta service reports the correct row count throughout, and restarting the BE (which drops the tablet cache) makes queries correct again, so this is a read-side defect confined to one BE's tablet cache. A full-compaction submission loads a MOW tablet into the shared, query-visible tablet cache with `sync_delete_bitmap=false`: - `be/src/agent/task_worker_pool.cpp:1684`: `bool sync_delete_bitmap = compaction_type != CompactionType::FULL_COMPACTION;` then `engine.tablet_mgr().get_tablet(tablet_id, false, sync_delete_bitmap)`. Routine cumu/base pass `true`, only FULL passes `false`. - `be/src/cloud/cloud_compaction_action.cpp:153`: same pattern for `PARAM_COMPACTION_FULL` on the HTTP action. When that `get_tablet` is the cache-miss load that materializes the tablet, the cache now holds a CloudTablet whose rowsets are synced to some version `V_ms` while its delete bitmap is missing every marker up to `V_ms`. Two independent mechanisms make this state effectively permanent for readers: 1. The sync fast path ignores the flag. `CloudTablet::sync_rowsets` returns OK when `_max_version >= options.query_version` without consulting `sync_delete_bitmap` (`be/src/cloud/cloud_tablet.cpp:317` and `:338`), so any later sync with `query_version <= V_ms` is a no-op and the short bitmap is served as is. 2. Even a real sync (`query_version > V_ms`) does not repair it. The delete-bitmap fetch requests full history (`begin=0`) only for the NEW rowsets of that sync; for OLD rowsets it requests only stamps in `(old_max, new_max]`, where `old_max = req.start_version() - 1` is the rowset watermark (`be/src/cloud/cloud_meta_mgr.cpp:811`, `:1136-1141`, `:1144-1152`). Markers stamped at or below `V_ms` that target pre-existing rowsets are outside every later fetch range. Consequence: until the state is cleared, every MOW reader on that BE sees un-deduplicated data for keys whose delete markers fall in the missing range. A plain scan issues exactly this sync shape (`be/src/exec/operator/olap_scan_operator.cpp:881-884`, `sync_delete_bitmap` defaults to true in `be/src/cloud/cloud_tablet.h:65`) and then reads the tablet's bitmap as it stands, with no per-rowset re-fetch anywhere on the read path (`be/src/storage/rowset_version_mgr.cpp:150` takes `_tablet_meta->delete_bitmap_ptr()` into the read source, which reaches the reader context at `be/src/storage/tablet/tablet_reader.cpp:199`). So overwritten or deleted rows resurface in query results. Readers also have no way to defend against or even detect the state: the tablet records no watermark for how far its delete bitmap is complete, so nothing distinguishes a fully synced bitmap from this truncated one. Attribution: the bitmap-less load was introduced by #43337 (12164900352), which added the `sync_delete_bitmap` parameter to `CloudMetaMgr::sync_tablet_rowsets` and `CloudTabletMgr::get_tablet`, threaded it into the cache-miss `load_tablet` path, and passed `false` for FULL on the HTTP compaction action. Before that commit a RUNNING merge-on-write tablet always had its bitmap synced on load. #62596 later mirrored the same pattern for the FE-pushed `ADMIN COMPACT ... type='FULL'` task, adding the second entry point. The intent of #43337 is sound on its own terms: full compaction recomputes the bitmap it needs (`CloudFullCompaction::_cloud_full_compaction_update_delete_bitmap`), so it genuinely does not need the synced one. The defect is that the skipping happens on the shared, query-visible cache load, so readers inherit the gap. One scoping note: `enable_compaction_rw_separation` does not gate this. That flag assigns compaction work to the last-active load cluster (`be/src/cloud/config.h:191-193`) and is consulted only in background candidate selection (`be/src/cloud/cloud_tablet_mgr.cpp:454` via `CloudClusterInfo::should_skip_compaction`); the two submission paths above never check it. A default single-cluster deployment is reachable. ### What You Expected? A MOW tablet resident in the query-visible tablet cache should never expose rowsets at version V without its delete bitmap being complete up to V. Equivalently, `sync_rowsets` with `sync_delete_bitmap=true` returning OK should guarantee bitmap completeness up to the requested version. ### How to Reproduce? Reproduced on a local cloud cluster (FoundationDB + MinIO + meta-service + cloud FE/BE) with a single BE, so the query necessarily lands on the BE that performed the load. On a multi-BE cluster a query routed to another BE looks correct, which can mask the problem. The steps below use a debug point to make the window deterministic, so no race has to be won. The bug does not depend on it: the window is simply the span between the cache-miss load and the moment the tablet leaves the cache. The BE needs `enable_debug_points = true`. 1. Create a MOW table and give it rows to overwrite: ```sql CREATE TABLE repro.mow_bitmap (k INT, v VARCHAR(20)) UNIQUE KEY(k) DISTRIBUTED BY HASH(k) BUCKETS 1 PROPERTIES ('replication_num'='1','enable_unique_key_merge_on_write'='true','disable_auto_compaction'='true'); INSERT INTO repro.mow_bitmap VALUES (1,'a1'),(2,'a2'),(3,'a3'),(4,'a4'),(5,'a5'); INSERT INTO repro.mow_bitmap VALUES (1,'b1'),(2,'b2'),(3,'b3'); ``` `SELECT count(*)` returns 5, as expected. 2. Restart the BE so the tablet is no longer resident in its tablet cache. 3. Block the compaction before it commits, then trigger a full compaction so its submission performs the cache-miss load: ```bash curl -X POST "http://<be>:8040/api/debug_point/add/CloudFullCompaction::modify_rowsets.block" curl -X POST "http://<be>:8040/api/compaction/run?tablet_id=<tablet_id>&compact_type=full" ``` 4. Query the table. It returns **8** rows: `1|a1` and `1|b1` are both visible, likewise for k=2 and k=3. The overwritten rows have resurfaced, and the UNIQUE key is no longer unique in the result. 5. Remove the debug point and let the compaction commit. The BE logs `finish CloudFullCompaction ... input_rows=8 output_rows=5` and the meta service reports `num_rows: 5`, yet the query still returns 8. Restarting the BE returns it to 5. ### Anything Else? Constraints any fix has to respect, from reading the surrounding code: - **Refilling `DeleteBitmap` is not enough: readers go through an aggregate cache with its own version watermark.** A scan reads `delete_bitmap->get_agg({rowset_id, seg_id, version})` (`be/src/storage/rowset/beta_rowset_reader.cpp:184`). On an exact key hit that returns the cached aggregate and never looks at the map (`be/src/storage/tablet/tablet_meta.cpp:1876-1884`); on a miss it starts from `_get_rowset_cache_version(bmk)` and ORs only map entries **above** that watermark (`:1889-1891`, with `enable_mow_get_agg_by_cache` defaulting to true). Markers recovered by a repair are by definition at or below the watermark, so a repair that only merges into the map can be invisible to every reader. `DeleteBitmap::merge` performs no invalidation (`:1783-1797`); the only invalidator is `remove_rowset_cache_version` (`:1833`), and all of its callers are rowset-removal paths. Compaction makes this deterministic rather than incidental: `prefill_dbm_agg_cache_after_compaction` (`be/src/storag e/tablet/base_tablet.cpp:2323`, called from all three cloud compactions, `enable_prefill_all_dbm_agg_cache_after_compaction` defaults to true) populates the cache and the watermark from whatever bitmap the tablet holds at that moment. A fix has to invalidate the aggregate cache and the watermark for the rowsets it refills. - **A fix cannot live in `CloudTablet::sync_rowsets` alone.** Callers reaching `CloudMetaMgr::sync_tablet_rowsets` directly (`be/src/cloud/cloud_meta_mgr.cpp:583-587`) bypass anything implemented there. That includes `CloudTablet::calc_delete_bitmap_for_compaction` (`be/src/cloud/cloud_tablet.cpp:1349`, `:1396`), the consumer whose damage is permanent: it derives the output rowset's delete bitmap from `tablet_meta()->delete_bitmap()` and commits it. Base and cumulative compaction can also skip their `prepare_compact` sync entirely through the `need_sync_tablet` heuristic (`be/src/cloud/cloud_base_compaction.cpp:63-77`), and the publish-side delete-bitmap task can skip its sync through `should_sync_rowsets()` (`be/src/cloud/cloud_engine_calc_delete_bitmap_task.cpp:169-201`). `sync_tablet_rowsets_unlocked` is the common point, and even there those two skip-the-sync paths need separate handling. - **Refetching everything from version 0 is too expensive to be the answer, and can fail permanently.** A `full_sync` requests the complete bitmap history of every rowset (`be/src/cloud/cloud_meta_mgr.cpp:1136-1141`) and bypasses the local bitmap cache shortcut (`:1119`). On a large tablet that can exceed the meta service's `max_get_delete_bitmap_byte` (1 GB, `cloud/src/common/config.h:340`); `retry_rpc` does not retry that error class (`be/src/cloud/cloud_meta_mgr.cpp:525-527`) and `enable_batch_get_delete_bitmap` defaults to false, so the request is unchunked. If such a refetch is also what clears the "needs repair" state, a tablet that fails it stays marked and every later query repeats the same doomed fetch, becoming unqueryable until it is evicted or the BE restarts, which is worse than the wrong rows it was meant to fix. The cost has to be proportional to the actual gap, not to the tablet's whole history. - **The bitmap fetch has to be independent of whether new rowsets arrived.** `CloudMetaMgr::sync_tablet_delete_bitmap` returns early on `rs_metas.empty()` (`be/src/cloud/cloud_meta_mgr.cpp:1098`), so on a tablet whose rowsets are already current an ordinary sync fetches no bitmap at all. Fix directions: 1. **Track a per-tablet delete-bitmap watermark** (the version up to which the bitmap is known complete) as first-class tablet state, decided inside `sync_tablet_rowsets_unlocked` rather than in one caller. A consumer needing version V while the watermark sits at W fetches only `(W, V]`, so the cost is proportional to the real gap and the 1 GB cliff is not in play; ordinary incremental syncs advance the watermark rather than leaving a sticky flag; and the refill invalidates the aggregate cache and `_rowset_cache_version` for the rowsets it touches, per the first constraint above. This looks like the cleanest direction. 2. **Keep `sync_delete_bitmap=false` loads out of the query-visible cache**: load a private tablet object for the compaction submission, or force a bitmap sync before publishing the tablet into the shared cache. Simpler to reason about, at the cost of either an extra tablet object or the sync #43337 removed. Two directions that look natural but do not work: - *A boolean "bitmap may be incomplete" flag plus an on-demand `full_sync` repair, decided in `CloudTablet::sync_rowsets`.* It runs into all four constraints above: the aggregate cache can make the refill invisible, the direct `CloudMetaMgr::sync_tablet_rowsets` callers bypass it, and the from-version-0 refetch is both disproportionate and able to wedge a tablet permanently when it fails. Restricting it to a version-pinned reader (`query_version > 0`) to spare compaction the cost also excludes base and cumulative compaction, which read the bitmap and commit what they derive from it, leaving the consumer with the worst failure mode unprotected. - *Making `CloudFullCompaction::prepare_compact` skip the bitmap so the repair never lands on it.* This is wrong for merge-on-write tables with cluster keys: their compaction reads with `_direct_next_block` and performs no key aggregation (`be/src/storage/iterator/vertical_block_reader.cpp:247-253`), so `tablet_meta()->delete_bitmap()`, passed in at `be/src/storage/merger.cpp:285-287`, is the only thing removing dead rows. Skipping it persists them into the compacted output. Also worth stating plainly: adding a bitmap-coverage check to the sync fast path alone is not sufficient. It removes mechanism 1, but mechanism 2 (the old-rowset fetch keyed off the rowset watermark) still leaves the pre-watermark markers missing after a real sync. There is a related but distinct narrow race in the same area: a stale-rowset GC sweep re-stamping a marker between a reader capturing rowsets and scanning the bitmap, the same exposure any MOW snapshot read faces. It has a different mechanism and probably a different fix (atomic capture plus scan, or a GC guard), so I kept it out of this issue. ### Are you willing to submit PR? - [x] Yes I am willing to submit a PR! -- 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]
