Skip to content

Enforce compaction capacity invariants - #474

Open
tjgreen42 wants to merge 7 commits into
mainfrom
background-compaction-1-engine
Open

Enforce compaction capacity invariants#474
tjgreen42 wants to merge 7 commits into
mainfrom
background-compaction-1-engine

Conversation

@tjgreen42

@tjgreen42 tjgreen42 commented Aug 29, 2026

Copy link
Copy Markdown
Collaborator

Summary

Make segment compaction, memtable spill, and force-merge operations fail
before changing physical index state when the current level layout cannot
accept another promoted segment.

This establishes the storage invariants required by later background
compaction work, but contains no scheduler, transaction-dispatch, or
pg_durable integration.

Why

BM25 indexes store immutable segments across levels L0 through L7. A merge
removes segments from one level and promotes a replacement segment to the
next level; L7 is terminal.

These page changes are WAL-logged with GenericXLog, so they are physical
changes that do not roll back with the surrounding SQL transaction. Capacity
or terminal-level failures therefore must be detected before the first page
mutation. Discovering an impossible layout partway through a spill or
force-merge could otherwise leave a partially changed index.

Within the current forceMerge(1) contract, force merge also needs a
production-enforced result: an empty or all-dead index may have zero segments,
while any other successful force merge finishes with exactly one. Success is
conditional on the live corpus fitting one representable segment.

What changed

  • Centralize pre-mutation checks for persisted per-level segment-count
    increments.
  • Make threshold compaction capacity-aware. If an over-threshold level cannot
    promote because its destination is full, compact the blocking destination
    first.
  • Add a side-effect-free force-merge planner and preflight that simulate all
    level-count transitions before any physical mutation.
  • Reject impossible terminal layouts, including multiple L7 segments, an L7
    segment with lower-level segments, or an L7 segment plus a term-bearing
    memtable that would create another segment.
  • Split spill preparation from spill finalization so force merge can determine
    whether the memtable will actually create an L0 segment before writing it.
  • Keep the valid lone-L7 plus termless-memtable case working: document-length
    accounting is finalized without creating a new segment.
  • Prevent the force-merge spill from running ordinary threshold compaction
    before the force-merge plan executes.
  • Enforce planner progress and the final zero-or-one-segment invariant with
    production errors, not assertion-only checks.
  • Add a testing-only segment-count limit GUC so capacity boundaries can be
    exercised without constructing 65,535 segments.

User-visible behavior

Ranking and query results are unchanged. Valid compaction layouts continue to
merge normally. Layouts that cannot make progress now fail atomically with a
specific capacity error instead of failing after partial physical mutation.

bm25_force_merge() succeeds with:

  • zero segments for an empty or all-dead index; or
  • one segment for an index containing live terms that fits one representable
    segment.

This PR covers segment-count capacity and force-merge execution invariants. It
does not make every large index force-mergeable: #473
tracks consistent fail-safe enforcement of the existing per-segment format
limits, and #456
tracks bounded/split merges or a wider format for corpora that cannot fit in
one segment.

Test coverage

  • Full destination levels are drained before lower levels promote.
  • Segment-count exhaustion rejects without changing the index.
  • Force-merge preflight covers terminal L7 layouts and prospective memtable
    spills.
  • Empty, all-dead, termless-memtable, and ordinary live-document cases preserve
    document accounting and query results.
  • Concurrent VACUUM/merge coverage recognizes only the expected terminal
    capacity races.
  • A source guard keeps the force-merge production postconditions from
    regressing into assertion-only checks.

Stack position

This is the bottom, independently mergeable layer of GitHub stack #479.
Later layers add the public APIs, transaction dispatch, and optional
pg_durable integration.

@codecov

codecov Bot commented Aug 29, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 90.81081% with 17 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
src/segment/merge.c 89.65% 12 Missing ⚠️
src/memtable/chain_source.c 80.00% 3 Missing ⚠️
src/index/metapage.c 83.33% 1 Missing ⚠️
src/index/state.c 50.00% 1 Missing ⚠️

📢 Thoughts on this report? Let us know!

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This run has an inline finding on the stale error-string match in test/scripts/vacuum_concurrent_merge.sh (the tolerated race message no longer matches merge.c's updated ereport text). Beyond that, I also looked at the new unmatched HOLD_INTERRUPTS() added to tp_release_index_lock (src/index/state.c:1286, no RESUME_INTERRUPTS() anywhere in the tree) and at force-merge's handling of single-segment (nonzero) level_counts in src/segment/merge.c — both were examined and ruled out as issues in this run's verification.

Extended reasoning...

One confirmed finding (stale error string in test/scripts/vacuum_concurrent_merge.sh, no longer matching the updated merge.c ereport text) is being posted inline, and additional verified findings from this run are not being surfaced in this review. Given findings are present, per the review protocol I should not approve. I independently re-examined two of the candidate concerns already investigated by the hunting pipeline — the new unmatched HOLD_INTERRUPTS() in tp_release_index_lock (confirmed via grep that no RESUME_INTERRUPTS() call exists anywhere in the codebase, consistent with the pipeline's description) and force-merge's treatment of nonzero single-segment level_counts in merge.c — and found nothing beyond what the pipeline already recorded as ruled out. Given the confirmed and dropped verified findings, a human should review this PR alongside the inline comment.

Comment thread test/scripts/vacuum_concurrent_merge.sh
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.

1 participant