executor: native CREATE INDEX CONCURRENTLY with fail-closed invalid-index recovery

[Kiran01bm]

Summary

First Phase 3 native-path operation (PLAT-38441): pkg/executor.BuildIndexConcurrently runs one named, schema-qualified CREATE INDEX CONCURRENTLY on a dedicated non-transactional session and owns the failure mode the statement is famous for — the invalid catalog entry a failed build leaves behind. Every invalid index is surfaced as a typed, fail-closed outcome naming the operator's explicit recovery; the executor never drops an index itself.

What

• BuildIndexConcurrently with its own wait policy: lock_timeout = 0 (a per-lock timeout would cancel the build's snapshot waits and create the very invalid index this executor exists to prevent), one finite statement_timeout budget bounded to PostgreSQL's 32-bit millisecond ceiling.
• Typed admission before any DB access: exactly one statement, CREATE INDEX ... CONCURRENTLY, named index, schema-qualified table, no IF NOT EXISTS (a name-only no-op can't prove the existing index is valid or even the requested one). pkg/statement.Op gains IfNotExists.
• Fail-closed failure verdict: prove the build's own backend stopped (only positive idle states count; NULL/disabled/unknown refuse), then read one atomic catalog snapshot — target identity (pinned OID) and index validity in a single SELECT sharing one MVCC snapshot — and report *InvalidIndexError carrying the original build failure and the recovery statement. A provably clean catalog returns the build failure alone.
• All proof queries are pg_catalog-qualified (relations, functions, OPERATOR(pg_catalog.=)): a search_path listing a user schema before pg_catalog cannot shadow the catalogs into a false clean. A regression test builds against a hostile search_path with impostor catalogs and was verified to fail without the qualification.
• Pre-existing invalid index under the requested name anywhere in the target schema refuses before execution — it may be another actor's in-progress build, and after a failure of our own the verdict could never tell it apart from our own debris.

Why

A failed CREATE INDEX CONCURRENTLY leaves an invalid index that every write still maintains but no query uses. PostgreSQL drops indexes by name, not identity, so automatic cleanup could destroy another actor's same-name index registered in the same window — the engine's one-migration-per-table lease (planned invariant LK-1) would close that, and until it exists recovery stays with the operator. Recovery SQL is returned only as data in the typed error.

Failure flow

CREATE INDEX CONCURRENTLY fails
        │
        ▼
┌─────────────────────────┐  unprovable (NULL/disabled/
│ prove build backend      │  unknown state, ctx expiry)
│ stopped (pg_stat_        │─────────────────────────────┐
│ activity, detached ctx)  │                              │
└───────────┬─────────────┘                              │
            │ stopped                                     ▼
            ▼                                   *InvalidIndexError
┌─────────────────────────┐  target dropped/     (fail closed, typed
│ one atomic catalog       │  replaced, or        recovery for the
│ snapshot: pinned OID +   │  inspection error    operator; never
│ index validity           │─────────────────────▶ DROP INDEX)
└───────────┬─────────────┘                              ▲
            │ clean                 invalid entry found   │
            ▼                    ─────────────────────────┘
   original build error
   (retry can start now)

[chatgpt-codex-connector]

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

[aparajon]

🤖 Review requested by Armand and performed by his agent — same two lenses used across this stack (#14, #13, #9, #8, #7, #6, #2): pg-sprite as an OSS-first, best-in-class Postgres DDL tool, and pg-sprite as a clean integration target for an orchestrator. Reviewed at head ee588e0. An adversarial correctness pass is posted separately.

This is the best PR in either stack, and the reasoning is better than the code — which is the right way round for a safety-critical package. Four things are worth naming because they are not obvious and someone will eventually want to "simplify" them:

• lock_timeout = 0 is the safe setting here, and the comment explains why. A concurrent build's waits for other transactions' snapshots are lock waits by implementation, so a session lock_timeout cancels a healthy build mid-wait — and that cancellation is precisely what manufactures the invalid index this executor exists to prevent. Disabling the per-lock timeout and bounding the whole statement instead inverts the usual advice, and the note about SHARE UPDATE EXCLUSIVE not blocking reads or writes queued behind it is the argument that makes it safe. Without that comment this is the first line a reviewer "fixes".
• Refusing IF NOT EXISTS. The clause checks only the name, so it succeeds as a no-op over an invalid or entirely unrelated index — and the executor would report success over something it cannot vouch for. Most tools would treat IF NOT EXISTS as an obvious idempotency win.
• maxOverallBudget clamped to math.MaxInt32 ms, because statement_timeout is a signed 32-bit millisecond count and a larger value doesn't fail loudly — it leaves the statement unbounded. That is exactly the class of detail that only shows up in production, at 3am, on the one build someone gave a 30-day budget.
• Proof queries qualified down to OPERATOR(pg_catalog.=). Qualifying the relations is common; qualifying the operator against a user-defined = in an earlier search_path schema is a level of paranoia I have not seen in a comparable tool, and there is a regression test for it.

The findings below are about the seam between this executor and the humans and systems that will use it — not the catalog logic, which holds up.

OSS lens

1. The one outcome that needs a human has no runbook, and the advice it does give is a single unconditional sentence. InvalidIndexError.Error() always renders the same shape — "may be invalid … recover with DROP INDEX CONCURRENTLY x" — across three states that call for different actions: proven debris from this build, an unproven verdict, and (reproduced in the adversarial comment) a healthy index that another actor is still building or has already finished. The type distinguishes them; the prose does not. The whole safety property of this executor is "never drop by name, because a name can't prove ownership" — and then the error hands the operator a name-based drop. This deserves a docs/ runbook the error points at: how to tell debris from an in-flight build (pg_index.indisvalid plus pg_stat_activity for a live CREATE INDEX CONCURRENTLY on that name), what to check before dropping, and what to do when the answer is "wait". Everything else in this repo documents itself; this path currently exists only inside a Go error string.

2. CREATE INDEX CONCURRENTLY is the single most-requested Postgres online-DDL primitive, and it isn't reachable from the CLI. internal/cli/ got a four-line comment touch in this PR. Nothing wrong with landing the engine first, but a reader arriving at the repo from the README cannot run the thing the PR title advertises. Worth stating the intended sequencing in docs/low-level-design.md's "Next step" (which this PR already rewrites) — whether the front door is migrate losing --dry-run, or a narrower verb — so the gap reads as planned rather than missing.

3. ErrUnqualifiedTable is the right refusal in the wrong place for a human. The argument is sound: the verdict re-resolves the table on another session where search_path can't be proven identical, so an unqualified name could become a false clean. But every human and nearly every ORM writes CREATE INDEX CONCURRENTLY i ON t (c), and their first experience of the tool will be a refusal of a statement PostgreSQL itself accepts. The library-level refusal should stay; the CLI should resolve the name once against the session's search_path and re-emit the qualified statement, telling the user what it resolved to. That keeps the proof intact and moves the burden off the user.

4. The commit message carries an internal tracker ID. In a public repo that's a dangling reference — an outside contributor reading git log to understand why Phase 3 is ordered this way hits an identifier they can't resolve. The rest of the message is genuinely good release-note material; the ID is the only part that doesn't travel.

Integration lens

5. The pool contract is load-bearing and unenforced. The doc comment says the pool "needs a second connection free for the post-failure verdict", and that a fully busy pool resolves indeterminate. That's honest, but it makes a correctness-relevant precondition discoverable only by reading Go doc comments — and the adversarial comment shows what violating it costs (a false-positive recovery instruction naming a valid, in-use index, after a wait as long as the entire build budget). Two cheap fixes, and I'd take both: acquire the verdict connection up front alongside the build session, so the resource is reserved rather than hoped for; and reject at admission when pool.Config().MaxConns < 2, the same way ConcurrentBudget.validate() rejects an unbounded budget. An unusable verdict is as much a "bounded by construction" concern as an unbounded statement.

6. No progress surface, on the one operation where progress is the entire operator experience. A concurrent build on a large table runs for hours, and an orchestrator's job during those hours is to render "where is it" onto a PR. PostgreSQL 12+ exposes pg_stat_progress_create_index (phase, blocks_done/blocks_total, tuples_done/tuples_total), and this executor already holds the one thing needed to query it — the build's backend PID, captured for the ownership proof. A Progress func(IndexBuildProgress) callback, or an exported poll-by-PID helper, turns this from a blocking call into something drivable. Without it any integration has to rediscover the PID itself, which means duplicating the ownership reasoning outside the package.

7. IndexBuildReport carries less than the plan and lint reports it will sit next to. Two strings, no duration, no server version, no index OID, no indication of which phase completed. Add pkg/plan: one versioned dry-run report for both front doors #8 argued that a report an orchestrator stores or forwards has to be self-describing; the same argument applies here and more strongly, because this report is evidence that a change was executed. The OID in particular is the identity a later reconciliation would want, and catalogVerdict already reads OIDs.

8. The typed outcomes are excellent for Go callers and invisible to everyone else. ErrPreexistingInvalidIndex, ErrBuildLeftInvalidIndex, ErrTargetIdentityChanged are the vocabulary a consumer must branch on, and today they're reachable only through errors.Is. pkg/lint established a stable string Code for exactly this reason; giving each executor outcome the same treatment keeps one vocabulary across the contracts rather than two shapes for the same idea.

Verified solid

The catalog reasoning holds under attack. The single-statement snapshot genuinely does keep the identity proof and the index inspection from straddling a concurrent replacement, and the deliberate absence of to_regclass inside it (it reads through the relation cache's separately refreshed snapshot) is a real distinction, correctly drawn. awaitBackendStopped fails closed on every state that isn't positive evidence — I confirmed against a live server that track_activities = off reports "disabled", which falls through to backendUnprovable and refuses immediately rather than waiting for something that will never become provable. The executor never drops: I could not construct a path where it removes an index, and the tests cover the two that matter (a valid index under the requested name, and debris on an unrelated table). Session hygiene is right — SET lock_timeout = 0 cannot leak back into the pool, because an unproven RESET hijacks and closes the connection rather than releasing it, and the same is done when the initial SET batch partially applies. The detached verdict context (context.WithoutCancel) is necessary and correctly scoped: cancellation may be exactly why the build failed, so the verdict cannot inherit it. ConcurrentBudget rejects both ends of the range with reasons. CGO_ENABLED=0 go build ./... passes at this head.

This review was generated by Claude Code (claude-fable-5).

[aparajon]

🤖 Adversarial correctness review requested by Armand and performed by his agent — separate from the two-lens pass. Method: take the executor's own safety claim ("it never drops an index, because a name-based drop cannot prove whose index it destroys") and ask whether the advice it gives an operator is held to the same standard, then verify against a real PostgreSQL 16 at head ee588e0. Findings 1–4 are reproduced live; the reproduction test is in the collapsed block at the end.

The catalog logic held up under everything I threw at it. What did not hold is the layer above it: the executor refuses to drop by name and then instructs a human to do exactly that, in states where it has not established that the named index is debris.

Findings, most severe first

1. InvalidIndexError tells the operator to DROP INDEX CONCURRENTLY a specific index in states where that index is provably healthy. The message shape is unconditional:

return fmt.Sprintf("index %s.%s may be invalid and needs explicit recovery; verify pg_index.indisvalid and recover with DROP INDEX CONCURRENTLY %s: %v", …)

Two live reproductions where following it destroys a good index:

A concurrent healthy build. A second actor — another pod, a retry, a re-driven job — asks for an index another session is still building. During the build the entry is legitimately indisvalid = false, the pre-build check sees it, and the second actor is told:

index t_41694_2.idx_shared may be invalid and needs explicit recovery; verify pg_index.indisvalid
and recover with DROP INDEX CONCURRENTLY "t_41694_2"."idx_shared":
an invalid index with this name already exists in the target schema

The first build then completed normally — exists=true valid=true. The advised recovery, run at the moment it was issued, would have destroyed a healthy in-flight build; run a minute later, a finished valid index. ErrPreexistingInvalidIndex's own comment says "it may be another actor's build still in progress", so the code knows — the message doesn't.

An indeterminate verdict. With no free pool connection the verdict can't run, and the executor fails closed by naming a drop over an index it never inspected:

budget=10s  elapsed=15.5s
err=index t_36593_1.idx_taken may be invalid and needs explicit recovery; verify pg_index.indisvalid
    and recover with DROP INDEX CONCURRENTLY "t_36593_1"."idx_taken":
    inspect build backend 77: context deadline exceeded
catalog afterwards: exists=true valid=true

Here the build failed in phase 1 on a name collision and created nothing at all; idx_taken is the pre-existing valid index it collided with, in use by queries. Failing closed on an unprovable verdict is right. Naming a DROP for an index the executor has explicitly just said it cannot inspect is not the same thing, and it is the one instruction that can't be walked back.

The distinction the message needs is already in the type — Cleanup is a typed sentinel, and ErrBuildLeftInvalidIndex is the only one of the three that means "this index is debris". ErrPreexistingInvalidIndex should say "an index by this name is invalid; it may be another actor's build in progress — check pg_stat_activity before doing anything", and an indeterminate verdict should say "the catalog could not be inspected; determine indisvalid yourself" with no statement to copy-paste. Same fail-closed posture, without the executor's own ownership argument being abandoned at the last step.

2. The verdict's deadline scales with the build budget, so a build that fails in milliseconds can block the caller for hours. The detached verdict context is b.Overall + sessionCleanupTimeout, and awaitBackendStopped polls through the pool — which needs a connection the build session isn't holding. When none is free, QueryRow blocks on acquire until that deadline expires. In the run above, a 10-second budget produced a 15.5-second wait for a failure that happened immediately, and the relationship is Overall + 5s regardless of how fast the failure was. Index-build budgets are realistically hours, so the same shape is a multi-hour hang on a saturated pool — for a build that already failed.

The verdict's work is two short catalog reads plus a 50ms poll loop; it has no reason to inherit the build's budget. A short independent bound (seconds, not the build budget) would reach the same fail-closed answer immediately instead of after the full budget. Note the pool doesn't have to be misconfigured for this: pkg/dbconn sets no MaxConns default, so pgx's applies (the greater of 4 and the CPU count) and four concurrent builds are enough to starve each other's verdicts on a small host.

3. An operator's pg_cancel_backend is reported as budget exhaustion. asConcurrentBudgetError maps SQLSTATE 57014 to BudgetError{Cause: CauseStatement, Budget: b.Overall} — but 57014 is query_canceled generally, not statement_timeout specifically. Cancelling a healthy build from another session, 1.8 seconds into a 5-minute budget:

typed as *BudgetError: cause=statement budget budget=5m0s

BudgetError's own documentation says it "reports that an execution attempt exceeded one of its budgets" and is "a refusal input, not an operational failure" — so a consumer branching on it concludes the change is too slow for the native path and escalates to a heavier strategy, when the truth is that a human deliberately stopped it and probably wants it left alone. (55P03 in the optimistic path has no such ambiguity; this is specific to 57014, and it matters more here because a long-running concurrent build is exactly the operation an operator is expected to cancel by hand.) The elapsed time against the budget is the corroborating signal already in hand: a statement that ran for 1.8s cannot have exhausted a 5m budget. Alternatively, widen the type — CauseStatement could become "cancelled by the budget or by an operator" and say so, which is at least true.

4. The success path is the only path that trusts the absence of an error. if buildErr == nil { return rep, nil } returns without looking at the catalog, while every failure gets a full verdict. The asymmetry is defensible today — on PostgreSQL 16 the shapes that leave an invalid index on success are unreachable through this admission gate (CREATE INDEX CONCURRENTLY on a partitioned table is refused with 0A000; CREATE INDEX ON ONLY parent does leave indisvalid = false on success, and I confirmed both, but the latter isn't concurrent and can't be admitted here). It's worth closing anyway, because the reasoning that makes it safe is a fact about a PostgreSQL version and this executor supports a five-major range whose newest members are the ones most likely to relax that restriction. One indisvalid read on success — the query already exists in invalidIndexByName — makes IndexBuildReport mean "I verified the index is valid" rather than "the server didn't complain", which is the standard the rest of this file holds itself to.

5. Two small things in the same file. errTableNotFound says it exists so "the post-failure verdict maps it to ErrTargetIdentityChanged" — but resolveTarget is only ever called before the build, and the verdict does its own inline lookup, so nothing ever tests for this sentinel. Today a missing table returns an untyped wrapped error a caller can only string-match. Separately, classifyBackendState lists "starting" as a running state; that value doesn't exist in PostgreSQL's pg_stat_activity.state vocabulary (NULL, active, idle, idle in transaction, idle in transaction (aborted), fastpath function call, disabled), so the arm is dead — harmless, but it reads as though the list was recalled rather than derived, which undercuts the fail-closed default: that is doing the real work.

Probed and held

The pg_catalog qualification defence is real, not decorative — I could not shadow the proofs. awaitBackendStopped genuinely refuses rather than waits when it has no visibility: with track_activities = off the state reads "disabled", falls through to backendUnprovable, and the build's leftover is reported immediately instead of after a pointless poll to the deadline. The single-statement snapshot does what its comment claims: the OID pin and the index inspection share one MVCC snapshot, and a table swapped under the same name yields ErrTargetIdentityChanged rather than a false clean. The executor never drops anything — I looked for a path and there isn't one, including the case where the colliding index is valid and the case where debris sits on an unrelated table. Session hygiene holds: lock_timeout = 0 cannot escape back into the pool, because an unproven RESET hijacks and closes rather than releasing, and a partially-applied SET batch is handled the same way. Caller cancellation after the catalog entry exists correctly produces ErrBuildLeftInvalidIndex with the original context.Canceled riding inside, and cancellation before the entry resolves clean with the backend stop proven first — both already covered by the PR's own tests, and both reproduce. ConcurrentBudget rejects sub-millisecond and over-MaxInt32 budgets with reasons. CGO_ENABLED=0 go build ./... passes at this head.

Reproduction tests

pkg/executor/adv15_integration_test.go — findings 1–4 against PostgreSQL 16

package executor_test

import (
	"context"
	"errors"
	"fmt"
	"testing"
	"time"

	"github.com/jackc/pgx/v5"
	"github.com/jackc/pgx/v5/pgxpool"
	"github.com/stretchr/testify/require"

	"github.com/block/pg-sprite/internal/testutil"
	"github.com/block/pg-sprite/pkg/dbconn"
	"github.com/block/pg-sprite/pkg/executor"
)

func advPool(t *testing.T, maxConns int32) (*pgxpool.Pool, string) {
	t.Helper()
	pool, err := dbconn.NewPool(t.Context(), dbconn.Config{URL: testutil.StartPostgres(t), MaxConns: maxConns})
	require.NoError(t, err)
	t.Cleanup(pool.Close)
	return pool, testutil.NewSchema(t, pool)
}

// Finding 3: an operator cancels a healthy build. What does the caller learn?
func TestAdvOperatorCancelIsReportedAsBudgetExhaustion(t *testing.T) {
	pool, schema := advPool(t, 8)
	_, err := pool.Exec(t.Context(), fmt.Sprintf(
		"CREATE TABLE %s.t (id int PRIMARY KEY, c int)", schema))
	require.NoError(t, err)

	// A repeatable-read snapshot blocks the build in its wait phase, so
	// there is time to cancel it from outside.
	blocker, err := pool.BeginTx(t.Context(), pgx.TxOptions{IsoLevel: pgx.RepeatableRead})
	require.NoError(t, err)
	var n int
	require.NoError(t, blocker.QueryRow(t.Context(),
		fmt.Sprintf("SELECT count(*) FROM %s.t", schema)).Scan(&n))
	t.Cleanup(func() { _ = blocker.Rollback(context.WithoutCancel(t.Context())) })

	done := make(chan error, 1)
	go func() {
		_, err := executor.BuildIndexConcurrently(t.Context(), pool,
			fmt.Sprintf("CREATE INDEX CONCURRENTLY idx_op_cancel ON %s.t (c)", schema),
			executor.ConcurrentBudget{Overall: 5 * time.Minute})
		done <- err
	}()

	require.Eventually(t, func() bool {
		var waiting int
		err := pool.QueryRow(t.Context(),
			`SELECT count(*) FROM pg_stat_activity
			  WHERE query LIKE 'CREATE INDEX CONCURRENTLY idx_op_cancel%' AND state = 'active'`).Scan(&waiting)
		require.NoError(t, err)
		return waiting == 1
	}, 30*time.Second, 50*time.Millisecond)

	_, err = pool.Exec(t.Context(),
		`SELECT pg_cancel_backend(pid) FROM pg_stat_activity
		  WHERE query LIKE 'CREATE INDEX CONCURRENTLY idx_op_cancel%' AND state = 'active'`)
	require.NoError(t, err)

	select {
	case err := <-done:
		t.Logf("caller sees: %v", err)
		var budgetErr *executor.BudgetError
		if errors.As(err, &budgetErr) {
			t.Logf("typed as *BudgetError: cause=%s budget=%s", budgetErr.Cause, budgetErr.Budget)
		}
	case <-time.After(90 * time.Second):
		t.Fatal("build did not return")
	}
}

// Finding 1a: a second actor asks for an index another session is still
// building. What is it told to do, and what would that have destroyed?
func TestAdvConcurrentHealthyBuildIsCalledDebris(t *testing.T) {
	pool, schema := advPool(t, 8)
	_, err := pool.Exec(t.Context(), fmt.Sprintf(
		"CREATE TABLE %s.t (id int PRIMARY KEY, c int)", schema))
	require.NoError(t, err)

	blocker, err := pool.BeginTx(t.Context(), pgx.TxOptions{IsoLevel: pgx.RepeatableRead})
	require.NoError(t, err)
	var n int
	require.NoError(t, blocker.QueryRow(t.Context(),
		fmt.Sprintf("SELECT count(*) FROM %s.t", schema)).Scan(&n))

	firstDone := make(chan error, 1)
	go func() {
		_, err := executor.BuildIndexConcurrently(t.Context(), pool,
			fmt.Sprintf("CREATE INDEX CONCURRENTLY idx_shared ON %s.t (c)", schema),
			executor.ConcurrentBudget{Overall: 5 * time.Minute})
		firstDone <- err
	}()

	require.Eventually(t, func() bool {
		exists, valid := indexState(t, pool, schema, "idx_shared")
		return exists && !valid
	}, 30*time.Second, 50*time.Millisecond)

	_, err = executor.BuildIndexConcurrently(t.Context(), pool,
		fmt.Sprintf("CREATE INDEX CONCURRENTLY idx_shared ON %s.t (c)", schema),
		executor.ConcurrentBudget{Overall: time.Minute})
	t.Logf("second actor sees: %v", err)
	require.ErrorIs(t, err, executor.ErrPreexistingInvalidIndex)

	require.NoError(t, blocker.Rollback(context.WithoutCancel(t.Context())))
	select {
	case err := <-firstDone:
		t.Logf("first (healthy) build result: %v", err)
	case <-time.After(60 * time.Second):
		t.Fatal("first build did not return")
	}
	exists, valid := indexState(t, pool, schema, "idx_shared")
	t.Logf("after the first build completed: exists=%v valid=%v", exists, valid)
}

// Findings 1b and 2: the verdict needs a second connection. How long does a
// build that failed in milliseconds keep the caller waiting, and what is it
// told about the valid index it collided with?
func TestAdvSaturatedPoolVerdictCostsTheWholeBudget(t *testing.T) {
	pool, schema := advPool(t, 1)
	_, err := pool.Exec(t.Context(), fmt.Sprintf(
		"CREATE TABLE %s.t (id int PRIMARY KEY, c int); CREATE INDEX idx_taken ON %s.t (c)", schema, schema))
	require.NoError(t, err)

	budget := executor.ConcurrentBudget{Overall: 10 * time.Second}
	start := time.Now()
	_, err = executor.BuildIndexConcurrently(t.Context(), pool,
		fmt.Sprintf("CREATE INDEX CONCURRENTLY idx_taken ON %s.t (id)", schema), budget)
	t.Logf("budget=%s  elapsed=%s  err=%v", budget.Overall, time.Since(start).Round(100*time.Millisecond), err)

	exists, valid := indexState(t, pool, schema, "idx_taken")
	t.Logf("catalog afterwards: exists=%v valid=%v", exists, valid)
}

// Finding 4: is there a CREATE INDEX shape where success leaves an invalid
// entry, and can this executor admit it?
func TestAdvSuccessPathDoesNotVerifyValidity(t *testing.T) {
	pool, schema := advPool(t, 8)
	_, err := pool.Exec(t.Context(), fmt.Sprintf(
		`CREATE TABLE %s.p (id int, c int) PARTITION BY RANGE (id);
		 CREATE TABLE %s.p1 PARTITION OF %s.p FOR VALUES FROM (0) TO (100)`, schema, schema, schema))
	require.NoError(t, err)

	_, err = pool.Exec(t.Context(), fmt.Sprintf(
		"CREATE INDEX CONCURRENTLY idx_part ON %s.p (c)", schema))
	t.Logf("raw CIC on a partitioned table: err=%v", err)

	_, err = pool.Exec(t.Context(), fmt.Sprintf(
		"CREATE INDEX idx_only ON ONLY %s.p (c)", schema))
	t.Logf("CREATE INDEX ON ONLY partitioned parent: err=%v", err)
	exists, valid := indexState(t, pool, schema, "idx_only")
	t.Logf("parent index: exists=%v valid=%v", exists, valid)
}

Observed (PostgreSQL 16.14, indexState reused from the PR's own native_integration_test.go):

--- TestAdvOperatorCancelIsReportedAsBudgetExhaustion
    caller sees: index t_41694_1.idx_op_cancel may be invalid and needs explicit recovery; …
    typed as *BudgetError: cause=statement budget budget=5m0s          # after 1.8s

--- TestAdvConcurrentHealthyBuildIsCalledDebris
    second actor sees: … recover with DROP INDEX CONCURRENTLY "t_41694_2"."idx_shared":
                       an invalid index with this name already exists in the target schema
    first (healthy) build result: <nil>
    after the first build completed: exists=true valid=true

--- TestAdvSaturatedPoolVerdictCostsTheWholeBudget
    budget=10s  elapsed=15.5s
    err=… recover with DROP INDEX CONCURRENTLY "t_36593_1"."idx_taken":
        inspect build backend 77: context deadline exceeded
    catalog afterwards: exists=true valid=true

--- TestAdvSuccessPathDoesNotVerifyValidity
    raw CIC on a partitioned table: err=ERROR: cannot create index on partitioned table "p" concurrently (SQLSTATE 0A000)
    CREATE INDEX ON ONLY partitioned parent: err=<nil>
    parent index: exists=true valid=false

The track_activities = off probe (held — the executor behaves correctly)

// With track_activities off, what does pg_stat_activity.state report, and
// what does the executor do with it?
func TestAdvTrackActivitiesOff(t *testing.T) {
	pool, schema := advPool(t, 8)
	_, err := pool.Exec(t.Context(), fmt.Sprintf(
		"CREATE TABLE %s.t (id int PRIMARY KEY, c int); INSERT INTO %s.t VALUES (1,1),(2,1)", schema, schema))
	require.NoError(t, err)

	_, err = pool.Exec(t.Context(), "ALTER SYSTEM SET track_activities = off")
	require.NoError(t, err)
	_, err = pool.Exec(t.Context(), "SELECT pg_reload_conf()")
	require.NoError(t, err)
	t.Cleanup(func() {
		ctx := context.WithoutCancel(t.Context())
		_, _ = pool.Exec(ctx, "ALTER SYSTEM RESET track_activities")
		_, _ = pool.Exec(ctx, "SELECT pg_reload_conf()")
	})
	require.Eventually(t, func() bool {
		var s *string
		if err := pool.QueryRow(t.Context(),
			"SELECT state FROM pg_stat_activity WHERE pid = pg_backend_pid()").Scan(&s); err != nil {
			return false
		}
		return s == nil || *s != "active"
	}, 10*time.Second, 200*time.Millisecond)

	// A unique build over duplicates fails after creating its entry.
	_, err = executor.BuildIndexConcurrently(t.Context(), pool,
		fmt.Sprintf("CREATE UNIQUE INDEX CONCURRENTLY idx_dis ON %s.t (c)", schema),
		executor.ConcurrentBudget{Overall: 20 * time.Second})
	t.Logf("build with track_activities off: %v", err)
}

Observed — the unprovable state is refused immediately rather than polled to the deadline, which is the documented intent:

state with track_activities off: "disabled"
build with track_activities off: index t_41897_1.idx_dis may be invalid … :
    build backend 77 reports state "disabled": cannot prove the statement stopped

This review was generated by Claude Code (claude-fable-5). Findings 1–4 were reproduced against a live PostgreSQL 16 using the tests above; finding 5 is static.

[aparajon]

🤖 Approving on Armand's behalf. My two-lens review and adversarial correctness pass are posted above — the findings there are for follow-up, not fix-before-merge blockers.

This approval was submitted by Claude Code (claude-fable-5) at Armand's direction.