Fix CREATE INDEX failing on 1GB+ vector builds - #50
Conversation
The build path collects every vector of a table into one growable buffer before handing it to the index builder. That buffer allocated with plain memory-allocation calls, which reject any single request of 1 GiB or more, so any table whose vectors reached that size failed CREATE INDEX outright, even though the same build succeeded on an earlier version of this code. - Switch the vector buffer's initial allocation and its doubling growth to the huge-allocation API - Apply the same fix to the other row-scaled buffers on the same build and load paths: the TID buffer collected during a build or rebuild, the one-shot id array handed to the index builder, and the TID map copied into the worker's cache after a build, rebuild, or checkpoint load - Add a regression test covering both ways the buffer's size crosses the limit: the initial allocation sized from the table's row-count estimate, and the buffer's doubling growth mid-scan on a table without accurate statistics Signed-off-by: Matt Welch <matt.welch@intel.com>
There was a problem hiding this comment.
Verdict
Correct and in scope. The change swaps every row-scaled build/load allocation from
palloc/repalloc to the huge-allocation API, restoring CREATE INDEX on tables whose
vector buffer crosses 1 GiB (about 175K rows at dim 1536). No correctness defect found on the
happy or failure paths. Coverage is precise: query-k and attribute-count allocations
(vamanascan.c:246-247, vamanabuild.c:1105-1106) are correctly left alone, and no palloc of
the multi-GB index blob exists (the SVS library reads it from a file). MemoryContextAllocHuge
and repalloc_huge come from utils/palloc.h via postgres.h, so no include is missing.
Size expressions are 64-bit throughout; the governed build path also has an explicit overflow
guard. B6 calibration independently confirms a 180k x 1536 build succeeds post-fix and fails
with "invalid memory alloc request size" pre-fix.
Gaps
1. Doubling-growth transient is unmeasured and ungated (PR-specific)
repalloc_huge growth holds the old and new buffers live at once, so Case 2's final doubling
from capacity 128000 to 256000 at dim 2000 puts old 1.02 GB plus new 2.05 GB, about 3 GB,
resident during the copy; plain palloc/repalloc capped this before, and PR #50 is what
enables it. The admission gate runs only after the scan, so it does not bound this scan-phase
transient. B6 could not measure whether the transient exceeds the final build peak, because
backend_peak_rss covers the whole statement and masks any scan-phase spike. Poll VmHWM during
the scan phase (before the build phase) before treating the gate as a real memory bound.
2. Capacity-pricing overshoot is real and common (quantified by B6)
The gate prices rawBuffer at buffer capacity, not actual rows. B6 measured up to 2.02x
over-charge with margin at the worst unanalyzed row (LeanVec, dim 1536: predicted 4897.5 MiB
vs measured 2423.8 MiB), and it fires on ordinary ANALYZE sampling variance, not just
unanalyzed tables: at dim 128 / 500000 rows, reltuples came back 499,998 (two short), crossing
the reltuples < rows threshold in SvsVectorBufferInit and doubling capacity to 999,996 (~2x),
called "close to a coin flip" at the row counts where doubling matters. A never-ANALYZEd
database, or one whose ANALYZE lands a hair low, pays ~2x its admission-gate budget. Changing
capacity->row-count pricing is out of PR #50's scope, but PR #50 is what makes the ~2x buffer
physically allocatable, so the two should be reviewed together.
3. Test-coverage asymmetry
Only 1 of 6 converted sites is tested: the vector float buffer. The other five are
ItemPointerData (6 B/row -> ~178M rows) or size_t id (8 B/row -> ~134M rows) and ship
untested because they cannot be triggered at a practical row count. State in the test header
or PR body that the tidMapping/ids conversions (vamanabuild.c:75, 360, 736, 1087, 1134,
vamanacache.c:212, vamanascan.c:134) are unverified and justified only by large-table
correctness and consistency.
4. Case 2 cost
test/t/38_build_large_vector_buffer.pl Case 2 inserts 128,001 rows @ dim 2000 (~1 GB) and
fills a ~1 GB resident buffer (transient ~3 GB during the final doubling, per gap 1) before
the gate refuses. Confirm it does not push TAP wall time or CI memory past limits.
test/t/38_build_large_vector_buffer.pl exercises only the vector buffer conversion (SvsVectorBufferInit/Append), the one row-scaled site reachable at a testable row count. The fix's other five converted sites, the TID buffer/mapping at four call sites and the id array in VamanaRunSVSBuild, only cross MaxAllocSize at 130 million or more rows and are not exercised by any test. - State in the test's header comment that those five sites are verified only by code reading and by sharing this file's MemoryContextAllocHuge/repalloc_huge pattern, not by an observed failure or a passing test at their own scale Signed-off-by: Matt Welch <matt.welch@intel.com>
|
Thanks for the review, addressing all four points. Test-coverage asymmetry. Fair — the PR read as if both test cases covered "the fix" broadly when they only cover one of six converted sites. Added an explicit disclosure to test/t/38's header comment and to the PR description: only the vector buffer (SvsVectorBufferInit/Append) is exercised by a test. The other five (tidBuffer/tidMapping at vamanabuild.c:75,736,1087,1134 and vamanacache.c:212 and vamanascan.c:134, plus the ids array at vamanabuild.c:360) are unverified beyond code reading and sharing the identical MemoryContextAllocHuge/repalloc_huge pattern — they only cross MaxAllocSize at 130M+ rows, which isn't practical to set up in a test. Scope beyond the reported repro. Intentional, confirmed. The task was scoped to grep both build paths and anything they call for the same bug class, on the reasoning that one invariant (no row-scaled allocation on this code path is capped at 1 GiB) is easier to verify at a glance than six separately-justified exceptions. Made that explicit in the PR description. Case 2 cost. You're right to flag it, but I think there's a mix-up worth catching: Case 2 itself (dim 2000, gate-refused) runs in ~6 seconds total, every time I've measured it. The ~77s figure belongs to a separate, manual, one-off reproduction I ran against a different table (dim 1536) under this worktree's own generous memory budget, which lets a full build run to completion instead of being gate-refused. I described it right next to the Case 2 discussion in the PR's Testing Notes, which reads like it's the same thing. Corrected the PR description to keep these clearly separate. Pre-existing overshoot. Agreed this is out of scope for this PR — reproduced and measured it independently (real backend RSS, not just the logged estimate: peak ~1.09 GiB against a 1024MB ceiling, 10ms-resolution polling), and filed it as intel-innersource/applications.databases.postgresql.pgvector-optimizations#187, tracked as MEM-08 in the backlog. Writeup at pgv-svs-dev-scripts/docs/memory-scan-time-overshoot/report.md. Updated PR description and test file pushed. |
2 participants
Description
The build path collects every vector of a table into one growable buffer before handing it to the index builder. That buffer allocated with plain memory-allocation calls, which reject any single request of 1 GiB or more. Any table whose vectors reached that size, for example roughly 175,000 rows at 1536 dimensions, well inside the default build-memory budget, failed
CREATE INDEX ... USING vamanaoutright, even though the same build succeeded on an earlier version of this code.This PR restores the huge-allocation path for that buffer, and, as a deliberate scope decision beyond the reported failure, applies the same fix to every other row-scaled allocation found on the same build and load code: the TID buffer collected during a build or a from-heap rebuild, the one-shot id array handed to the index builder, the TID map copied into the background worker's cache after a build or checkpoint load, and the TID map restored when a checkpointed index is loaded from disk. Those five conversions are not reachable at any table size anyone would actually build (they only matter at 130 million or more rows); they are included so that one invariant holds everywhere on this code path, not because any of them was observed to fail. See Testing Notes for what is and is not covered by a test.
A regression test covers both ways the vector buffer's size crosses the 1 GiB limit: the initial allocation, sized from the table's row-count estimate, and the buffer's doubling growth mid-scan on a table without accurate statistics. The growth case is proven by the build-memory admission gate's own refusal, which only runs after a full table scan completes; its refusal naming the memory budget, rather than an allocation-size error, is itself the proof that the scan succeeded.
Related Issues
None.
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 changedNo
docs/update is included: this is an internal allocation implementation detail with no user-facing behavior or architecture change, and the invariant it depends on (this buffer routinely exceeds 1 GiB and must use the huge-allocation API) is documented inline in a header comment instead.Testing Notes
makecompletes with no errors and no warnings from any file this PR touches. It is left unchecked above only because the build emits one pre-existing warning from a vendored header (ISO C90 forbids mixed declarations and codeinsvs/c/svs_c.h, included bysrc/svs_wrapper.c), unrelated to this change and present onmainas well.Test coverage is asymmetric, deliberately. A new TAP file,
test/t/38_build_large_vector_buffer.pl, covers only the vector buffer (SvsVectorBufferInit/Append), the one conversion reachable at a testable row count:CREATE INDEXsucceeds and the resulting index serves a nearest-neighbour query. Runs in under 2 seconds.The other five converted sites (the TID buffer/mapping at four call sites, and the id array) are not exercised by this or any other test. They only cross 1 GiB at 130 million or more rows, which is not practical to set up in a test. Their correctness rests on reading the code and on sharing the identical
MemoryContextAllocHuge/repalloc_hugepattern this test does verify at the one site that is testable, not on an observed failure or a passing test at their own scale.Verified the test fails on the pre-fix code (both cases fail with
invalid memory alloc request size, at the exact byte counts the arithmetic predicts) and passes once the fix is applied, by reverting the source change with a saved patch, rebuilding, and confirming the failure, then reapplying and confirming success.Also re-ran three real reproducers directly, beyond the new automated test. The second of these is a separate, one-off, manual reproduction, not part of the automated test suite, and its 77-second wall time belongs only to it, not to Case 2 above:
CREATE INDEXnow succeeds and a nearest-neighbour query returns results.CREATE INDEXwas allowed to run to completion rather than being refused, and finished a full build in about 77 seconds, with a nearest-neighbour query returning results afterward.CREATE INDEXnow completes in about 110 seconds, matching the build time measured on an earlier commit before this regression was introduced, with a peak backend memory usage of about 3.2 GiB, and the resulting index serves a nearest-neighbour query. The index was dropped afterward, leaving the table unchanged.Full test suite run on this branch: 38 TAP files and 1092 TAP tests all passed (one file skips by design, requiring an opt-in flag for an unrelated ~8 GB test), and all 7 SQL regression test files passed.
Audited the pre-existing
-Wclobberedcompiler warnings this build produces (eight, across five files) and confirmed the set is unchanged before and after this fix; the warnings on the two buffers this PR touches are unaffected because the reasoning that made them safe (each variable is only read, never reassigned, inside its guarded region) does not depend on which allocator populated it.