[fix](binlog) Fix missing DELETE events in row binlog - #68034
Merged
luwei16 merged 1 commit intoSep 17, 2026
Merged
Conversation
### What problem does this PR solve?
Issue Number: None
Related PR: None
Problem Summary: Predicate deletes only persist delete predicates and therefore cannot produce row-level DELETE events. A merge-on-write unique table with light delete enabled incorrectly used that path even when ROW binlog was enabled, leaving downstream consumers with stale rows. Route these deletes through row-bearing DELETE FROM USING execution, and reject any ROW-binlog predicate delete that still reaches the legacy handler.
### Release note
Ensure DELETE on merge-on-write unique tables with ROW binlog emits row-level DELETE events even when light delete is enabled. Predicate DELETE on ROW-binlog tables now reports an explicit unsupported error.
### Check List (For Author)
- Test: Regression test, FE unit test, and manual test
- Regression test: row_binlog_p0/test_row_binlog_mow_light_delete
- Unit Test: org.apache.doris.nereids.trees.plans.commands.DeleteFromCommandTest
- Manual test: verified ROW binlog DELETE events with historical values disabled and enabled
- Behavior changed: Yes; MOW ROW-binlog deletes use row-bearing execution, while unsupported predicate deletes fail explicitly
- Does this need documentation: No
luwei16
requested review from
924060929,
englefly,
gavinchou,
liaoxin01,
morrySnow and
starocean999
as code owners
September 15, 2026 17:05
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Contributor
Author
|
run buildall |
luwei16
changed the title
gavinchou
approved these changes
Sep 15, 2026
Contributor
FE Regression Coverage ReportIncrement line coverage |
luwei16
changed the title
Contributor
Author
|
/review |
Contributor
|
Codex automated review failed and did not complete. Error: You've hit your usage limit. Visit https://chatgpt.com/codex/settings/usage to purchase more credits or try again at Sep 19th, 2026 8:24 AM. The selected account is excluded until 2026-09-19T08:24:00Z. Please trigger /review again; another configured account may be available. |
morrySnow
approved these changes
Sep 16, 2026
Contributor
Author
|
run performance |
Contributor
TPC-H: Total hot run time: 17095 ms |
Contributor
TPC-DS: Total hot run time: 83250 ms |
Contributor
ClickBench: Total hot run time: 14.95 s |
Contributor
Author
|
Local review conclusion: PASS. Reviewed the exact range schema: doris-repo-review/v1
status: PASS
pr: apache/doris#68034
commit: 60761180f3c84e50778759c866401fd293e3ac46
base: 0aea7140de64a7162d0a0d764e3dc1db7f40dbae
reviewed_at: 2026-09-17T16:22:26+08:00
reviewer: luwei16
model: gpt-5.6-sol
effort: xhigh
findings: {blocker: 0, major: 0, minor: 0, nit: 0}
rounds: 1
converged: true |
morningman
added a commit
that referenced
this pull request
Sep 18, 2026
…on PRs from master in merge order (#67783 #68034 #68033 #68057 #68094 #68087) (#68151) Cherry-picked from #67783, #68034, #68033, #68057, #68094, #68087 Batch pick of every merged PR carrying the `incremental-computation` label that `branch-incremental-computation` does not have yet (no `incremental-computation-picked` label), in the order they landed on master (`git log --first-parent`). One commit per PR, each created with `git cherry-pick -x` so the message ends with `(cherry picked from commit <master sha>)`. Follows the same convention as #67830, #68017 and #68073. | # | Master commit | PR | Title | |---|---|---|---| | 1 | e545b13 | #67783 | [fix](policy) Enforce row policies on MOW time travel | | 2 | f7a0842 | #68034 | [fix](binlog) Fix missing DELETE events in row binlog | | 3 | e85575e | #68033 | [fix](binlog) Preserve row binlog compaction policy | | 4 | 21160d7 | #68057 | [fix](binlog) Decouple row binlog compaction from CCR binlog config | | 5 | 04aa5a1 | #68094 | [fix](binlog) Persist row binlog config updates in cloud mode | | 6 | da8feed | #68087 | [fix](binlog) Report streams with missing base tables as stale | Not included on purpose: - The 20 labelled PRs that already carry `incremental-computation-picked` (#62606 in the fork point, #67508 via #67712, the nine of #67830, the six of #68017, the two of #68073, and #68050 whose content this branch got directly through #68012). - #68012 carries the label but is a PR against this branch itself (merged as `6f7c87fa892`); nothing to pick. - #67820 is still open on master; this branch already carries its content via #67861. ### Prerequisite check For every pick I listed the master commits between the fork point (`efedf10c7e3`) and the pick that touch the same files and are not on this branch, and checked whether the pick's behavior depends on them. - **#67783** declares no related PR. It builds on the MOW time-travel rewrite (`BindRelation.buildMowTimeTravelUnion`, #67480) which is already here. Two unlabelled master commits overlap: #66770 (authorization plugin SPI: reworks the data-mask / row-filter API of `LogicalCheckPolicy` and its test) and #67811 (removes the `isPlayNereidsDump()` check in `CheckPolicy`). Neither is a functional prerequisite: the fix consists of `getPolicyTable()` unwrapping `OlapTableWrapper`, `CheckPolicy` collecting the whole filter chain below the policy mask, and `BindRelation` putting a `LogicalCheckPolicy` above each union branch — none of that uses the #66770 API. Not picked; adapted instead (see below). - **#68034**, **#68033**, **#68087** (#68087 relates to #67173, which is before the fork point): no unpicked master commit touches any of their files; the picks applied cleanly and are byte-identical to master. - **#68057**: only `be/test/cloud/cloud_compaction_test.cpp` overlaps with the unrelated #67972 (refresh tablet meta of continuously ingested tablets); auto-merged, the pick only adds new `TEST_F` blocks. Main-code hunks are byte-identical to master. - **#68094**: overlaps with #68090 / #67972 / #66598 (`cloud_tablet.cpp`), #67295 / #67618 / #68090 (`base_tablet.{h,cpp}`), #66598 / #67637 (`meta_service.cpp`), #66598 (`cloud.proto`, `cloud_tablet_test.cpp`) and #67761 / #66598 / #67637 (`meta_service_test.cpp`). All auto-merged. The fix — `BaseTablet::binlog_config()` under the meta lock, `BinlogConfig::operator==`, `CloudTablet::sync_meta()` refreshing the binlog config, `update_tablet` in the meta service accepting `binlog_config`, and `CloudSchemaChangeHandler` routing ROW-binlog property updates through it — does not use anything those commits add. Its hunks are byte-identical to master except one trailing context line in `sync_meta()` (`last_sync_tablet_meta_time_s` comes from #67972). Only #67783 needed adaptation, recorded in its commit message: - `LogicalCheckPolicy.java`: the conflicting context was master's `parsePolicyExpression()` helper (#66770), which does not exist here; the new `getPolicyTable()` is inserted in the same place without it. - `CheckRowPolicyTest.java`: the data-mask mock is written against this branch's per-column `AccessControllerManager.evalDataMaskPolicy(..., column) -> Optional<DataMaskPolicy>` instead of master's `evalDataMaskPolicies(..., Set<String>) -> Map<String, DataMaskSpec>`. The masks are the same (concat for the random-distribution table; the non-movable `k2` mask plus identity masks for the hidden reconstruction columns of the MOW table). The master-only `Or` import (#66770) is not carried; `Collections` / `Locale` imports were added because they arrive with #66770 on master. ### Drift check against master Each pick's `+`/`-` lines are identical to the master commit's, except for the #67783 adaptation above. After the six picks, the touched files still differ from master at `da8feed859d` in: `cloud_tablet.cpp`, `base_tablet.{h,cpp}`, `cloud_compaction_test.cpp`, `cloud_tablet_test.cpp`, `meta_service.cpp`, `meta_service_test.cpp`, `cloud.proto`, `CheckPolicy.java`, `LogicalCheckPolicy.java`, `CheckRowPolicyTest.java`. Replaying the nine unpicked master commits listed above (#67637, #67618, #66770, #66598, #67972, #67811, #67761, #67295, #68090) in a temporary index and removing this branch's own #67861 (`GetTsoRecoveryTransactions` in `cloud.proto` / `meta_service_test.cpp`) brings every file to zero diff against master, except the two `LogicalCheckPolicy.java` / `CheckRowPolicyTest.java` hunks of #66770 that overlap the adaptation. Nothing else is left over. ### Verification - FE: `run-fe-ut.sh --run` on this branch (regenerates thrift/protobuf, compiles fe-core main + test) with the test classes touched by the picks plus `DeleteFromCommandTest` from #68034's checklist: 5 classes, 58 tests, 0 failures, 0 errors, BUILD SUCCESS — `CheckRowPolicyTest` 9 (the three tests #67783 adds included), `ExplainTableStreamPlanTest` 24, `CloudSchemaChangeHandlerTest` 18 (the tests of #68033 and #68094 included), `DropTableStreamTest` 5, `DeleteFromCommandTest` 2. - FE checkstyle on fe-core: 0 violations. - BE: `-fsyntax-only` with the flags of the Release build (`compile_commands.json`, regenerated `gen_cpp` headers incl. the new `TabletMetaInfoPB.binlog_config`) passes for `cloud/cloud_tablet.cpp`, `cloud/cloud_storage_engine.cpp`, `storage/olap_server.cpp`, `storage/tablet/tablet.cpp`, `storage/tablet/base_tablet.cpp`, and with `-DBE_TEST -fno-access-control` for `test/cloud/cloud_compaction_test.cpp`, `test/cloud/cloud_tablet_test.cpp`, `test/storage/compaction/compaction_task_test.cpp`, `test/storage/tablet/tablet_test.cpp`. - Meta service: the two-line `update_tablet` hunk of #68094 only uses the generated `TabletMetaInfoPB::has_binlog_config()/binlog_config()` and `TabletMetaCloudPB::mutable_binlog_config()` accessors, all present in the regenerated headers. - The two new groovy suites (`row_binlog_p0/test_row_binlog_mow_light_delete`, `time_travel_p0/test_mow_time_travel_row_policy`) parse cleanly (groovy parser check). --------- Co-authored-by: morrySnow <zhangwenxin@selectdb.com> Co-authored-by: Luwei <814383175@qq.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What problem does this PR solve?
Issue Number: None
Related PR: None
Problem Summary: Predicate deletes only persist delete predicates and therefore cannot produce row-level DELETE events. A merge-on-write unique table with light delete enabled incorrectly used that path even when ROW binlog was enabled, leaving downstream consumers with stale rows. Route these deletes through row-bearing DELETE FROM USING execution, and reject any ROW-binlog predicate delete that still reaches the legacy handler.
Release note
Ensure DELETE on merge-on-write unique tables with ROW binlog emits row-level DELETE events even when light delete is enabled. Predicate DELETE on ROW-binlog tables now reports an explicit unsupported error.
Check List (For Author)