From 8f7ef16d37ae77eb7a673c54fba690917092ef03 Mon Sep 17 00:00:00 2001 From: Kiran Muddukrishna Date: Thu, 29 Oct 2026 10:03:59 +1100 Subject: [PATCH 1/2] migrate: require --allow-destructive for an accepted DROP INDEX --accept-blocking accepts the lock, not the loss of the index. A dropped index the application still reads through is a common mistake, so the refusal lists the checks to make before acknowledging it. --- README.md | 8 ++- demo/tour.sh | 47 +++++++++++---- docs/capabilities.md | 9 ++- docs/cli-output-examples.md | 9 ++- docs/invariants.md | 13 ++-- docs/limitations.md | 4 +- docs/lock-budgeted-passthrough.md | 8 +++ docs/refusal-classes.md | 2 +- docs/schemabot-integration.md | 4 +- .../cli/accept_blocking_integration_test.go | 7 ++- internal/cli/allow_destructive_test.go | 12 ++-- internal/cli/cli.go | 13 ++-- internal/cli/docs_test.go | 1 + pkg/capabilities/capabilities.yaml | 2 +- pkg/migrate/accept_blocking.go | 16 +++++ .../accept_blocking_integration_test.go | 59 ++++++++++++++++++- pkg/migrate/allow_destructive.go | 29 +++++++-- .../allow_destructive_integration_test.go | 4 +- pkg/migrate/allow_destructive_test.go | 15 ----- pkg/migrate/migrate.go | 28 ++++----- 20 files changed, 207 insertions(+), 83 deletions(-) diff --git a/README.md b/README.md index 7641db99..29e82a02 100644 --- a/README.md +++ b/README.md @@ -132,7 +132,10 @@ refusal — never a silently wrong or incomplete result: `--allow-destructive SCHEMA.TABLE` names the table; the acknowledged run is audited and its verdict carries `allowed_destructive: true`. A drop that adds the same column back gets the guidance to change its type in place, - which keeps its values. + which keeps its values. A `DROP INDEX` run under `--accept-blocking` needs + `--allow-destructive` too: accepting the lock is not accepting the loss, + and the refusal lists the checks (`idx_scan` on every replica, foreign + keys, uniqueness) that catch an index the application still uses. - **Destructive desired-state plans are refused as a whole** — `migrate --desired` creates missing tables and converges supported changes, but never infers permission to discard live structure. A new primary key or unique constraint over a live @@ -150,7 +153,8 @@ refusal — never a silently wrong or incomplete result: `migrate --accept-blocking SCHEMA.TABLE` runs an otherwise-refused plain `DROP INDEX` (not `CASCADE`), `REINDEX INDEX`, or `REINDEX TABLE` deliberately under the engine's bounded lock and statement budgets, with - the verdict marked `executed-without-online-safety` (exit 3). Every other + the verdict marked `executed-without-online-safety` (exit 3). A `DROP INDEX` + also needs `--allow-destructive` naming the same table. Every other refusal — including a blocking index build on a partitioned parent — still exits 2. Design: [docs/lock-budgeted-passthrough.md](docs/lock-budgeted-passthrough.md). diff --git a/demo/tour.sh b/demo/tour.sh index ffea6e8a..b19e3eba 100755 --- a/demo/tour.sh +++ b/demo/tour.sh @@ -203,31 +203,55 @@ execute_refused() { fi } -# execute_accepted table sql +# execute_accepted table allowed_destructive sql # # Runs a gate-refused blocking index statement under the --accept-blocking -# acknowledgement and requires the without-online-safety verdict: its own +# acknowledgement plus --allow-destructive (which a DROP INDEX needs and a +# REINDEX ignores), and requires the without-online-safety verdict: its own # exit code 3 (never 0, which stays exclusive to online-safe execution), -# the passthrough marker, and the table the acknowledgement named. The -# statement budget is spelled on the command line because the flag refuses -# to inherit the default as the accepted outage bound. +# the passthrough marker, whether the verdict records an acknowledged loss, +# and the table the acknowledgements named. The statement budget is spelled +# on the command line because the flag refuses to inherit the default as +# the accepted outage bound. execute_accepted() { - local table="$1" sql="$2" out status=0 - step "execute (accept blocking on $table): $sql" + local table="$1" allowed="$2" sql="$3" out status=0 + step "execute (accept blocking and the loss on $table): $sql" if [ "$CHECK" = 1 ]; then out=$("$PGS" migrate --url "$PG_DSN" --json --alter "$sql" \ - --accept-blocking "$table" --statement-timeout 30s) || status=$? + --accept-blocking "$table" --allow-destructive "$table" --statement-timeout 30s) || status=$? assert_eq "accepted exit of [$sql]" 3 "$status" assert_eq "outcome of [$sql]" "executed-without-online-safety" "$(jq -r '.outcome' <<<"$out")" assert_eq "blocking_passthrough of [$sql]" true "$(jq -r '.blocking_passthrough' <<<"$out")" + assert_eq "allowed_destructive of [$sql]" "$allowed" "$(jq -r '.allowed_destructive // false' <<<"$out")" assert_eq "table of [$sql]" "$table" "$(jq -r '.table' <<<"$out")" assert_eq "statement_timeout of [$sql]" 30s "$(jq -r '.statement_timeout' <<<"$out")" else - "$PGS" migrate --url "$PG_DSN" --alter "$sql" --accept-blocking "$table" --statement-timeout 30s \ + "$PGS" migrate --url "$PG_DSN" --alter "$sql" --accept-blocking "$table" --allow-destructive "$table" \ + --statement-timeout 30s \ || echo "(exit $? — expected 3: committed without an online-safety guarantee)" fi } +# execute_accepted_needs_loss table sql +# +# The same DROP INDEX under --accept-blocking alone: accepting the lock is +# not accepting the loss, so it is refused (exit 2, destructive-change) and +# nothing runs. +execute_accepted_needs_loss() { + local table="$1" sql="$2" out status=0 + step "execute (accept blocking only on $table): $sql" + if [ "$CHECK" = 1 ]; then + out=$("$PGS" migrate --url "$PG_DSN" --json --alter "$sql" \ + --accept-blocking "$table" --statement-timeout 30s) || status=$? + assert_eq "refusal exit of [$sql]" 2 "$status" + assert_eq "reason of [$sql]" "destructive-change" "$(jq -r '.reason' <<<"$out")" + assert_eq "table of [$sql]" "$table" "$(jq -r '.table' <<<"$out")" + else + "$PGS" migrate --url "$PG_DSN" --alter "$sql" --accept-blocking "$table" --statement-timeout 30s \ + || echo "(refused, exit $? — expected 2: a dropped index also needs --allow-destructive)" + fi +} + # execute_accept_mismatch wrong_table sql # # The acknowledgement names the table the statement locks; naming another @@ -486,8 +510,9 @@ run_exec() { # runs first so the index is still there to drop afterwards. execute_refused index-statement "DROP INDEX orders_total_idx" execute_accept_mismatch public.users "DROP INDEX orders_total_idx" - execute_accepted public.orders "DROP INDEX orders_total_idx" - execute_accepted public.orders "REINDEX TABLE orders" + execute_accepted_needs_loss public.orders "DROP INDEX orders_total_idx" + execute_accepted public.orders true "DROP INDEX orders_total_idx" + execute_accepted public.orders false "REINDEX TABLE orders" # A drop discards the column's values, so it is refused until the # acknowledgement names the table, then runs natively. refused_destructive "ALTER TABLE users DROP COLUMN status" diff --git a/docs/capabilities.md b/docs/capabilities.md index 68328175..25985f0c 100644 --- a/docs/capabilities.md +++ b/docs/capabilities.md @@ -118,7 +118,8 @@ them is a limitation: silently run the blocking form. Index maintenance (`DROP INDEX`, `REINDEX`) is the second case: the online form is a plain `CONCURRENTLY` statement, which `migrate` refuses to wrap and names so you run it directly; for a single-relation plain form, - `--accept-blocking` runs it under budgets and exits 3. + `--accept-blocking` runs it under budgets and exits 3 (a `DROP INDEX` also needs + `--allow-destructive`). Every matrix table carries an **"Online-safety problem?"** column: "Yes" means there is a readers-and-writers problem for an online engine to solve (pg-sprite solves it, plans to, @@ -240,7 +241,7 @@ or the only online form is a direct statement the operator runs). | Operation | Status | Engine path | Online-safety problem? | Behavior and why | | --- | --- | --- | --- | --- | | `CREATE [UNIQUE] INDEX` on a plain table — including partial, expression, covering (`INCLUDE`), GIN/GiST/BRIN | ✅ | native, safer sequence | Yes | Executed as (or rewritten to) `CREATE INDEX CONCURRENTLY`, with validity verification, typed invalid-index outcomes, and a proven recovery for abandoned leftovers (`RebuildAbandonedIndex`, or `DropAbandonedIndex` to remove the leftover without rebuilding; both library-only; [runbook](invalid-index-recovery.md)) | -| `DROP INDEX` | ❌ | — | Yes — the online form is a direct `CONCURRENTLY` statement that `migrate` does not drive | `migrate` never runs this statement online-safely: the plain form is refused and names `DROP INDEX CONCURRENTLY`, and the concurrent form is refused as already safe — run it directly against the database. A single-index plain `DROP INDEX` without `CASCADE` can be run as-is under bounded budgets with `--accept-blocking SCHEMA.TABLE` (exit 3, never 0); `--accept-blocking` checks the replica identity under the table's lock and refuses the drop as `unsupported-statement`, naming the identity index. `diff` reports an index to remove as a **destructive** `DROP INDEX` and hints to run it through `migrate --alter`, which ends in this `index-statement` refusal — run `DROP INDEX CONCURRENTLY` directly instead. `diff` refuses dropping the table's replica identity index, or one a partition's is attached under, with `replica-identity`. `plan` and `lint` refuse `DROP INDEX … CASCADE` with `cascading-drop`, since `DROP INDEX CONCURRENTLY` does not accept `CASCADE` | +| `DROP INDEX` | ❌ | — | Yes — the online form is a direct `CONCURRENTLY` statement that `migrate` does not drive | `migrate` never runs this statement online-safely: the plain form is refused and names `DROP INDEX CONCURRENTLY`, and the concurrent form is refused as already safe — run it directly against the database. A single-index plain `DROP INDEX` without `CASCADE` can be run as-is under bounded budgets with `--accept-blocking SCHEMA.TABLE` plus `--allow-destructive SCHEMA.TABLE` for the loss (exit 3, never 0; without the second acknowledgement it is refused with `destructive-change`); `--accept-blocking` checks the replica identity under the table's lock and refuses the drop as `unsupported-statement`, naming the identity index. `diff` reports an index to remove as a **destructive** `DROP INDEX` and hints to run it through `migrate --alter`, which ends in this `index-statement` refusal — run `DROP INDEX CONCURRENTLY` directly instead. `diff` refuses dropping the table's replica identity index, or one a partition's is attached under, with `replica-identity`. `plan` and `lint` refuse `DROP INDEX … CASCADE` with `cascading-drop`, since `DROP INDEX CONCURRENTLY` does not accept `CASCADE` | | `REINDEX` | ❌ | — | Yes — the online form is a direct `CONCURRENTLY` statement that `migrate` does not drive | `migrate` never runs this statement online-safely: the plain form is refused and names `REINDEX ... CONCURRENTLY`, and the concurrent form is refused as already safe — run it directly against the database. A plain `REINDEX INDEX` or `REINDEX TABLE` can be run as-is under bounded budgets with `--accept-blocking SCHEMA.TABLE` (exit 3, never 0). `REINDEX SCHEMA`, `DATABASE`, and `SYSTEM` stay refused. A desired schema cannot express a reindex, so `diff` never produces one | | `ALTER INDEX ... RENAME TO` | ✅ | native, as-is | Yes | Metadata-only: application SQL names tables and columns, not indexes. `migrate` resolves the index's owning table from the catalog, preflights and privilege-checks that table, and runs the rename under the brief budgets after locking the table in `SHARE UPDATE EXCLUSIVE` and re-checking that the name is still a plain index of it, then confirms before the commit that the index now carrying the new name is the one it checked, since the rename locks only the index and another session can move the name meanwhile (else the permanent `renamed-index-drift` code, nothing renamed — `ALTER INDEX ... RENAME` would also rename a table). An index that backs a primary-key, unique, or exclusion constraint renames the constraint, which `ON CONFLICT ON CONSTRAINT` names, so it is refused as `rewrite-required` (`app-breaking-rename`, guidance `coordinated-constraint-rename`); `--force` does not apply to an index rename. Offline `lint` cannot tell the two kinds of index apart and reports neither. A desired file cannot express a rename | | Index build on a **partitioned parent** | 🟡 | native, planned flow | Yes | PostgreSQL has no parent-level `CONCURRENTLY`; the blocking form is refused by policy (`--force` does not bypass it, and `--accept-blocking` does not reach it yet). The partition-aware flow — `CREATE INDEX ON ONLY` → per-partition CIC → `ATTACH PARTITION`, with crash-resume per leaf — is planned | @@ -371,7 +372,9 @@ unmistakably marked as executed without an online-safety guarantee resolved from the catalog, and `--statement-timeout` must be spelled on the same command line. Only changes the engine understands but cannot run *safely* are eligible: today a single-relation plain `DROP INDEX` (never `CASCADE`, which also drops dependent objects such -as a foreign key on another table), `REINDEX INDEX`, or `REINDEX TABLE`. Refusals for +as a foreign key on another table), `REINDEX INDEX`, or `REINDEX TABLE`. A `DROP INDEX` +also discards the index, so it needs `--allow-destructive` naming the same table; without +it the drop is refused with `destructive-change` before anything runs. Refusals for unrecognized SQL never are, and the refused statement's `blocking_passthrough_eligible` field on the plan report says whether a refusal qualifies. An exhausted lock budget is still a refusal (exit 2, nothing ran); a statement cancelled by its budget is a failure diff --git a/docs/cli-output-examples.md b/docs/cli-output-examples.md index 6d2eae7d..97fc310d 100644 --- a/docs/cli-output-examples.md +++ b/docs/cli-output-examples.md @@ -264,7 +264,10 @@ An operator-accepted blocking refusal has a distinct marked outcome and exit takes the schema-qualified table whose lock the operator accepts, must match the index's owning table exactly (a mismatch executes nothing and exits 1), and requires an explicit `--statement-timeout`; it cannot be combined with -`--force` or `--dry-run`. The text rendering marks the outcome as a warning: +`--force` or `--dry-run`. A `DROP INDEX` also discards the index, so it needs +`--allow-destructive` naming the same table; without it the drop is refused +with `destructive-change` (exit 2) and nothing runs. The text rendering marks +the outcome as a warning: ```text executed without online safety (accepted blocking refusal) @@ -273,10 +276,11 @@ executed without online safety (accepted blocking refusal) statement: DROP INDEX public.users_email_idx safer: DROP INDEX CONCURRENTLY budgets: lock 3s, statement 10m + allowed: the statement's data loss was acknowledged (allow-destructive) ``` ```console -$ pg-sprite migrate --alter 'DROP INDEX public.users_email_idx' --accept-blocking public.users --lock-timeout 3s --statement-timeout 10m --json +$ pg-sprite migrate --alter 'DROP INDEX public.users_email_idx' --accept-blocking public.users --allow-destructive public.users --lock-timeout 3s --statement-timeout 10m --json { "outcome": "executed-without-online-safety", "reason": "index-statement", @@ -284,6 +288,7 @@ $ pg-sprite migrate --alter 'DROP INDEX public.users_email_idx' --accept-blockin "statement": "DROP INDEX public.users_email_idx", "table": "public.users", "safer_idiom": "DROP INDEX CONCURRENTLY", + "allowed_destructive": true, "blocking_passthrough": true, "lock_timeout": "3s", "statement_timeout": "10m" diff --git a/docs/invariants.md b/docs/invariants.md index 63218c22..dfb8c263 100644 --- a/docs/invariants.md +++ b/docs/invariants.md @@ -851,7 +851,8 @@ would without the flag — including a `DROP INDEX ... CASCADE`, whose refusal s ineligible one because its scope exceeds the acknowledged table — and a statement the gate admits runs its normal path. The acknowledgement is not `--force`: the two cannot be combined, and a dry run reports the -refusal as it stands rather than the accepted execution. The front door requires the +refusal as it stands rather than the accepted execution. It accepts the lock only: an +accepted `DROP INDEX` also needs the destructive acknowledgement (RF-10). The front door requires the statement budget to be stated on the same command line, because the accepted outage bound is a decision, not a default. *Enforced:* `migrate.AcceptedRefusal`, `MigrateCmd.Validate`, `migrate.gateRefusal` (the refusal site), `TestAcceptedRefusal`, `TestAcceptBlockingFlagGrammar`, @@ -1400,8 +1401,11 @@ Each refusal is a preflight **error with a stated reason** — never a warning, other gate, and an acknowledged run is audited at warn level and carries `allowed_destructive: true` on its verdict. The declarative path takes no acknowledgement and refuses a destructive plan whole. A planner refusal runs nothing and keeps its own - reason. A `DROP INDEX` stays with the statement-kind gate and `--accept-blocking`, which - the destructive acknowledgement does not reach. Outage-risk statements are covered by + reason. A `DROP INDEX` — unique or not — is gate-refused first; when `--accept-blocking` + accepts that refusal, the drop still runs only if `--allow-destructive` names the same + owning table, and otherwise it is refused with `destructive-change` before anything + executes, the detail listing the checks that catch an index the application still uses. + Accepting the lock is not accepting the loss. Outage-risk statements are covered by RF-5 and AB-1..AB-5 (`--force`, `--accept-blocking`) and by the LK-2 budgets. *Enforced:* `pkg/migrate` (`run`'s destructive gate through `destructiveOperations` and `destructiveChangeVerdict`, `checkAllowDestructiveAck`, `AdmitDestructive` for the dry @@ -1412,7 +1416,8 @@ Each refusal is a preflight **error with a stated reason** — never a warning, `TestRunRejectsAMismatchedDestructiveAcknowledgement`, `TestRunRefusesAnUnacknowledgedDropColumnRoutedToCopyAndSwap`, `TestRunNeedsBothForceAndTheDestructiveAcknowledgement`, - `TestRunAllowDestructiveDoesNotLiftADropIndexRefusal`, `TestAdmitDestructive`, + `TestRunAllowDestructiveDoesNotLiftADropIndexRefusal`, + `TestRunAcceptBlockingNeedsTheDestructiveAcknowledgementToDropAnIndex`, `TestAdmitDestructive`, `TestMigrateDryRunReportsTheDestructiveRefusal`, `TestAllowDestructiveFlagGrammar`, `demo/tour.sh` (`refused_destructive`). *Source:* [design principles](design-principles.md#declarative-review-first-workflow) diff --git a/docs/limitations.md b/docs/limitations.md index 4fb40385..d617d6d7 100644 --- a/docs/limitations.md +++ b/docs/limitations.md @@ -10,12 +10,12 @@ not escape hatches: | Change | Current behavior | | --- | --- | -| `DROP COLUMN`, `DROP CONSTRAINT`, and `ALTER COLUMN ... DROP NOT NULL` on `migrate --alter` | Refused with `destructive-change` (class `by-design`, exit 2) before any route runs the statement — native, safer sequence, `--force`, or copy-and-swap — and `--dry-run` ends in the same refusal verdict: each discards data or a guarantee the schema was providing, and pg-sprite does not infer that the loss is intended. `--allow-destructive SCHEMA.TABLE` (`Options.AllowDestructive` as a library caller) names the resolved table and runs the statement through every other gate as usual; its verdict carries `allowed_destructive: true` and the run logs one warning naming the table. A mismatched acknowledgement executes nothing and exits 1. It acknowledges only the loss: a forced type rewrite that also drops a column needs `--force` too. It cannot be combined with `--accept-blocking` or `--desired`, and it does not reach `DROP INDEX`, which keeps its own refusal below. A statement that drops a column and adds one of the same name back is refused with the `alter-column-type-in-place` guidance: `ALTER COLUMN ... TYPE ... USING ...` keeps the values the drop would discard. A planner refusal of a drop (`replica-identity`, `dependent-view`, `cascading-drop`) keeps its own reason, since no acknowledgement lifts it. | +| `DROP COLUMN`, `DROP CONSTRAINT`, and `ALTER COLUMN ... DROP NOT NULL` on `migrate --alter` | Refused with `destructive-change` (class `by-design`, exit 2) before any route runs the statement — native, safer sequence, `--force`, or copy-and-swap — and `--dry-run` ends in the same refusal verdict: each discards data or a guarantee the schema was providing, and pg-sprite does not infer that the loss is intended. `--allow-destructive SCHEMA.TABLE` (`Options.AllowDestructive` as a library caller) names the resolved table and runs the statement through every other gate as usual; its verdict carries `allowed_destructive: true` and the run logs one warning naming the table. A mismatched acknowledgement executes nothing and exits 1. It acknowledges only the loss: a forced type rewrite that also drops a column needs `--force` too. It cannot be combined with `--desired`. It does not lift a `DROP INDEX` refusal on its own; it is the second acknowledgement a `DROP INDEX` under `--accept-blocking` needs (below). A statement that drops a column and adds one of the same name back is refused with the `alter-column-type-in-place` guidance: `ALTER COLUMN ... TYPE ... USING ...` keeps the values the drop would discard. A planner refusal of a drop (`replica-identity`, `dependent-view`, `cascading-drop`) keeps its own reason, since no acknowledgement lifts it. | | Index builds on a partitioned parent | PostgreSQL cannot build an index concurrently at the parent level. PostgreSQL supports a plain blocking build, but pg-sprite refuses it by policy because it takes `ACCESS EXCLUSIVE`; `--force` does not bypass this decision. `migrate --accept-blocking` does not reach this refusal yet either — it covers the single-relation `DROP INDEX` / `REINDEX` refusals — so an operator who chooses a maintenance-window blocking build runs it outside the CLI today; a library caller can run it through `executor.ExecuteAcceptedBlocking`, which marks the verdict as executed without online safety ([lock-budgeted-passthrough.md](lock-budgeted-passthrough.md)). The partition-aware `CREATE INDEX ON ONLY` → per-partition `CREATE INDEX CONCURRENTLY` → `ATTACH PARTITION` flow is planned but not yet implemented. | | `ADD CONSTRAINT ... USING INDEX` on a partitioned parent | PostgreSQL does not support adopting an existing index on a partitioned parent in any supported version. pg-sprite refuses before execution. | | `ADD PRIMARY KEY` / `ADD UNIQUE` with a storage parameter the online sequence cannot render | The online sequence builds the key's index with `CREATE UNIQUE INDEX CONCURRENTLY` and carries the key's `WITH (...)` parameters onto it, each name quoted and each value a quoted literal. A parameter it cannot render faithfully, one whose value is not a plain word, number, or string, contains a backslash, or whose name is namespaced, would be dropped or changed, so no sequence is built. Running the statement as written builds the index under `ACCESS EXCLUSIVE`, so it is refused as `rewrite-required` with the `unique-index-then-constraint` guidance. Build the index concurrently with the parameter, then attach it with `ADD CONSTRAINT ... USING INDEX`. | | `ADD FOREIGN KEY ... NOT VALID` on a partitioned parent | PostgreSQL does not support this before version 18, so pg-sprite refuses it on versions 14–17. It is supported on version 18 and later. | -| `DROP INDEX` and `REINDEX`, plain or `CONCURRENTLY` | `migrate` never runs index maintenance online-safely. The plain form is refused with `index-statement` and names its `CONCURRENTLY` idiom; the concurrent form is refused as already safe, so run it directly against the database. `migrate --accept-blocking SCHEMA.TABLE` runs a single-relation plain `DROP INDEX`, `REINDEX INDEX`, or `REINDEX TABLE` as-is under bounded budgets and exits 3 ([lock-budgeted-passthrough.md](lock-budgeted-passthrough.md)). `diff`'s hint to run a removed index through `migrate --alter` ends in this refusal; run `DROP INDEX CONCURRENTLY` directly. | +| `DROP INDEX` and `REINDEX`, plain or `CONCURRENTLY` | `migrate` never runs index maintenance online-safely. The plain form is refused with `index-statement` and names its `CONCURRENTLY` idiom; the concurrent form is refused as already safe, so run it directly against the database. `migrate --accept-blocking SCHEMA.TABLE` runs a single-relation plain `DROP INDEX`, `REINDEX INDEX`, or `REINDEX TABLE` as-is under bounded budgets and exits 3 ([lock-budgeted-passthrough.md](lock-budgeted-passthrough.md)). A `DROP INDEX`, unique or not, also needs `--allow-destructive` naming the same table: without it the drop is refused with `destructive-change` (exit 2) before anything runs, and the detail lists the checks to make first — `idx_scan` is 0 on the primary and every replica over a full application cycle, no foreign key relies on the index, and nothing needs a unique index's guarantee. `diff`'s hint to run a removed index through `migrate --alter` ends in this refusal; run `DROP INDEX CONCURRENTLY` directly. | | `RENAME COLUMN` and `RENAME TO` | Without `--force`, refused with `not-native-safe-rewrite-required`, nothing executed: the catalog flip is instant, but every running application instance that still queries the old name breaks the moment it commits, and no safer sequence can be built from the statement alone because the dual-write and the read switch live in the application. The plan report, `suggest`, and the verdict carry the manual path as typed guidance — `expand-contract-column` for a column (add the new column, dual-write and backfill, switch reads, drop the old column as its own reviewed change) and `coordinated-table-rename` for a table (coordinate the rename with the deploy that adopts the new name, or keep the old name readable through a view until every instance has moved) — and `lint` reports the rename as an error. `--force SCHEMA.TABLE` runs the submitted rename as a blind bounded attempt under the usual budgets and size guard, for the operator who has coordinated the deploy; on a table above `--max-table-size` (default 1 GiB) the forced rename is refused as `not-native-safe-table-too-large` until that threshold is raised. | | `ALTER INDEX ... RENAME TO` on an index that backs a primary-key, unique, or exclusion constraint | Refused with `not-native-safe-rewrite-required`, class `by-design`, guidance `coordinated-constraint-rename`, nothing executed: PostgreSQL renames the constraint with its index, and `ON CONFLICT ON CONSTRAINT` and error handling that name the old constraint break the moment it commits. `--force` does not apply to an index rename. A plain index rename runs. The refusal needs the catalog, so offline `lint` and `suggest` do not report it. | | `ATTACH PARTITION` | Without `--force`, refused with `not-native-safe-rewrite-required`, nothing executed: the engine does not yet construct the validated `CHECK` that lets the attach skip its scan. Pre-prove the bound yourself, then run the attach directly. `--force SCHEMA.TABLE` runs the blocking `ATTACH PARTITION` as submitted under bounded budgets, taking the blocking lock. | diff --git a/docs/lock-budgeted-passthrough.md b/docs/lock-budgeted-passthrough.md index 96e6debd..387a71a8 100644 --- a/docs/lock-budgeted-passthrough.md +++ b/docs/lock-budgeted-passthrough.md @@ -133,10 +133,18 @@ a bare boolean: pg-sprite migrate \ --alter 'DROP INDEX app.orders_created_at_idx' \ --accept-blocking app.orders \ + --allow-destructive app.orders \ --lock-timeout 3s \ --statement-timeout 10m ``` +A `DROP INDEX` needs both acknowledgements, each naming the owning table: `--accept-blocking` +accepts the lock, and `--allow-destructive` accepts losing the index (RF-10). The second is +checked after the owning table resolves and before anything executes; without it the drop is +refused with `destructive-change`, and the detail lists the checks that catch an index the +application still uses. `REINDEX INDEX` and `REINDEX TABLE` discard nothing and need only +`--accept-blocking`. + The value names the table whose lock the operator accepts, because that is the relation the blocking form makes unavailable: a plain `DROP INDEX` takes `ACCESS EXCLUSIVE` on the index's table, and a parent index build locks the parent. The partitioned-parent statements and diff --git a/docs/refusal-classes.md b/docs/refusal-classes.md index cd833aa1..fcd0d265 100644 --- a/docs/refusal-classes.md +++ b/docs/refusal-classes.md @@ -84,7 +84,7 @@ routed to provisioning and never to the data change runner. | `not-native-safe-rewrite-required` | The submitted form blocks the table and the planner cannot construct the required safer sequence | `capability-boundary` | — | | `not-native-safe-rewrite-required` | A `RENAME COLUMN` or `RENAME TO` (`app-breaking-rename`): no SQL exists to substitute, and the deliberate path is the `guidance` rollout and then the statement under `--force` | `by-design` | — | | `backend-unavailable` | The plan routes to copy-and-swap for an operation the engine does not carry yet | `capability-boundary` | — | -| `destructive-change` | Neither front door infers permission to discard live data or structure: desired-state admission refuses a destructive plan whole, and `migrate --alter` refuses a statement that drops a column, constraint, or `NOT NULL` until `--allow-destructive` names the table (RF-10) | `by-design` | — | +| `destructive-change` | Neither front door infers permission to discard live data or structure: desired-state admission refuses a destructive plan whole, and `migrate --alter` refuses a statement that drops a column, constraint, or `NOT NULL` — and a `DROP INDEX` under `--accept-blocking` — until `--allow-destructive` names the table (RF-10) | `by-design` | — | | `plan-fingerprint-mismatch` | The recomputed plan is not the reviewed plan | `environmental` | — | | `create-collision` | A target or claimed name is occupied at apply time by a relation or a standalone type | `environmental` | — | | `declarative-foreign-key` | A desired-state plan adds or drops a foreign key, or creates a table that declares one; desired-state execution does not run foreign-key changes yet | `capability-boundary` | — | diff --git a/docs/schemabot-integration.md b/docs/schemabot-integration.md index 1075dbac..4b1f46a0 100644 --- a/docs/schemabot-integration.md +++ b/docs/schemabot-integration.md @@ -631,7 +631,9 @@ and PostgreSQL `lock_timeout`). Contract points for the PostgreSQL adapter: acknowledgement is `migrate.ErrAllowDestructiveMismatch` with a zero verdict and nothing executed. An acknowledged run's verdict carries `allowed_destructive: true`, the machine-readable audit record to store beside the operator's consent. The acknowledgement - covers only the loss: a statement that also needs `Force` still needs it. `RunDesired` + covers only the loss: a statement that also needs `Force` still needs it, and a `DROP INDEX` + run with `Options.AcceptBlocking` needs both, each naming the index's owning table. + `RunDesired` rejects `AllowDestructive` (`ErrAllowDestructiveNotSupported`) and keeps refusing a destructive plan whole, so a declarative drop the operator approves runs as its own imperative statement. diff --git a/internal/cli/accept_blocking_integration_test.go b/internal/cli/accept_blocking_integration_test.go index af4943c9..9dec9657 100644 --- a/internal/cli/accept_blocking_integration_test.go +++ b/internal/cli/accept_blocking_integration_test.go @@ -15,8 +15,9 @@ import ( "github.com/block/pg-sprite/pkg/verdict" ) -// --accept-blocking turns the gate refusal of a plain DROP INDEX into a -// bounded blocking execution: the command dials, runs the statement as +// --accept-blocking, with --allow-destructive for the dropped index, turns +// the gate refusal of a plain DROP INDEX into a bounded blocking execution: +// the command dials, runs the statement as // submitted, prints the without-online-safety verdict, and returns the // sentinel the entry point maps to its own exit code — never the refusal's // and never the plain success's. @@ -36,6 +37,7 @@ func TestMigrateAcceptBlockingRunsDropIndex(t *testing.T) { cmd := newMigrateCmd(url, fmt.Sprintf("DROP INDEX %s.users_email_idx", schema)) cmd.AcceptBlocking = schema + ".users" + cmd.AllowDestructive = schema + ".users" cmd.JSON = true var out strings.Builder err = cmd.run(t.Context(), &out) @@ -46,6 +48,7 @@ func TestMigrateAcceptBlockingRunsDropIndex(t *testing.T) { require.NoError(t, json.Unmarshal([]byte(out.String()), &v)) assert.Equal(t, verdict.OutcomeExecutedWithoutOnlineSafety, v.Outcome) assert.True(t, v.BlockingPassthrough) + assert.True(t, v.AllowedDestructive) assert.Equal(t, schema+".users", v.Table) assert.Equal(t, verdict.ReasonIndexStatement, v.Reason) assert.Equal(t, "3s", v.LockTimeout) diff --git a/internal/cli/allow_destructive_test.go b/internal/cli/allow_destructive_test.go index 1518c40d..12dd2171 100644 --- a/internal/cli/allow_destructive_test.go +++ b/internal/cli/allow_destructive_test.go @@ -10,9 +10,9 @@ import ( // --allow-destructive acknowledges data loss, a risk none of the other // acknowledgements covers, so the grammar combines it with --force, -// --dry-run, and --restart, and refuses it beside --accept-blocking, the -// separate policy path for refused index statements, and beside --desired, -// which refuses destructive plans whole. +// --dry-run, --restart, and --accept-blocking (an accepted DROP INDEX +// needs both), and refuses it beside --desired, which refuses destructive +// plans whole. func TestAllowDestructiveFlagGrammar(t *testing.T) { parse := func(args ...string) error { c := New("test") @@ -43,10 +43,8 @@ func TestAllowDestructiveFlagGrammar(t *testing.T) { require.NoError(t, parse("--restart", "public.orders")) }) - t.Run("rejects --accept-blocking", func(t *testing.T) { - err := parse("--accept-blocking", "public.orders", "--statement-timeout", "10m") - require.Error(t, err) - assert.Contains(t, err.Error(), "--allow-destructive cannot be combined with --accept-blocking") + t.Run("combines with --accept-blocking", func(t *testing.T) { + require.NoError(t, parse("--accept-blocking", "public.orders", "--statement-timeout", "10m")) }) t.Run("rejects --desired", func(t *testing.T) { diff --git a/internal/cli/cli.go b/internal/cli/cli.go index b64862f0..f7fa9e43 100644 --- a/internal/cli/cli.go +++ b/internal/cli/cli.go @@ -112,8 +112,8 @@ type MigrateCmd struct { IndexBuildTimeout time.Duration `help:"Overall bound (statement_timeout) for one concurrent index build step; expect large tables to need a generous value." default:"30m"` ValidateTimeout time.Duration `help:"Overall bound (statement_timeout) for one VALIDATE CONSTRAINT step; expect large tables to need a generous value." default:"30m"` Force string `help:"Run the submitted form as-is, overriding a safer-sequence substitution or a rewrite-required/backend-unavailable refusal. The value is the typed acknowledgement: it must name the resolved schema-qualified target table exactly. The forced run is still parsed, preflighted, size-guarded, and budget-bounded; planner refusals (no known safe path) and unsupported statement kinds cannot be forced." placeholder:"SCHEMA.TABLE"` - AcceptBlocking string `help:"Run one refused blocking index statement as-is, without online safety: a plain single-relation DROP INDEX (not CASCADE), REINDEX INDEX, or REINDEX TABLE. The value is the typed acknowledgement: it must name the schema-qualified table whose lock you accept (the index's owning table) exactly. The statement runs in an engine-owned transaction under --lock-timeout and an explicitly supplied --statement-timeout, which must be longer than --lock-timeout; an exhausted lock budget refuses (exit 2), a cancelled or failed statement fails (exit 1), a commit exits 3. Every other refusal stands. Cannot be combined with --force or --dry-run." placeholder:"SCHEMA.TABLE"` - AllowDestructive string `help:"Run a statement that discards live data or structure: DROP COLUMN, DROP CONSTRAINT, or ALTER COLUMN ... DROP NOT NULL. Without it such a statement is refused (exit 2, reason destructive-change) before anything runs, on every route it could take. The value is the typed acknowledgement: it must name the resolved schema-qualified target table exactly. It acknowledges only the loss: the statement is still classified, routed, preflighted, size-guarded, and budget-bounded, and a statement that also needs --force needs both. With --dry-run the plan is reported as the acknowledged run would take it. Cannot be combined with --accept-blocking or --desired." name:"allow-destructive" placeholder:"SCHEMA.TABLE"` + AcceptBlocking string `help:"Run one refused blocking index statement as-is, without online safety: a plain single-relation DROP INDEX (not CASCADE), REINDEX INDEX, or REINDEX TABLE. The value is the typed acknowledgement: it must name the schema-qualified table whose lock you accept (the index's owning table) exactly. The statement runs in an engine-owned transaction under --lock-timeout and an explicitly supplied --statement-timeout, which must be longer than --lock-timeout; an exhausted lock budget refuses (exit 2), a cancelled or failed statement fails (exit 1), a commit exits 3. Every other refusal stands. A DROP INDEX also needs --allow-destructive naming the same table, or it is refused (exit 2, reason destructive-change). Cannot be combined with --force or --dry-run." placeholder:"SCHEMA.TABLE"` + AllowDestructive string `help:"Run a statement that discards live data or structure: DROP COLUMN, DROP CONSTRAINT, ALTER COLUMN ... DROP NOT NULL, or a DROP INDEX run under --accept-blocking. Without it such a statement is refused (exit 2, reason destructive-change) before anything runs, on every route it could take. The value is the typed acknowledgement: it must name the resolved schema-qualified target table exactly (for a DROP INDEX, the index's owning table). It acknowledges only the loss: the statement is still classified, routed, preflighted, size-guarded, and budget-bounded, and a statement that also needs --force or --accept-blocking needs both. With --dry-run the plan is reported as the acknowledged run would take it. Cannot be combined with --desired." name:"allow-destructive" placeholder:"SCHEMA.TABLE"` LockAttempts int `help:"Maximum bounded attempts when native DDL exceeds lock_timeout; 1 disables retry." default:"3"` LockBackoff time.Duration `help:"Initial exponential backoff between lock-timeout attempts." default:"100ms"` LockBackoffMax time.Duration `help:"Maximum exponential backoff between lock-timeout attempts." default:"1s"` @@ -144,9 +144,9 @@ type MigrateCmd struct { // copy-and-swap. --allow-destructive acknowledges data loss, a different // risk from the one --force accepts, so the two combine; a dry run with it // reports the plan the acknowledged run would take, and a restart re-runs -// the same statement, so both combine too. It cannot ride with -// --accept-blocking, a separate policy path the statement-kind gate takes -// before the destructive gate is reached. +// the same statement, so both combine too. It also combines with +// --accept-blocking, which an accepted DROP INDEX needs both of: one +// accepts the lock, the other the loss. func (c *MigrateCmd) Validate(kctx *kong.Context) error { if err := c.validateInput(); err != nil { return err @@ -160,9 +160,6 @@ func (c *MigrateCmd) Validate(kctx *kong.Context) error { if c.DryRun && c.AcceptBlocking != "" { return errors.New("--accept-blocking cannot be combined with --dry-run: the dry run reports the refusal as it stands") } - if c.AllowDestructive != "" && c.AcceptBlocking != "" { - return errors.New("--allow-destructive cannot be combined with --accept-blocking: accepted blocking is a separate policy path for refused index statements, which the destructive acknowledgement does not govern") - } if c.Force != "" && c.AcceptBlocking != "" { return errors.New("--accept-blocking cannot be combined with --force: each acknowledges a different decision, and a statement reaches one") } diff --git a/internal/cli/docs_test.go b/internal/cli/docs_test.go index 6e9578ee..54c81745 100644 --- a/internal/cli/docs_test.go +++ b/internal/cli/docs_test.go @@ -100,6 +100,7 @@ func TestCLIOutputExamplesMatchPipelineOutput(t *testing.T) { verdict.ByDesign(verdict.ReasonIndexStatement).WithSite(verdict.RefusalSiteIndexSingleRelation), 3*time.Second, 10*time.Minute) require.NoError(t, err) + acceptedBlocking.AllowedDestructive = true rewriteRequired := alterReport(t, "ALTER TABLE users ADD COLUMN nickname text UNIQUE", "public", "users", usersFacts()) copyAndSwapRouted := alterReport(t, "ALTER TABLE users ALTER COLUMN id TYPE text", diff --git a/pkg/capabilities/capabilities.yaml b/pkg/capabilities/capabilities.yaml index 068ffda1..2ebba8e0 100644 --- a/pkg/capabilities/capabilities.yaml +++ b/pkg/capabilities/capabilities.yaml @@ -242,7 +242,7 @@ rows: migrate: refused diff: supported refusal_reason: index-statement - reason_notes: "`migrate` never runs this statement online-safely: the plain form is refused and names `DROP INDEX CONCURRENTLY`, and the concurrent form is refused as already safe — run it directly against the database. A single-index plain `DROP INDEX` without `CASCADE` can be run as-is under bounded budgets with `--accept-blocking SCHEMA.TABLE` (exit 3, never 0); `--accept-blocking` checks the replica identity under the table's lock and refuses the drop as `unsupported-statement`, naming the identity index. `diff` reports an index to remove as a **destructive** `DROP INDEX` and hints to run it through `migrate --alter`, which ends in this `index-statement` refusal — run `DROP INDEX CONCURRENTLY` directly instead. `diff` refuses dropping the table's replica identity index, or one a partition's is attached under, with `replica-identity`. `plan` and `lint` refuse `DROP INDEX … CASCADE` with `cascading-drop`, since `DROP INDEX CONCURRENTLY` does not accept `CASCADE`" + reason_notes: "`migrate` never runs this statement online-safely: the plain form is refused and names `DROP INDEX CONCURRENTLY`, and the concurrent form is refused as already safe — run it directly against the database. A single-index plain `DROP INDEX` without `CASCADE` can be run as-is under bounded budgets with `--accept-blocking SCHEMA.TABLE` plus `--allow-destructive SCHEMA.TABLE` for the loss (exit 3, never 0; without the second acknowledgement it is refused with `destructive-change`); `--accept-blocking` checks the replica identity under the table's lock and refuses the drop as `unsupported-statement`, naming the identity index. `diff` reports an index to remove as a **destructive** `DROP INDEX` and hints to run it through `migrate --alter`, which ends in this `index-statement` refusal — run `DROP INDEX CONCURRENTLY` directly instead. `diff` refuses dropping the table's replica identity index, or one a partition's is attached under, with `replica-identity`. `plan` and `lint` refuse `DROP INDEX … CASCADE` with `cascading-drop`, since `DROP INDEX CONCURRENTLY` does not accept `CASCADE`" - id: reindex area: "indexes" operation: "`REINDEX`" diff --git a/pkg/migrate/accept_blocking.go b/pkg/migrate/accept_blocking.go index 4dbb4647..ad1883be 100644 --- a/pkg/migrate/accept_blocking.go +++ b/pkg/migrate/accept_blocking.go @@ -71,6 +71,21 @@ func acceptBlocking(ctx context.Context, pool *pgxpool.Pool, st statement.Statem return verdict.Verdict{}, fmt.Errorf("%w: %s locks %q, got %q; nothing was executed", ErrAcceptBlockingMismatch, st.Kind(), table, opts.AcceptBlocking) } + if opts.AllowDestructive != "" { + if err := checkAllowDestructiveAck(table, opts.AllowDestructive); err != nil { + return verdict.Verdict{}, err + } + } + // INV: RF-10 — accepting the lock is not accepting the loss: a dropped + // index runs only when the destructive acknowledgement also names its + // table. + dropsIndex := st.Kind() == statement.KindDropIndex + if dropsIndex { + if opts.AllowDestructive == "" { + return dropIndexDestructiveVerdict(st, table), nil + } + auditAllowDestructive(opts.audit(), st, table) + } budget := executor.BlockingBudget{ LockTimeout: opts.Budget.Brief.LockTimeout, StatementTimeout: opts.Budget.Brief.StatementTimeout, @@ -131,6 +146,7 @@ func acceptBlocking(ctx context.Context, pool *pgxpool.Pool, st statement.Statem return verdict.Verdict{}, fmt.Errorf("%w: %w", executor.ErrInvariantViolation, err) } v.Table = table + v.AllowedDestructive = dropsIndex v.Detail = fmt.Sprintf("the operator accepted the blocking form: it ran as-is under budgets (lock %s, statement %s) "+ "with no online-safety guarantee; %s was locked for the statement's duration", budget.LockTimeout, budget.StatementTimeout, table) diff --git a/pkg/migrate/accept_blocking_integration_test.go b/pkg/migrate/accept_blocking_integration_test.go index bd096e16..0ed8f957 100644 --- a/pkg/migrate/accept_blocking_integration_test.go +++ b/pkg/migrate/accept_blocking_integration_test.go @@ -19,10 +19,13 @@ import ( // acceptBlockingOptions is the runnable policy with the acknowledgement // set and the brief budgets tightened so a contended or slow statement -// resolves in well under a second. +// resolves in well under a second. The destructive acknowledgement names +// the same table, because an accepted DROP INDEX needs both; it changes +// nothing for a REINDEX, which discards nothing. func acceptBlockingOptions(table string) migrate.Options { opts := runOptions() opts.AcceptBlocking = table + opts.AllowDestructive = table opts.Budget.Brief = executor.Budget{LockTimeout: 100 * time.Millisecond, StatementTimeout: 5 * time.Second} return opts } @@ -80,6 +83,7 @@ func TestRunAcceptBlockingExecutesEligibleRefusals(t *testing.T) { acceptBlockingOptions(schema+".orders")) require.NoError(t, err) assertAccepted(t, v, schema) + assert.True(t, v.AllowedDestructive, "a dropped index is a destructive change the verdict records") assert.False(t, indexExists(t, pool, schema, "orders_created_idx"), "the accepted DROP INDEX must have committed") }) @@ -91,6 +95,7 @@ func TestRunAcceptBlockingExecutesEligibleRefusals(t *testing.T) { acceptBlockingOptions(schema+".orders")) require.NoError(t, err) assertAccepted(t, v, schema) + assert.False(t, v.AllowedDestructive, "a REINDEX discards nothing") assert.True(t, indexExists(t, pool, schema, "orders_created_idx"), "REINDEX keeps the index") }) @@ -207,6 +212,58 @@ func TestRunAcceptBlockingRejectsFalseAcknowledgements(t *testing.T) { }) } +// Accepting the lock is not accepting the loss. A DROP INDEX under +// --accept-blocking alone is refused as a destructive change before +// anything runs, with the checks that catch an index the application still +// uses; a destructive acknowledgement naming another table is a usage +// error. The index here is a plain, non-unique one: the gate covers every +// index drop, not only the unique ones that also carry a guarantee. +func TestRunAcceptBlockingNeedsTheDestructiveAcknowledgementToDropAnIndex(t *testing.T) { + url := testutil.StartPostgres(t) + pool, err := dbconn.NewPool(t.Context(), dbconn.Config{URL: url}) + require.NoError(t, err) + defer pool.Close() + schema := testutil.NewSchema(t, pool) + _, err = pool.Exec(t.Context(), fmt.Sprintf(` + CREATE TABLE %[1]s.orders ( + id int PRIMARY KEY, + created_at timestamptz + ); + CREATE TABLE %[1]s.customers (id int PRIMARY KEY); + CREATE INDEX orders_created_idx ON %[1]s.orders (created_at)`, schema)) + require.NoError(t, err) + drop := parseOne(t, fmt.Sprintf("DROP INDEX %s.orders_created_idx", schema)) + + t.Run("refused without the destructive acknowledgement", func(t *testing.T) { + opts := acceptBlockingOptions(schema + ".orders") + opts.AllowDestructive = "" + + v, err := migrate.Run(t.Context(), pool, drop, opts) + + require.NoError(t, err) + assert.Equal(t, verdict.OutcomeRefused, v.Outcome) + assert.Equal(t, verdict.ReasonDestructiveChange, v.Reason) + assert.Equal(t, verdict.ClassByDesign, v.Class) + assert.Equal(t, schema+".orders", v.Table, "the refusal names the owning table the acknowledgement must spell") + assert.False(t, v.BlockingPassthrough) + assert.False(t, v.AllowedDestructive) + assert.Contains(t, v.Detail, "idx_scan") + assert.Contains(t, v.Detail, "--allow-destructive "+schema+".orders") + assert.True(t, indexExists(t, pool, schema, "orders_created_idx"), "the refused drop must execute nothing") + }) + + t.Run("a destructive acknowledgement naming another table executes nothing", func(t *testing.T) { + opts := acceptBlockingOptions(schema + ".orders") + opts.AllowDestructive = schema + ".customers" + + v, err := migrate.Run(t.Context(), pool, drop, opts) + + require.ErrorIs(t, err, migrate.ErrAllowDestructiveMismatch) + assert.Equal(t, verdict.Verdict{}, v) + assert.True(t, indexExists(t, pool, schema, "orders_created_idx")) + }) +} + // The caller's context ending while the accepted statement runs is an // outcome the client cannot establish: the statement was submitted and // the server decides whether its transaction rolled back. The verdict says diff --git a/pkg/migrate/allow_destructive.go b/pkg/migrate/allow_destructive.go index eadf2ba7..9edef635 100644 --- a/pkg/migrate/allow_destructive.go +++ b/pkg/migrate/allow_destructive.go @@ -128,12 +128,29 @@ func AdmitDestructive(st statement.Statement, schema string, rs router.Statement return v, true, nil } -// auditAllowDestructive records the acknowledged data loss before anything -// executes. The record is warn-level and unconditional, as the force audit -// is, and the verdict's AllowedDestructive field is its machine-readable -// twin. -func auditAllowDestructive(audit *slog.Logger, st statement.Statement) { +// dropIndexDestructiveVerdict is the refusal for an accepted blocking DROP +// INDEX without the destructive acknowledgement. Every index drop counts, +// not only a unique one: the common mistake is dropping an index the +// application still reads through, so the detail lists the checks that +// catch it before naming the acknowledgement. table is the index's owning +// table, resolved from the catalog. +func dropIndexDestructiveVerdict(st statement.Statement, table string) verdict.Verdict { + detail := "DROP INDEX discards an index that queries, foreign-key checks, or a uniqueness guarantee may " + + "still rely on, and undoing it is a full index build; before dropping, confirm that " + + "pg_stat_user_indexes.idx_scan for the index is 0 on the primary and on every replica (each instance " + + "keeps its own counters) over a window since stats_reset that covers the application's full cycle, " + + "that no foreign key's referencing columns rely on it, and, for a unique index, that nothing still " + + "needs the uniqueness it enforces — then rerun with --allow-destructive " + table + v := verdict.Verdict{Statement: st.SQL(), Table: table, Detail: detail} + return v.WithRefusal(destructiveChangeRefusal()) +} + +// auditAllowDestructive records the acknowledged data loss on table before +// anything executes. The record is warn-level and unconditional, as the +// force audit is, and the verdict's AllowedDestructive field is its +// machine-readable twin. +func auditAllowDestructive(audit *slog.Logger, st statement.Statement, table string) { audit.Warn("destructive change acknowledged; the statement will discard live data or structure", - "table", qualified(st), + "table", table, "kind", st.Kind().String()) } diff --git a/pkg/migrate/allow_destructive_integration_test.go b/pkg/migrate/allow_destructive_integration_test.go index 26fb7373..d2724149 100644 --- a/pkg/migrate/allow_destructive_integration_test.go +++ b/pkg/migrate/allow_destructive_integration_test.go @@ -369,9 +369,9 @@ func TestRunGuidesADropAndReAddToAnInPlaceTypeChange(t *testing.T) { assert.EqualValues(t, 500*501/2, total, "every qty value is still there") } -// The destructive acknowledgement does not reach a DROP INDEX: the +// The destructive acknowledgement alone does not lift a DROP INDEX: the // statement-kind gate refuses it first in favor of DROP INDEX -// CONCURRENTLY, and only --accept-blocking governs that refusal. +// CONCURRENTLY, and only --accept-blocking accepts that refusal. func TestRunAllowDestructiveDoesNotLiftADropIndexRefusal(t *testing.T) { f := newDestructiveFixture(t) opts := runOptions() diff --git a/pkg/migrate/allow_destructive_test.go b/pkg/migrate/allow_destructive_test.go index d6ce689e..bbf0908f 100644 --- a/pkg/migrate/allow_destructive_test.go +++ b/pkg/migrate/allow_destructive_test.go @@ -26,21 +26,6 @@ func routeOne(t *testing.T, sql string) (statement.Statement, router.Statement) return st, router.Route([]planner.Plan{classified}).Statements[0] } -// The destructive acknowledgement and accepted blocking are separate -// policy paths; both at once is rejected before any database work. -func TestRunRejectsAllowDestructiveWithAcceptBlocking(t *testing.T) { - st, err := statement.ParseOne("DROP INDEX app.orders_created_idx") - require.NoError(t, err) - opts := DefaultOptions() - opts.AllowDestructive = "app.orders" - opts.AcceptBlocking = "app.orders" - - v, err := Run(t.Context(), nil, st, opts) - require.Error(t, err) - assert.Contains(t, err.Error(), "AllowDestructive") - assert.Equal(t, verdict.Verdict{}, v) -} - // Desired-state execution refuses a destructive plan whole, so the // acknowledgement has nothing to acknowledge there and is rejected as a // typed sentinel before any database work. diff --git a/pkg/migrate/migrate.go b/pkg/migrate/migrate.go index 845fecb2..9d2a54ba 100644 --- a/pkg/migrate/migrate.go +++ b/pkg/migrate/migrate.go @@ -82,19 +82,19 @@ type Options struct { // from the catalog — exactly; empty means every refusal stands. // Ineligible refusals stand regardless. Force and AcceptBlocking are // mutually exclusive: a statement reaches one routing decision, and - // one acknowledgement names it. + // one acknowledgement names it. An accepted DROP INDEX also needs + // AllowDestructive: accepting the lock is not accepting the loss. AcceptBlocking string // AllowDestructive is the typed acknowledgement that the statement may - // discard live data or structure: a dropped column, constraint, or NOT - // NULL. Without it such a statement is refused before anything runs, - // with reason destructive-change, on every route it could take. It must - // name the resolved schema-qualified target table exactly; empty means + // discard live data or structure: a dropped column, constraint, NOT + // NULL, or index. Without it such a statement is refused before + // anything runs, with reason destructive-change, on every route it + // could take. It must name the resolved schema-qualified target table + // exactly — for a DROP INDEX, the index's owning table; empty means // destructive statements are refused. It acknowledges only the loss: // the statement still meets every other gate, so a statement that also - // needs Force needs both. It cannot be combined with AcceptBlocking, - // whose statements the statement-kind gate refuses before this one, and - // [RunDesired] rejects it. + // needs Force or AcceptBlocking needs both. [RunDesired] rejects it. AllowDestructive string // MaxTableSizeBytes is the threshold above which a blind bounded @@ -182,9 +182,6 @@ func (o Options) validate() error { if o.Force != "" && o.AcceptBlocking != "" { return errors.New("migrate: Options.Force and Options.AcceptBlocking are mutually exclusive; a statement reaches one routing decision and one acknowledgement names it") } - if o.AllowDestructive != "" && o.AcceptBlocking != "" { - return errors.New("migrate: Options.AllowDestructive and Options.AcceptBlocking are mutually exclusive; accepted blocking is a separate policy path the destructive acknowledgement does not govern") - } if o.CopySwap.Restart != "" && (o.Force != "" || o.AcceptBlocking != "") { return errors.New("migrate: Options.CopySwap.Restart acknowledges a copy-and-swap run starting over, which Options.Force and Options.AcceptBlocking both route away from; name one acknowledgement") } @@ -224,9 +221,10 @@ func (o Options) retry() executor.RetryPolicy { // checkpoint row the next Run over the same statement resumes from. // // A statement that discards live data or structure — a dropped column, -// constraint, or NOT NULL — is refused with reason destructive-change -// before any route runs it, unless Options.AllowDestructive names its -// table; an acknowledged run's verdict carries AllowedDestructive. +// constraint, NOT NULL, or (on the accepted blocking path) index — is +// refused with reason destructive-change before any route runs it, unless +// Options.AllowDestructive names its table; an acknowledged run's verdict +// carries AllowedDestructive. // // A bare table name resolves on the pool's own search_path, so two pools // with different paths can address different tables from the same @@ -319,7 +317,7 @@ func run(ctx context.Context, pool *pgxpool.Pool, st statement.Statement, opts O return destructiveChangeVerdict(st, destructive) } allowedDestructive = true - auditAllowDestructive(opts.audit(), st) + auditAllowDestructive(opts.audit(), st, qualified(st)) } if opts.CopySwap.Restart != "" { auditRestart(opts.audit(), st) From b525b5d3f09a62fc38c991e9a7ebff1b0c910cd2 Mon Sep 17 00:00:00 2001 From: Kiran Muddukrishna Date: Thu, 29 Oct 2026 13:21:49 +1100 Subject: [PATCH 2/2] migrate: prove REINDEX still needs only --accept-blocking --- .../accept_blocking_integration_test.go | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/pkg/migrate/accept_blocking_integration_test.go b/pkg/migrate/accept_blocking_integration_test.go index 0ed8f957..7664734b 100644 --- a/pkg/migrate/accept_blocking_integration_test.go +++ b/pkg/migrate/accept_blocking_integration_test.go @@ -109,6 +109,25 @@ func TestRunAcceptBlockingExecutesEligibleRefusals(t *testing.T) { assertAccepted(t, v, schema) }) + // A REINDEX discards nothing, so the blocking acknowledgement alone + // runs it; only a dropped index needs the destructive one too. + t.Run("REINDEX needs only the blocking acknowledgement", func(t *testing.T) { + for _, sql := range []string{ + "REINDEX INDEX %s.orders_created_idx", + "REINDEX TABLE %s.orders", + } { + schema := seed(t) + opts := acceptBlockingOptions(schema + ".orders") + opts.AllowDestructive = "" + + v, err := migrate.Run(t.Context(), pool, parseOne(t, fmt.Sprintf(sql, schema)), opts) + require.NoError(t, err, sql) + assertAccepted(t, v, schema) + assert.False(t, v.AllowedDestructive, "%s records no acknowledged loss", sql) + assert.True(t, indexExists(t, pool, schema, "orders_created_idx"), "REINDEX keeps the index") + } + }) + t.Run("REINDEX TABLE of a materialized view acknowledges the view", func(t *testing.T) { // LOCK TABLE cannot name a materialized view, so this is the one // target the engine does not lock before the statement; the