Skip to content

CPU management for index build - #16

Open
klmckeig wants to merge 1 commit into
mainfrom
build-cpu-management
Open

CPU management for index build#16
klmckeig wants to merge 1 commit into
mainfrom
build-cpu-management

Conversation

@klmckeig

@klmckeig klmckeig commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

Description

Index builds (CREATE INDEX and 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 ParallelContext worker 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 in pg_stat_activity, since core fixes backend_type to "parallel worker" for any ParallelContext worker 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-database maintenance_num_threads = 0) now resolves to serial (1 thread), matching core's own meaning of that setting, instead of falling back to nproc-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 into vamana_databases.c, since they're used by both the launcher's bulk scan and a new single-database lookup, and reading vamana_databases is that file's existing responsibility.

Related Issues

Type of Change

  • Bug fix
  • New feature
  • Refactor / code cleanup
  • Documentation update
  • Test addition or update
  • Build / CI change

Pre-Merge Checklist

Build

  • make completes without errors or warnings
  • make install completes successfully

Tests

  • If this PR introduces no new behavior: existing regression tests (make installcheck) and TAP tests (test/t/) pass with no failures
  • If this PR introduces new behavior: test cases covering it were added to test/sql/ and/or test/t/
  • If this PR adds a standalone unit-test module under test/modules/: it builds and passes (make -C test/modules/<module> installcheck)

Documentation

  • Relevant docs under docs/ updated if architecture or usage changed

Testing 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.

Signed-off-by: klmckeig <kelly.l.mckeighan@intel.com>
@klmckeig
klmckeig marked this pull request as ready for review September 2, 2026 00:43
@klmckeig
klmckeig requested a review from a team September 2, 2026 00:43

@matt-welch matt-welch left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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_workers for build
operations and svs.search_num_threads for 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants