statement: refuse accepted-blocking DROP INDEX ... CASCADE

[Kiran01bm]

migrate --accept-blocking now refuses DROP INDEX ... CASCADE, so the one table the operator acknowledges is the only table the statement touches.

Why

--accept-blocking <table> acknowledges one table's lock, but DROP INDEX ... CASCADE also drops whatever depends on the index — a foreign key on another table, for instance — locking and changing a table the operator never named (AB-3, AB-4). Neither the gate nor the executor's shape check looked at the drop behaviour, so the cascading form was admitted.

What

• The parser classifies a cascading DROP INDEX as a scope wider than one relation (IndexTargetOther), and records Cascades(). The gate then files it at the existing ineligible index-statement-other site, and the executor's shape check, which already requires a single relation, refuses it too.
• Plain and explicit RESTRICT drops stay eligible.
• The refusal's advice no longer suggests DROP INDEX CONCURRENTLY, which PostgreSQL rejects with CASCADE; it says to drop the dependents explicitly first.
• AB-3 / AB-4 Enforced: text, the passthrough design, the plan-report and capability docs, and the --accept-blocking help updated (upholds AB-3, AB-4).

Before / after

orders has a foreign key on the unique index users_email_key of users, and the operator acknowledges only users. Before, the drop ran and took the foreign key on orders with it; after, it is refused and nothing changes.

migrate --accept-blocking app.users --statement-timeout 30s
        --alter 'DROP INDEX app.users_email_key CASCADE'

Before                                     After

parsed as a drop of one index on           parsed as a drop wider than one
one table                                  table, because of CASCADE
        │                                          │
        ▼                                          ▼
the gate treats it as eligible for         the gate treats it as never
--accept-blocking                          eligible for --accept-blocking
        │                                          │
        ▼                                          ▼
the index belongs to app.users, which      the acknowledgement is not consulted
matches the acknowledgement
        │                                          │
        ▼                                          ▼
LOCK users, then DROP ... CASCADE:         nothing runs
the index and the foreign key on
orders are both dropped
        │                                          │
        ▼                                          ▼
exit 3; orders was locked and changed      refused, exit 2; the index and the
though nobody named it                     foreign key on orders are intact

[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 1/2 (adversarial correctness) of 9b60ce2: 0 blocking, 2 non-blocking.

I read the whole diff against 4a2dcd2 (the merge base) and the surrounding code: ParseOne, Gate/gateRefusal/indexAdvice, AcceptedRefusal/acceptBlocking, executeAcceptedBlocking and acceptedBlockingShape, and the Force and diff paths that could reach a DROP INDEX. The fix is correct and closed on both layers. A DROP INDEX ... CASCADE now parses as IndexTargetOther, so the gate files it at the ineligible index-statement-other site (the acknowledgement is never consulted), and the executor's shape check refuses it a second time for a library caller that skips the gate. Gate runs before Force, so --force cannot reach it either. I confirmed against real PostgreSQL 16 that the hazard is real: DROP INDEX ... CASCADE on a unique index that a foreign key on another table references drops that foreign key, while plain DROP INDEX and explicit RESTRICT fail with SQLSTATE 2BP01 and change nothing, so keeping those two eligible is safe.

Invariants. AB-3 is extended (the scope rule is added and its Enforced: line now names acceptedBlockingShape and the ParseOne classification; every test named in it exists and passes). AB-4 is upheld (a cascading drop is an ineligible refusal that stands as it would without the flag; migrate.gateRefusal is now named in Enforced:). AB-1, AB-2 and AB-5 are untouched. Nothing weakens an invariant. CI is green across PostgreSQL 14 to 18, and I re-ran the new unit and integration tests at this head (all pass).

Non-blocking

1. DROP INDEX CONCURRENTLY ... CASCADE is refused as "already the safe concurrent idiom" (pre-existing, but this PR is where cascade became a known fact)

indexAdvice checks st.Concurrent() first (verdicts.go#L48) and only then st.Cascades() (verdicts.go#L51). So the combined form gets the no-online-safety-problem / direct-operator verdict with the detail "this is already the safe concurrent idiom ... run it directly against the database". PostgreSQL rejects that statement (DROP INDEX CONCURRENTLY does not support CASCADE, SQLSTATE 0A000, which I ran), and the new advice text a few lines below says exactly that. Nothing unsafe happens, since nothing executes and the server refuses the statement; the operator who follows the advice gets a server error instead of a drop. The text is identical on the base, so this is not a regression. Fix: test st.Cascades() before st.Concurrent(), or add a combined message that says the statement is invalid in PostgreSQL.

Test that fails on 9b60ce2

// PostgreSQL rejects DROP INDEX CONCURRENTLY ... CASCADE, so the refusal must
// not describe it as the safe concurrent idiom to run directly.
func TestGateConcurrentCascadeIsNotCalledSafe(t *testing.T) {
	st, err := statement.ParseOne("DROP INDEX CONCURRENTLY billing.i CASCADE")
	require.NoError(t, err)
	v, refused := Gate(st)
	require.True(t, refused)
	assert.NotContains(t, v.Detail, "already the safe concurrent idiom")
}

--- FAIL: TestGateConcurrentCascadeIsNotCalledSafe (0.49s) on 9b60ce2: "this is already the safe concurrent idiom; pg-sprite does not drive this maintenance form yet — run it directly against the database" should not contain "already the safe concurrent idiom".

2. "Plain and explicit RESTRICT drops stay eligible" is pinned at classification only, not by an end-to-end run

The PR description and the lock-budgeted passthrough doc both state that explicit RESTRICT stays eligible. The tests that cover it stop at classification: TestParseOneKinds (statement_test.go#L116), TestAcceptedRefusal ("drop index restrict") and TestAcceptedBlockingAdmission. No integration test runs DROP INDEX ... RESTRICT (or a plain drop of an index that has a dependent foreign key) through --accept-blocking, so nothing would notice if a later change to how the behavior is read made RESTRICT quietly start failing, or started treating "dependents exist" as something other than a server refusal. What I ran against PostgreSQL 16 through ExecuteAcceptedBlocking: a plain drop and a RESTRICT drop of the index the foreign key depends on both fail with SQLSTATE 2BP01 ("because other objects depend on it") and the foreign key on the other table is still there afterward. A small integration test with exactly that shape would pin the safe half of the contract that the cascade test pins for the unsafe half.

Verified

• Mutation testing (each mutant applied in a throwaway worktree, then restored; the PR's own tests, unit and integration, as the oracle):

Mutant	Caught by
ParseOne ignores cascade (len(objects) == 1 only)	TestParseOneKinds, TestAcceptedRefusal, TestGate, TestAcceptedBlockingAdmission, TestMigrateAcceptBlockingLeavesIneligibleRefusalsAlone, and the end-to-end TestMigrateAcceptBlockingRefusesCascadeAndKeepsDependentForeignKey (the cascade then ran)
cascades never set	TestParseOneKinds, TestAcceptedRefusal, TestGate, TestAcceptedBlockingAdmission, CLI ineligible-refusal test
acceptedBlockingShape admits every drop and reindex	TestAcceptedBlockingAdmission (cascade, multi-index and REINDEX SCHEMA rows), TestExecuteAcceptedBlockingRefusesCascadeDrop (the cascade ran and the foreign key was dropped)
gateRefusal ignores the target (always the eligible site)	TestAcceptedRefusal, TestGate, TestAcceptedBlockingEligibleRowsArePinned, TestIndexStatementAcceptedBlockingEligibility, TestGateVerdictAcceptedBlockingEligibility, CLI ineligible-refusal test
cascade branch removed from indexAdvice	TestGate (the concurrent safer idiom is suggested)

Every production change in the diff has at least one killing test, and both layers (gate and executor shape check) are independently pinned.

• Reach: Gate runs before Force in both migrate.Run (migrate.go#L219) and the CLI front door, and I found no other route from an operator-supplied statement to execution. ParseOps (the diff/planner classifier) does not read the drop behavior, so a library caller that classifies a cascading drop there and rewrites it concurrently gets a server error (the statement in item 1), not an execution.
• Docs: README, capabilities (.md and .yaml), plan-report, refusal-classes, the passthrough design doc, the --accept-blocking help text, and AB-3/AB-4 all say the same thing as the code.

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

[aparajon]

🤖 Review 2/2 (OSS adoption, integration ease for importers) of 9b60ce2: 0 blocking, 3 non-blocking.

Lens check for pkg/statement, pkg/executor and pkg/migrate as an importer sees them. The additions are small and additive: one new accessor (Statement.Cascades()), a broadened meaning for IndexTargetOther, and the IndexRelation() of a cascading drop now being the zero value. I searched the SchemaBot checkout (origin/main) and found no import that reads IndexTarget, IndexRelation or the accept-blocking surface today, so nothing downstream breaks; the points below concern what the next importer will see. The PR description, before/after diagram and the doc updates read well for a first-time adopter: every place that tells operators what --accept-blocking accepts now carries the CASCADE exclusion.

Non-blocking

1. IndexTarget's type comment still says it "names one relation"; a one-index DROP INDEX ... CASCADE now answers IndexTargetOther and a zero IndexRelation()

The constant docs were updated, but the type comment above them was not (statement.go#L98: "identifies whether an index-maintenance statement names one relation"), and IndexRelation() is documented as the relation a single-relation statement names (statement.go#L185). Under the new parse, DROP INDEX app.i CASCADE names exactly one relation yet reports Other with an empty relation (statement.go#L303). An importer that reads IndexTarget as "how many relations" and falls back to IndexRelation() for the name (to show it in a UI, say) gets nothing for a statement that plainly names an index. Two ways to close it, either fine: keep IndexTarget as the pure count of relations named, populate IndexRelation for the cascading single-name drop, and let the gate and the shape check read Cascades() explicitly (the registry walks IndexTargets() only for target shape, so no completeness change); or keep the current model and reword the type and IndexRelation comments so "Other" is documented as "wider than one relation, for any reason, see Cascades()".

2. A library caller that hits the executor's refusal gets a bare sentinel with no reason

executeAcceptedBlocking returns ErrUnsupportedAcceptedBlocking unwrapped for a statement that fails the shape check (accepted_blocking.go#L119), so ExecuteAcceptedBlocking(ctx, pool, "DROP INDEX s.i CASCADE", ...) reports only "statement is not an accepted blocking index statement". That is the second-line defence for exactly the importer that skips migrate.Gate, and it is the importer least able to see why. Its neighbour refuseReplicaIdentityDrop explains itself and says what to do next. Wrapping the sentinel with a reason chosen from the parsed statement (cascading drop, several indexes, wrong scope) would make the library path as self-explaining as the CLI path, and the test can keep asserting ErrorIs.

3. The cascade refusal does not name the way forward that is already supported

The new detail tells the operator to "drop each dependent object explicitly, then the index" (verdicts.go#L51). That is right when dependents exist, but it is the long road. The common case is an operator who wrote CASCADE out of habit: resubmitting the same drop without CASCADE is eligible for --accept-blocking, and if a dependent does exist PostgreSQL itself refuses it (SQLSTATE 2BP01, nothing changes, which I ran). Saying so (for example "or resubmit without CASCADE; PostgreSQL then refuses the drop if anything depends on the index") turns a dead-end refusal into the safe retry. saferIdiom is correctly empty (the concurrent form is invalid with CASCADE), so the detail prose is the only guidance a consumer of the plan report gets. A smaller nit in the same area: the changed README line now runs past the paragraph's wrap width.

Verified

• Checked the importer surface by search of the SchemaBot checkout: no use of IndexTarget, IndexRelation(), AcceptBlocking or the passthrough fields, so the broadened IndexTargetOther has no known caller to break.
• Verified the plan-report and capability docs against the code: the cascade refusal is filed at the ineligible site (so the eligibility registry answers false for it, asserted in TestGate), the CLI integration test asserts outcome refused with blocking_passthrough false, and no field or enum value was added to the report, so no format_version bump is needed.
• Confirmed the user-visible output by running the built binary: migrate --accept-blocking app.users --statement-timeout 30s --alter 'DROP INDEX app.users_email_key CASCADE' refuses before dialing with class: by-design and the new detail; a multi-index cascade gets the same text.

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

[aparajon]

🤖 Stamping with comments: 0 blocking, 5 non-blocking (2 in review 1/2, 3 in review 2/2; see the two review comments above).

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

[Kiran01bm]

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

Verdict: all 5 non-blocking findings fixed in a follow-up commit that also merges main.

#	Concern	Status
C1-F1	DROP INDEX CONCURRENTLY ... CASCADE is refused as the safe concurrent idiom to run directly, which PostgreSQL rejects	fixed — at both layers: gateRefusal files a concurrent DROP INDEX as no-online-safety-problem / direct-operator only when it names one index, so a cascading or multi-index one is by-design like the plain form, and indexAdvice checks Cascades() first and names the one-index rule; TestGateConcurrentCascadeIsNotCalledSafe asserts the class and owner for both shapes and fails on the old rule; refusal-classes.md rows updated
C1-F2	the eligible plain and RESTRICT drops are pinned only at classification	fixed — two integration tests run a plain and a RESTRICT drop of an index a foreign key on another table depends on through ExecuteAcceptedBlocking, assert SQLSTATE 2BP01, and that the index and the foreign key remain
C2-F2	a library caller hitting the executor's shape refusal gets a bare sentinel	fixed — acceptedBlockingIneligibility returns the reason (concurrent form, cascading drop, several indexes, wider scope, not an index statement) and the error wraps ErrUnsupportedAcceptedBlocking with it; errors.Is still holds; AB-3's Enforced line names the renamed function
C2-F3	the cascade refusal does not name the supported retry	fixed — the detail and the executor reason add "or resubmit without CASCADE; PostgreSQL then refuses the drop if anything depends on the index"; lock-budgeted-passthrough.md says the same, and the README line is re-wrapped
C2-F1	IndexTarget and IndexRelation() comments still read as a relation count	fixed — the current model is kept and the comments document IndexTargetOther as wider than one relation for any reason (several indexes, a schema or database scope, or a cascading drop, see Cascades()), with IndexRelation() empty for it

The concurrent-form detail also drops "yet", since these rows are T3 with no roadmap item (raised on #188).

Source: block/pg-sprite#189, review comments 6230051382 and 6230051890 and review 5556373128 at head 9b60ce2.