Repository navigation
Credit and bound memory from aborted vamana inserts - #64
matt-welch wants to merge 6 commits into
Conversation
Aborted inserts into a vamana index never credited their memory back to the per-database residency budget, so repeated insert/abort cycles permanently inflated recorded residency until new work was falsely refused despite real headroom in the index. A direct re-measure-after-delete fix was tried and measured dead: deleting half of a test index's rows freed only ~1.5% of the expected memory, since soft-deleted vectors stay resident until an explicit compaction runs. - Track insert growth through to real commit/abort and credit it back to the budget only on confirmed abort - Bound credited-but-unreclaimed memory with a compaction backstop so insert-only aborts can't grow it unboundedly - Reconcile accounting after worker checkpoints, which already compact the index as a side effect of saving it but never told accounting so - Add a residency_bytes_reclaimable stats column and ten new TAP test cases exercising the credit, bound, and reconciliation paths Signed-off-by: Matt Welch <matt.welch@intel.com>
Merging main's switch-statement refactor (worker write dispatch split into VamanaWorkerExecuteInsertSlot/DeleteSlot/MaintenanceSlot) left two copies of the INSERT handling: the new helper function, and the original inline case in the switch, which is the only one actually called. The orphaned helper still called SvsMemoryReanchorInsert with its old three- argument signature, which no longer matches the five-argument growth/generation-tracking signature this branch introduced, so the build failed to compile. Delete the unused duplicate helper and keep the inline switch case, which already has the correct call. Signed-off-by: Matt Welch <matt.welch@intel.com>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
|
||
| index_close(indexRel, AccessShareLock); | ||
| indexRel = NULL; | ||
| PG_TRY(); |
PerformCheckpoint() opens indexRel inside PG_TRY and reads it again inside PG_CATCH to close it on the error path, but the variable was not declared volatile. PostgreSQL's PG_TRY is built on setjmp/longjmp, and the compiler is free to keep a non-volatile local in a register across the setjmp, so a longjmp back into PG_CATCH can see a stale value instead of what PG_TRY actually assigned. Here that risks either leaking the relation lock or double-closing it, depending on what the stale read turns out to be. CodeQL flagged this on the PR. Declare indexRel volatile, matching the pattern already used for similar PG_TRY-scoped locals elsewhere in the worker code. Signed-off-by: Matt Welch <matt.welch@intel.com>
asonje
left a comment
asonje
left a comment
There was a problem hiding this comment.
Checkpoint's error path leaves reclaimableBytes/numDeleted stale after SVS has already compacted in memory
PerformCheckpoint (src/vamana_checkpoint.c:134-191) assumes a failed save means no compaction happened, and restores accordingly:
int priorNumDeleted = cache->numDeleted;
cache->numDeleted = 0; // anticipating save's compaction
PG_TRY();
{
...
VamanaSaveIndexToDisk(indexRel, cache->svsIndex, MAIN_FORKNUM, cache);
...
cache->residentBytes = SVSGetIndexMemoryUsage(cache->svsIndex);
...
SvsMemoryReconcileResident(MyDatabaseId, cache->indexRelid, cache->residentBytes, headroomVectors);
slotAdvanced = VamanaSlotAdvance(cache->replicationSlot, checkpoint_lsn);
}
PG_CATCH();
{
cache->numDeleted = priorNumDeleted; // <-- premise is false, see below
...
PG_RE_THROW();
}
That premise doesn't hold. VamanaSaveIndexToDisk (src/vamanaio.c:517-569) does, in order: SVSSaveIndex() (which, per this PR's own comment, compacts the live in-memory graph as a side effect before writing anything), then VamanaSaveTidMapAtomically(), then VamanaMarkIndexSaved(). If anything fails after SVSSaveIndex() succeeds — the TID-map write, VamanaMarkIndexSaved's own buffer step, or anything in PerformCheckpoint after VamanaSaveIndexToDisk returns
(SvsMemoryReconcileResident, VamanaSlotAdvance) — the live graph has already been compacted, but the catch block puts cache->numDeleted back to its pre-attempt value and never calls SvsMemoryReconcileResident. Accounting and the live graph
now disagree, and nothing fixes it until an unrelated later write on the same index happens to re-measure and self-correct.
Reproduced locally with an injection point placed at the point in VamanaMarkIndexSaved that fires after SVSSaveIndex and the TID-map write have both already succeeded (vamanaio.c:505, vamana-mark-index-saved-error):
baseline: committed=202636 reclaimable=0
after rollback (before any checkpoint): committed=202636 reclaimable=69160 # full credit, as designed
after the forced-failed checkpoint: committed=200260 reclaimable=69160 # looks untouched, as the code intends
after one more ordinary insert: committed=202884 reclaimable=0 # collapses on its own
The last line is the tell: nothing ran a COMPACT and the checkpoint never succeeded, yet reclaimable fell straight to 0 and committed landed back near the true pre-rollback baseline. The only way that happens is if SVSSaveIndex had already compacted the graph during the failed checkpoint attempt, and the next insert's fresh SVSGetIndexMemoryUsage() reading exposed the gap, which SvsMemoryReanchorInsert's shrink-clamp silently absorbed. Accounting was wrong for the entire window between the failed checkpoint and that incidental insert.
To reproduce:
- Build this branch against a PostgreSQL compiled with --enable-injection-points, with the injection_points extension available.
- Create a vamana index on a small seed table (~200 rows is enough to get real growth from a 100-row insert; a larger seed table has enough SVS-side capacity headroom to absorb the insert without any raw growth, which hides this entirely).
- In one session: BEGIN; INSERT ... 100 rows; ROLLBACK; and confirm pg_stat_vamana_worker.residency_bytes_reclaimable > 0 for the database.
- SELECT injection_points_attach('vamana-mark-index-saved-error', 'error');
- ALTER SYSTEM SET svs.checkpoint_operations = 1; SELECT pg_reload_conf();, then issue one more write on the index to trip the checkpoint debounce. Confirm the log shows "not checkpointed this cycle, will retry".
- SELECT injection_points_detach('vamana-mark-index-saved-error');
- Issue one more ordinary insert on the same index and re-check residency_bytes_committed/residency_bytes_reclaimable — reclaimable drops to 0 and committed lands near the true pre-rollback figure, with no COMPACT or successful checkpoint having run in between.
Suggested direction: don't gate the compaction-side bookkeeping (cache->numDeleted = 0, the eventual SvsMemoryReconcileResident call) on the entire VamanaSaveIndexToDisk + reconcile sequence succeeding — gate it on
SVSSaveIndex itself having succeeded, since that's the actual compaction boundary. A failure strictly before SVSSaveIndex runs should still restore priorNumDeleted (that part of the catch block is correct); a failure at or after it should commit
the zeroed numDeleted and still attempt SvsMemoryReconcileResident with whatever residentBytes measurement is available, rather than unconditionally reverting on every exception in the span.
PerformCheckpoint assumed a failed save meant no compaction happened, and unconditionally restored numDeleted to its pre-attempt value on any error. But SVSSaveIndex compacts the live graph as a side effect before VamanaSaveIndexToDisk writes anything, so a later step (the TID-map write, or VamanaMarkIndexSaved) can still fail after the real compaction already landed. Accounting was then left stale until an unrelated later write happened to self-correct it, reopening the exact false-lockout failure mode this residency accounting exists to prevent. VamanaSaveIndexToDisk now reports whether SVSSaveIndex itself succeeded, and PerformCheckpoint's error path uses that to decide: restore the prior baseline only if compaction never ran; otherwise commit the zeroed numDeleted and reconcile committed/reclaimable with a fresh measurement. The first version of this fix re-read the index's capacity headroom from its error handler, which could try to re-lock a metapage buffer the triggering failure had left exclusively locked, self-deadlocking the worker. SvsMemoryReconcileResident's headroom argument is now optional; the error path omits it, since compaction alone never changes vector count and the existing figure is still correct. Signed-off-by: Matt Welch <matt.welch@intel.com>
3 participants
Description
When a transaction inserts rows into a vamana index and then aborts (rolled back explicitly, or by backend crash), the memory those inserts consumed stays physically resident in the index, but the memory accounting layer never gets credit for it back. Repeated insert/abort cycles permanently inflate a database's recorded residency until it hits the admission budget and new work is refused, even though the underlying index has plenty of real headroom. We first tried the smallest possible fix: re-measuring the index's actual memory usage immediately after a delete. That was tested directly and measured dead. A controlled test built 2000 vectors, deleted 1000 of them (50%) before any compaction ran, and measured the index's reported memory usage before and after: it freed only about 24,000 of an expected ~800,000 bytes, roughly 1.5% instead of the expected 50%. The underlying index library keeps soft-deleted vectors' memory resident until an explicit compaction runs, so a bare re-measure would under-correct the stranded budget by about 98% and ship something that looks fixed without being fixed.
The fix that actually works has to track the real lifecycle of that memory: how much an insert grew the index, carried from the point of insertion through to the transaction's real commit or abort, credited back to the budget only when an abort is confirmed, and bounded so an insert-only workload can't accumulate unbounded uncompacted debt. That lifecycle touches the insert path, the worker's undo log (reused rather than duplicated, since it already has the correct transaction/subtransaction lifetime rules), and the accounting core, which is why the change touches as many files as it does.
Two pieces of the design are not optional extras:
A smaller alternative was considered and rejected: unconditionally compacting the index on every aborted insert that deleted rows. That is the smallest possible diff, but a single insert-only workload driving ~60 aborts/second would force ~60 full index compactions/second. Each compaction holds the index's exclusive lock, which blocks every search against that index. That swaps a budget-accounting bug for a search-availability outage, which is worse.
PerformCheckpoint()opens an index relation handle inside aPG_TRYblock and reads that same handle again insidePG_CATCHto close it on the error path, but the variable wasn't declaredvolatile. SincePG_TRYis built onsetjmp/longjmp, a non-volatile local can be kept in a register across thesetjmp, so alongjmpback intoPG_CATCHcan observe a stale value instead of what thePG_TRYblock actually assigned. We confirmed by compiling both versions and comparing generated code that the compiler was in fact dropping the catch-pathindex_close()call as dead code; it only looked harmless today because the one caller aborts the transaction regardless, which independently releases the lock. Fixed by declaring the variablevolatile, matching the pattern already used for similarPG_TRY-scoped locals elsewhere in the worker code. (A GitHub Advanced Security alert on the same lines turned out to be an unrelated, previously-established false-positive class for this codebase'sPG_TRYmacro usage, not a report of this issue; the two were initially conflated.) No behavior change on the success path, confirmed by re-running the full test suite against the rebuilt binary.A reviewer then found a second, more serious gap in the same function:
PerformCheckpoint()'s error handling assumed a failed checkpoint meant no compaction had happened, and unconditionally restored its pre-attempt bookkeeping on any exception. But the on-disk save compacts the live graph as its first step, before the later steps (the TID-map write, marking the index saved) that can still fail; if one of those later steps fails, the graph has already been compacted in memory while the accounting layer is told nothing happened. That reopens this PR's own target bug through a different door: accounting sits stale, with no bound on how long, until some unrelated later write on the same index happens to self-correct it. Fixed by having the on-disk save report whether the compaction step itself succeeded, independent of whether the save as a whole did, and reconciling accounting with a fresh measurement in the error path whenever it did, instead of only restoring the old baseline. The error-path reconciliation deliberately does not recompute the index's capacity headroom, since doing so would re-read the index's metapage and could try to re-lock a buffer that the triggering failure left exclusively locked by this same backend until the transaction aborts; compaction alone never changes vector count, so the existing headroom figure is still correct without re-reading it.Related Issues
Fixes an unbounded false-positive admission lockout caused by aborted inserts never crediting memory back to the per-database residency budget, including a second path into the same failure mode via a checkpoint that fails after already compacting the live graph.
Type of Change
Pre-Merge Checklist
Build
makecompletes without errors or warningsmake installcompletes successfullyTests
test/sql/and/ortest/t/test/modules/: it builds and passes (make -C test/modules/<module> installcheck)Documentation
docs/updated if architecture or usage changed (no public-facing docs describe this internal accounting behavior; none needed updating)Testing Notes
test/t/58_residency_rollback_reconcile.plnow has 11 test cases / 36 subtests, covering: baseline insert/abort credit accuracy, bounded growth of uncompacted credited memory under repeated abort cycles, the compaction backstop actually firing once a threshold is crossed, checkpoint-triggered reconciliation, backend-crash abort handling, conservation of total accounted memory (committed + credited) across the various abort paths, and (new) a checkpoint that fails after SVS has already compacted the live graph (T11, using an injection point at the exact post-compaction failure site; skipped automatically on a build without--enable-injection-points).Verified the original fix is load-bearing, not just test-shaped, by reverting pieces of it and re-running:
Also ran, as additional confirmation (not required for this change): the full SQL regression suite (clean pass) and the existing TAP tests covering related accounting and worker-crash paths (all passing, no regressions).
After declaring the checkpoint relation handle
volatile: rebuilt clean (make clean && make -j && make install, zero warnings) and re-ran the suite; all 33 subtests then in the file still passed with the same recorded byte values as before that fix, confirming it only corrects the compiler-visibility hazard and changes no behavior.After the checkpoint-reconciliation-on-failure fix (
T11): the first version of this fix recomputed capacity headroom from inside the error handler and hung the worker for the fullwait_for_logtimeout instead of failing cleanly, which is how the metapage-buffer self-deadlock described above was caught. With that call removed from the error path, rebuilt clean (zero warnings) and ran the full suite:Tests=36,Result: PASS.T11's own numbers from that run: before the failed checkpoint,committed=401894,reclaimable=2400(confirming real credited debt going in); immediately after the failed checkpoint, with no further insert,committed=401918,reclaimable=0(the ~24-byte committed delta is just the one-row insert used to trip the checkpoint debounce, not drift).T11is not yet included in a prove-it-fails cycle against the pre-fix code; the behavior it exercises was confirmed broken via direct reasoning about the call sequence and the reviewer's own manual injection-point reproduction, not yet by reverting this specific fix and rerunning this specific test.Build and all test runs were performed against a clean rebuild (
make clean && make -j && make install).