feat(engine): route concurrent index builds to the dedicated executor

[Kiran01bm]

Route CREATE INDEX CONCURRENTLY to pg-sprite's dedicated concurrent-build executor and run partition admission at apply time in the PostgreSQL engine.

Why

The optimistic apply path executed every native-safe statement through the transactional executor, but PostgreSQL refuses CREATE INDEX CONCURRENTLY inside a transaction block — so a concurrent index build in a plan would fail at apply with a raw server error instead of being handled by the executor purpose-built for it. Partition admission also only ran at plan time, so a table partitioned between plan and apply would hit an unclassified server error mid-statement rather than a typed refusal.

What

• executeOptimistic routes concurrent CREATE INDEX statements to executor.BuildIndexConcurrently under a dedicated 4-minute budget; plain (non-concurrent) index builds still go through ExecuteNative.
• The budget is deliberately below the 5-minute apply ceiling so the server-side deadline fires first and exhaustion surfaces as the typed budget verdict (permanent refusal), never an ambiguous client-side cancellation.
• Partition admission (preflight.CheckPartitionSupport) runs in the executing session for partitioned parents; *preflight.UnsupportedPartitionedParentError classifies as a permanent refusal (unsupported-partitioned-parent) whose detail is pg-sprite's fixed English sentence — safe to render verbatim by construction.
• *executor.InvalidIndexError (pre-existing or unrecovered leftover invalid index) classifies as a retryable failure whose detail names the index and the operator action from typed fields only; the full cause goes to server logs.
• Unit coverage for the new refusal classification; integration coverage for the happy-path concurrent build (catalog-valid index), the partitioned-parent refusal, and the pre-existing-invalid-index retryable failure.

Before / after

Before:
  plan change ──▶ executeOptimistic ──▶ ExecuteNative (transactional)
                                          └─ CREATE INDEX CONCURRENTLY ──▶ raw server error
                                             (cannot run in a transaction block)

After:
  plan change ──▶ executeOptimistic
                    ├─ partitioned parent? ──▶ CheckPartitionSupport ──▶ typed permanent refusal
                    ├─ CREATE INDEX CONCURRENTLY ──▶ BuildIndexConcurrently (4m budget)
                    │     ├─ built + catalog-valid ──▶ completed
                    │     ├─ invalid index found/left ──▶ retryable failure (names index, operator action)
                    │     └─ budget exhausted ──▶ typed budget verdict
                    └─ everything else ──▶ ExecuteNative (unchanged)

[Copilot]

Pull request overview

Routes PostgreSQL CREATE INDEX CONCURRENTLY statements away from the transactional optimistic executor (which PostgreSQL rejects) and adds apply-time partition admission so partitioning changes between plan/apply produce a typed refusal instead of a raw server error.

Changes:

• Route concurrent index builds to executor.BuildIndexConcurrently with a dedicated 4-minute budget, while keeping non-concurrent statements on ExecuteNative.
• Run partition admission (preflight.CheckPartitionSupport) at apply time for partitioned parents and classify UnsupportedPartitionedParentError as a permanent refusal.
• Add unit + integration coverage for concurrent index build success, partitioned-parent refusal, and invalid-index retryable failure.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File	Description
pkg/engine/postgres/apply.go	Adds apply-time partition admission and dedicated concurrent index execution path; classifies invalid-index failures as retryable with sanitized operator-facing detail.
pkg/engine/postgres/apply_test.go	Extends refusal classification coverage to include partitioned-parent admission errors.
pkg/engine/postgres/postgres_integration_test.go	Adds integration tests for concurrent index build success, partitioned-parent refusal, and invalid-index retryable failure (one setup needs adjustment to avoid system catalog writes).

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[chatgpt-codex-connector]

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

[aparajon]

🤖 Adversarial correctness review, requested by @aparajon and performed by their agent. Reviewed at head 8be70760.

Verdict: the routing and the classification ladder are right, and the ordering argument behind them is the good kind — the invalid-index arm being checked before the refusal and budget arms is subtle and correct. Nothing here blocks. My one finding is that the invariant those two time bounds depend on is asserted as arithmetic rather than as the property it protects, and the thing it protects is exactly the operator-facing detail this PR adds.

Findings

1. TestConcurrentIndexBudgetFitsUnderApplyCeiling pins 4m < 5m, but the property its own doc comment describes is a headroom budget, and losing that race silently un-does this PR's best feature. The ceiling wraps the whole background apply (apply.go:137), so the 60 seconds between the two constants has to cover pool acquisition, the preflight table read, LookupTargetFacts, CheckPartitionSupport, and the post-failure catalog verdict that runs after the 4-minute build deadline expires — the comment names that chain explicitly. The assertion cannot see any of it: raise concurrentIndexBudget to 4m59s and the test stays green while the headroom goes to a second. What is on the other side of that race is not a slower failure but a worse one. If the ceiling fires first, the context dies mid-CREATE INDEX CONCURRENTLY; PostgreSQL leaves an invalid index on the target, the catalog verdict has no live context left to run in, and the error classifies through the external-cancellation arm to the generic tail — "PostgreSQL schema change failed; see server logs", retryable, naming no index. That is precisely the outcome invalidIndexDetail was written to prevent, reachable exactly when the assumption the test doesn't check stops holding. A named concurrentIndexHeadroom constant with assert.GreaterOrEqual(t, optimisticApplyCeiling-concurrentIndexBudget, concurrentIndexHeadroom) would make the constant that must not move the one the test is about.

2. (optional, design) A blocking CREATE INDEX still executes as submitted, and this PR is where that becomes a deliberate split worth stating. The new branch routes Concurrent() builds to the dedicated executor and sends everything else to ExecuteNative, which is correct for the transaction-block constraint. But pg-sprite's own statement gate documents the stronger policy — a blocking CREATE INDEX is substituted with its concurrent build — and SchemaBot instead runs the submitted form under optimisticStatementLimit, where lock_timeout bounds acquiring ACCESS EXCLUSIVE and not holding it. So a plain CREATE INDEX on a large table can block writes for up to the statement limit before timing out. That may well be the intended contract (SchemaBot executes the planned statement rather than rewriting it), and it is not a regression — but the else side of this new branch is now the only place a reader would look for it, so it deserves a sentence there.

Action items

1. (Finding 1) Introduce a named headroom constant and assert on optimisticApplyCeiling - concurrentIndexBudget instead of on the two constants' relative order, so a future budget change cannot quietly consume the margin the catalog verdict runs in.
2. (optional) Say in the non-concurrent branch of executeOptimistic why a blocking CREATE INDEX is executed as planned rather than substituted with its concurrent form.

Verified (tried to break, couldn't)

The ordering claim is the load-bearing one and it holds under attack: InvalidIndexError is checked in runOptimisticApply before classifyRefusal, and refusalForCause independently declines it before the budget arm, so a budget-cancelled build that leaves its own invalid index reports the index rather than the exhaustion no matter which path reaches it — and the second guard is not dead code, since classifyRefusal has two further callers in postgres.go. The advice ladder is genuinely fail-safe per verdict: only CodeInvalidIndexOwnLeftover names a drop, default catches CodeInvalidIndexUnproven and any future code with investigation steps, and every branch interpolates only Schema/Index through %q and sanitizeReasonText, never the wrapped Build/Cleanup errors — the new table test pins that with explicit NotContains on raw server text. I chased two failures that turned out not to exist. DROP INDEX CONCURRENTLY and REINDEX CONCURRENTLY share the same transaction-block constraint and are not routed by the new branch, but they never reach it: validateOptimisticApply gates on preflight.RequiredTier, whose default arm rejects both kinds at acceptance, so they are refused before any work is queued rather than failing raw at execute. And Retryable = true on the invalid-index arm is not a retry storm — it converts to state.Task.FailedRetryable and adoptableEngineTerminalStates explicitly refuses to adopt it, so recovery is operator-driven and nothing auto-retries into an occupied index name. Partition admission is correctly scoped to table.Partitioned() and re-reads server facts from the executing session rather than trusting plan time, and its refusal is asserted non-retryable. Copilot's objection to the pg_index update in the integration test does not reproduce: the harness runs the schemabot role as the image's superuser, superusers may do DML on system catalogs, and the integration shard that owns this package is green on the head — the CI split is designed to fail loudly rather than silently skip a package. No test functions were removed and no assertions weakened. go build ./... plus the pkg/engine/postgres/... and pkg/tern/... suites pass locally at head, and CI is 38/38 green.

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

[aparajon]

🤖 Approving on @aparajon's behalf after the adversarial correctness review above. The findings there are yours to pick up as follow-ups — flagging them, not gating on them.

This stamp was left by Claude Code (claude-opus-5).

[morgo]

🤖 Approved on Morgan's behalf by his AI agent.

PostgreSQL-only (pkg/engine/postgres/), nothing on the MySQL path, green CI, and +151 test lines against +104 production lines. It also strictly improves a path that fails outright today — a concurrent build reaching the transactional executor produces a raw server error, so there's no regression surface to speak of.

The things I checked rather than took on faith:

• The routing predicate is typed, not textual. statement.Kind() == pgstatement.KindCreateIndex && statement.Concurrent() — no string match on "CONCURRENTLY", so it can't be fooled by casing, whitespace, or a comment, and it can't false-positive on an unrelated statement that happens to contain the word.
• The budget ordering reasoning holds. 4 minutes under the 5-minute apply ceiling means the server-side deadline fires first, so exhaustion surfaces as the typed budget verdict instead of an ambiguous client-side cancellation. That's the right direction to get wrong-proof: the ambiguous outcome is the one you can't classify.
• The invalid-index handling is the best part. Checking InvalidIndexError before the refusal and budget arms is subtle and correct — a budget-cancelled build leaves its own invalid index, and if the budget arm won the race the operator would be told about exhaustion when the actionable fact is the index they need to clear. The three-way split (own leftover → name the drop; pre-existing → check pg_stat_activity first, it may be another actor's live build; unproven → inspect pg_index.indisvalid, never a statement to run) fails safe in the right direction each time, and building the detail only from typed identifiers rather than the wrapped errors keeps raw server text out of operator-facing output.

Worth noting for whoever sequences the PostgreSQL work: this PR gets the CREATE INDEX CONCURRENTLY constraint exactly right — a concurrent build can't run inside a transaction block, so it's routed to a dedicated non-transactional executor with its own budget and catalog verification. #1220 takes the opposite approach in the storage bootstrap path, emitting non-concurrent CREATE INDEX inside a per-table transaction, which holds ACCESS EXCLUSIVE for the build. Different contexts and I don't think this PR's approach transplants directly — but the reasoning here is the more careful of the two, and it's the one I'd want carried across if the bootstrap path ever grows index convergence on a large table.