Repository navigation
[fix](be) Fix data race on Segment::_footer_pb - #67720
stinger1206 wants to merge 4 commits into
Conversation
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
run buildall |
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
TPC-H: Total hot run time: 16745 ms |
TPC-DS: Total hot run time: 82135 ms |
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
ClickBench: Total hot run time: 14.82 s |
gavinchou
left a comment
gavinchou
left a comment
There was a problem hiding this comment.
pls resolve the comment
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
COMPLETE static review of PR #67720 at head 341c9a86f5979db474c0c8ba1cb0cf255ed13c72. One required round converged: both full-review agents and the separate risk-focused agent returned NO_NEW_VALUABLE_FINDINGS. I found no new, distinct inline issue.
-
Goal and proof: The three production reads/writes of
Segment::_footer_pbin the changed code now use_footer_pb_lock, and a localshared_ptrkeeps the footer alive after unlocking. That mechanically removes unsynchronized access to this weak-pointer object. No test was added for the race, and the existing footer-cache tests are sequential. I did not run builds or tests, as the review instructions prohibit them; the PR's manual testing claims are not independent validation here. -
Scope and code shape: The two-file change is focused.
Segmentalready contains a mutex and is noncopyable. The added mutex is included insizeof(Segment)metadata accounting. -
Concurrency and lock order: The new critical sections contain only weak-pointer promotion or assignment; footer parsing, cache operations, and
ByteSizeLong()remain outside them. Reader callbacks takeColumnReaderCache::_cache_mutexbefore the new mutex, and I found no reverse acquisition. The footer is parsed and normalized before cache publication; downstream consumers use const references or copies. No distinct deadlock, use-after-free, or mutable-protobuf race was substantiated. -
Lifecycle and parallel paths: Initial footer loading, column-reader creation, traversal, metadata sizing, and cache lookup/insert were traced. Concurrent cache misses can still parse and replace entries, but their handles retain valid footers; that behavior predates this patch. The live weak-pointer fast return can bypass an evicted cache entry and LRU recency. This is already raised in inline thread 4144557940 on this head, so I did not duplicate it. That unranked recency concern is not an independently confirmed P0/P1 blocking comment.
-
Crash-cause limitation: The PR body and added header comment say Daemon memory refresh calls
Segment::get_metadata_size()concurrently with footer rebinding. CurrentMemoryProfile::refresh_memory_overview_profile()instead reads an aggregate bvar viaget_all_segments_estimate_size(). ProductionSegment::update_metadata_size()reachesget_metadata_size()during_open, before the object is published; its other call is underBE_TEST. Reader footer callbacks are serialized by their cache mutex, and the two production traversal callers open local Segment objects. I could not establish the stated same-Segment production interleaving from current call sites, so this static review does not verify that the mutex resolves the reported production crash. The rationale deserves correction and a reachable reproducer if that crash claim is to be relied on. -
Other critical checkpoints: No configuration, dynamic-update, FE/BE variable, protocol, storage-format, persistence, transaction, data-write, MoW, or version-visibility change is present. No new observability path is needed for the lock itself. The added lock is on footer acquisition and metadata sizing rather than per-row reads; I found no substantive new performance issue. No additional user focus was supplied.
Final sweep covered both changed files, every _footer_pb access, all risk items, and the live existing thread. There are no unresolved candidate findings or new inline comments.
### What problem does this PR solve? Issue Number: close apache#67719 Problem Summary: `Segment::_footer_pb` is a non-atomic `std::weak_ptr<SegmentFooterPB>`. `_get_segment_footer()` reads it via `lock()` and rebinds it (`_footer_pb = footer_pb_shared`) from query/compaction threads whenever the cached footer expires, while `get_metadata_size()` calls `_footer_pb.lock()` from the memory maintenance thread without synchronization. Concurrent assignment and `lock()` on the same non-atomic `std::weak_ptr` is a data race and therefore undefined behavior; the observed production crashes (SIGSEGV in `SegmentFooterPB::ByteSizeLong`, in `~SegmentFooterPB` under `LRUCache::prune_if`, and at unrelated sites, healed by restart with segment files byte-identical) are consistent with process-memory corruption caused by this UB. Per review feedback, remove `_footer_pb` entirely instead of guarding it. `_get_segment_footer()` now goes through `StoragePageCache` on every access, so eviction and recency are handled consistently. This also fixes a second problem with the old weak_ptr fast path: callers could keep using a footer without a cache lookup, so the cache did not observe those accesses and its LRU order no longer reflected real usage. `get_metadata_size()` no longer looks at the footer: it is owned and tracked by `StoragePageCache` (which `_open()`'s `_meta_mem_usage` estimation already excludes), and a lookup there would bump LRU recency on every memory-maintenance sweep without any real query access. ### Release note None ### Check List (For Author) - Test: Manual test (canary BE under production load) - Behavior changed: No - Does this need documentation: No
341c9a8 to
31a388f
Compare
|
run buildall |
TPC-H: Total hot run time: 28819 ms |
TPC-DS: Total hot run time: 151125 ms |
ClickBench: Total hot run time: 24.5 s |
|
Two notes from #69811, which fixed the same race and which I'm closing in favor of this PR:
|
… test ### What problem does this PR solve? Issue Number: close apache#67719 Related PR: apache#67720 Problem Summary: PR apache#67720 removed Segment::_footer_pb, but be/test/storage/segment/segment_footer_cache_test.cpp still called segment_ptr->_footer_pb.reset() in GetSegmentFooterPropagatesIoContext, so the BE UT build failed to compile. Per review feedback in apache#67720 (apache#67720 (comment)), drop the stale line: the cache erase() right before it already forces the reload through the cache-only path, so the SyncPoint on Segment::_parse_footer::io_ctx still fires. Also port SegmentFooterCacheTest.ConcurrentGetSegmentFooterWithEviction from apache#69811 (thanks to @airborne12 for the original reproducer): 8 threads call _get_segment_footer() on a segment while half of them keep erasing the footer's cache page; every caller must get a valid, identical footer regardless of concurrent evictions. Adapted to this PR's cache-only design: the get_metadata_size() check is dropped because that method no longer reads the footer. ### Release note None ### Check List (For Author) - Test: Unit Test (this change itself is a test; BE UT run deferred to CI) - Behavior changed: No - Does this need documentation: No
|
Thank you @airborne12! Both suggestions are taken in d07b797:
|
|
run buildall |
TPC-H: Total hot run time: 28288 ms |
TPC-DS: Total hot run time: 150829 ms |
ClickBench: Total hot run time: 24.3 s |
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
|
/review |
There was a problem hiding this comment.
Completed the exact-head static review of PR #67720 at head d07b797f0d03cccc93163aa7d7d82eda4b2be411 in one converged round. The change removes Segment's weak footer pointer, routes footer reads through StoragePageCache, removes the footer from Segment metadata estimates, and adds concurrent eviction coverage.
Goal and proof: the current implementation copies the footer's shared_ptr while a PageCacheHandle is live, so LRU erase, eviction, or replacement cannot free the value before callers receive it. I traced initial open, column-reader creation, traversal, variant/column-reader callbacks, and cache invalidation. The added tests cover forced misses, IO-context propagation through direct and public open paths, cache-key fields, page-type lookup, and concurrent erase/miss/reinsert behavior.
Concurrency and lifecycle: LRU handles retain entries through erase/replacement; concurrent misses may parse equivalent immutable footers but do not cross-contaminate segments. The cache key includes path, file size, and footer offset, and the rowset-writer invalidation recomputes the same key. The cache wrapper owns and tracks footer bytes on normal and zero-capacity DummyLRU paths, while Segment and SegmentLoader estimates intentionally exclude them. ExecEnv teardown stops StorageEngine before destroying the page cache and destroys the page cache before CacheManager. No lock-order, status-propagation, protocol, persistence, transaction, MoW, version-visibility, configuration, or observability regression was found.
Existing review context: inline thread 4144557940 and reply 4182174702 raised the prior weak-pointer/eviction concern. I independently confirmed that concern is addressed by this head and did not duplicate it. No additional user focus was supplied.
Validation was static only as required: I inspected the diff, related call paths, tests, ledger, and live PR state; I did not run builds or tests. Final sweep covered all four changed files and every recorded risk candidate. No new inline finding is warranted.
Review converged after 1 round(s).
| VLOG_DEBUG << fmt::format("Segment footer of {}:{}:{} is missing, try to load it", | ||
| _file_reader->path().native(), _file_reader->size(), | ||
| _file_reader->size() - 12); | ||
| std::shared_ptr<SegmentFooterPB> footer_pb_shared; | ||
| RETURN_IF_ERROR(_parse_footer(footer_pb_shared, stats, source_io_ctx)); |
There was a problem hiding this comment.
_footer_pb if this member is deleted, then I think you should modify _parse_footer method to const method. Since there are many threads may call parse footer concurrently and parse footer should not modify Segment's member.
And you should add comment to parse footer method.
|
Please make I'd also like to see the cache-pressure case checked before merging. |
### What problem does this PR solve? Issue Number: close apache#67719 Related PR: apache#67720 Problem Summary: Per review feedback on apache#67720, now that Segment::_footer_pb is removed, _parse_footer() never modifies Segment members — many threads call it concurrently via _get_segment_footer(), each parsing into a caller-owned output object. Mark it const, together with the _write_error_file() helper it calls, and add a comment on the definition stating the contract: concurrent cache misses may parse independent, content-identical footer copies; sharing is handled by StoragePageCache in _get_segment_footer(). ### Release note None ### Check List (For Author) - Test: No need to test (const-correctness and comment only; no behavior change) - Behavior changed: No - Does this need documentation: No
### What problem does this PR solve? Issue Number: close apache#67719 Related PR: apache#67720 Problem Summary: Per review feedback on apache#67720, add two tests to SegmentFooterCacheTest: - RepeatedFooterAccessesRefreshLruOrder proves repeated footer accesses refresh the LRU order: against a single-shard index cache measured to hold exactly two footer pages, A and B are loaded, A is re-accessed (asserted a cache hit via a parse-count sync point), and a third footer-sized insert evicts B — the least recently used page — while the re-accessed A survives. Entry weights (charge + handle overhead) are measured and the eviction arithmetic asserted up front, so the scenario is self-verifying. - HeldFooterStaysValidAcrossCacheEvictionAndReload pins the VariantExternalMetaReader pattern under eviction: a held footer copy stays valid across eviction and reload, the reloaded footer is valid and content-identical (compared via SerializeAsString), and the index cache tracker is charged exactly while the entry is cached (footer bytes plus entry overhead), uncharged at eviction, re-charged identically at reload, and unaffected by holders that outlive the entry. The LRU test constructs a temporary StoragePageCache. Under BE_TEST, CacheManager::register_cache() replaces same-type entries and unregister_cache() is a no-op, so any constructed StoragePageCache hijacks the CacheManager registrations and leaves them dangling after destruction; the test restores the global cache registrations on exit (RAII, also on ASSERT failure), including for the pre-existing local cache in TestSemgnetFooterPBPage, which had the same dangling-registration issue. The held-footer test uses the existing global cache. SyncPoint enable/disable is RAII-guarded so ASSERT failures cannot leave processing enabled for subsequent tests. ### Release note None ### Check List (For Author) - Test: Unit Test (SegmentFooterCacheTest 8/8 passed in a local ASAN UT build — both new tests plus all six pre-existing tests; full BE UT deferred to CI) - Behavior changed: No - Does this need documentation: No
|
Hi, I've addressed the review comments and pushed two additional commits.
All 8 Regarding the cache-pressure concern, the tests confirm that a footer remains valid when held by a reader after cache eviction, while the cache's memory charge is released. The current implementation may re-parse the footer in this situation. I haven't been able to run a representative performance comparison against #69811. Would the regression coverage be sufficient to proceed, or could the maintainers help with the benchmark? The changes are ready for another review. Thanks! |
What problem does this PR solve?
Issue Number: close #67719
Problem Summary:
Segment::_footer_pb(std::weak_ptr<SegmentFooterPB>) was a second footeraccess path outside
StoragePageCache:_get_segment_footer()reads it vialock()and rebinds it(
_footer_pb = footer_pb_shared) from query/compaction threads whenever thecached footer expires, while
get_metadata_size()also accesses the samenon-atomic
std::weak_ptr. Concurrent assignment andlock()on the sameweak_ptr, if these paths overlap, is a data race and therefore undefined
behavior.
footer object was still alive — including while the cache entry was present —
so the cache could not observe real footer accesses and its LRU order could
diverge from actual usage. After eviction the object could even remain alive
through other
shared_ptrreferences.In production (4.1.3) we observed intermittent BE SIGSEGVs, with innermost
frames such as
SegmentFooterPB::ByteSizeLong(Segment::get_metadata_size)and
~SegmentFooterPBunderLRUCache::prune_if, plus crashes at unrelatedsites. Every crash was healed by a BE restart without repair and the implicated
segment files stayed byte-identical, which is consistent with transient
in-process memory/lifetime corruption rather than persistent on-disk
corruption. The exact production failure interleaving has not been proven.
See #67719 for a TSAN micro-repro of the unsafe
std::weak_ptraccesspattern.
Fix:
_footer_pbentirely;_get_segment_footer()always accesses the footer throughStoragePageCache(single access path: lookup → parse & insert on miss), so eviction and LRU
recency are handled consistently;
get_metadata_size()no longer reads the footer: footer memory is tracked byStoragePageCache's own memory tracking, and doing a lookup there only foraccounting would artificially refresh LRU recency on every metadata-size
sweep without any real query access. This also matches
_open()'s_meta_mem_usageestimation, which already excludes the footer;StoragePageCache::insertcomment that recommended storinga weak_ptr beside the cache (with
Segment::_get_segment_footeras theexample) to instead recommend going through cache lookup for every access.
Release note
Fix unsafe Segment footer caching that could cause BE crashes and bypass
StoragePageCache LRU accounting.
Check List (For Author)
Test
Unit test:
SegmentFooterCacheTest.ConcurrentGetSegmentFooterWithEviction(adapted from [fix](be) Guard Segment::_footer_pb against concurrent re-assignment #69811, thanks to @airborne12 for the original reproducer):
8 threads concurrently call
_get_segment_footer()on a segment while halfof them keep erasing the footer's cache page, verifying that every caller
gets a valid, identical footer regardless of concurrent evictions. A similar
original workload in [fix](be) Guard Segment::_footer_pb against concurrent re-assignment #69811 reproduced a heap-use-after-free under ASAN
within 20 runs on the old weak_ptr implementation (a thread read the footer
through
SegmentFooterPB::ByteSizeLongafter another thread freed it instd::weak_ptrassignment); with the cache-only design of this PR thespecific unsafe weak_ptr reassignment path is no longer present.
Manual test: the standalone TSAN micro-repro of the unsafe
std::weak_ptraccess pattern (attached to [Bug] BE SIGSEGV caused by data race on Segment::_footer_pb #67719) demonstrates the data race. The current
cache-only implementation is deployed on a production canary BE and the crash
family has not recurred under the previously-crashing workload.
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)