CPU management for index build - #16
Conversation
Signed-off-by: klmckeig <kelly.l.mckeighan@intel.com>
matt-welch
left a comment
There was a problem hiding this comment.
PR16 Review — CPU Management for Index Build
Verdict: Request Changes (two items below; both are small fixes)
Build clean, all tests pass (regression 6/6, TAP 19/19, both standalone unit modules). The grant/request shmem protocol, the parked-worker accounting, SvsComputeCpuGrants, and PG_FINALLY in SvsRunGovernedBuild all look correct. The 0 = serial fix matches core semantics and the new TAP test covers it at the round-trip level.
Two things need fixing before merge.
Must-change
SvsReleaseBuildRequestSlot writes status before clearing the payload fields
src/vamanaworker.c, line 1850
void
SvsReleaseBuildRequestSlot(SvsBuildRequest *req)
{
pg_atomic_write_u32(&req->status, SVS_BUILD_REQUEST_PENDING); // line 1850 -- status first
req->requested = 0;
req->granted = 0;
pg_atomic_write_u32(&req->pid, 0);
}The struct comment at vamanaworker.h:165 says the protocol is: write plain fields, then pg_write_barrier, then publish via status. Both SvsClaimBuildRequestSlot (line 1771) and PublishBuildGrant in the launcher follow that ordering. This function inverts it.
After line 1850, status is PENDING but requested is still non-zero. The launcher's AppendPendingBuildRequests reads pid (non-zero), sees status == PENDING, pairs a pg_read_barrier, and reads requested. If it gets in during that window it reads the stale pre-zeroed value and tries to grant threads for a build that already finished. Today that phantom grant is harmless because PublishBuildGrant finds no matching PID after pid is cleared. But this violates the only memory-ordering invariant documented for the struct and is a trap for whoever adds meaning to the PENDING state next.
Fix:
void
SvsReleaseBuildRequestSlot(SvsBuildRequest *req)
{
req->requested = 0;
req->granted = 0;
pg_write_barrier();
pg_atomic_write_u32(&req->status, SVS_BUILD_REQUEST_PENDING);
pg_atomic_write_u32(&req->pid, 0);
}ARCHITECTURE.md §4.1 describes the old build thread path
docs/dev/ARCHITECTURE.md, line 122
Current text:
PostgreSQL passes thread count via
max_parallel_maintenance_workersfor build
operations andsvs.search_num_threadsfor search operations
After this PR, max_parallel_maintenance_workers is only the fallback when maintenance_num_threads IS NULL in vamana_databases. The actual build path goes through SvsRunGovernedBuild, a request/grant round trip with the launcher, and parked ParallelContext workers. The sentence implies a direct pass-through that no longer exists.
Non-blocking notes
SVS builder handles leak on ERROR during grant wait
src/vamanabuild.c, around line 418
builder, storage, and algorithm are allocated before SvsRunGovernedBuild. If SvsWaitForBuildGrant raises an ERROR (user cancel, timeout, postmaster death), the longjmp skips the cleanup label and those handles leak. This pattern existed before this PR but the PR adds a new ERROR exit path via the grant wait that wasn't there before. Not asking for a fix here, just noting it so it doesn't get lost.
No test for the timeout path in 19_build_thread_grant.pl
The test covers the happy path and sequential slot exhaustion but not the case where the launcher doesn't respond within vamana_worker_timeout_ms. Not a blocker, but that code path returns a distinct error and would be worth a test eventually.
Description
Index builds (
CREATE INDEXand rebuild-from-table) now go through CPU-governed thread accounting instead of setting a thread count directly.Before building, the backend asks the running launcher for a build-thread grant, then reserves that many parked
ParallelContextworker processes so PostgreSQL's own parallel-worker pool accounting reflects the build's thread usage. The actual SVS graph build runs with that granted thread count. Both the grant and the reserved worker pool are released once the build finishes, whether it succeeds, errors, or times out waiting for a grant.Each parked worker reports itself via
application_name(e.g.vamana: db=<name> build slot <n>/<total> (requested <r>, granted <g>)), visible inpg_stat_activity, since core fixesbackend_typeto"parallel worker"for anyParallelContextworker and that field can't be customized.If no grant arrives before
svs.worker_timeout_ms, or every pending-build slot for the database is already in use, the build fails with an error naming the launcher; no silent fallback to an ungoverned thread count.max_parallel_maintenance_workers = 0(or unset per-databasemaintenance_num_threads = 0) now resolves to serial (1 thread), matching core's own meaning of that setting, instead of falling back tonproc-1.The launcher reclaims a build-thread slot whose owning backend is no longer alive, as a structural backstop for the small, shared per-database request pool.
Two small helpers (
SvsDatabasesQualifiedName,SvsResolveNullableThreadCount) moved from the launcher intovamana_databases.c, since they're used by both the launcher's bulk scan and a new single-database lookup, and readingvamana_databasesis that file's existing responsibility.Related Issues
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
Full regression suite (6/6) and full TAP suite (19/19 files, 533 tests) pass. Both standalone
test/modules/unit-test modules (svs_cpu_budget_test,svs_parallel_build_test) build and pass independently of the main extension.