[BugFix] Distinguish bundled segment slices in page/segment caches (backport #72129)#72345
Merged
Merged
Conversation
Contributor
Author
|
Cherry-pick of ab6ea26 has failed: To fix up this pull request, you can check it out locally. See documentation: https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/checking-out-pull-requests-locally |
Contributor
Author
|
@mergify[bot]: Backport conflict, please reslove the conflict and resubmit the pr |
b9b95e7 to
fb04c8c
Compare
xiangguangyxg
approved these changes
Apr 30, 2026
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
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.
Why I'm doing:
Multiple caches in shared-data key on
(filename, offset)orfilenamewithout a slice disambiguator. When a physical
.datfile is written inbundle format (a single file holding per-tablet slices at different
offsets), two slices of the same file can therefore collide on the same
cache key, and a lookup for slice B returns slice A's cached bytes —
slice A silently duplicates in query results, slice B vanishes.
The data-loss path: storage page cache
be/src/storage/rowset/page_io.cpp::read_and_decompress_pagekeyed thepage cache on
encode_cache_key(read_file->filename(), page_pointer.offset).For a
BundleSeekableInputStream,page_pointer.offsetisstream-relative (0-based within the slice), so two slices of the same
bundled file produce identical cache keys at equal intra-slice positions.
Reproducer on a shared-data primary-key range-distributed table:
After the merge,
SELECT COUNT(DISTINCT id)returns 22,998 instead of25,000. 1,998 ids appear twice (slice-A rows read twice), 2,002 ids
disappear entirely (slice-B rows replaced by slice-A's cached pages). The
missing-id ranges are exactly the offset-1 slice contents of the two
bundled segments.
Runtime verification: setting
disable_storage_page_cache=trueon all CNsand repeating the same flow returns 25000/25000/25000 — confirming the
page cache is the sole source of the corruption.
Two adjacent defense-in-depth fixes (same mechanism)
TabletManager::load_segmentkeyed onsegment_info.pathalone. Two rowsets of a bundled file in one tabletwould share a
Segmentobject whose_segment_file_infowas bound towhichever slice loaded first. In isolation this does not cause the
symptom above — the page cache collision is what corrupts reads — but
it is the same fragility, and any future read path that bypasses the
page cache would surface the bug.
Segment::update_cache_sizeandturn_off_batch_update_cache_sizestill passedfile_name()(pathonly) to
update_segment_cache_sizeafter the Segment-cache key wasforked. For segments with non-zero
bundle_file_offsetthe lookup incache_segment_if_presentwould miss, leaving the cache entry'smemory cost unrefreshed.
Audit of other
(filename, offset)-style cachesEvery adjacent cache was reviewed for the same class of bug; none are
affected:
formats/parquet/page_reader.cppBundleWritableFile.CacheInputStreamblock cacheio/cache_input_stream.cppmodification_time.storage/sstable/table.cpp(cache_id, handle.offset)with per-Tablecache_idfromblock_cache->new_id(); SST files are not written viaBundleWritableFile.lake/meta_file.cpp::delvec_cache_key(tablet_id, DelvecPagePB); delvec files are not bundled;DelvecPagePBembeds version + offset + size.rowset/metadata_cache.cpprowset_id_str; no file offset.lake/tablet_manager.cpptablet_metadata_location(tablet_id, version).The only writer that produces bundles is
BundleWritableFileContext, wiredonly into
DeltaWriter/GeneralTabletWriter/PkTabletWriter(datasegments);
PkTabletSSTWriterusesfs->new_writable_filedirectly, sosstables are never bundled.
What I'm doing:
Encapsulate the slice disambiguator on the types that carry it, instead
of leaving each cache call site to know about BundleFile. Three caches
share one principle: the cache key must name the page/segment's
position in the physical file, not in the slice.
1. Page cache —
be/src/storage/rowset/page_io.cppio::SeekableInputStream::page_cache_key(int64_t stream_offset).Default encodes
(filename, stream_offset)(the existing binary format,moved out of
page_io.cpp).BundleSeekableInputStreamoverrides it to fold its slice base offsetinto the encoding, returning a key unique to the page's absolute byte
position in the physical file.
SeekableInputStreamWrapperforwards to_impl; all wrappers(
CacheInputStream,CountedSeekableInputStream,ThrottledSeekableInputStream,EncryptSeekableInputStream) inheritit automatically.
read_and_decompress_pagecallsread_file->page_cache_key(opts.page_pointer.offset)and never namesbundle_file_offset.2. Lake Segment metacache —
be/src/fs/fs.h,be/src/storage/lake/tablet_manager.cppFileInfo::cache_key()returns"<path>#<offset>"for bundledslices (
bundle_file_offset > 0) and the barepathotherwise.Non-bundled layout unchanged.
TabletManager::load_segmentkeys bothlookup_segmentandcache_segment_if_absentonsegment_info.cache_key().3. Segment cache-size refresh —
be/src/storage/rowset/segment.cppSegment::update_cache_sizeandturn_off_batch_update_cache_sizeroute through
_segment_file_info.cache_key(), so the insert key (setby
load_segment) and the update-size key stay identical for bundledslices by construction.
The previous public
SeekableInputStream::bundle_file_offset()accessorand the static
TabletManager::segment_cache_key()helper are removed;the BundleFile detail only exists inside
BundleSeekableInputStream::page_cache_keyandFileInfo::cache_key.Future caches keyed on stream/file identity get correct bundled-slice
handling without any new conditionals.
Tests
New unit tests:
PageIOTest.test_bundle_slices_do_not_collide_in_page_cachewrites twodistinct pages back-to-back into one physical file, wraps each with its
own
BundleSeekableInputStreamat offsets 0 andslice_a_size, warmsthe cache on slice A, then reads slice B at the same stream-relative
offset 0 and asserts (1) cache miss and (2) the decoded bytes are slice
B's, not A's.
LakeTabletManagerTest.segment_cache_key_distinguishes_bundle_slicescovers the
FileInfo::cache_key()rule directly.LakeTabletManagerTest.load_segment_returns_distinct_slices_for_same_pathseeds the metacache at one offset and confirms the lookup at another
does not collide.
Existing
PageIOTestandLakeTabletManagerTestsuites remain green(6/6 and 26/26 respectively).
End-to-end verification on a shared-data test cluster with the reproducer
above: the post-merge
SELECT COUNT(DISTINCT id)returns 25,000 with allexpected per-id values, where the same recipe previously returned 22,998
with mixed slice-A / slice-B corruption.
Fixes #64986
What type of PR is this:
Does this PR entail a change in behavior?
Checklist:
Bugfix cherry-pick branch check:
This is an automatic backport of pull request [BugFix] Distinguish bundled segment slices in page/segment caches #72129 done by Mergify.