[fix](compaction) Release exhausted segment contexts during vertical compaction#65963
Draft
Yukang-Lian wants to merge 4 commits into
Draft
[fix](compaction) Release exhausted segment contexts during vertical compaction#65963Yukang-Lian wants to merge 4 commits into
Yukang-Lian wants to merge 4 commits into
Conversation
Issue Number: N/A Expose process-wide current and peak counters for active vertical merge segment contexts, with an isolated-test reset hook. Add NONOVERLAPPING Unique Key coverage that reproduces segment-scaled context retention and verifies the corresponding memory amplification with equal input data.
Track the last row-source position for each segment and release value-path contexts once no later row source can reference them. Keep returned blocks alive through shared ownership and cover row, sparse batch, spill, memory, and content regressions.
Assert rowset-bounded context peaks on both sparse unique-key and aggregate-key value paths. Document the intentional block-list release semantics and make the source exhaustion helper defensive in release builds.
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
### What problem does this PR solve? Issue Number: None Related PR: apache#65963 Problem Summary: Value-path call sites manually paired RowSourcesBuffer::advance() with the exhausted-context release check. Centralize that invariant in consume_row_sources() so callers cannot advance the row-source cursor without performing the matching lifecycle check. This is a behavior-preserving cleanup of the existing fix. ### Release note None ### Check List (For Author) - Test: Unit Test - ASAN: VerticalCompactionTest.TestUniqueKeyNonOverlappingSegmentContextRetention - ASAN: VerticalCompactionTest.TestUniqueKeySegmentContextMemoryAmplification - ASAN: VerticalCompactionTest.TestAggKeyVerticalMerge - ASAN: SparseColumnOptimizationTest.RowBatchSharedPtrLifetime - Behavior changed: No - Does this need documentation: No
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.
What problem does this PR solve?
Issue Number: None
Related PR: None
Problem Summary:
Problem
Vertical compaction lazily opens one segment per input rowset for valid
NONOVERLAPPING rowsets. In the value-column merge path, however, a segment
context was retained after its last row source had already been consumed.
Its segment reader and decoded Blocks were not released until the whole value
group was destroyed.
As a result, the number of retained contexts grew with the total input segment
count instead of staying near the merge way/input rowset count. This caused
vertical compaction peak memory to grow linearly with segment count even when
the rowsets were non-overlapping.
This was delayed resource release during compaction, not a permanent leak:
the contexts were eventually released when the value group ended.
Root cause
RowSourcesBufferrecorded which segment produced each output row, but didnot expose when a source had been consumed for the last time. The value paths
therefore had no lifecycle boundary at which they could safely release an
exhausted
VerticalMergeIteratorContext.Fix
correct when
RowSourcesBufferspills and is reset byseek_to_begin()between value groups.
segment iterator and context-owned Blocks. Returned
IteratorRowRefandRowBatchobjects retain their ownshared_ptr<Block>, so their data remainsvalid.
next_row(), UNIQUE fallbackunique_key_next_row(), and UNIQUEsparse-batch
unique_key_next_batch().vertical_compaction_active_segment_contextsandvertical_compaction_active_segment_contexts_peak.sizing, and MemTracker accounting unchanged.
Verification
The before/after memory test uses identical deterministic input: 10
NONOVERLAPPING input rowsets, 32,000 rows, an 8 KiB VARCHAR payload per row,
and compaction batch size 32.
Adding 400 segments increased the pre-fix peak by about 203 MiB. After the
fix, the two memory peaks differ by only 10,250 bytes. Repeated runs produced
the same context and memory peaks.
Additional lifecycle coverage verifies:
enabled.
seek_to_begin()preserve global source-position tracking.Focused ASAN BE unit tests passed:
build-support/check-format.shandgit diff --checkpassed. The changedproduction code also compiled with
-Wall -Wextra -Werrorin the ASAN unittest build. Production-file clang-tidy is currently blocked by the existing
unmatched
NOLINTENDatbe/src/core/types.h:576on master, unrelated tothis change.
Release note
Reduce vertical compaction peak memory by releasing exhausted segment readers
and decoded Blocks during value-column merging.
Check List (For Author)
Test
Behavior changed:
after their final row source is handed to the caller.
Does this need documentation?
Check List (For Reviewer who merge this PR)