morningman opened a new pull request, #66432:
URL: https://github.com/apache/doris/pull/66432
### What problem does this PR solve?
Issue Number: close #66390
Related PR: #63850
Problem Summary:
A BE received SIGSEGV in `VMergeIteratorContext::compare()` while
`VMergeIterator::init()` was building its merge heap during a ROW-binlog
regression workload.
Root cause chain:
1. Any row-binlog scan (`binlog_scan_type != NONE`, including `APPEND_ONLY`)
forced a key-ordered merged read with `read_orderby_key_num_prefix_columns ==
0`.
2. With a zero prefix, `read_orderby_key_columns` stays null, so both merge
comparators (`VMergeIteratorContext::compare()` at the rowset level and
`VCollectIterator::LevelIteratorComparator` at the rowset-tree level) fall back
to comparing the first `num_key_columns` block positions.
`Schema::num_key_columns()` counts the key columns of the **whole tablet
schema**, independent of the projection, so the fallback silently assumes the
projection starts with the full ordered key prefix.
3. `MIN_DELTA` / `DETAIL` satisfy that contract because they widen the
storage projection with every key column. `APPEND_ONLY` takes the `direct_mode`
branch (`return_columns = SQL projection`), which may omit some or all key
columns (e.g. `SELECT v1 FROM t@incr(... "incrementType" = "APPEND_ONLY")` on a
table with two key columns).
4. `Block::compare_at()` has only DCHECK bounds checks, so in a release
build the first heap comparison reads past the block's column array and kills
the BE. This is reachable whenever a rowset has more than one segment
(`force_key_ordered_read` makes `is_merge_iterator()` true even for
non-overlapping segments) or rowsets overlap. A projection that has enough
columns but not the leading keys would instead be compared on the wrong columns
— a silent misordering.
Fix (three layers):
1. **Root cause (`olap_scanner.cpp`)**: only `MIN_DELTA` / `DETAIL` — the
modes that actually group by key and reconstruct BEFORE/AFTER rows, and whose
projection is widened with the full key prefix — force the key-ordered merged
read. `APPEND_ONLY` does no key grouping (it is a plain op-filter + TSO-range
stream), so it now reads unordered like a plain scan, which also removes an
unnecessary merge-heap cost from the highest-throughput mode. ORDER BY / TopN
pushdown params stay disabled for all binlog scan types, as before.
2. **Defense in `VMergeIteratorContext::init()`**: validate the compare
contract once the first block is loaded — explicit compare columns must point
inside the block; the default key-prefix comparison requires the projection to
start with exactly the schema's key column ids in order; the sequence tie-break
position must be in range. Violations return an `InternalError` carrying
tablet/rowset/version/projection details instead of an out-of-bounds read.
3. **Same defense in `VCollectIterator::Level1Iterator::init()`**: validate
every child's first block against the same contract before anything is pushed
into the merge heap.
### Release note
Fixed a BE crash (SIGSEGV in the storage merge comparator) when an
APPEND_ONLY row-binlog incremental scan projected fewer columns than the
table's key columns. APPEND_ONLY binlog scans no longer force a key-ordered
storage read; their row order (without ORDER BY) may differ from before.
### Check List (For Author)
- Test <!-- At least one of them must be included. -->
- [x] Regression test
- [x] Unit Test
- [ ] Manual test (add detailed scripts or steps below)
- [ ] No need to test or manual test. Explain why:
- [ ] This is a refactor/code format and no logic has been changed.
- [ ] Previous test can cover this change.
- [ ] No code files have been changed.
- [ ] Other reason <!-- Add your reason? -->
- Behavior changed:
- [ ] No.
- [x] Yes. APPEND_ONLY row-binlog scans no longer force a per-tablet
key-ordered read, so their output order without ORDER BY may change (SQL never
guaranteed it; existing binlog/table-stream regression suites all use
order-insensitive assertions or explicit ORDER BY). Queries that used to crash
or silently mis-merge now either work (APPEND_ONLY) or fail with a diagnosable
`InternalError` (contract violations).
- Does this need documentation?
- [x] No.
- [ ] Yes. <!-- Add document PR link here. eg:
https://github.com/apache/doris-website/pull/1214 -->
### Check List (For Reviewer who merge this PR)
- [ ] Confirm the release note
- [ ] Confirm test cases
- [ ] Confirm document
- [ ] Add branch pick label <!-- Add branch pick label that this PR should
merge into -->
🤖 Generated with [Claude Code](https://claude.com/claude-code)
--
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]
