feat(executor): run accepted blocking statements under both budgets

[Kiran01bm]

Runs an operator-accepted blocking index statement in an engine-owned transaction under explicit lock and statement budgets.

Why

Today an operator who accepts a refused blocking form has to copy the SQL into psql, losing the engine's lock-queue protection (a bounded lock_timeout) and its typed outcome contract.

What

• Add ExecuteAcceptedBlocking with a validated BlockingBudget (both bounds required, non-zero, representable): one statement, one engine-owned transaction, SET LOCAL bounds on the executing session, no rewrite, exactly one attempt. The design doc explains why the accepted path does not reuse native execution's three-attempt lock retry: a second attempt re-queues behind the same holder and multiplies the budget the operator accepted.
• Admit only the shapes the eligibility registry can accept (plain single-relation DROP INDEX / REINDEX INDEX|TABLE, non-concurrent CREATE INDEX); everything else is a typed admission error before any session is acquired.
• Map outcomes to stable codes: 55P03 → lock budget (nothing ran), 57014 → statement budget, 25001 → unsupported-accepted-blocking (permanent: the server will not run REINDEX on a partitioned relation inside the engine-owned transaction, and would refuse it identically on every retry), other PostgreSQL errors → execution failure, a failed COMMIT or caller cancellation after submission → outcome unknown until the catalog is inspected.
• Register the executor-enforced invariants AB-1 (both bounds on the executing session) and AB-2 (lock exhaustion executes nothing); the front-door invariants and the RF-5/RF-6 amendments move to the step that ships the flag.
• Integration tests prove the exact bounds from inside the executing transaction, that lock exhaustion leaves the index valid, and that REINDEX TABLE / REINDEX INDEX on a partitioned parent is reported as outside the path with the index untouched.

The flag and the front-door wiring land in later PRs; nothing reaches this executor yet.

Before / after

Two accepted statements, both single-relation by shape. The first is the common maintenance-window case; the second is the shape the server cannot run inside a transaction.

DROP INDEX app.orders_created_at_idx        (accepted, lock 3s, statement 10m)

before                                      after
──────────────────────────────────────────  ──────────────────────────────────────────
refused, exit 2                             engine-owned transaction
operator pastes SQL into psql:                SET LOCAL lock_timeout = '3s'
  no lock_timeout → queues behind a long      SET LOCAL statement_timeout = '10min'
  reader and blocks every writer behind it    DROP INDEX app.orders_created_at_idx
  outcome: whatever psql printed            lock not granted in 3s → budget-lock-exceeded,
                                              nothing ran, index still valid (AB-2)
                                            granted → committed, typed outcome, 1 attempt

REINDEX TABLE app.events                    (app.events is partitioned)

before                                      after
──────────────────────────────────────────  ──────────────────────────────────────────
admitted; server raises SQLSTATE 25001      admitted; server raises SQLSTATE 25001
→ execution-failed (retryable)              → unsupported-accepted-blocking (permanent)
an adapter retries the identical failure    the adapter stops; the operator runs the
                                            REINDEX outside a transaction block

🤖 Drafted with Amp (Claude Opus 4.6); reviewed and edited by the author.

[chatgpt-codex-connector]

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

[Kiran01bm]

🤖 Adversarial review response — created by Kiran's code review agent (Amp, Claude Opus 4.6) — pull/107, follow-up commit

Both findings are fixed in the follow-up commit, which also merges origin/main (the squashed registry PR landed with IndexTargets() / IndexTarget.String() and its reviewed registry tests; the merge takes main's side of pkg/statement/statement.go and pkg/migrate/refusal_registry_test.go).

#	Finding	Status	Explanation
1	REINDEX TABLE / REINDEX INDEX on a partitioned relation passes admission (single-relation by shape) but PostgreSQL refuses it inside the engine-owned transaction with SQLSTATE 25001; acceptedBlockingStatementError had no 25001 case, so the always-reproducible failure surfaced as execution-failed, which Permanent() does not list, and an adapter could retry it forever	Fixed	acceptedBlockingStatementError now maps SQLSTATE 25001 (sqlstateActiveSQLTransaction) to ErrUnsupportedAcceptedBlocking, wrapping the server error, so the outcome is unsupported-accepted-blocking, which is permanent. The sentinel's doc comment now covers "the server will not run it inside the engine-owned transaction". TestAcceptedBlockingStatementErrorMapsSQLSTATE gains a 25001 row and asserts the mapped code is permanent and the original SQLSTATE is still reachable through errors.As. A new integration test, TestExecuteAcceptedBlockingRefusesReindexOnPartitionedRelation, runs both REINDEX TABLE and REINDEX INDEX against a partitioned parent on a real server and asserts ErrorIs(ErrUnsupportedAcceptedBlocking), pgErr.Code == "25001", Permanent(), and that the parent index is untouched. The eligibility table row and the unsupported-accepted-blocking row in docs/execution-model.md name this boundary.
2	The "Engine-owned session and budgets" paragraph the AB invariants cite still says the accepted statement runs "using the same bounded runner and retry policy as other brief native execution", but executeAcceptedBlocking hard-codes one attempt while native execution retries three times	Fixed	The paragraph now states the statement runs in exactly one attempt and explains why that differs from brief native execution: the operator accepted one bounded ACCESS EXCLUSIVE acquisition, and a second attempt would re-queue behind the same holder and stack another lock_timeout of blocked sessions behind the engine's request, turning the stated budget into a multiple of itself. It links AB-2 as the typed final outcome and adds a paragraph on the REINDEX-on-partitioned boundary from finding 1.

Decisions to veto

• Finding 1 is handled by mapping the server's own refusal rather than adding a catalog preflight that resolves the REINDEX target and checks relkind. The statement package exposes no relation name for REINDEX, a preflight would add a catalog round trip and a race window for every accepted statement, and the server refuses before touching any partition, so nothing is lost by letting it decide. A preflight can still be added later if a plan-time signal is wanted.
• The mapped outcome reuses unsupported-accepted-blocking instead of introducing a new code. The meaning is the same — this statement is outside what the accepted-blocking path can run — and the wrapped error text and SQLSTATE distinguish the server-side case for anyone reading the detail.
• The single-attempt policy is kept rather than switching to executeBoundedAttempt / executeWithLockRetry; the design paragraph now says so and why, matching the AB-2 invariant this PR registered.

Source: #107, scratch review scratch/code-reviews/pg-sprite-pr107-review.md at head 3197a758; fixes in 40c9509

[chatgpt-codex-connector]

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

[aparajon]

🤖 1/2 — the executor and what it admits. AB-1/AB-2, the design doc, and the stack are in 2/2.

Reviewed 231ab81f..40c9509 — the effective delta against main is 8 files, +447/−7 (the branch merges main rather than rebasing, so git log still shows the pre-squash a109572; see 2/2). This is the code that actually takes ACCESS EXCLUSIVE on an operator's table, so I did not read the admission check — I mutated it, then probed the boundary cases against a real server. Ten mutations, go test ./pkg/executor -run 'AcceptedBlocking|Blocking|Code', base green (full ./pkg/{executor,migrate,statement,verdict} green too, 163s).

Mutation	Result
Concurrent() guard dropped from acceptedBlockingShape	caught — TestAcceptedBlockingAdmission/DROP_INDEX_CONCURRENTLY_app.i
single-relation check dropped for DROP/REINDEX	caught — .../{DROP_INDEX_app.i,_app.j, REINDEX_SCHEMA_app}
budget lower bound dropped	caught — TestBlockingBudgetValidation/zero_lock
budget upper bound dropped	caught — TestBlockingBudgetValidation/statement_over_ceiling
25001 arm removed (becomes a retryable execution failure)	caught — TestAcceptedBlockingStatementErrorMapsSQLSTATE/cannot_run_in_transaction_block, TestExecuteAcceptedBlockingRefusesReindexOnPartitionedRelation
ctx.Err() guard dropped	caught — TestAcceptedBlockingStatementErrorMapsSQLSTATE
lock SQLSTATE mapped to the statement cause	caught — .../lock, TestExecuteAcceptedBlockingLockBudgetExecutesNothing
the two new codes dropped from Permanent()	caught — TestCodePermanentClassifiesEveryCode/{invalid-blocking-budget,unsupported-accepted-blocking}
SET LOCAL becomes session SET	survived
commit failure reported as success (return rep, nil)	survived

The head commit is the best thing in the diff. REINDEX on a partitioned relation is single-relation by every shape test you would write, and PostgreSQL refuses it inside a transaction block before touching a partition — so it is admitted, submitted, and then rejected by the server for a reason that will never change. Classifying that 25001 as unsupported-accepted-blocking rather than as a retryable execution failure is the right call, and TestExecuteAcceptedBlockingRefusesReindexOnPartitionedRelation asserting parent_i is still valid afterwards is the assertion that makes it mean something. Same for TestExecuteAcceptedBlockingLockBudgetExecutesNothing: AB-2's claim is about the catalog, and the test reads the catalog rather than trusting the error type.

And TestExecuteAcceptedBlockingAppliesBothBoundsOnExecutingSession is a genuinely good idea — an IMMUTABLE plpgsql function that raises a different SQLSTATE depending on whether it observes both exact bounds proves the settings reached the session that ran the DDL, not a sibling. Finding 3 is about the one thing it cannot see.

---

1. There are two admission surfaces and they disagree in both directions

AcceptedBlockingEligible (pkg/migrate/refusal_registry.go:210) decides eligibility from a typed refusal — site for the index-statement row, cause for the partitioned-parent row. acceptedBlockingShape (accepted_blocking.go:125-137) decides admission from the SQL text. Nothing connects them: ExecuteAcceptedBlocking takes a raw string, and there is no reference to AcceptedBlockingEligible anywhere outside its own tests.

That is not a wiring gap to be closed later, because a SQL-only check cannot reproduce the registry. The second eligible row is unsupported-partitioned-parent / parent-blocking-index-build, and whether a CREATE INDEX earns that cause depends on whether the target is partitioned — a catalog fact the statement does not carry. So the executor approximates the row by admitting every non-concurrent CREATE INDEX. I ran both surfaces over the same statements against the real parser:

Statement	RefusesPartitionedParent cause	registry	acceptedBlockingShape
CREATE INDEX i ON app.events (account_id) on a partitioned parent	parent-blocking-index-build	eligible	admitted
CREATE INDEX i ON ordinary_table (account_id)	— (gateRefusal returns ok=false for KindCreateIndex, so there is no refusal at all)	never eligible	admitted
ALTER TABLE app.events ADD CONSTRAINT events_pkey PRIMARY KEY (id, created_at) on a partitioned parent	parent-blocking-index-build	eligible	refused, permanently
ALTER TABLE app.events ADD CONSTRAINT events_uq UNIQUE (id) on a partitioned parent	parent-blocking-index-build	eligible	refused, permanently

Both directions are real:

Wider than the registry. A plain non-concurrent CREATE INDEX on an ordinary table is the single statement this engine exists to not run — its whole planner story is constructing CREATE INDEX CONCURRENTLY as the safer form. The gate never refuses it, so no refusal exists, so no eligibility decision is ever made about it, and ExecuteAcceptedBlocking will nonetheless run it under ACCESS EXCLUSIVE for anyone who calls the exported function with that string. TestAcceptedBlockingAdmission:90 pins {sql: "CREATE INDEX i ON app.t (id)", want: true} — so the widening is asserted rather than noticed, and the only integration test that drives a CREATE INDEX through this executor (:87, the observe_bounds test) uses exactly that never-eligible shape.

Narrower than the registry. alterBuildsIndex (statement.go:293-310) flags ADD CONSTRAINT … PRIMARY KEY and … UNIQUE, so ALTER TABLE on a partitioned parent earns parent-blocking-index-build and is eligible by the row this PR's sibling just pinned. acceptedBlockingShape's default: return false then produces ErrUnsupportedAcceptedBlocking, which Permanent() reports true. An operator told at plan time that their statement is eligible would get a permanent "outside the accepted blocking path" at execution — the worst version of a two-surface disagreement, because the first surface is what the operator reads before typing the flag.

The design already names the missing piece. Step 2 of the rollout (lock-budgeted-passthrough.md:402-408) asks for three invariants, and the third is "no ineligible, unclassified, environmental, or invariant-violation refusal reaches execution." The marker added in this PR defers it to step 4 "with the flag," which is defensible for a front-door gate — but the deferred invariant is precisely the one that would have caught the table above, and its absence is why the executor got to invent a second admission rule instead of consuming the first.

The fix that collapses both directions is to stop deriving admission from SQL: have ExecuteAcceptedBlocking take the admitted verdict.Refusal (or a small typed admission value minted from it) alongside the statement, and check migrate.AcceptedBlockingEligible — or, if the import direction is wrong, move the eligibility decision behind an interface the executor holds. Then CREATE INDEX is admitted exactly when the refusal that authorized it says so, ALTER TABLE ADD PRIMARY KEY on a parent is admitted for the same reason it was eligible, and the 25001 arm remains the only thing acceptedBlockingShape still needs to know. If that is deliberately step 4's job, the least this PR should do is say so on ExecuteAcceptedBlocking — its doc comment currently says it "runs one admitted blocking index statement," and today the only thing admitting it is itself.

2. Every 57014 is reported as statement-budget exhaustion, including an operator's pg_cancel_backend

acceptedBlockingStatementError:150-151 maps sqlstateQueryCanceled straight to &BudgetError{Cause: CauseStatement, Budget: b.StatementTimeout}. But 57014 is not only statement_timeout — it is also what an operator's pg_cancel_backend raises, and this executor is the one an operator reaches for precisely when they are watching a blocking DDL hold ACCESS EXCLUSIVE and may decide to stop it.

I staged that: a DROP INDEX waiting on a held lock under a 60-second statement budget, cancelled from another session at ~1.5s.

err  = execution exceeded its statement budget (1m0s) and was cancelled
code = budget-statement-exceeded    Permanent() = false
BudgetError{Cause: statement budget, Budget: 1m0s, Attempts: 1}

The message is not imprecise, it is false — a statement that ran for a second and a half is reported as having exhausted a minute — and an operator reading the outcome after their own intervention is told the budget did it. internal_test.go:52 pins this ({name: "statement", code: sqlstateQueryCanceled, want: CodeBudgetStatementExceeded}), so it is asserted rather than overlooked, which is why I am raising it as a decision rather than a slip.

The package already decided this question the other way, and said why. asConcurrentBudgetError (native.go:1199-1217) splits the same SQLSTATE three ways — budget when elapsed >= b.Overall, ErrCancelledByCaller when the caller's context ended, ErrCancelledExternally otherwise — and native.go:172-181 gives the reason in one sentence: "It is deliberately not a *BudgetError: a budget exhaustion invites escalation to a heavier strategy, while a deliberate cancel usually means the change should be left alone." That is exactly the wrong-way-round decision this path now invites, and it invites it with Permanent() == false, against a statement whose next attempt re-queues behind the same holder — the multiplication lock-budgeted-passthrough.md:214-221 argues the single attempt exists to prevent.

And this PR's own doc change describes the case it cannot produce. execution-model.md:293 — a table this diff edits — defines cancelled-externally as "cancelled from outside the executor — not by its caller and not by its budget; an operator's pg_cancel_backend". That is the probe above, verbatim, and the accepted-blocking path cannot emit that code. :292 does the same for cancelled-by-caller ("the caller's own context ended while the statement ran and the budget had not elapsed"), which is exactly the ctx.Err() arm at :140-141 — and that arm instead emits blocking-outcome-unknown, whose own row at :289 reads "reached an ambiguous client boundary," which a cancelled context is not.

Both codes already exist in the file being edited: CodeCancelledExternally at code.go:37, and both sentinels are already wired in sentinelCode (code.go:256-259). And the precedence rule needs one input this function does not take — the signature is the tell. acceptedBlockingStatementError(ctx, err, b) has no elapsed, even though the caller computes rep.Duration = clock.Now().Sub(start) on the line immediately above every call. Passing it and branching on elapsed >= b.StatementTimeout reproduces native.go's rule exactly, with no new machinery and no new seam; here statement_timeout is always a real server deadline, so there is not even a caller-owned mode to special-case.

The ctx.Err() arm at :140-141 is the same question one step over. It returns BlockingOutcomeUnknownError, where the rest of the package returns ErrCancelledByCaller — and by AB-2's own reasoning the outcome here is known, not unknown: a statement cancelled inside the engine-owned transaction aborts it, and the deferred rollback means nothing committed. That is fail-closed, so it is safe, but it spends the one code that means "we genuinely cannot tell" on a boundary where we can, which blunts it at the boundary where we truly cannot (finding 4). If the intent is that a lost connection may leave a COMMIT in flight, then the condition to test is the lost connection, not the caller's context.

3. SET LOCAL is the whole of AB-1's mechanism and nothing pins the keyword

Changing both SET LOCAL to plain SET at :108-109 leaves the suite green. TestExecuteAcceptedBlockingAppliesBothBoundsOnExecutingSession cannot see it: both forms are visible to the statement inside the transaction, which is all that test asks.

What differs is everything after the commit. I ran the same accepted-blocking DROP INDEX with each form and then read the pooled session twenty times:

Form	current_setting('lock_timeout') / statement_timeout after the call
SET LOCAL (as written)	3s / 30s — the ambient values, 20/20
SET (mutant)	137ms / 2468ms — the accepted-blocking budget, 20/20

So under the mutant every later user of that pooled connection silently inherits the operator's one-off blocking budget: a lock_timeout an order of magnitude tighter than the engine's own, applied to work that never asked for it. That is the same class of hazard as #81 ("keep the execution bounds where a pooler would drop them"), running the other way, and SET LOCAL is also what makes this correct under a transaction-pooling pooler — a real reason the keyword is load-bearing rather than stylistic.

AB-1 states it as the rule — "Transaction-local settings override ambient defaults" — so this is the invariant's mechanism with no test under it. One assertion closes it: after ExecuteAcceptedBlocking returns, read current_setting('lock_timeout') from the pool and require the ambient value. Against a pool with more than one connection that is probabilistic, so either force pool_max_conns=1 for that test or assert over enough reads to cover the pool; the twenty-read loop above was reliable here.

4. The ambiguous-commit branch is the fail-closed heart of the executor and nothing produces it in a test

Making tx.Commit failure return rep, nil instead of &BlockingOutcomeUnknownError{} (:117-120) leaves the suite green — a blocking DDL whose commit status is unknown reported as a clean success, with err == nil.

code_test.go:69 does pin BlockingOutcomeUnknownError → CodeBlockingOutcomeUnknown, and accepted_blocking_internal_test.go:74-79 pins the cancelled-context arm of acceptedBlockingStatementError. What has no coverage is the executor actually producing the error at the one boundary where the catalog and the client genuinely disagree. blocking-outcome-unknown is a first-class code in execution-model.md with the operator instruction "inspect the catalog before retrying"; it is the only outcome in this executor where the safe answer is "we do not know," and it is the one branch a mutation can delete for free.

A real commit failure is awkward to stage from an integration test, but the package already has the seam for this shape of problem — executeAcceptedBlocking takes a progress.Clock precisely so the exported wrapper stays clean. A narrow begin(ctx) (blockingTx, error) parameter on the internal function, with a fake whose Commit returns an error, pins the branch as a unit test and costs one interface with three methods. Worth it: this is the outcome the design leans on to justify the single attempt.

---

Smaller notes

• 2BP01 is the nearest neighbour of the 25001 the head commit just fixed, and it is still retryable. DROP INDEX … CASCADE on a constraint-backed index returns ERROR: cannot drop index … because constraint … requires it (SQLSTATE 2BP01) — I confirmed against the server, and it lands in default: → BlockingExecutionError → CodeExecutionFailed, which Permanent() reports false. It will fail identically on every attempt. The single-attempt rationale at lock-budgeted-passthrough.md:214-221 is that a second attempt "would re-queue behind the same holder, stack another lock_timeout of blocked readers and writers"; a retryable code hands exactly that multiplication to the caller for a statement that can never succeed. Not necessarily this PR's job — CodeExecutionFailed is the right bucket for the deadlocks and disk-full errors that are retryable — but the head commit established that a deterministic server refusal earns a permanent classification, and this is the same shape one SQLSTATE over.
• DROP INDEX … CASCADE itself is fine, which is worth recording. I expected CASCADE to let a single-relation DROP INDEX reach a foreign key on a different table, breaking the "one table's lock is being accepted" premise the eligibility row rests on. It does not: PostgreSQL refuses to drop a constraint-backed index at all (the 2BP01 above), so an index's dependents are effectively constraints and constraints are already protected. The single-relation site holds.
• rep is discarded on the two early failure paths. :102 and :111 return BlockingReport{} after rep was populated at :98, so a begin or SET LOCAL failure loses the SQL and the budgets, while a statement failure (:115) keeps them. Either is defensible; the inconsistency is what will surprise whoever renders the report in step 3.
• ParseOne is what makes tx.Exec(ctx, sql) safe, since a no-argument pgx Exec goes out over the simple protocol and would happily run a second statement. ParseOne rejects n != 1 (statement.go:192-194), so the admitted string is one statement by construction. Nothing to change — but it is the load-bearing reason this function can pass caller-supplied SQL straight through, and there is no comment at :113 saying so.
• Attempts: 1 on both BudgetErrors matches the deliberate single attempt and reads correctly against docs/execution-model.md.

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

[aparajon]

🤖 2/2 — AB-1, AB-2, the design doc, and the stack. The executor and what it admits are in 1/2.

The registry entries are the part of this change that outlives the code, so I read them as claims and checked each against what the executor actually enforces rather than against what the PR says it enforces. Both are true as written. What follows is four places where the text is narrower or wider than the mechanism, one acceptance criterion that now has no enforcement point anywhere, and the stack note.

1. AB-1's "engine-owned session" is a borrowed one, and the word carrying the guarantee is SET LOCAL

AB-1 (invariants.md:301) opens with "one engine-owned session and transaction." The session comes from *pgxpool.Pool: the executor owns it for the length of the transaction and then hands it back. That is precisely why SET LOCAL is the right mechanism and why the entry's second sentence — "Transaction-local settings override ambient defaults" — is the one doing the work.

But the first sentence reads as though the session were the scope, and the enforceable scope is the transaction. After COMMIT the connection is back in the pool at ambient values, and that property is the untested one (finding 3 in 1/2). The difference is not academic: it decides whether a future path may set the bounds once per session instead of once per transaction. Under a transaction-pooling pooler the session form is wrong — #81 is this repo's own account of why — and AB-1 as written does not rule it out, while the code does. Naming the transaction as the scope in the rule closes that.

2. AB-2 states for the lock case a guarantee the transaction gives in every case

"Lock-budget exhaustion executes nothing" is true, and TestExecuteAcceptedBlockingLockBudgetExecutesNothing earns it by reading the catalog rather than trusting the error type. But the reason is not specific to 55P03. It is that the statement runs in a transaction that commits only on the success path: statement-timeout, the 25001 refusal, 2BP01, an operator's cancel — none of them can leave a partial change either, and the deferred tx.Rollback covers them all.

Stating it only for the lock case invites the reading that the others might have executed something, which is both false and the opposite of what the design wants believed. The rule that is true, that the code already enforces, and that is more useful to an operator is:

An accepted blocking statement either commits in full or changes nothing; the single boundary at which the outcome cannot be known is a failed COMMIT.

That subsumes AB-2, covers the four other failure modes, and names the one real exception rather than leaving it unsaid — which is item 3. AB-2's *Enforced:* SQLSTATE is correct (55P03, optimistic.go:42), and the "is not retried" clause is honest: Attempts: 1 is set on both BudgetErrors.

3. blocking-outcome-unknown is a first-class outcome with no invariant

execution-model.md:289 gives the code an operator instruction — "inspect the catalog before retrying" — and BlockingOutcomeUnknownError is the fail-closed heart of this executor: the only place it says I do not know. The AB family has no entry for it.

Since the family is created in this PR, and since the single-attempt rationale at lock-budgeted-passthrough.md:214-221 leans on the outcome being either known-nothing or known-committed, an AB-3 to the effect that an unproven outcome is never reported as success is the entry I would expect beside the other two. It is also the branch nothing produces in a test (finding 4 in 1/2), which is the usual reason a rule everyone believes never gets written down. Note that finding 2 in 1/2 pushes the other way on the same code: today blocking-outcome-unknown is also emitted for a cancelled caller context, which is not an ambiguous boundary. Writing AB-3 would force that distinction to be made rather than inherited.

4. Criterion 3's "explicitly supplied" clause now lands nowhere

Acceptance criterion 3 (lock-budgeted-passthrough.md:99) is stronger than AB-1: the bounds must be ones "the operator supplied explicitly on that invocation; no caller-owned, defaulted, or unbounded session is eligible."

AB-1 keeps "non-zero" and "before a session is acquired" and drops "explicitly supplied," which is the right call — validate() sees two time.Duration values and cannot distinguish an operator's number from a constant a caller compiled in. The zero-value-invalid design does enforce not defaulted by omission, which is the half the executor can own, and it enforces it well.

The other half is a front-door property that step 4 delivers when the flag parses the operator's values. Nothing in this PR or the registry says so. Right now the full strength of criterion 3 exists only in the design doc's acceptance table, the registry holds the weaker half, and there is no marker recording that the rest is owed — which is how a criterion ends up quietly satisfied by halves. A clause on step 4, or a "(step 4)" note beside AB-1, is enough.

5. The docs half of the two-surface problem

The by-design eligibility row (:187) gains the REINDEX-on-partitioned carve-out. That is exactly right, it matches the head commit, and it is the model for what the rest of the table needs — one clause naming the shape and the code it earns.

But that table is the authoritative statement of what may run, and two of the four rows in finding 1's table are still absent from it:

• CREATE INDEX on an ordinary table. Eligible by no row — gateRefusal produces no refusal for KindCreateIndex at all, so no eligibility decision is ever made — yet acceptedBlockingShape admits it and TestAcceptedBlockingAdmission:90 pins the admission. The table says nothing because from the registry's point of view there is nothing to say, and that silence is exactly what makes the executor's extra admission invisible to a reader of the design.
• ALTER TABLE … ADD CONSTRAINT … PRIMARY KEY / … UNIQUE on a partitioned parent. Eligible by row :188 — alterBuildsIndex gives it parent-blocking-index-build — and permanently refused by the executor. Row :188's rationale ("the plain PostgreSQL statement") reads as though every member of the row were an index statement. They are not, and the row is the thing an operator consults before typing the flag.

If CREATE INDEX on an ordinary table is genuinely meant to be runnable here, that is a new registry row with its own rationale, not an executor-side default.

6. Step 2's marker defers the one invariant that is not about the flag

Step 2 (:402-408) asks for three invariants; this PR ships two, and the marker defers the third — "no ineligible, unclassified, environmental, or invariant-violation refusal reaches execution" — to step 4 "with the flag," alongside the RF-5/RF-6 amendments.

Deferring the RF amendments is clearly right: they describe what --accept-blocking makes true, and the flag does not exist. The third invariant is a different animal. It is not about the flag; it is about what the executor accepts, and the executor exists and is exported as of this commit. ExecuteAcceptedBlocking is callable today by anything in-tree with no gate between a caller's string and ACCESS EXCLUSIVE — which is how the ordinary-table CREATE INDEX got in. So either that invariant belongs in this step after all, or ExecuteAcceptedBlocking stays unexported until step 4 has something to gate it with. The marker's wording ("the executor-owned AB-1 and AB-2 invariants ship here") frames the split as a question of ownership; it is really a question of reachability.

Smaller notes

• The AB family starts without the backstop the RF family has. RF-7 is held by TestRefusalRegistryIsComplete (refusal_registry_test.go:83), which is why a new refusal value cannot quietly skip classification. Nothing in this repo reads docs/invariants.md — I checked every test file — so AB-1 and AB-2 are prose with two good integration tests beneath them and no structural guarantee that a third accepted-blocking path is held to the same rule. Not something to build in this PR; worth knowing it is absent.
• AGENTS.md:43-45 is the bar the two surviving mutations fall short of — "no behavior lands without a test that would fail without it… the full suite is a merge gate." SET LOCAL and the commit branch are both behaviors this PR introduces that the suite does not hold. One small test each; both sketched in 1/2.
• The branch merges main rather than rebasing. 40c9509 is Merge: 3197a75 231ab81, so git log origin/main.. still shows the pre-squash a109572 and the PR reads as though feat: classify accepted-blocking refusal eligibility #105's work were part of it. The effective delta is the 8 files I reviewed. Worth a rebase before merge — more than usually so, because feat: classify accepted-blocking refusal eligibility #105 landed as a squash and the duplicate will not resolve itself.
• The rollout-criteria row (invariants.md:537) reads correctly against what the two integration tests actually assert.
• Both *Source:* anchors resolve — #engine-owned-session-and-budgets at :209, #failure-and-interruption-semantics at :311 — and the AB-2 backlink at :221 matches the generated heading anchor.

---

Approving. Nothing in the registry is false, which is the bar that matters most for entries that will be cited for years. Of the doc items, item 5 is the one I would fix before merge: it is the operator-visible half of 1/2's finding 1, and the REINDEX carve-out right above it shows the shape of the fix. Item 6 is a judgement call the author is better placed to make than I am.

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

[aparajon]

🤖 Approving. Reviewed in two comments above — the executor and its admission surface in 1/2, the AB registry entries and design doc in 2/2. Ten mutations plus three server-side probes; base tests and lint green.

The head commit's 25001 handling is the best thing in the diff, and both new integration tests assert at the catalog rather than at the error type, which is the right instinct for an executor that takes ACCESS EXCLUSIVE.

Four findings, in the order I would fix them: the 57014 collapse (every cancellation, including an operator's pg_cancel_backend, is reported as statement-budget exhaustion with a false duration and a retryable code — the package already splits this three ways and this PR's own execution-model.md rows describe the case); the two admission surfaces disagreeing in both directions; SET LOCAL and the ambiguous-commit branch each unheld by a test; and the eligibility table not yet covering the two shapes the executor and registry disagree on.

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

[Kiran01bm]

🤖 Adversarial review response — created by Kiran's code review agent (Amp, Claude Opus 4.6) — pull/107, follow-up commit

Both findings are fixed in the follow-up commit, which also merges origin/main (the squashed registry PR landed with IndexTargets() / IndexTarget.String() and its reviewed registry tests; the merge takes main's side of pkg/statement/statement.go and pkg/migrate/refusal_registry_test.go).

#	Finding	Status	Explanation
1	REINDEX TABLE / REINDEX INDEX on a partitioned relation passes admission (single-relation by shape) but PostgreSQL refuses it inside the engine-owned transaction with SQLSTATE 25001; acceptedBlockingStatementError had no 25001 case, so the always-reproducible failure surfaced as execution-failed, which Permanent() does not list, and an adapter could retry it forever	Fixed	acceptedBlockingStatementError now maps SQLSTATE 25001 (sqlstateActiveSQLTransaction) to ErrUnsupportedAcceptedBlocking, wrapping the server error, so the outcome is unsupported-accepted-blocking, which is permanent. The sentinel's doc comment now covers "the server will not run it inside the engine-owned transaction". TestAcceptedBlockingStatementErrorMapsSQLSTATE gains a 25001 row and asserts the mapped code is permanent and the original SQLSTATE is still reachable through errors.As. A new integration test, TestExecuteAcceptedBlockingRefusesReindexOnPartitionedRelation, runs both REINDEX TABLE and REINDEX INDEX against a partitioned parent on a real server and asserts ErrorIs(ErrUnsupportedAcceptedBlocking), pgErr.Code == "25001", Permanent(), and that the parent index is untouched. The eligibility table row and the unsupported-accepted-blocking row in docs/execution-model.md name this boundary.
2	The "Engine-owned session and budgets" paragraph the AB invariants cite still says the accepted statement runs "using the same bounded runner and retry policy as other brief native execution", but executeAcceptedBlocking hard-codes one attempt while native execution retries three times	Fixed	The paragraph now states the statement runs in exactly one attempt and explains why that differs from brief native execution: the operator accepted one bounded ACCESS EXCLUSIVE acquisition, and a second attempt would re-queue behind the same holder and stack another lock_timeout of blocked sessions behind the engine's request, turning the stated budget into a multiple of itself. It links AB-2 as the typed final outcome and adds a paragraph on the REINDEX-on-partitioned boundary from finding 1.

Source: #107, scratch review scratch/code-reviews/pg-sprite-pr107-review.md at head 3197a758; fixes in 40c9509