eldenmoon opened a new pull request, #70424:
URL: https://github.com/apache/doris/pull/70424
### What problem does this PR solve?
A short-circuit point query (`enable_short_circuit_query=true` with the BE
row cache enabled, i.e.
`disable_storage_row_cache=false`) on a merge-on-write unique table with a
**partial** row store returns
stale rows, and can crash the BE.
`row_store_columns` (for example `"row_store_columns" = "id,v"`) makes the
FE take the short-circuit plan,
but the hidden `__DORIS_ROW_STORE_COL__` JSONB only holds the configured
columns
(`JsonbSerializeUtil::block_to_jsonb` filters on the row-store column ids).
The point-query executor
nevertheless filled and read `RowCache` exactly like for a full row store:
1. `MowKeyProbe::maybe_invalidate_row_cache` invalidates the cache only when
`TabletSchema::has_row_store_for_all_columns()`, so an `UPDATE`/`INSERT`
left the previous entry in place.
Short-circuit point queries kept returning the pre-update row until the
300 s
`point_query_row_cache_stale_sweep_time_sec` sweep.
2. A cache hit skips `lookup_row_key` and therefore has no row location, so
`_lookup_row_data` filled only the
row-store columns. A query that reads a column outside the row store, or
`__DORIS_DELETE_SIGN__` (which the
FE adds as a point-query predicate), then crashed the BE in
`VExpr::filter_column_with_selector` with
`Check failed: origin_column->size() == count (0 vs. 1)`.
Repro (reported on 4.1.3, also reproduces on master):
```sql
CREATE TABLE t (id BIGINT, v INT, c VARCHAR(64))
UNIQUE KEY(id) DISTRIBUTED BY HASH(id) BUCKETS 1
PROPERTIES ("enable_unique_key_merge_on_write"="true", "replication_num"="1",
"row_store_columns"="id,v");
INSERT INTO t VALUES (1, 100, 'old');
-- warm twice through the short-circuit plan -> 1, 100
UPDATE t SET v = 200, c = 'new' WHERE id = 1;
SELECT /*+ SET_VAR(enable_short_circuit_query=false) */ id, v, c FROM t
WHERE id = 1; -- 1, 200, new
SELECT /*+ SET_VAR(enable_short_circuit_query=true) */ id, v FROM t
WHERE id = 1; -- 1, 100 (stale)
SELECT /*+ SET_VAR(enable_short_circuit_query=true) */ id, v, c FROM t
WHERE id = 1; -- BE crash
```
With `disable_storage_row_cache=true` both short-circuit statements return
`1, 200, new`, which is the
workaround the reporter used.
### What exactly does this PR change?
- `PointQueryExecutor` now consults and fills the row cache only when the
tablet's row store holds every column
(`has_row_store_for_all_columns()`) — the same predicate the write path
already uses to invalidate it. Full
row store point queries are unchanged; partial row store queries read the
column store as before but no longer
use a cache entry that can neither be invalidated nor completed.
- A BE unit test drives the real executor over a partial-row-store tablet
and asserts the correct rows and zero
cache hits on both a cold and a warm run (before the fix the warm run hits
the cache and dies on the same
`origin_column->size() == count` check).
- A regression suite covers the `UPDATE` repro (stale value and the
three-column read) plus a full-row-store
control that must keep using the cache.
No FE change, no change to write-path invalidation, the cache key, the LRU
policy or
`has_row_store_for_all_columns()`. Making the row cache serve partial row
stores would require a row-location
lookup on every hit (and delete-sign handling), which defeats the purpose of
the cache; that is out of scope
here.
### Test
- `./build.sh --be` (ASAN).
- `./run-be-ut.sh --run --filter='PointQuery*:*PointQuery*'` — 7 tests
passed, including
`SegmentLayouts/PointQuerySegmentReuseTest.PartialRowStoreDoesNotUseRowCache/{0,1}`
and the existing
`LookupUpAndRowCacheHit/{0,1}` (local toolchain needed
`GLIBC_COMPATIBILITY=OFF` for the UT link).
- New suite `point_query_p0/test_row_cache_partial_row_store` and the
neighbouring
`test_point_query`, `test_update`, `test_rowstore`,
`test_short_circuit_rowstore_nested_complex` suites pass.
- Manual repro on the fixed BE with the row cache enabled: warm/warm `1,
100`; after `UPDATE` the
short-circuit query returns `1, 200`, `id, v, c` returns `1, 200, new`
twice in a row, BE stays healthy and
`be.out` gains no `Check failed`.
--
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]
