Skip to content

[fix](compaction) Release exhausted segment contexts during vertical compaction#65963

Draft
Yukang-Lian wants to merge 4 commits into
apache:masterfrom
Yukang-Lian:codex/vertical-compaction-segment-observation
Draft

[fix](compaction) Release exhausted segment contexts during vertical compaction#65963
Yukang-Lian wants to merge 4 commits into
apache:masterfrom
Yukang-Lian:codex/vertical-compaction-segment-observation

Conversation

@Yukang-Lian

Copy link
Copy Markdown
Collaborator

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

RowSourcesBuffer recorded which segment produced each output row, but did
not 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

  • Record each source's last global row-source position. The position remains
    correct when RowSourcesBuffer spills and is reset by seek_to_begin()
    between value groups.
  • After returning the last row or batch for a source, release that context's
    segment iterator and context-owned Blocks. Returned IteratorRowRef and
    RowBatch objects retain their own shared_ptr<Block>, so their data remains
    valid.
  • Cover AGG next_row(), UNIQUE fallback unique_key_next_row(), and UNIQUE
    sparse-batch unique_key_next_batch().
  • Add current and peak Bvars for active vertical-compaction segment contexts:
    vertical_compaction_active_segment_contexts and
    vertical_compaction_active_segment_contexts_peak.
  • Keep the key-group heap merge, DUP batch path, compaction selection, batch
    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.

Input Context peak before Context peak after Memory peak before Memory peak after
100 segments 100 10 51.89 MiB 6.1145 MiB
500 segments 500 10 255.14 MiB 6.1047 MiB

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:

  • UNIQUE fallback: 3 rowsets x 4 segments, context peak reduced from 12 to 3.
  • UNIQUE sparse batch: context peak is 3, with sparse optimization confirmed
    enabled.
  • AGG row path: 2 rowsets x 2 segments, context peak is 2.
  • Current context count returns to zero after every compaction.
  • Output keys, 8 KiB payloads, delete signs, and row counters are unchanged.
  • Spill and seek_to_begin() preserve global source-position tracking.

Focused ASAN BE unit tests passed:

6/6:
  TestRowSourcesBuffer
  TestRowSourcesBufferSpillThreshold
  TestUniqueKeyVerticalMerge
  TestUniqueKeyNonOverlappingSegmentContextRetention
  TestUniqueKeySegmentContextMemoryAmplification
  TestUniqueKeyVerticalMergeWithNullableSparseColumn

3/3:
  TestAggKeyVerticalMerge
  RowBatchSharedPtrLifetime
  RowBatchVector

Review follow-up 2/2:
  TestUniqueKeyNonOverlappingSegmentContextRetention
  TestAggKeyVerticalMerge

build-support/check-format.sh and git diff --check passed. The changed
production code also compiled with -Wall -Wextra -Werror in the ASAN unit
test build. Production-file clang-tidy is currently blocked by the existing
unmatched NOLINTEND at be/src/core/types.h:576 on master, unrelated to
this 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

    • Regression test
    • Unit Test
    • Manual test (detailed verification is included above)
    • 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
  • Behavior changed:

    • No.
    • Yes. Exhausted value-path segment contexts are released immediately
      after their final row source is handed to the caller.
  • Does this need documentation?

    • No.
    • Yes.

Check List (For Reviewer who merge this PR)

  • Confirm the release note
  • Confirm test cases
  • Confirm document
  • Add branch pick label

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.
@hello-stephen

Copy link
Copy Markdown
Contributor

Thank you for your contribution to Apache Doris.
Don't know what should be done next? See How to process your PR.

Please clearly describe your PR:

  1. What problem was fixed (it's best to include specific error reporting information). How it was fixed.
  2. Which behaviors were modified. What was the previous behavior, what is it now, why was it modified, and what possible impacts might there be.
  3. What features were added. Why was this function added?
  4. Which code was refactored and why was this part of the code refactored?
  5. Which functions were optimized and what is the difference before and after the optimization?

### 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants