BUGFIX: per-scan TupleDesc/slot leak and per-rescan state leak - #276
Open
dylon wants to merge 2 commits into
Open
BUGFIX: per-scan TupleDesc/slot leak and per-rescan state leak#276dylon wants to merge 2 commits into
dylon wants to merge 2 commits into
Conversation
…ound
TableSlot::from_index_heap_pointer creates a TupleTableSlot with
table_slot_create, which goes through MakeTupleTableSlot and pins the heap
relation's rowtype TupleDesc on the current ResourceOwner. Both the slot and
that pin are released only by TableSlot's Drop, via
ExecDropSingleTupleTableSlot -- but the wrapper was constructed only on the
success path. The `if !valid { return None; }` branch therefore leaked the slot
and the descriptor reference: the raw PgBox::from_pg handle is non-owning, so
its Drop is a no-op.
That branch is taken whenever index_fetch_tuple finds no snapshot-visible
version in the HOT chain -- a deleted, updated-away, or not-yet-vacuumed TID --
which the scan explicitly expects. It is reached from the rescore path
(get_full_distance_for_resort in both the sbq and plain storage
implementations, driven by next_with_resort), and SBQ always rescores, with
resort enabled by default. On a table with ongoing updates or deletes this
leaks once per rescored candidate. The descriptor pin surfaces as
"resource was not closed: TupleDesc ... (<oid>,-1)" at ResourceOwner release,
and the accumulated slots grow the backend without bound.
Arm the RAII wrapper immediately after table_slot_create and fetch through it,
so every exit -- the early return and any unwind -- releases both. Callers are
unchanged; they already handle None.
Add a regression test that deletes a row and then calls
from_index_heap_pointer with a snapshot that cannot see it, asserting both that
the call returns None and that the relation's rowtype descriptor reference
count is unchanged.
Fixes timescale#211
TSVScanState::initialize published the scan's StorageState with PgMemoryContexts::CurrentMemoryContext.leak_and_drop_on_delete, tying its lifetime to the executor's per-query context. amrescan calls initialize again for every rescan and overwrote the pointer without dropping the state it replaced, so a nested-loop or lateral join orphaned one response iterator -- plus a cloned SbqQuantizer -- per outer row, all held until the query finished. amendscan freed nothing at all: its entire body was gated behind a DEBUG1 log-level check, so even a single scan's state survived until per-query teardown. Give the state real ownership instead. initialize releases the previous StorageState before publishing the next one with Box::into_raw; release_storage reclaims it with Box::from_raw and nulls the pointer, so it is idempotent; amendscan releases unconditionally; and a Drop impl on TSVScanState is the backstop for scans torn down without amendscan, such as an aborted query. The field stays a raw pointer so the existing call sites, which need a mutable borrow of TSVScanState alongside the state itself, are unchanged. amendscan's debug-stats block now tolerates a scan that was never rescanned, rather than expecting storage to be present, since that block now always runs. Add a regression test: a lateral join that rescans the index once per outer row over a deliberately churned heap. Besides covering the rescan release path, it guards the release itself -- an incorrect ownership transfer would double free or use freed memory there rather than merely leak.
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.
Release scan resources deterministically: fix per-scan TupleDesc/slot leak and per-rescan state leak
Fixes #211.
Summary
Two leaks on the diskann scan (rescore) path let a backend grow without bound.
The first is the
TupleDesc reference leakreported in #211 — it is not a cosmeticwarning; it is accompanied by a leaked
TupleTableSloton every dead/invisiblerescored candidate, and on a table with ongoing
UPDATE/DELETEtraffic it growsuntil the host runs out of memory.
1.
TupleTableSlot+ rowtypeTupleDescleaked when a heap pointer has no visible tupleTableSlot::from_index_heap_pointer(src/util/table_slot.rs) creates the slot withtable_slot_create, which goes throughMakeTupleTableSlot→PinTupleDesc(rd_att)and so registers a descriptor pin on the current
ResourceOwner. That pin — and theslot itself — are released only by
TableSlot'sDrop(
ExecDropSingleTupleTableSlot).The wrapper was constructed only on the success path:
PgBox::from_pgis non-owning, so itsDropis a no-op — the early return leaks both.!validis hit wheneverindex_fetch_tuplefinds no snapshot-visible version in theHOT chain (deleted, updated-away, or not-yet-vacuumed TIDs), which the scan explicitly
expects. It is reached from the rescore path —
get_full_distance_for_resortin bothsbq/storage.rsandplain/storage.rs, driven bynext_with_resort— and SBQ alwaysrescores, with resort on by default. So on a churned table this leaks once per
rescored candidate, and the descriptor pin surfaces as the
resource was not closed: TupleDesc ... (<oid>,-1)warning from #211.Introduced by b480530 ("Fix HOT update handling"), which added the
!validearlyreturn; before that the slot was always wrapped.
Fix: arm the RAII wrapper immediately after
table_slot_createand fetch throughit, so every exit path — the early return and any unwind — drops the slot and releases
the descriptor. No caller changes: both callers already handle
None.2. Per-rescan
StorageStateaccumulation, andamendscanfreeing nothingTSVScanState::initializepublished the scan'sStorageStatewithPgMemoryContexts::CurrentMemoryContext.leak_and_drop_on_delete(...), i.e. tied to theexecutor's per-query context.
amrescancallsinitializeagain for every rescanand overwrote the pointer without dropping the previous state, so a nested-loop or
lateral join orphaned one response iterator (plus a cloned
SbqQuantizer) per outerrow, all held until the query ended.
amendscan's entire body was gated behindmin_level <= DEBUG1and freed nothing, so even a single scan's state lived untilper-query teardown.
Fix: make the state genuinely owned —
Box::into_rawininitializeafterreleasing the previous one, a
release_storage()helper that nulls as it frees (so itis idempotent), an unconditional release in
amendscan, and aDropimpl onTSVScanStateas the backstop for scans torn down withoutamendscan(e.g. an abortedquery). The field stays a raw pointer so the existing call sites, which need
&mut TSVScanStatealongside the state, are unchanged.amendscan's debug-stats blocknow tolerates a scan that was never rescanned instead of
expect-ing storage.Impact (why #211 is not benign)
Observed on PostgreSQL 18.4 with pgvectorscale 0.9.0 serving a ~740k-row,
1024-dimension table under continuous re-indexing (so a steady supply of dead tuples),
with dense ANN queries plus lateral k-NN batch jobs:
resource was not closed: TupleDesc ... (17176,-1)warning fired at roughly650/min, one per rescored dead candidate.
82 GB — and the host OOM-killed. Backends that were idle still held multiple GB,
i.e. the growth was retained, not query working set.
work_mem = 4MB,maintenance_work_mem = 64MB.growth completely: the same workload then held ~1.4 GB across all backends and stayed
flat.
Minimal reproduction
PostgreSQL's own ResourceOwner reports each unreleased descriptor pin, so the leak is
directly observable without driving a backend to OOM. This takes a few hundred rows,
one query, and a couple of seconds:
On PostgreSQL 18.4 with pgvectorscale 0.9.0 (
57c88b7), that single query prints:60 slots and descriptor pins allocated and never freed by one small query.
16796is
leak_mve's rowtype OID, and each line is oneTupleTableSlotthattable_slot_createallocated and pinned but nothing ever dropped. Nothing here isreclaimed until the ResourceOwner is torn down, and each is charged to the backend
until then — which is why a busy table walks a backend up into the tens of GB.
The control run matters: with no dead tuples the rescore never takes the
!validbranch and nothing leaks, which localises the defect precisely to that early return.
The same leak measured as backend memory
Scaling the reproduction up — 4 000 rows, half of them deleted, then ~8 000 index
rescans driven from a lateral join inside a single backend — shows the leaked
slots accumulating as resident memory. Same server, same query, only the extension
differs:
Unpatched, the backend grows monotonically for as long as the workload runs and never
gives the memory back; patched, it is flat. Extrapolate that to a production table
under continuous churn and it is the 82 GB OOM described above.
Verification
re-running
table_slot_releases_tupledesc_on_dead_tuplegives:cargo pgrx test --no-default-features --features 'pg_test pg18 build_parallel' -- pg18→
84 passed; 0 failed; 10 ignoredon PostgreSQL 18.4 (a--enable-cassertbuild), including both new tests.
cargo fmt --checkclean.Tests
util::table_slot::tests::table_slot_releases_tupledesc_on_dead_tuple— inserts arow, records its TID, deletes it, and calls
from_index_heap_pointerwith a snapshotthat cannot see it. Asserts the call returns
Noneand that the relation'srowtype
TupleDescreference count is unchanged. Fails before the fix (the count isleft incremented), passes after.
access_method::scan::tests::diskann_rescan_releases_state_each_iteration— alateral join that rescans the index once per outer row over a deliberately churned
heap. Covers the rescan release path and would crash on a double free or
use-after-free rather than merely leak.
Existing accuracy/scan suites are unchanged and still pass.