memory management foundation - #18
Conversation
Signed-off-by: klmckeig <kelly.l.mckeighan@intel.com>
Signed-off-by: klmckeig <kelly.l.mckeighan@intel.com>
asonje
left a comment
asonje
left a comment
There was a problem hiding this comment.
1 — src/svs_memory.c:315
residencyWithoutEstimate = entry->residencyBytesCommitted - reservation->estimateBytes; is an unguarded uint64 subtraction, and :326 stores the result on the !fits branch. If the counter is ever below this reservation's estimate the value wraps and is written back, after which the database is permanently unusable — there is no restart-free recovery, because nothing in the module subtracts a counter back down from 2^64.
Reproduced with the shipped test wrappers only, no code change. One index's estimate stays pending while another index's unload floors the counter to 0; the pending index's handoff then pins the database:
residency_budget | residency_bytes_committed
41943040 | -5242880 -- = 18446744073704308736
ERROR: build of index 8 would exceed database 903's residency budget
DETAIL: Requested 1048576 bytes, 18446744073704308736 already committed, 41943040 byte budget.
Every later call on that database is refused forever: reconcile_load returns false, reserve_build errors as above.
SubtractFloored already exists at :149 for exactly this. :315 and :214 are the two subtractions that don't use it.
2 — src/svs_memory.c:551
The reaper always releases reservation->estimateBytes, but SvsMemoryHandoffBuild:316-320 has already moved the counter onto measuredBytes and left estimateBytes stale. Reaping a CONFIRMED record therefore releases a quantity the counter is no longer holding.
Measured — truth 28311552, estimate 10485760, measured 7340032:
after handoff: 28311552
after reap: 17825792 -- over-released by 3145728 = estimate - measured
after the worker's real load of that index: 25165824 -- permanently 3145728 short
An estimate that under-predicts would fail the build it was sizing, so estimates will in practice run high, which makes over-release the usual direction: the counter under-reports and the database can then be admitted past its own budget.
The trigger is the larger half of this. Nothing clears ownerPid at confirm — :373 is its only writer of 0 — and the header at svs_memory.h:58-61 states the intended rule: "ownerPid is the reserving backend while RESERVED/CONFIRMED, and 0 once RESIDENT." So after CREATE INDEX commits, the record sits CONFIRMED still tagged with the creating backend, and the worker loads lazily. A normal client disconnect therefore makes OwnerPidIsDead true, and the reaper deletes a valid reservation for a committed, on-disk index. The worker's later load finds no record, falls into the cold branch at :380, and runs the cold-load budget check — the one meant for a genuinely unaccounted index — so a tight but legal budget can refuse to load an index that is already committed and on disk. The record's estimate-vs-measured figure is lost with it.
A PID check cannot separate the two cases: a backend that crashed mid-build and one that committed and disconnected are both a dead PID. The only thing that distinguishes them is the reservation's state — and with three states, CONFIRMED means both "abandoned mid-build" and "committed, awaiting load," so the reaper has no way to tell. Worth settling before callers are written against the three-state enum.
Note the build half of this same function avoids the problem by accident of having a self-zeroing field: the comment above ReapEntryReservations reasons explicitly about the missing distinction — "for a build reservation that never reached HandoffBuild, the only case buildPeakBytes is still nonzero" — and guards with if (… > 0). The residency release one line up has no such proxy field, and the comment states the bug as intent: "its residency bytes always."
3 — src/svs_memory.c:343-349
SvsMemoryAbortBuild releases the caller's buildPeak argument and the record's estimateBytes unconditionally, without consulting state. If the build path confirms via SvsMemoryHandoffBuild before it finishes — and it must, since the handoff is what rebases the counter from estimate to measurement — then any error after a successful handoff runs the abort handler against a CONFIRMED record: a failed serialize, a later statement in the same transaction erroring, a user cancel. Both quantities are then released a second time.
Measured, both warnings from a single call:
WARNING: ... releasing a build peak: releasing 5242880 bytes but only 0 committed
WARNING: ... releasing an aborted build's residency estimate: releasing 10485760 bytes but only 7340032 committed
The per-database counters floor at 0, but SubtractGlobalBuildCommitted takes the peak off totalBuildCommittedGlobal while it still holds other databases' live builds, so the breach crosses databases. Confirmed: with db 912 holding a legitimate 95 MB build against the 100 MB ceiling, db 913's 10 MB build is accepted after db 911's spurious abort — 105 MB admitted against a 100 MB limit.
The reaper gets this right at :554 by reading reservation->buildPeakBytes under an if (… > 0) guard. This is the one release path that trusts its caller's argument over the record.
4 — src/svs_memory.c:368-378
The existing-reservation branch of SvsMemoryReconcileLoad returns true unconditionally and adjusts nothing, on the premise stated in its comment: "a fresh handoff already reconciled to this exact figure … so it always succeeds." The premise does not hold, and fits is this function's only signal to its caller.
The consequence that matters: a lowered residency budget is silently not enforced. Reload an index after its budget has been reduced below what is already committed and this branch never reads residencyBudget:
admit(900, 100MB); reconcile_load(60MB) -> t
admit(900, 50MB); -- accepted silently
reconcile_load(60MB) -> t -- 62914560 committed, 52428800 budget
That is not accounting drift, it is a ceiling reporting OK while exceeded. A caller that means to react to an over-budget load has nothing to react to: as written, fits can only be false when no record exists at all.
Also measured: when the worker's figure differs from the backend's, the record takes the new value and the counter keeps the old one, so SvsMemoryAccountUnload:419 subtracts the record's value and floors the counter with a warning — which composes with the :315 wrap into the permanent pin described in G1.
Two further provenances, argued from code rather than run, both worth checking. A caller that zeroes the counter at worker restart and re-accounts each index by measurement as it reloads would find the ledger pins at 0, because the reservation array survives worker death — SvsMemoryResetDatabaseAccounting:726-730 is the only thing that clears it, and its own comment restricts it to first construction or slot release — so every reload takes this branch and adds nothing. And CacheEmptyTableIndex (src/vamanaworkerindex.c:106-115) caches a NULL handle for any index on an empty table, an ordinary state rather than an error, so measuring after that yields 0, which this branch writes into the record and :419 later subtracts as 0, leaving that index's bytes charged for the life of the slot.
Suggest auditing every provenance by which a record can be live at load time and either handling it or asserting it away. A branch whose correctness rests on a claim about callers that don't exist in the tree yet is hard to keep true as they land.
5 — src/svs_memory.c:263
SvsMemoryReserveBuild has no duplicate-relid guard, and FindReservation:76-83 returns the first match by array position. Reserving for an index that already has a record therefore creates a second one, and every accessor in the module goes through FindReservation, so only the first is ever reachable.
The live route is a rebuild of a currently resident index: VamanaRebuildFromTable (src/vamanabuild.c:530, reached from src/vamanaworkerindex.c:138) runs a build for a relid whose RESIDENT record still exists.
Measured: both records charged (18874368 total), then the rebuild's handoff_build mutated the resident record (→ CONFIRMED, measured 6291456) and never touched the reservation it was called for. Freeing the first surfaces the orphan:
state | owner_pid | estimate_bytes | measured_bytes | build_peak_bytes
RESERVED | 2807374 | 8388608 | 0 | 1048576
8388608 residency bytes and a 1048576 build peak that only the reaper can release — and build_bytes_committed had already gone to 0 at the handoff, so that peak would be released a second time. Erroring when FindReservation(entry, relid) != NULL is the fix.
6 — src/vamanaworkerindex.c:194
opts is captured from indexRel->rd_options, then :199–:200 run LoadIndexFromDiskOrRebuild (heap scans, relation opens, a subtransaction) and FinalizeIndexCacheEntry (opens a replication slot), all of which process invalidations, and :209–:210 dereference the pointer. A relcache rebuild frees rd_options.
The comment at :202–:207 makes the case itself: it describes this point as "the same relcache invalidation that would fire from ALTER INDEX SET," while the code holds a pointer across exactly that invalidation. Re-reading indexRel->rd_options at the call site is the whole fix.
Not pre-existing: the base tree's GetOrLoadIndexBody has no such capture, and its only other rd_options read, CacheEmptyTableIndex:108, dereferences immediately.
7 — src/svs_memory.c:300-303
SvsMemoryHandoffBuild releases the build peak against both the per-database and global counters before checking that the reservation exists, so the "no pending build reservation" ERROR at :305–:308 leaves both build counters already decremented. A mis-sequenced caller under-counts the build axis instead of failing cleanly. Moving FindReservation above the release is the fix. (Coverage confirms :306–:307 is never reached by the suite.)
8 — src/svs_memory.c:214 and :229
projectedGlobalTotal = header->totalResidencyCommittedGlobal - entry->residencyBudget + residencyBudget; is unguarded. It is currently safe — totalResidencyCommittedGlobal is written only at :229 and :718, residencyBudget only at :230 and :721, each pair under the header lock, so sum(residencyBudget) == totalResidencyCommittedGlobal holds — but that invariant is load-bearing and recorded nowhere, and it is the same unguarded-subtract shape as :315, which demonstrably does wrap (G1). An Assert(entry->residencyBudget <= header->totalResidencyCommittedGlobal) naming it.
Two things SvsMemoryAdmitDatabase does not validate.
A zero budget is accepted and silently un-admits the database. admit(921, 0) sets residencyBudget = 0, after which RequireAdmitted:65-74 errors on every build, load and insert, SvsMemoryReadStats returns no row at all, and the error text is "database 921 has no SVS memory accounting entry" — indistinguishable from a database that was never enrolled, while a 5 MB index is still charged to it. If whatever resolves the budget can ever yield 0 (a NULL override, an unset default), enrollment quietly disables the database instead of failing loudly.
No backstop on a decrease. Nothing compares the new budget against residencyBytesCommitted; admit(900, 50MB) against 60 MB committed is accepted with no log line. If a catalog trigger is meant to reject such a decrease, a WARNING here would catch a bypassed or not-yet-installed one — and per G4, admitting below the committed total is precisely what makes the ceiling silently unenforceable.
9 — src/svs_memory.c:85-108
AllocateReservation sets relid, searchScratchBytesPerQuery, the two cached reloptions and buildPeakBytes — but not state, ownerPid, reservedAt, estimateBytes or measuredBytes — and FreeReservation clears only relid. A freed slot keeps its byte counts, and a fresh reservation inherits them until its caller overwrites them. Both current callers do overwrite all five, so there is no live bug; an allocator that depends on every caller finishing the job is the concern, in a module other callers will be written against.
It also makes the InvalidOid sentinel dangerous rather than merely odd: FindReservation(entry, InvalidOid) is the free-slot search, so SvsMemoryAccountUnload(db, InvalidOid) would subtract a free slot's stale measuredBytes, and SvsMemoryReconcileLoad(db, InvalidOid, m) would mark a free slot RESIDENT while leaving it allocatable. Zeroing the whole record in AllocateReservation plus one Assert(OidIsValid(relid)) per entry point closes it.
Lowest-severity item in this set — every real call site derives relid from RelationGetRelid, so reaching it takes a caller bug.
10 — src/svs_memory.c:475-491
SvsMemoryReanchorInsert never compares the result against residencyBudget. The arithmetic is correct: across several pending batches, with committed = M0 + d1 + d2, reanchoring batch 1 at M1 subtracts M0 + d1 and adds M1, leaving M1 + d2 — and FindOldestInsertReservation picking the oldest is what keeps that true even after the reaper released a batch. So this is an observability gap, not a correctness one.
But a re-measurement can carry residencyBytesCommitted past residencyBudget with nothing logged, and that counter is what every other gate and any external decrease-validation reads. A DBA would see build refusals and rejected budget changes with no indication that an insert took the database over budget. Refusing isn't an option here (the rows are already applied); a WARNING naming the index and the overage is.
11 — src/vamanaworkerstats.c:290
The uint64 counter is cast to int64 and sql/svs--0.1.0.sql:149 declares residency_bytes_committed bigint, so any value above 2^63 renders negative in pg_stat_vamana_worker. In the wrap state from G1 the view reports -5242880 rather than a large number — the one place a DBA would look to diagnose it shows a nonsensical value instead of an alarming one.
12 — src/svs_memory.c:191-201 and :686-693
SvsMemoryResidencyBudget and SvsMemoryResolveResidencyBudget have no callers anywhere in the tree — not in src/, not in the test harness — and coverage confirms :191-201 is entirely unexecuted. Either wire them up or drop them; shipping an exported accessor that has never run once is a trap for the first caller that reaches for it.
13 — test/modules/svs_memory_test/svs_memory_test.c:41-105
Every wrapper casts PG_GETARG_INT64 straight to uint64, so the suite can be driven with values production cannot produce:
SELECT svs_memory_reserve_insert(902, 1, -1); -- returns t
-- residency_bytes_committed: 10485760 -> 10485759
committed + deltaBytes <= budget wraps to committed - 1 <= budget, so the call reports success and decreases the counter. Rejecting negatives in the wrappers keeps the suite honest about what the module actually has to handle.
Test Gaps1 — the reaper's release body is never executedCoverage on a
The reap is the module's only mechanism for reclaiming a reservation whose owner is gone, so it is the last thing that should ship unexercised — and G2 is a reap bug that a single dead-owner case would have caught. It is reachable from one session with a test-only 2 — no ceiling comparison is exercised at equalityAll six are 3 — the fake shmem removes two error paths and the whole slot-release path
4 —
|
Signed-off-by: klmckeig <kelly.l.mckeighan@intel.com>
Findings fixed#1 — unguarded uint64 subtraction. #2 — reaper misaccounts a CONFIRMED reservation. The reaper now reclaims only #3 — #4 — #5 — #6 — stale #7 — #8 — #9 — reservation record not fully reset. #10 — #11 — uint64 cast to int64 in the stats view could render negative. Added a clamp at all six cast sites. A future counter bug now shows as an implausible large number, not a negative one. #12 — no callers for #13 — test wrappers accepted negative arguments. Added a helper that rejects a negative argument at the wrapper boundary. Used at all six call sites. Test gaps from the follow-up comment#1 — false claim fixed. #2 — boundary coverage added. Every #4 — #5 — divergent-measurement coverage added. A reconcile with a measured value different from the handoff value now has a regression case. The reload-after-lowered-budget case is no longer reachable, since finding #8 now blocks that budget decrease outright. #7 — stats-column coverage added. All new stats columns now have a regression assertion, including unprivileged access and NULL-safety for #8 — GUC boundary round-trips added. All five new memory GUCs now have a valid-lower-boundary and valid-upper-boundary round-trip test. DeferredTest gap #1's TAP test (dead-owner reap). No production caller yet creates a live build or insert reservation in real shared memory. Writing this test today needs a temporary debug-only hook in the extension. Deferred until a real call site exists. Test gap #3 (two error paths and lock contention). The module's test harness uses a private, single-process stand-in by design. Two error paths and any lock contention need a real shared-memory layer under multi-process load. This needs new infrastructure, not a test change. Test gap #6 (concurrency). Same limitation as #3. The harness has no second process and no real shared memory to contend over. A concurrency test needs a real worker racing a real backend. Test gap #7's remaining half (nonzero Not in scopeTest gap #9 (three TAP files skip on a default build). The skips key off |
Signed-off-by: klmckeig <kelly.l.mckeighan@intel.com>
2 participants
Description
This PR adds memory accounting for SVS index builds, cached indexes, and search-scratch buffers.
New GUCs:
svs.max_build_memory,svs.max_residency_memory,svs.default_residency_memory,svs.max_search_work_mem,svs.default_search_work_mem. Each is in MB. Each needs a SIGHUP reload. Each accepts 1 to INT_MAX. Default is 100MB. Eachmax_*GUC sets a cluster-wide ceiling. Eachdefault_*GUC sets the fallback for a database with no override.Catalog:
vamana_databases.total_memory_mbis gone. Two columns replace it:residency_memoryandsearch_work_mem. Each must be greater than 0, or NULL. NULL means "use the default GUC". A new table,svs_index_residency, stores each index's resident bytes. The table exists but has no reader or writer yet.Accounting module:
src/svs_memory.cis new. It tracks build and residency memory in one place. A reservation moves through three states: RESERVED, CONFIRMED, RESIDENT. The module can admit a database, reserve or hand off or abort a build, reconcile a load, account an unload, reserve or reanchor an insert batch, reap dead reservations, and read stats. It checks a per-database budget and the cluster-wide ceiling. NoCREATE INDEXor index-load code calls this module yet. This PR adds the engine only.Shared memory: each per-database worker slot gets new fields:
residencyBudget,residencyBytesCommitted,buildBytesCommitted,searchScratchBytesInFlight, a new lock (memLock), and two override fields. The shared header gets two new cluster-wide totals. The launcher writes each database's override into shared memory at slot reservation and on every reconcile pass.Dead-reservation reaping: a reservation is reclaimed when its owning backend is dead. The check also catches a recycled PID: it compares the current backend's start time against the reservation's timestamp. This runs at launcher startup and on every reconcile cycle.
Reloption-triggered invalidation: changing
search_window_sizeoruse_search_historyon an index invalidates its cached search-scratch cost. The cost is recomputed the next time the index loads. Other reloption changes, such asgraph_degree, do not trigger this.Measurement wrapper:
SVSGetIndexMemoryUsageandSVSGetIndexMemoryBreakdownwrap the existing SVS memory-usage calls.Stats views:
pg_stat_vamana_workergets six new columns:residency_bytes_committed,build_bytes_committed,residency_memory_limit,residency_drift,search_work_mem_limit,search_scratch_bytes_in_flight. The view computesresidency_driftwith a join againstsvs_index_residency.pg_stat_vamana_worker_slotgets one new column:search_scratch_bytes_per_query. This value is per index, so it stays on the slot view, not the worker view.Related Issues
N/A
Type of Change
Pre-Merge Checklist
Build
makecompletes without errors or warningsmake installcompletes successfullyTests
make installcheck) and TAP tests (test/t/) pass with no failurestest/sql/and/ortest/t/test/modules/: it builds and passes (make -C test/modules/<module> installcheck)Documentation
docs/updated if architecture or usage changedTesting Notes
svs_cpu_budget_test,svs_memory_test,svs_parallel_build_test.svs_memory_testexercises admission, build reserve/handoff/abort, load reconcile, unload, insert reserve/reanchor, dead-reservation reap, and both ceilings. It uses a local stand-in for the shared-memory lookup, since this module runs no real worker or launcher.test/t/12_launcher.plnow checks thatresidency_memoryandsearch_work_memoverrides resolve to the correct byte values inpg_stat_vamana_worker. It also checks that an unprivileged role can read the new columns for its own row only.