diff --git a/README.md b/README.md index 48f2ebcb..4a5c6bb7 100644 --- a/README.md +++ b/README.md @@ -40,7 +40,8 @@ advisory `suggest` command are implemented. `pg-sprite migrate --alter '…'` cl routes the statement, then executes the routed SQL — the planner's safer native sequence by default when the submitted form blocks (reported in the verdict's `executed_sql`), a bounded optimistic native attempt otherwise. A gated `--force` runs the submitted form as-is under the -same budgets. Changes without an available backend get a structured refusal (exit code 2). +same budgets, and a statement that discards data runs only when `--allow-destructive` names +its table. Changes without an available backend get a structured refusal (exit code 2). Desired-state execution — converging a live table onto a `CREATE TABLE` file, including creating the table when it does not exist yet — is available through `migrate --desired schema.sql` and `migrate.RunDesired` in [`pkg/migrate`](pkg/migrate/desired.go). @@ -119,6 +120,13 @@ refusal — never a silently wrong or incomplete result: `REPLICA IDENTITY` first, then re-plan. - **Retyping or dropping a column a view reads is refused** — PostgreSQL would refuse it anyway; the plan names the views to drop or redefine first. +- **Destructive statements need a typed acknowledgement** — `migrate --alter` + refuses a statement that drops a column, a constraint, or a `NOT NULL` with + `destructive-change` (exit 2) on every route and in `--dry-run`, unless + `--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. - **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 @@ -197,10 +205,10 @@ the machine-readable shape is in **Refuse: no safe path exists, so nothing runs.** A rewrite the copy-and-swap backend does not carry yet (a volatile column default, for one) refuses; the dry run exits 2 so CI can gate on it without parsing JSON (the full four-code ladder is under -[Exit codes](#exit-codes)). The exit-code gate stops refusals only — -a destructive-but-executable change (`DROP COLUMN`) warns and exits 0, so a -gate that must stop drops checks `.statements[].destructive` in the -`--json` report. Watch it in +[Exit codes](#exit-codes)). A destructive change (`DROP COLUMN`) is refused +the same way, exit 2 with reason `destructive-change`, until +`--allow-destructive SCHEMA.TABLE` names the table; the acknowledged dry run +still marks it `.statements[].destructive` in the `--json` report. Watch it in [docs/demos/refuse.gif](docs/demos/refuse.gif); the machine-readable shape is in [docs/cli-output-examples.md](docs/cli-output-examples.md). @@ -210,7 +218,7 @@ findings exit non-zero, warnings alone pass. Watch it in [docs/demos/lint.gif](docs/demos/lint.gif); the machine-readable shape is in [docs/cli-output-examples.md](docs/cli-output-examples.md). -More shapes — every disposition as JSON, destructive warnings, and exit +More shapes — every disposition as JSON, destructive refusals, and exit codes — are in [docs/cli-output-examples.md](docs/cli-output-examples.md). ## Install @@ -265,7 +273,7 @@ vouch for it as online-safe. | Exit | Meaning | Anything committed? | Online-safe? | |---|---|---|---| | 0 | Executed through an online-safe path; or a dry run, `diff`, or `pull` found nothing to refuse | yes (dry run, `diff`, `pull`: nothing runs) | yes | -| 1 | Failed: a PostgreSQL error after execution started (rolled back), or an operational or usage error before it — bad flags, an unreachable database, a mismatched `--force` acknowledgement | no; a safer sequence that stopped mid-flight keeps its committed prefix, named in the verdict's `executed_sql`; unknown when the verdict's `code` is `commit-outcome-unknown` — the failed step's `COMMIT` may have landed, so inspect the catalog before retrying | not applicable | +| 1 | Failed: a PostgreSQL error after execution started (rolled back), or an operational or usage error before it — bad flags, an unreachable database, a mismatched `--force` or `--allow-destructive` acknowledgement | no; a safer sequence that stopped mid-flight keeps its committed prefix, named in the verdict's `executed_sql`; unknown when the verdict's `code` is `commit-outcome-unknown` — the failed step's `COMMIT` may have landed, so inspect the catalog before retrying | not applicable | | 2 | Refused: no online-safe path, and the verdict names the typed `reason` (and `class`) automation switches on | no | not applicable | | 3 | Committed through the accepted-blocking passthrough — the operator explicitly accepted a blocking form, and the engine ran it under bounded budgets without vouching for online safety | yes | no | diff --git a/SAFETY.md b/SAFETY.md index 6a29b114..df414936 100644 --- a/SAFETY.md +++ b/SAFETY.md @@ -57,12 +57,14 @@ re-introspected, re-classified, re-routed, and re-preflighted at execution time, the loop wrongly admits still cannot make the core exceed a lock budget or skip a preflight. **One admission check has no core backstop: the destructive guard.** The core has no concept of destructiveness — `pkg/executor` and `pkg/preflight` never check it — so refusing a -destructive desired-state plan rests entirely on `RunDesired`'s admission gate and on the -classifier's `Destructive` derivation in `pkg/planner`, and its failure mode is data loss (a -falsely-admitted `DROP COLUMN` commits), not a refusal or a bounded failed attempt. Those two -sites are the exception to the periphery posture: treat `destructiveOp` and the desired-state -admission gate with the core's review bar — spec-first, test-first, small diffs — even though -their packages stay periphery for everything else they do. +destructive desired-state plan rests entirely on `RunDesired`'s admission gate, refusing an +unacknowledged destructive imperative statement rests entirely on `Run`'s destructive gate +(RF-10), and both rest on the classifier's `Destructive` derivation in `pkg/planner`. Their +failure mode is data loss (a falsely-admitted `DROP COLUMN` commits), not a refusal or a +bounded failed attempt. Those three sites are the exception to the periphery posture: treat +`destructiveOp`, the desired-state admission gate, and the imperative destructive gate with +the core's review bar — spec-first, test-first, small diffs — even though their packages stay +periphery for everything else they do. ## Rules inside the core diff --git a/demo/README.md b/demo/README.md index 4dd302ab..edcf46b3 100644 --- a/demo/README.md +++ b/demo/README.md @@ -12,11 +12,11 @@ The tour walks five sections, each runnable on its own via | Section | What it shows | Writes? | | --------- | ------------------------------------------------------------------------------------------------------------------- | ------------------- | -| `dryrun` | One statement per planner route, reason, and disposition: metadata-only, fast-default, binary-coercible, the safer idioms, a rewrite-required suggestion, type-rewrite, volatile-default, an app-breaking rename (refused as rewrite-required), destructive, relocation, and a refusal | no | +| `dryrun` | One statement per planner route, reason, and disposition: metadata-only, fast-default, binary-coercible, the safer idioms, a rewrite-required suggestion, type-rewrite, volatile-default, an app-breaking rename (refused as rewrite-required), a destructive drop (planned under `--allow-destructive`, refused as `destructive-change` without it), relocation, and a refusal | no | | `diff` | The declarative front door: a routed convergence plan for an existing table, for a missing one, and a typed create-shape refusal (exit code 2) for a missing table declared `IF NOT EXISTS` | no | | `pull` | Existing-database onboarding: export one desired file per demo table, then prove each produces a zero-change `diff` | no | | `offline` | `lint` (gates on error findings), `suggest` (advises), `fmt` (canonicalizes) — no database | no | -| `exec` | Real executions: a native add, the concurrent index substitution, the four-step `SET NOT NULL` sequence, a genuine type rewrite run through copy-and-swap (`executed-via-copy-and-swap`, exit code 0), a structured refusal (exit code 2) for a rewrite the copy-and-swap engine does not carry yet, and the `--accept-blocking` passthrough: the plain `DROP INDEX` refused, refused again with a mismatched acknowledgement (exit code 1, nothing runs), then committed without online safety (exit code 3) along with a `REINDEX TABLE` | yes (seeded tables) | +| `exec` | Real executions: a native add, the concurrent index substitution, the four-step `SET NOT NULL` sequence, a genuine type rewrite run through copy-and-swap (`executed-via-copy-and-swap`, exit code 0), a structured refusal (exit code 2) for a rewrite the copy-and-swap engine does not carry yet, and the `--accept-blocking` passthrough: the plain `DROP INDEX` refused, refused again with a mismatched acknowledgement (exit code 1, nothing runs), then committed without online safety (exit code 3) along with a `REINDEX TABLE`, and a `DROP COLUMN` refused as `destructive-change` (exit code 2) until `--allow-destructive` names the table | yes (seeded tables) | `make demo` reseeds [seed.sql](seed.sql) first, so every run starts from the same state and the tour is rerunnable. The compose database is left running @@ -31,8 +31,8 @@ make demo-check # same tour, asserting on --json fields and exit codes; needs ``` `CHECK=1` makes the tour assert on the typed JSON contract — plan report -routes/reasons/destructive flags and `format_version`, verdict outcomes and -reasons, the substituted `executed_sql` shape (step count plus a +routes/reasons/destructive flags and `format_version`, verdict outcomes, +reasons, and classes, the `allowed_destructive` audit field, the substituted `executed_sql` shape (step count plus a distinguishing fragment, so a regression that drops `CONCURRENTLY` or collapses the `SET NOT NULL` sequence turns the job red), statement counts — and on exit codes (`0` success, `2` refusal, `3` accepted-blocking commit, diff --git a/demo/tour.sh b/demo/tour.sh index 925781ce..ffea6e8a 100755 --- a/demo/tour.sh +++ b/demo/tour.sh @@ -51,12 +51,14 @@ assert_eq() { fi } -# dry_run route reason destructive disposition sql +# dry_run route reason destructive disposition sql [allow_destructive_table] # # Demo mode prints the routed plan. Check mode asserts the statement's # route, first decision reason, destructive flag, and the report's # disposition from the JSON plan report. Dry-run never writes, so these -# rows can rerun in any order. +# rows can rerun in any order. A destructive statement dry-runs to a plan +# only under --allow-destructive, so its row names the table to +# acknowledge; refused_destructive covers the unacknowledged form. # # Exit code: an execute-disposition plan exits 0; any plan that would not # execute (rewrite-required, unavailable, refuse) exits with the refusal @@ -64,21 +66,73 @@ assert_eq() { # contract is pinned exactly. dry_run() { local route="$1" reason="$2" destructive="$3" disposition="$4" sql="$5" out status=0 + local ack=() + if [ -n "${6:-}" ]; then + ack=(--allow-destructive "$6") + fi step "$sql" if [ "$CHECK" = 1 ]; then - out=$("$PGS" migrate --url "$PG_DSN" --dry-run --json --alter "$sql") || status=$? + out=$("$PGS" migrate --url "$PG_DSN" --dry-run --json --alter "$sql" ${ack[@]+"${ack[@]}"}) || status=$? if [ "$disposition" = execute ]; then assert_eq "dry-run exit of [$sql]" 0 "$status" else assert_eq "dry-run refusal exit of [$sql]" 2 "$status" fi - assert_eq "plan format_version of [$sql]" 13 "$(jq -r '.format_version' <<<"$out")" + assert_eq "plan format_version of [$sql]" 14 "$(jq -r '.format_version' <<<"$out")" assert_eq "disposition of [$sql]" "$disposition" "$(jq -r '.disposition' <<<"$out")" assert_eq "route of [$sql]" "$route" "$(jq -r '.statements[0].route' <<<"$out")" assert_eq "reason of [$sql]" "$reason" "$(jq -r '.statements[0].decisions[0].reason' <<<"$out")" assert_eq "destructive of [$sql]" "$destructive" "$(jq -r '.statements[0].destructive' <<<"$out")" else - "$PGS" migrate --url "$PG_DSN" --dry-run --alter "$sql" || echo "(exit $?)" + "$PGS" migrate --url "$PG_DSN" --dry-run --alter "$sql" ${ack[@]+"${ack[@]}"} || echo "(exit $?)" + fi +} + +# refused_destructive sql +# +# A statement that discards data runs only when --allow-destructive names +# its table. Without it, the dry run and the apply both end in the +# destructive-change verdict (class by-design) and the refusal exit code 2; +# the dry run prints that verdict, not a plan report, so it carries no +# format_version. Nothing runs either way, so the row can rerun. +refused_destructive() { + local sql="$1" out status=0 + step "refuse (destructive, unacknowledged): $sql" + if [ "$CHECK" = 1 ]; then + out=$("$PGS" migrate --url "$PG_DSN" --dry-run --json --alter "$sql") || status=$? + assert_eq "destructive dry-run exit of [$sql]" 2 "$status" + assert_eq "destructive dry-run outcome of [$sql]" refused "$(jq -r '.outcome' <<<"$out")" + assert_eq "destructive dry-run reason of [$sql]" destructive-change "$(jq -r '.reason' <<<"$out")" + assert_eq "destructive dry-run class of [$sql]" by-design "$(jq -r '.class' <<<"$out")" + assert_eq "destructive dry-run format_version of [$sql]" null "$(jq -r '.format_version' <<<"$out")" + status=0 + out=$("$PGS" migrate --url "$PG_DSN" --json --alter "$sql") || status=$? + assert_eq "destructive exit of [$sql]" 2 "$status" + assert_eq "destructive outcome of [$sql]" refused "$(jq -r '.outcome' <<<"$out")" + assert_eq "destructive reason of [$sql]" destructive-change "$(jq -r '.reason' <<<"$out")" + assert_eq "destructive class of [$sql]" by-design "$(jq -r '.class' <<<"$out")" + else + "$PGS" migrate --url "$PG_DSN" --alter "$sql" || echo "(refused, exit $? — expected: a destructive statement needs --allow-destructive naming its table)" + fi +} + +# execute_destructive table sql +# +# Runs a statement that discards data under the --allow-destructive +# acknowledgement and requires the executed-natively verdict with exit 0 +# and the allowed_destructive audit field, for the table the +# acknowledgement named. +execute_destructive() { + local table="$1" sql="$2" out status=0 + step "execute (allow destructive on $table): $sql" + if [ "$CHECK" = 1 ]; then + out=$("$PGS" migrate --url "$PG_DSN" --json --alter "$sql" --allow-destructive "$table" 2>/dev/null) || status=$? + assert_eq "acknowledged destructive exit of [$sql]" 0 "$status" + assert_eq "outcome of [$sql]" executed-natively "$(jq -r '.outcome' <<<"$out")" + assert_eq "allowed_destructive of [$sql]" true "$(jq -r '.allowed_destructive' <<<"$out")" + assert_eq "table of [$sql]" "$table" "$(jq -r '.table' <<<"$out")" + else + "$PGS" migrate --url "$PG_DSN" --alter "$sql" --allow-destructive "$table" fi } @@ -200,7 +254,7 @@ diff_plan() { if [ "$CHECK" = 1 ]; then out=$("$PGS" diff --url "$PG_DSN" --desired "$desired" --schema public --json) || status=$? assert_eq "diff exit of [$desired]" 0 "$status" - assert_eq "diff format_version of [$desired]" 13 "$(jq -r '.format_version' <<<"$out")" + assert_eq "diff format_version of [$desired]" 14 "$(jq -r '.format_version' <<<"$out")" assert_eq "statement count of [$desired]" "$count" "$(jq -r '.statements | length' <<<"$out")" case "$(jq -r '.statements[0].sql' <<<"$out")" in *"$fragment"*) ;; @@ -228,7 +282,7 @@ diff_refused() { if [ "$CHECK" = 1 ]; then out=$("$PGS" diff --url "$PG_DSN" --desired "$desired" --schema public --json) || status=$? assert_eq "diff refusal exit of [$desired]" 2 "$status" - assert_eq "diff format_version of [$desired]" 13 "$(jq -r '.format_version' <<<"$out")" + assert_eq "diff format_version of [$desired]" 14 "$(jq -r '.format_version' <<<"$out")" assert_eq "diff disposition of [$desired]" refuse "$(jq -r '.disposition' <<<"$out")" assert_eq "diff reason of [$desired]" unsupported-statement "$(jq -r '.statements[0].reason' <<<"$out")" assert_eq "diff class of [$desired]" "$class" "$(jq -r '.statements[0].class' <<<"$out")" @@ -263,7 +317,7 @@ run_pull() { out=$("$PGS" diff --url "$PG_DSN" --schema public --desired "$desired" --json) || status=$? assert_eq "diff exit of [$desired]" 0 "$status" # Plan report contract version (plan.FormatVersion), same pin as diff_plan. - assert_eq "diff format_version of [$desired]" 13 "$(jq -r '.format_version' <<<"$out")" + assert_eq "diff format_version of [$desired]" 14 "$(jq -r '.format_version' <<<"$out")" assert_eq "diff disposition of [$desired]" execute "$(jq -r '.disposition' <<<"$out")" assert_eq "zero diff of [$desired]" 0 "$(jq -r '.statements | length' <<<"$out")" else @@ -289,9 +343,10 @@ run_dryrun() { dry_run native fast-default false execute "ALTER TABLE users ADD COLUMN joined timestamptz DEFAULT now()" dry_run copy-and-swap volatile-default false unavailable "ALTER TABLE users ADD COLUMN started timestamptz DEFAULT clock_timestamp()" dry_run native app-breaking-rename false rewrite-required "ALTER TABLE users RENAME COLUMN name TO full_name" - dry_run native metadata-only true execute "ALTER TABLE users DROP COLUMN status" + dry_run native metadata-only true execute "ALTER TABLE users DROP COLUMN status" public.users dry_run copy-and-swap relocation false unavailable "ALTER TABLE users SET TABLESPACE pg_default" dry_run refuse unsupported-operation false refuse "ALTER TABLE users ENABLE ROW LEVEL SECURITY" + refused_destructive "ALTER TABLE users DROP COLUMN status" } review_rls() { @@ -359,7 +414,7 @@ run_offline() { if [ "$CHECK" = 1 ]; then out=$("$PGS" suggest --json risky.sql) || status=$? assert_eq "suggest exit" 0 "$status" - assert_eq "suggest format_version" 4 "$(jq -r '.format_version' <<<"$out")" + assert_eq "suggest format_version" 5 "$(jq -r '.format_version' <<<"$out")" assert_eq "suggest count" 2 "$(jq -r '.suggestions | length' <<<"$out")" else "$PGS" suggest risky.sql || echo "(exit $?)" @@ -433,6 +488,10 @@ run_exec() { 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" + # 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" + execute_destructive public.users "ALTER TABLE users DROP COLUMN status" } case "$section" in diff --git a/docs/capabilities.md b/docs/capabilities.md index f353e9ad..68328175 100644 --- a/docs/capabilities.md +++ b/docs/capabilities.md @@ -212,10 +212,10 @@ or the only online form is a direct statement the operator runs). | `ADD COLUMN ... GENERATED ... STORED` | 🟡 | copy-and-swap | Yes | Table rewrite; routes to copy-and-swap, which does not carry stored generated columns yet — the copy must **recompute, never copy,** them on the shadow table | | `ALTER COLUMN ... SET EXPRESSION` (changed stored generated expression) | 🟡 | copy-and-swap | Yes | Planned but not executed. `diff` shows a changed stored generated expression as one `SET EXPRESSION` statement of kind `set-generated-expression`, refused with `unsupported-statement`, rather than a drop and re-add that would also drop the column's indexes. It recomputes every row under `ACCESS EXCLUSIVE`, and the statement needs PostgreSQL 17 or later | | `ADD COLUMN` with inline `UNIQUE`/`PRIMARY KEY`/`REFERENCES`/`CHECK` | 🟡 | native, planned flow | Yes | The inline constraint does its index build or validation scan under the `ADD COLUMN`'s `ACCESS EXCLUSIVE` lock; refused with guidance to add the column first, then build the constraint online | -| `DROP COLUMN` | ✅ | native, as-is | Yes | Metadata-only; flagged **destructive** in the plan report. Refused with `replica-identity` when a replica identity index the drop reaches keys on the column: the table's own, or one on a partition or inheritance child below it unless the statement says `ONLY`. Refused with `dependent-view`, naming the views, when a view or materialized view reads the column, on a table the drop reaches. Refused with `cascading-drop` when the statement says `CASCADE`, which would also drop the views that read the column | +| `DROP COLUMN` | ✅ | native, as-is | Yes | Metadata-only; flagged **destructive** in the plan report. `migrate` refuses it with `destructive-change` unless `--allow-destructive` names the table (`migrate --desired` refuses a destructive plan whole); dropping a column and adding it back in one statement is refused with the `alter-column-type-in-place` guidance. Refused with `replica-identity` when a replica identity index the drop reaches keys on the column: the table's own, or one on a partition or inheritance child below it unless the statement says `ONLY`. Refused with `dependent-view`, naming the views, when a view or materialized view reads the column, on a table the drop reaches. Refused with `cascading-drop` when the statement says `CASCADE`, which would also drop the views that read the column | | `ALTER COLUMN TYPE`, binary-coercible (proven against live column facts) | ✅ | native, as-is | Yes | Catalog relabel, e.g. `varchar(50)` → `varchar(100)`, `varchar` → `text`; refused with `dependent-view`, naming the views, when a view or materialized view reads the column; PostgreSQL itself refuses the change when a rule or `STORED` generated column depends on it — see [binary-coercible-type-changes.md](binary-coercible-type-changes.md#no-rewrite-is-not-no-cost) | | `ALTER COLUMN TYPE`, general (or with `USING`) | ✅ | copy-and-swap | Yes | Table rewrite; runs through copy-and-swap: the changed shape is built as a shadow table, filled in chunks (a `USING` expression is applied per row), verified, and swapped in under the source's name. Refused with `dependent-view`, naming the views, when a view or materialized view reads the column, before the route runs. The route's preflight still refuses table shapes and environments it cannot run against (`copy-and-swap-refused`), and a `USING` expression is admitted only when the copy can reproduce it: immutable, naming only the retyped column, not the primary key (`shadow-statement-conversion`). A statement that also drops a column and adds one of the same name is refused when the copy cannot fill the new column — the primary key, a source identity column, or a column that refuses NULL with no default of its own or its domain's (`shadow-statement-readd`) — see [limitations.md](limitations.md) | -| `SET DEFAULT` / `DROP DEFAULT` / `DROP NOT NULL` | ✅ | native, as-is | Yes | Metadata-only | +| `SET DEFAULT` / `DROP DEFAULT` / `DROP NOT NULL` | ✅ | native, as-is | Yes | Metadata-only. `DROP NOT NULL` is flagged **destructive**: `migrate` refuses it with `destructive-change` unless `--allow-destructive` names the table | | `SET NOT NULL` | ✅ | native, safer sequence | Yes | Executed as the native four-step pattern: `ADD CONSTRAINT ... CHECK (col IS NOT NULL) NOT VALID` → online `VALIDATE` → `SET NOT NULL` (catalog flip, PG 12+) → drop the scaffold check | | `RENAME COLUMN` / `RENAME TABLE` | ❌ | — | Yes — the hazard is to the application, and no SQL exists for `migrate` to substitute | Metadata-only for PostgreSQL but **app-breaking** across deployed instances: code still using the old name breaks the instant the rename commits, and no safer SQL exists to substitute. Without `--force`, `migrate` refuses it as `rewrite-required`, class `by-design`, with the `expand-contract-column` or `coordinated-table-rename` guidance (and `lint` reports it at error severity). `--force SCHEMA.TABLE` runs the rename as submitted under bounded budgets and records the override; above `--max-table-size` the forced rename is refused as `not-native-safe-table-too-large`. A desired file cannot express a rename — `diff` shows a renamed column as a drop and an add | | `SET TABLESPACE` | 🟡 | copy-and-swap | Yes | Physical relocation is a rewrite; routes to copy-and-swap, which does not carry a tablespace move yet | @@ -231,7 +231,7 @@ or the only online form is a direct statement the operator runs). | `ADD CONSTRAINT ... NOT VALID` / `... USING INDEX` / `VALIDATE CONSTRAINT` | ✅ | native, as-is | Yes | Already the online idiom; executed as-is | | `ADD FOREIGN KEY ... NOT VALID` on a **partitioned parent** | ✅ | native, safer sequence | Yes — server version 18 or later | Supported on version 18 and later; refused on 14–17 with an environmental class | | `EXCLUDE` constraints (and unrecognized constraint forms) | ❌ | — | Yes — unsolvable today | No online pattern exists in PostgreSQL — the build scans under `ACCESS EXCLUSIVE` with no `NOT VALID`/`USING INDEX` equivalent. Refused; revisit only if PostgreSQL grows one | -| `DROP CONSTRAINT` | ✅ | native, as-is | Yes | Metadata-only; flagged **destructive**. Refused with `replica-identity` when the constraint owns the table's replica identity index, or the index a partition's is attached under. Refused with `cascading-drop` when the statement says `CASCADE`, which would also drop other tables' foreign keys on the constraint | +| `DROP CONSTRAINT` | ✅ | native, as-is | Yes | Metadata-only; flagged **destructive**. `migrate` refuses it with `destructive-change` unless `--allow-destructive` names the table. Refused with `replica-identity` when the constraint owns the table's replica identity index, or the index a partition's is attached under. Refused with `cascading-drop` when the statement says `CASCADE`, which would also drop other tables' foreign keys on the constraint | ### Indexes diff --git a/docs/cli-output-examples.md b/docs/cli-output-examples.md index 2f744cb5..4228932a 100644 --- a/docs/cli-output-examples.md +++ b/docs/cli-output-examples.md @@ -24,9 +24,10 @@ the dry run exits 0 when every statement is executable and 2 when any would be refused — including a target table that does not exist, so a typo'd name cannot gate green — as defined with the [diagnostic codes](postgres-online-ddl-reference.md#dry-run-diagnostic-codes); -CI can gate on the exit code without parsing JSON. The gate is refusals only: a -destructive-but-executable change (`DROP COLUMN`) warns and exits 0 — a gate -that must stop drops checks `.statements[].destructive` in the JSON report. +CI can gate on the exit code without parsing JSON. A destructive change (`DROP +COLUMN`, `DROP CONSTRAINT`, `DROP NOT NULL`) is one of those refusals until +`--allow-destructive` names the table; the acknowledged plan still marks it +`.statements[].destructive` in the JSON report. Exit 2 covers every refusal cause; the exit code answers *should this proceed*, the report's typed `reason` and disposition answer *why not*. The contract is `migrate`'s: `lint` exits 1 when a script has error-severity findings @@ -36,7 +37,8 @@ greenfield case (the plan creates the table), not a refusal, unlike the dry run. Statement kinds `migrate` does not support (`DROP INDEX`, `REINDEX`, `CREATE TABLE`, and anything that is not `ALTER TABLE` or `CREATE INDEX`) emit the same refusal verdict on `--dry-run` as on apply — -a verdict, not a plan report — and exit 2. So do statements whose routed +a verdict, not a plan report — and exit 2. So does an unacknowledged +destructive statement, and so do statements whose routed steps the executor refuses from their shape alone before running anything (an unnamed or `IF NOT EXISTS` index build, `DETACH PARTITION`). The dry run does not evaluate the apply's size guard or privilege preflight; those need @@ -57,7 +59,7 @@ and applies to `insufficient-privileges`, both exit 2. The JSON report schema is - [Routed to copy-and-swap (`type-rewrite`) — exit 0](#routed-to-copy-and-swap-type-rewrite--exit-0) - [Refused: the engine does not carry the operation yet (`backend-unavailable`) — exit 2](#refused-the-engine-does-not-carry-the-operation-yet-backend-unavailable--exit-2) - [Refused: target facts forbid the plan (`unsupported-partitioned-parent`) — exit 2](#refused-target-facts-forbid-the-plan-unsupported-partitioned-parent--exit-2) - - [Executable but destructive (`destructive`) — exit 0](#executable-but-destructive-destructive--exit-0) + - [Refused until acknowledged: a destructive change (`destructive-change`) — exit 2](#refused-until-acknowledged-a-destructive-change-destructive-change--exit-2) - [Lint](#lint) - [Blocking idioms flagged (`blocking-idiom`) — exit 0](#blocking-idioms-flagged-blocking-idiom--exit-0) - [Pull](#pull) @@ -78,7 +80,7 @@ canonical statement of the ladder; a wording fix lands there first. | Exit | Meaning | Anything committed? | Online-safe? | |---|---|---|---| | 0 | Executed through an online-safe path; or a dry run, `diff`, or `pull` found nothing to refuse | yes (dry run, `diff`, `pull`: nothing runs) | yes | -| 1 | Failed: a PostgreSQL error after execution started (rolled back), or an operational or usage error before it — bad flags, an unreachable database, a mismatched `--force` acknowledgement | no; a safer sequence that stopped mid-flight keeps its committed prefix, named in `executed_sql`; unknown when `code` is `commit-outcome-unknown` — the failed step's `COMMIT` may have landed, so inspect the catalog before retrying | not applicable | +| 1 | Failed: a PostgreSQL error after execution started (rolled back), or an operational or usage error before it — bad flags, an unreachable database, a mismatched `--force` or `--allow-destructive` acknowledgement | no; a safer sequence that stopped mid-flight keeps its committed prefix, named in `executed_sql`; unknown when `code` is `commit-outcome-unknown` — the failed step's `COMMIT` may have landed, so inspect the catalog before retrying | not applicable | | 2 | Refused: no online-safe path; the verdict names the typed `reason` and `class` | no | not applicable | | 3 | Committed through the accepted-blocking passthrough, under bounded budgets, without an online-safety guarantee | yes | no | @@ -110,7 +112,7 @@ is a one-line summary; the linked reference entry is authoritative. | [`unsupported-partitioned-parent`](postgres-online-ddl-reference.md#unsupported-partitioned-parent) | The routed plan builds an index concurrently but the target is a partitioned parent, where PostgreSQL cannot `CREATE INDEX CONCURRENTLY`. Refused. | | [`unsupported-statement`](postgres-online-ddl-reference.md#unsupported-statement) | The planner knows no safe path for the statement (for example `SET UNLOGGED`, `CLUSTER ON`). Refused — the same typed reason the run path's refusal verdict carries. | | [`table-not-found`](postgres-online-ddl-reference.md#table-not-found) | The target table does not exist, so classification fell back to zero facts; running without `--dry-run` would fail. The dry run exits 2 and the report carries `table_exists: false`. | -| [`destructive`](postgres-online-ddl-reference.md#destructive) | The change discards live data or structure — a dropped column, constraint, index, or `NOT NULL` (dropping a `DEFAULT` is not destructive). A warning alongside the routing decision, not a refusal; `DROP TABLE` never reaches classification, it refuses as `unsupported-statement`. | +| [`destructive`](postgres-online-ddl-reference.md#destructive) | The change discards live data or structure — a dropped column, constraint, index, or `NOT NULL` (dropping a `DEFAULT` is not destructive). A warning alongside the routing decision on a plan `--allow-destructive` acknowledged; without it `migrate` refuses the statement as `destructive-change`; `DROP TABLE` never reaches classification, it refuses as `unsupported-statement`. | | [`blocking-idiom`](lint-report.md#codes-code) | Lint-only code: the submitted form blocks readers or writers and a safer native form exists; the finding's `suggestion` carries the safer SQL when the linter can construct it. | ## Refusal reasons @@ -129,7 +131,7 @@ in `detail`. The set is closed and pinned by test (`verdict.Reasons()`). | `not-native-safe-budget-exceeded` | The optimistic attempt exceeded its lock or statement budget and was cancelled; the verdict's `cause` narrows which budget fired. The same `cause` field carries the partitioned-parent shape under `unsupported-partitioned-parent`; both are refusal causes, unrelated to the plan statement's create-shape `cause`. | | `not-native-safe-rewrite-required` | The submitted form blocks and must run as a safer native sequence, but none could be constructed. | | `backend-unavailable` | The change routes to copy-and-swap for an operation the engine does not carry yet (a volatile column default, a stored generated column, a relocation, an added column whose type rewrites); a type rewrite of an existing column (`ALTER COLUMN TYPE`) executes through the route instead. | -| `destructive-change` | The desired-state plan discards live structure — a dropped column, constraint, index, or `NOT NULL` — and desired-state execution runs no destructive statement; run the drop deliberately instead ([execution model](execution-model.md)). | +| `destructive-change` | The change discards live data or structure — a dropped column, constraint, index, or `NOT NULL`. `migrate --alter` runs it only when `--allow-destructive` names the table, and a refused statement that drops a column and adds it back carries the `alter-column-type-in-place` guidance; desired-state execution runs no destructive statement, so run the drop deliberately through `migrate --alter` instead ([execution model](execution-model.md)). | | `plan-fingerprint-mismatch` | The plan recomputed at execution time does not carry the pinned fingerprint: the plan a reviewer approved is not the plan that would execute, so nothing runs ([execution model](execution-model.md)). | | `declarative-foreign-key` | The 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; the plan still shows the statement, and `migrate --alter` runs `ADD FOREIGN KEY` through the online `NOT VALID` + `VALIDATE` sequence. | | `copy-and-swap-refused` | The change routes to copy-and-swap and the route's own gates declined it: the preflight found a table shape or an environment it cannot run against, or a shadow operation refused mid-run to keep an invariant. The verdict's `cause` carries the route's own refusal token (`copy-and-swap-…` from the preflight, `shadow-…` from the shadow operations) and `class` its routing class; [refusal-classes.md](refusal-classes.md#copy-and-swap-refused-keyed-on-copyswaprefusalcause) lists every cause. | @@ -158,7 +160,7 @@ itself. ```console $ pg-sprite migrate --alter 'ALTER TABLE users ADD COLUMN note text' --dry-run --json { - "format_version": 13, + "format_version": 14, "source": "alter", "schema": "public", "table": "users", @@ -199,7 +201,7 @@ plans the safer online sequence instead: the decision carries it in ```console $ pg-sprite migrate --alter 'ALTER TABLE users ADD CONSTRAINT users_email_key UNIQUE (email)' --dry-run --json { - "format_version": 13, + "format_version": 14, "source": "alter", "schema": "public", "table": "users", @@ -301,7 +303,7 @@ column first, then build the constraint as a separate, named ```console $ pg-sprite migrate --alter 'ALTER TABLE users ADD COLUMN nickname text UNIQUE' --dry-run --json { - "format_version": 13, + "format_version": 14, "source": "alter", "schema": "public", "table": "users", @@ -343,7 +345,7 @@ no native SQL to show. ```console $ pg-sprite migrate --alter 'ALTER TABLE users ALTER COLUMN id TYPE text' --dry-run --json { - "format_version": 13, + "format_version": 14, "source": "alter", "schema": "public", "table": "users", @@ -397,7 +399,7 @@ column, a relocation — so the plan is `unavailable` and nothing executes. ```console $ pg-sprite migrate --alter 'ALTER TABLE users ADD COLUMN joined timestamptz DEFAULT clock_timestamp()' --dry-run --json { - "format_version": 13, + "format_version": 14, "source": "alter", "schema": "public", "table": "users", @@ -434,7 +436,7 @@ The refusal cause is the report-level `reason`. ```console $ pg-sprite migrate --alter 'CREATE INDEX events_created_idx ON events (created)' --dry-run --json { - "format_version": 13, + "format_version": 14, "source": "alter", "schema": "public", "table": "events", @@ -466,15 +468,40 @@ $ pg-sprite migrate --alter 'CREATE INDEX events_created_idx ON events (created) } ``` -### Executable but destructive (`destructive`) — exit 0 +### Refused until acknowledged: a destructive change (`destructive-change`) — exit 2 -A `DROP COLUMN` routes to execute — the destructive flag is surfaced for -the reviewer or orchestrator to gate on; `migrate` itself does not block it. +A `DROP COLUMN` discards every value the column holds, so `migrate` refuses it +before any route runs it — and the dry run ends in that same refusal verdict, +not a plan report — until `--allow-destructive` names the table. The verdict's +`reason` and `class` are the ones desired-state execution uses for a +destructive plan; on `migrate --alter` that reason always means the +acknowledgement would lift it. `DROP CONSTRAINT` and `ALTER COLUMN ... DROP NOT +NULL` are refused the same way. A statement that drops a column and adds it back +under the same name also carries `"guidance": "alter-column-type-in-place"`: +change the column with `ALTER COLUMN ... TYPE ... USING ...` to keep its values. ```console $ pg-sprite migrate --alter 'ALTER TABLE users DROP COLUMN email' --dry-run --json { - "format_version": 13, + "outcome": "refused", + "reason": "destructive-change", + "class": "by-design", + "statement": "ALTER TABLE public.users DROP email", + "table": "public.users", + "detail": "the statement discards live data or structure (DROP COLUMN email); pg-sprite runs no destructive statement unless the destructive acknowledgement names its table — review what is lost, then rerun with --allow-destructive public.users" +} +``` + +With the acknowledgement the dry run reports the plan the acknowledged apply +takes — still flagged `destructive` for a reviewer or orchestrator to gate on — +and exits 0; the apply's verdict then carries `"allowed_destructive": true`, +the machine-readable audit record, and the run logs one warning naming the +table. A mismatched acknowledgement executes nothing and exits 1. + +```console +$ pg-sprite migrate --alter 'ALTER TABLE users DROP COLUMN email' --dry-run --allow-destructive public.users --json +{ + "format_version": 14, "source": "alter", "schema": "public", "table": "users", @@ -601,7 +628,7 @@ CREATE TABLE users ( ```console $ pg-sprite diff --desired /tmp/users.sql --json { - "format_version": 13, + "format_version": 14, "source": "diff", "schema": "public", "table": "users", diff --git a/docs/design-principles.md b/docs/design-principles.md index 96f30250..977583d1 100644 --- a/docs/design-principles.md +++ b/docs/design-principles.md @@ -128,9 +128,12 @@ the phased build plan should be traceable back to one of these. goes straight into the classifier). Both feed the identical executor. - **Dry-run first.** `diff`/`--dry-run` prints the exact statements and their classification (native vs copy-and-swap) **without executing** — the natural review and CI hook. -- **Destructive diffs are gated; renames are never guessed.** Dropping a column/constraint - requires explicit confirmation; a missing-plus-new column pair is a drop+add unless a rename - is stated explicitly. Intent is required, not inferred. +- **Destructive changes are gated; renames are never guessed.** Dropping a column, a + constraint, or a `NOT NULL` requires explicit confirmation on both front doors: the + imperative `migrate --alter` refuses the statement unless `--allow-destructive` names the + table, and desired-state execution refuses a destructive plan whole. A missing-plus-new + column pair is a drop+add unless a rename is stated explicitly. Intent is required, not + inferred. ## PostgreSQL / Aurora-specific diff --git a/docs/high-level-design.md b/docs/high-level-design.md index 658f5efb..f5378216 100644 --- a/docs/high-level-design.md +++ b/docs/high-level-design.md @@ -191,7 +191,9 @@ The engine accepts a change two ways, both feeding the **same** planner pipeline Declarative did the harder front-end work (introspect + diff + ordering); both front-ends now share classify → route, with imperative input handing the user's statement to the *same* classifier. Schemas can therefore live as reviewed, version-controlled files and CI can compute -"what would change". Destructive-diff confirmation and explicit rename intent are planned. The +"what would change". Destructive changes need an explicit acknowledgement on the imperative +front door (`--allow-destructive`) and are refused whole on the declarative one; explicit +rename intent is planned. The diff algorithm and its safety rules are detailed in the [low-level design](low-level-design.md#declarative-mode-desired-state-schema-diff). diff --git a/docs/invariants.md b/docs/invariants.md index f37c199d..246065f0 100644 --- a/docs/invariants.md +++ b/docs/invariants.md @@ -1361,6 +1361,34 @@ Each refusal is a preflight **error with a stated reason** — never a warning, `pkg/migrate` (`acceptBlocking` builds the outcome from the gate's own verdict), `TestRunAcceptBlockingExecutesEligibleRefusals`. *Source:* [lock-budgeted passthrough](lock-budgeted-passthrough.md#verdict-reports-and-exit-code). +- **RF-10** — No statement that discards data, and no statement that would leave the table + unreadable or unwritable beyond the engine's lock budget, runs without a loud, typed + acknowledgement naming the table. A destructive statement — one whose plan drops a column, + a constraint, or a `NOT NULL` — is refused with `destructive-change` (class `by-design`) + before any route runs it (native, safer sequence, forced, or copy-and-swap) and in a dry + run, unless `--allow-destructive` (`Options.AllowDestructive`) names the resolved + schema-qualified table; a mismatched acknowledgement is a typed usage error and nothing + executes. The acknowledgement covers only the data loss: the statement still meets every + 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 + 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 + run, `auditAllowDestructive`; `ErrAllowDestructiveMismatch`), `RunDesired`'s admission + (`ErrAllowDestructiveNotSupported`), `pkg/planner` (`destructiveOp`), + `TestRunRefusesAnUnacknowledgedDropColumn`, `TestRunRefusesAnUnacknowledgedDropConstraint`, + `TestRunRefusesAnUnacknowledgedDropNotNull`, `TestRunExecutesAnAcknowledgedDropColumn`, + `TestRunRejectsAMismatchedDestructiveAcknowledgement`, + `TestRunRefusesAnUnacknowledgedDropColumnRoutedToCopyAndSwap`, + `TestRunNeedsBothForceAndTheDestructiveAcknowledgement`, + `TestRunAllowDestructiveDoesNotLiftADropIndexRefusal`, `TestAdmitDestructive`, + `TestMigrateDryRunReportsTheDestructiveRefusal`, `TestAllowDestructiveFlagGrammar`, + `demo/tour.sh` (`refused_destructive`). *Source:* + [design principles](design-principles.md#declarative-review-first-workflow), + SchemaBot's `--allow-unsafe` acknowledgement for destructive plans. - **RF-11** — A table is judged idle only when the connected server's statistics prove no use inside the window: no other session holds a lock on it, both last-scan times are recorded as before the window or absent, the server is not in recovery and tracks counts — for the @@ -1476,6 +1504,7 @@ about **how we write and review the code**. | LK-3 | 4–6 | cancellation/claim race test | | LK-5 | 3 (native recovery) | stale-observation fail-closed tests, never-drops-valid, not-droppable skip, shared-budget test | | AB-1, AB-2 | accepted-blocking rollout step 2 | exact session bounds + lock exhaustion leaves catalog unchanged | +| RF-10 | imperative destructive gate | unacknowledged drop column / constraint / NOT NULL refused with durable state intact, acknowledged runs carry the verdict field, mismatch executes nothing, copy-and-swap and forced routes gated, DROP INDEX untouched, dry run ends in the same refusal, demo refusal smoke | | AB-3, AB-4, AB-5, RF-9 | accepted-blocking rollout step 4 | catalog-resolved acknowledgement, mismatch executes nothing, ineligible refusals untouched, grammar rejects `--force`/`--dry-run`/missing budget, accepted verdict identity and exit 3, demo exit-code smoke | | LK-4, ST-5 | 7 | dropped-connection cutover, fidelity checklist | | ST-1, ST-2, ST-3, ST-4 | 8 | kill/resume, cross-version refuse, orphan-slot reap, failover reconcile | diff --git a/docs/limitations.md b/docs/limitations.md index 445e6e55..47818db8 100644 --- a/docs/limitations.md +++ b/docs/limitations.md @@ -10,6 +10,7 @@ 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. | | 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`. | @@ -27,7 +28,7 @@ not escape hatches: | `ALTER COLUMN TYPE ... USING ` the copy cannot reproduce | The copy-and-swap route (above) runs the retype, but the shadow builder admits a `USING` expression only when it can evaluate it again, identically, in the chunk copy, the verifier's recopy, and every change the catch-up flushes. Three rules, each refused as `shadow-statement-conversion` before any shadow is committed: the expression must be **immutable** by the server's own classification (no `now()`, `random()`, `current_setting()`, session-dependent casts such as `text` to `timestamptz`, or functions declared `STABLE` or `VOLATILE`; checked by trying it as a stored generated column's expression on the empty shadow inside the build's transaction, then rolling that attempt back), it must **name only the column it retypes**, bare and unqualified (no neighbouring column, no `t.col`, no whole-row `t.*`), and it must not retype the **primary key**, whose values every chunk range, watermark, and resume clear are expressed in. The expression is also refused when the deparser cannot prove it rendered the text faithfully. Rewrite the expression in terms of the retyped column alone and of `IMMUTABLE` functions, or retype without `USING` so the server's assignment cast converts the value; the refusal names the column, the expression, and that rewrite. The spellings most often refused, and what to write instead, are in [`USING` expressions the copy reproduces](#using-expressions-the-copy-reproduces) below. | | A schema change to the source table while a copy-and-swap runs | Unsupported: logical decoding does not stream DDL, and the copy does not lock it out — a chunk holds `ACCESS SHARE` on the source only for its own transaction, so `ALTER TABLE` on the source completes between chunks. The copy selects the source by column name, so what happens depends on whether every copied name still resolves. A copied column renamed or dropped stops the copy at its next chunk with the server's `undefined_column` error (SQLSTATE `42703`), which is not a typed refusal; that chunk stays in flight and nothing from it reaches the shadow. A change that leaves every copied name resolving to a column of a compatible type — two columns swapping names, a column renamed and re-added under the old name, a widened type — is not detected by the copy, which completes, and after a swap or a re-add the rows copied since hold another column's values or the new column's default. Once the replication stream has decoded a change to the table, a later relation description that differs stops it with `ErrSourceShapeChanged`. None of these reaches the swap: the verifier compares source and shadow rows by column name, and the cutover gate re-fingerprints the source and refuses any changed column as `cutover-schema-drift` ([refusal-classes.md](refusal-classes.md#shadow-operation-refusals-keyed-on-refusalcause)), after which the shadow is dropped and the change re-planned. | | A concurrent index build whose name is occupied by an invalid index, or whose table carries quarantined debris | The build refuses before running and never drops the occupant: an invalid entry is either abandoned debris or someone's build still in progress, and a drop by name cannot tell them apart. The refusal is typed by proof (`invalid-index-abandoned`, `invalid-index-build-in-flight`, `invalid-index-other-table`, `invalid-index-not-droppable`, `invalid-index-builder-unobservable`), and a table carrying an entry a previous recovery quarantined (`pgsprite_abandoned_`) but did not drop is refused the same way, so debris never goes quiet. The proven removal — `executor.RebuildAbandonedIndex`, or `executor.DropAbandonedIndex` to remove the leftover without running the build; both library-only today, no CLI verb — takes a bounded `SHARE UPDATE EXCLUSIVE` lock on the table (which any running concurrent index command holds), re-verifies the entry by OID, renames it to an identity-derived name and drops that, all within one shared budget; it refuses a visible in-flight build, another table's entry, or an entry the server will not drop concurrently (a partitioned table's index, an index partition, a constraint's index), and reports a lock budget rather than wait behind a build it cannot see. From the CLI, the [runbook](invalid-index-recovery.md) applies. | -| A copy-and-swap statement that drops a column and adds one of the same name | The re-added column is a new column: every row holds its default (or NULL), as under a direct `ALTER TABLE`, and the dropped column's values are not copied into it ([copy-and-swap D1](copy-and-swap-design.md#d1--no-durable-scratch-database)). Three such re-adds have no default to take or are the key the copy is driven by, and the shadow build refuses them with `shadow-statement-readd` inside its own transaction, before any shadow, fence, or checkpoint row exists: re-adding the primary key column, re-adding a source identity column, and re-adding a column that refuses NULL (by its own `NOT NULL` or its domain's) with no default of its own or on its type. The remedy depends on which: a primary key is refused whatever the new column declares, so a default does not help — change the key in place with `ALTER COLUMN TYPE`; a source identity column is refused whatever the new column declares too — drop it in one schema change and add the new column in the next; any other column runs once the re-added column has a default (its own, or its domain's). A nullable, defaulted, or generated re-add builds and runs. A re-add declared as an identity or `serial` builds, copies, and then stops at the swap with `cutover-swap-mismatch`, leaving the shadow and the hold on the source for `--restart`, as any new identity or sequence-owned column added through copy-and-swap does. The refusal covers only these named re-add shapes: a new column the server cannot fill in other ways — a `CHECK` that rejects the column's default or NULL, a `NOT NULL` column added without a drop beside a retype, or the key replaced under a new name — still builds and stops at the first chunk or the copier's shape check, failing closed; a first-chunk stop is the server's class `23` error, a data fault, so the run abandons itself and the verdict is `failed` with `code: data-fault`. A run checkpointed by an earlier build for a drop-and-re-add statement no longer resumes (`resume-row-mismatch`, because the copy list it recorded names the re-added column); abandon it and run again rather than retrying the resume. | +| A copy-and-swap statement that drops a column and adds one of the same name | Like any drop, it runs only under `--allow-destructive`; without it the statement is refused with `destructive-change` and the `alter-column-type-in-place` guidance before the shadow build starts. Under the acknowledgement, the re-added column is a new column: every row holds its default (or NULL), as under a direct `ALTER TABLE`, and the dropped column's values are not copied into it ([copy-and-swap D1](copy-and-swap-design.md#d1--no-durable-scratch-database)). Three such re-adds have no default to take or are the key the copy is driven by, and the shadow build refuses them with `shadow-statement-readd` inside its own transaction, before any shadow, fence, or checkpoint row exists: re-adding the primary key column, re-adding a source identity column, and re-adding a column that refuses NULL (by its own `NOT NULL` or its domain's) with no default of its own or on its type. The remedy depends on which: a primary key is refused whatever the new column declares, so a default does not help — change the key in place with `ALTER COLUMN TYPE`; a source identity column is refused whatever the new column declares too — drop it in one schema change and add the new column in the next; any other column runs once the re-added column has a default (its own, or its domain's). A nullable, defaulted, or generated re-add builds and runs. A re-add declared as an identity or `serial` builds, copies, and then stops at the swap with `cutover-swap-mismatch`, leaving the shadow and the hold on the source for `--restart`, as any new identity or sequence-owned column added through copy-and-swap does. The refusal covers only these named re-add shapes: a new column the server cannot fill in other ways — a `CHECK` that rejects the column's default or NULL, a `NOT NULL` column added without a drop beside a retype, or the key replaced under a new name — still builds and stops at the first chunk or the copier's shape check, failing closed; a first-chunk stop is the server's class `23` error, a data fault, so the run abandons itself and the verdict is `failed` with `code: data-fault`. A run checkpointed by an earlier build for a drop-and-re-add statement no longer resumes (`resume-row-mismatch`, because the copy list it recorded names the re-added column); abandon it and run again rather than retrying the resume. | ## `USING` expressions the copy reproduces @@ -101,7 +102,7 @@ with a typed refusal — never a silently wrong or incomplete result: | Greenfield table creation | Reachable through desired-state execution (`migrate --desired` or `migrate.RunDesired`): a plan whose table does not exist routes to the executor create path (`executor.ExecuteCreate`). Planning refuses every clause that binds to an existing object the absence proof does not cover (`PARTITION OF`, `INHERITS`, `LIKE`, `OF`, and `REFERENCES`), plus `IF NOT EXISTS` and duplicate claimed relation names. Apply re-checks the same shape rules before anything executes. `CONCURRENTLY` is refused upstream at desired-file parse (`statement.ParseDesired`); the create path's admission re-checks it as defense in depth. | | Changed stored generated column expression | Planned, never executed. `diff` plans the change as one `ALTER TABLE ... ALTER COLUMN ... SET EXPRESSION AS (...)` statement of kind `set-generated-expression`, refused as `unsupported-statement`: it recomputes every row under an `ACCESS EXCLUSIVE` lock, and no online path exists yet. Planning it this way, rather than as a drop and re-add of the column, leaves indexes on the column out of the plan. `SET EXPRESSION` needs PostgreSQL 17 or later; on older servers the plan still shows it, refused, and the only path is to drop and re-add the column, which also drops its indexes. A change to a generated column's type still fails the diff. | | Virtual generated columns (PostgreSQL 18+) | Not modeled. A desired file cannot declare `VIRTUAL`, export refuses a live virtual column (`ErrUnrenderableVirtual`), and a desired file that declares the column any other way fails the diff, so a virtual expression is never converged as a column default. | -| Changed index or constraint definition | A redefinition diffs to drop-and-recreate, the drop is destructive, and desired-state execution refuses any plan containing a destructive statement — the whole plan, including the harmless recreate. Run the drop deliberately first (`DROP INDEX CONCURRENTLY` directly against the database; `ALTER TABLE ... DROP CONSTRAINT` through the imperative front door), then rerun — the remaining plan converges the recreate. One shape is not a redefinition: a new primary key or unique constraint whose name and index match a live unique index takes that index over with `ADD CONSTRAINT ... USING INDEX`, so the table keeps its guarantee and its index throughout. A primary key's columns are set NOT NULL online first. The statement names only the index, so execution reads the index again under the table's lock and refuses with `adopted-index-drift`, attaching nothing, if it was dropped, replaced under the same name, or left invalid since planning. A deferrable constraint takes the index over with its declared deferrability. An unfinished index build and a partitioned parent keep the drop and rebuild. A live constraint left `NOT VALID` with the desired definition is not a redefinition either: it plans as one `VALIDATE CONSTRAINT`, which a foreign key's plan shows refused with `declarative-foreign-key`. The statement names only the constraint, so execution reads it again under the validation's own lock and refuses with `validated-constraint-drift`, validating nothing, if it was dropped or replaced under the same name since planning. | +| Changed index or constraint definition | A redefinition diffs to drop-and-recreate, the drop is destructive, and desired-state execution refuses any plan containing a destructive statement — the whole plan, including the harmless recreate. Run the drop deliberately first (`DROP INDEX CONCURRENTLY` directly against the database; `ALTER TABLE ... DROP CONSTRAINT` through the imperative front door with `--allow-destructive`), then rerun — the remaining plan converges the recreate. One shape is not a redefinition: a new primary key or unique constraint whose name and index match a live unique index takes that index over with `ADD CONSTRAINT ... USING INDEX`, so the table keeps its guarantee and its index throughout. A primary key's columns are set NOT NULL online first. The statement names only the index, so execution reads the index again under the table's lock and refuses with `adopted-index-drift`, attaching nothing, if it was dropped, replaced under the same name, or left invalid since planning. A deferrable constraint takes the index over with its declared deferrability. An unfinished index build and a partitioned parent keep the drop and rebuild. A live constraint left `NOT VALID` with the desired definition is not a redefinition either: it plans as one `VALIDATE CONSTRAINT`, which a foreign key's plan shows refused with `declarative-foreign-key`. The statement names only the constraint, so execution reads it again under the validation's own lock and refuses with `validated-constraint-drift`, validating nothing, if it was dropped or replaced under the same name since planning. | ## What desired-state execution converges today @@ -119,7 +120,7 @@ composition of the model boundaries above with those gates. At a glance: | Rebuild an index left invalid by an unfinished concurrent build | Plans as the same `create-index` change — the invalid entry, though it carries the desired name and definition, does not deliver the index, and the diff never emits a drop for it. Execution then meets the occupied name and stops by proof: a `failed` outcome carrying `invalid-index-abandoned` (the invalid-index row above); remove the leftover per the [runbook](invalid-index-recovery.md), or, as a library caller, run the create through `executor.RebuildAbandonedIndex`, and rerun. A build still in progress looks the same to the diff (`indisvalid` stays false until the build's final wait ends) and plans the same create; execution then stops with `invalid-index-build-in-flight` rather than build twice. | | Remove or redefine an index that is currently invalid | The diff leaves the invalid entry alone: it never emits a drop for an entry that may be someone's build still running, so `diff` shows no change for a removed index and only the `create-index` for a redefined one (execution then meets the occupied name as in the row above). This also means the `_ccnew` leftover of an interrupted `REINDEX ... CONCURRENTLY`, which no desired file names, does not block the table's other changes; clear it per the [runbook](invalid-index-recovery.md). On a partitioned parent, where an invalid index means unattached partition indexes rather than an unfinished build, indexes are compared by name and definition alone. | | Add a constraint (`UNIQUE`, `CHECK`); `SET NOT NULL` | Converges via the safer online sequence; not size-guarded either. | -| Relax a `NOT NULL` | Refused, whole plan: dropping `NOT NULL` discards the same guarantee its constraint form would, so it is destructive. Run it deliberately through the imperative front door — it executes natively there — then rerun. | +| Relax a `NOT NULL` | Refused, whole plan: dropping `NOT NULL` discards the same guarantee its constraint form would, so it is destructive. Run it deliberately through the imperative front door — it executes natively there once `--allow-destructive` names the table — then rerun. | | Change an index or constraint definition | Refused, whole plan — the drop-and-recreate row above. | | Narrow a column type (`varchar(255)` → `varchar(50)`) | Converges through copy-and-swap: the plan's disposition is `execute` with the statement routed to the `copy-and-swap` backend, and desired-state execution runs it with the same preflight gates and flags as `migrate --alter` (the Copy-and-swap row above). The route's preflight is read at admission, before any statement runs, so a table or server the route refuses — no `--free-disk`, no logical decoding without `--quiesced`, a trigger, a dependent view — refuses the whole plan with nothing executed, native statements included. The dry run and `diff` do not read the preflight — the plan report says `execute` on the route alone, and whether the route can run here, now, with these flags, is answered when the plan is executed. A plan that retypes two columns runs two copy-and-swap passes on the table, one per statement; the second replaces the done row the first leaves behind. | | A bounded-attempt edit on a table above the size threshold | Refused at that statement (`not-native-safe-table-too-large`): with the default 1 GiB threshold, adding a column to a larger table refuses until the threshold is raised. | diff --git a/docs/low-level-design.md b/docs/low-level-design.md index 394bcd1a..4e95dd20 100644 --- a/docs/low-level-design.md +++ b/docs/low-level-design.md @@ -270,11 +270,13 @@ live schema (introspected) ─┘ │ ### Safety rules (inherited philosophy: surprise-free, decisions-not-options) -- **Destructive-diff confirmation is planned.** The future flag will gate dropping a column, - constraint, or index — anything that loses data or a guarantee (a unique index discards the - same uniqueness guarantee as a unique constraint) — never inferred silently from "it's - missing in the desired file"; no such confirmation flag exists today (destructive statements - are flagged in the plan output). +- **Destructive changes need confirmation.** Dropping a column, constraint, or index — + anything that loses data or a guarantee (a unique index discards the same uniqueness + guarantee as a unique constraint) — is never inferred silently from "it's missing in the + desired file": desired-state execution refuses a destructive plan whole, and the imperative + front door runs a dropped column, constraint, or `NOT NULL` only when + `--allow-destructive SCHEMA.TABLE` names the table (RF-10); destructive statements are + flagged in the plan output either way. - **Unsupported constructs are refused, never guessed.** The desired file admits one unqualified `CREATE TABLE` plus `CREATE INDEX` statements on it; each rule is a typed error. Foreign keys are planned but never executed: the scratch build first creates an empty diff --git a/docs/plan-report.md b/docs/plan-report.md index 6575c631..217d573d 100644 --- a/docs/plan-report.md +++ b/docs/plan-report.md @@ -25,7 +25,7 @@ kinds, guidance, causes, classes, owners) and the fingerprint serialization are changing the fingerprint definition is a contract change and bumps `format_version`, even if no field is added or renamed. -The current version is **13**: version 13 added the `coordinated-constraint-rename` guidance code and classifies the rename of an index that backs a primary-key, unique, or exclusion constraint as `app-breaking-rename`; version 12 added the `expand-contract-column` and `coordinated-table-rename` guidance codes and made `app-breaking-rename` decisions yield the `rewrite-required` disposition; version 11 added the `cascading-drop` reason; version 10 added the `dependent-view` reason and the `dependent_views` decision field; version 9 added the `validate-constraint` and `validate-foreign-key` kinds; version 8 added the `replica-identity` reason; version 7 added the `set-generated-expression` kind; version 6 added the `add-foreign-key` and `drop-foreign-key` +The current version is **14**: version 14 added the `alter-column-type-in-place` guidance code (carried by a `destructive-change` refusal verdict, never by a report statement); version 13 added the `coordinated-constraint-rename` guidance code and classifies the rename of an index that backs a primary-key, unique, or exclusion constraint as `app-breaking-rename`; version 12 added the `expand-contract-column` and `coordinated-table-rename` guidance codes and made `app-breaking-rename` decisions yield the `rewrite-required` disposition; version 11 added the `cascading-drop` reason; version 10 added the `dependent-view` reason and the `dependent_views` decision field; version 9 added the `validate-constraint` and `validate-foreign-key` kinds; version 8 added the `replica-identity` reason; version 7 added the `set-generated-expression` kind; version 6 added the `add-foreign-key` and `drop-foreign-key` kinds, the `foreign-key` cause, and the `declarative-foreign-key` reason; version 5 added `blocking_passthrough_eligible` to every refused statement; version 4 added the `class` and `owner` fields on the report and on refused statements, each drawn from a closed vocabulary (see Classes and Owners); @@ -194,6 +194,7 @@ code's full manual path; this contract embeds it, so a new guidance code bumps t | `expand-contract-column` | Add the new column, dual-write and backfill it, switch reads, then drop the old column as its own change — instead of renaming the column under running application code. | | `coordinated-table-rename` | Coordinate the table rename with the application deploy that adopts the new name, then run it with `--force` naming the table. | | `coordinated-constraint-rename` | The renamed index backs a constraint, which the rename renames: coordinate the rename with the application deploy that stops naming the old constraint (`ON CONFLICT ON CONSTRAINT`, error handling). | +| `alter-column-type-in-place` | Never on a report statement: a `migrate` refusal verdict with reason `destructive-change` carries it when one ALTER TABLE drops a column and adds it back under the same name. Change the column in place with ALTER COLUMN ... TYPE ... USING ..., which keeps its values. | ### Causes (`cause`, greenfield create-shape refusals only) @@ -315,7 +316,7 @@ in `pkg/plan` — if the code drifts from this page, CI fails. ```json { - "format_version": 13, + "format_version": 14, "source": "alter", "schema": "app", "table": "orders", @@ -358,7 +359,7 @@ A desired state that drops an index and adds a column with a constant default: ```json { - "format_version": 13, + "format_version": 14, "source": "diff", "schema": "app", "table": "orders", @@ -425,7 +426,7 @@ A desired state for a table that does not exist yet, whose `CREATE TABLE` carrie ```json { - "format_version": 13, + "format_version": 14, "source": "diff", "schema": "app", "table": "gadgets", diff --git a/docs/postgres-online-ddl-reference.md b/docs/postgres-online-ddl-reference.md index 3b4d28ad..af793b62 100644 --- a/docs/postgres-online-ddl-reference.md +++ b/docs/postgres-online-ddl-reference.md @@ -300,10 +300,11 @@ plan classified from zero facts must not read as green. The exit code answers *should this proceed*; the report answers *why not* — exit 2 covers every refusal cause (no safe path, backend unavailable, target facts, missing table), so automation that needs the cause reads the typed `reason` and -disposition from the `--json` report. A destructive-but-executable change -(`DROP COLUMN`, `DROP TABLE`) is **not** a refusal: it warns and exits 0. A -gate that must stop drops checks `.statements[].destructive` in the JSON -report. A real run adds the two codes that need something to have executed — +disposition from the `--json` report. A destructive change (`DROP COLUMN`, +`DROP CONSTRAINT`, `DROP NOT NULL`) is one of those refusals: `migrate` and its +dry run refuse it with `destructive-change` (exit 2) until `--allow-destructive` +names the table, and the acknowledged plan still marks it +`.statements[].destructive` in the JSON report. A real run adds the two codes that need something to have executed — **1** when execution failed (a dry run exits 1 only for an operational error such as an unreachable database, because it executes nothing) and **3** when the operator explicitly accepted a blocking form and it committed without an @@ -578,7 +579,7 @@ an unclassified statement is never executed. | Verdict | Lock | Scan / rewrite | Dry-run exit | |---|---|---|---| -| warning — does not change the routing decision | per the routing decision | per the routing decision | unchanged by this code | +| warning on a plan `--allow-destructive` acknowledged; without it `migrate` refuses the statement as `destructive-change` | per the routing decision | per the routing decision | 2 unless acknowledged | The change discards live data or structure: `DROP COLUMN`, `DROP TABLE`, truncating conversions, a dropped constraint or index, or `DROP NOT NULL` @@ -586,7 +587,10 @@ truncating conversions, a dropped constraint or index, or `DROP NOT NULL` `DROP DEFAULT` is deliberately not destructive — a default guarantees nothing about existing rows and is recreated by a metadata-only statement). Emitted alongside the routing decision as a warning so destructive intent is always -visible in review. +visible in review. `migrate --alter` runs such a statement only when +`--allow-destructive SCHEMA.TABLE` names the table; without it the apply and the +dry run end in a `destructive-change` refusal verdict instead of a plan, and +`migrate --desired` refuses a destructive plan whole. ### `concurrent-in-transaction` diff --git a/docs/refusal-classes.md b/docs/refusal-classes.md index d0bc3038..cd833aa1 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` | Desired-state execution will not infer permission to discard live structure | `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` 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 8a86c97b..1d921451 100644 --- a/docs/schemabot-integration.md +++ b/docs/schemabot-integration.md @@ -613,6 +613,24 @@ and PostgreSQL `lock_timeout`). Contract points for the PostgreSQL adapter: the change out as the guidance names or, after coordinating the application deploy, runs the rename with `--force SCHEMA.TABLE`. A rewrite-required refusal of class `capability-boundary` is the other case: a blocking form the planner cannot rewrite yet. +- **Data loss needs the operator's acknowledgement, passed through.** `migrate.Run` refuses a + statement that drops a column, a constraint, or a `NOT NULL` with + `destructive-change`, class `by-design`, unless `Options.AllowDestructive` names the + resolved schema-qualified table (RF-10) — the same acknowledgement the CLI takes as + `--allow-destructive SCHEMA.TABLE`. SchemaBot already asks for this decision: map its + unsafe acknowledgement (`--allow-unsafe` on a plan whose change `IsUnsafe` for data loss) + to `Options.AllowDestructive` for that statement's table, and never set it for a change + the operator did not acknowledge. The refusal maps to `engine.ExecutionModeBlocked` until + then; on `migrate --alter` the reason always means the acknowledgement would lift it, and + a statement that drops a column and adds it back carries `guidance` + `alter-column-type-in-place`, the in-place retype that keeps the values. A mismatched + 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` + rejects `AllowDestructive` (`ErrAllowDestructiveNotSupported`) and keeps refusing a + destructive plan whole, so a declarative drop the operator approves runs as its own + imperative statement. - **Plan-time verdicts are advisory; the executor re-resolves.** The stored verdict exists so operators learn about engine limitations at plan time, but it is estimate-based and stale by apply time. SchemaBot's own engines re-evaluate the refusal/policy predicate per statement @@ -683,7 +701,9 @@ anywhere else, with `--dry-run`, with `--desired`, or alongside the other two acknowledgements (`--force`, `--accept-blocking`): each names a different decision about a different path, and a command carrying two of them is a contradiction, not a combination. An acknowledgement that names another table than the statement's resolved target is the same -error. +error. `--allow-destructive` is not one of them: it acknowledges the statement's data loss, +and a restart runs the same statement again, so a destructive copy-and-swap restarts with +both. Through the library, the verbs are `migrate.Run` with `CopySwapOptions.Restart` set to the acknowledgement, and `schemachange.RunOptions.Restart` on the `Runner` directly; the diff --git a/docs/suggest-report.md b/docs/suggest-report.md index 1c8a877f..6af561f8 100644 --- a/docs/suggest-report.md +++ b/docs/suggest-report.md @@ -16,8 +16,8 @@ and the closed vocabularies below (caveats, guidance codes, and the embedded pla reasons and execution contracts): adding a value to any of them is a contract change and bumps `format_version`, even if no field is added or renamed. -The current version is **4**: version 4 added `coordinated-constraint-rename` to the -Guidance vocabulary; version 3 added `expand-contract-column` and +The current version is **5**: version 5 added `alter-column-type-in-place` to the +Guidance vocabulary; version 4 added `coordinated-constraint-rename`; version 3 added `expand-contract-column` and `coordinated-table-rename` to the Guidance vocabulary and admitted `app-breaking-rename` as a suggestion `reason`; version 2 added `name-constraint-then-validate` and `unique-index-then-constraint` to the Guidance vocabulary. @@ -183,6 +183,18 @@ it commits. Coordinate the rename with the application deploy that stops naming constraint. Offline `suggest` and `lint` cannot tell such an index from a plain one, so they never emit this code. +### `alter-column-type-in-place` + +**Emitted for:** never by a suggestion. A `pg-sprite migrate` refusal verdict with reason +`destructive-change` carries it when one `ALTER TABLE` drops a column and adds a column +of the same name back (`DROP COLUMN qty, ADD COLUMN qty bigint`). + +The drop discards every value the column holds and the re-added column starts empty, +which is rarely what a change of type means. To keep the values, change the column in +place: `ALTER TABLE ... ALTER COLUMN qty TYPE bigint USING qty::bigint` converts each +value instead. If discarding the values is the intent, rerun the statement with +`--allow-destructive` naming the table. + ## The rewrite table: operation → safer form → caveats This is the specification of the advice — which safer native idiom each risky-as-written @@ -211,7 +223,7 @@ A constructed rewrite (`pg-sprite suggest --json` over ```json { - "format_version": 4, + "format_version": 5, "suggestions": [ { "statement": 1, @@ -232,7 +244,7 @@ A guidance suggestion (`ALTER TABLE t ALTER COLUMN c SET NOT NULL, ADD COLUMN d ```json { - "format_version": 4, + "format_version": 5, "suggestions": [ { "statement": 1, diff --git a/integration/supabase/ddl_matrix_test.go b/integration/supabase/ddl_matrix_test.go index 890dfb85..08fd82e6 100644 --- a/integration/supabase/ddl_matrix_test.go +++ b/integration/supabase/ddl_matrix_test.go @@ -111,6 +111,10 @@ type nativeDDLCase struct { // app-breaking rename) so the matrix proves what the forced run // preserves, not that the statement executes unasked. forced bool + // destructive acknowledges a statement that discards data, which + // migrate refuses without the acknowledgement, so the matrix proves + // what the acknowledged run preserves. + destructive bool } // Adding a constant default fills existing rows without replacing the table. @@ -157,8 +161,9 @@ func TestDropColumnDefaultSucceeds(t *testing.T) { // Relaxing nullability removes NOT NULL without changing existing data. func TestDropNotNullSucceeds(t *testing.T) { runNativeDDL(t, nativeDDLCase{ - name: "drop_not_null", - ddl: "ALTER TABLE %s ALTER COLUMN body DROP NOT NULL", + name: "drop_not_null", + ddl: "ALTER TABLE %s ALTER COLUMN body DROP NOT NULL", + destructive: true, proof: `SELECT attnotnull::text FROM pg_attribute WHERE attrelid='%s'::regclass @@ -278,8 +283,9 @@ func TestRenameColumnForcedSucceeds(t *testing.T) { // The explicit DROP COLUMN statement removes the column; this is not desired-plan consent. func TestDropColumnStatementSucceeds(t *testing.T) { runNativeDDL(t, nativeDDLCase{ - name: "drop_column", - ddl: "ALTER TABLE %s DROP COLUMN note", + name: "drop_column", + ddl: "ALTER TABLE %s DROP COLUMN note", + destructive: true, proof: `SELECT count(*)::text FROM pg_attribute WHERE attrelid='%s'::regclass @@ -303,6 +309,9 @@ func runNativeDDL(t *testing.T, tc nativeDDLCase) { if tc.forced { opts.Force = "public." + name } + if tc.destructive { + opts.AllowDestructive = "public." + name + } changeWith(t, pool, fmt.Sprintf(tc.ddl, table), opts) var got string require.NoError(t, pool.QueryRow(t.Context(), fmt.Sprintf(tc.proof, table)).Scan(&got)) diff --git a/internal/cli/allow_destructive_integration_test.go b/internal/cli/allow_destructive_integration_test.go new file mode 100644 index 00000000..4cf11cf7 --- /dev/null +++ b/internal/cli/allow_destructive_integration_test.go @@ -0,0 +1,187 @@ +package cli + +import ( + "encoding/json" + "fmt" + neturl "net/url" + "strings" + "testing" + + "github.com/jackc/pgx/v5/pgxpool" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/block/pg-sprite/internal/testutil" + "github.com/block/pg-sprite/pkg/dbconn" + "github.com/block/pg-sprite/pkg/migrate" + "github.com/block/pg-sprite/pkg/plan" + "github.com/block/pg-sprite/pkg/router" + "github.com/block/pg-sprite/pkg/verdict" +) + +// newOrdersTable creates the table the destructive-change tests drop a +// column from, with rows whose values the drop would discard, and returns +// the database URL, a pool on it, and the table's schema. +func newOrdersTable(t *testing.T) (string, *pgxpool.Pool, string) { + t.Helper() + url := testutil.StartPostgres(t) + pool, err := dbconn.NewPool(t.Context(), dbconn.Config{URL: url}) + require.NoError(t, err) + t.Cleanup(pool.Close) + schema := testutil.NewSchema(t, pool) + _, err = pool.Exec(t.Context(), fmt.Sprintf(` + CREATE TABLE %s.orders ( + id bigint PRIMARY KEY, + note text + )`, schema)) + require.NoError(t, err) + _, err = pool.Exec(t.Context(), fmt.Sprintf(` + INSERT INTO %s.orders (id, note) + SELECT g, 'note ' || g + FROM generate_series(1, 100) g`, schema)) + require.NoError(t, err) + return url, pool, schema +} + +func noteColumnExists(t *testing.T, pool *pgxpool.Pool, schema string) bool { + t.Helper() + var exists bool + require.NoError(t, pool.QueryRow(t.Context(), ` + SELECT EXISTS ( + SELECT 1 FROM information_schema.columns + WHERE table_schema = $1 + AND table_name = 'orders' + AND column_name = 'note')`, + schema).Scan(&exists)) + return exists +} + +// Without --allow-destructive, a DROP COLUMN is refused with the refusal +// exit and the destructive-change verdict, and the column keeps its values. +func TestMigrateRefusesAnUnacknowledgedDropColumn(t *testing.T) { + url, pool, schema := newOrdersTable(t) + + cmd := newMigrateCmd(url, fmt.Sprintf("ALTER TABLE %s.orders DROP COLUMN note", schema)) + cmd.JSON = true + var out strings.Builder + require.ErrorIs(t, cmd.run(t.Context(), &out), verdict.ErrRefused) + + var v verdict.Verdict + require.NoError(t, json.Unmarshal([]byte(out.String()), &v)) + assert.Equal(t, verdict.OutcomeRefused, v.Outcome) + assert.Equal(t, verdict.ReasonDestructiveChange, v.Reason) + assert.Equal(t, verdict.ClassByDesign, v.Class) + assert.False(t, v.AllowedDestructive) + assert.True(t, noteColumnExists(t, pool, schema), "nothing was executed") +} + +// With --allow-destructive naming the table, the drop executes with the +// online-safe exit and the verdict records the acknowledgement. +func TestMigrateRunsAnAcknowledgedDropColumn(t *testing.T) { + url, pool, schema := newOrdersTable(t) + + cmd := newMigrateCmd(url, fmt.Sprintf("ALTER TABLE %s.orders DROP COLUMN note", schema)) + cmd.AllowDestructive = schema + ".orders" + cmd.JSON = true + var out strings.Builder + require.NoError(t, cmd.run(t.Context(), &out)) + + var v verdict.Verdict + require.NoError(t, json.Unmarshal([]byte(out.String()), &v)) + assert.Equal(t, verdict.OutcomeExecuted, v.Outcome) + assert.True(t, v.AllowedDestructive) + assert.False(t, noteColumnExists(t, pool, schema)) +} + +// A dry run ends in the refusal the apply would: the destructive-change +// verdict and the refusal exit, with no plan report. With the +// acknowledgement it reports the plan the acknowledged apply takes, and +// executes nothing either way. +func TestMigrateDryRunReportsTheDestructiveRefusal(t *testing.T) { + url, pool, schema := newOrdersTable(t) + alter := fmt.Sprintf("ALTER TABLE %s.orders DROP COLUMN note", schema) + + refused := newMigrateCmd(url, alter) + refused.DryRun = true + refused.JSON = true + var out strings.Builder + require.ErrorIs(t, refused.run(t.Context(), &out), verdict.ErrRefused) + var wire map[string]any + require.NoError(t, json.Unmarshal([]byte(out.String()), &wire)) + assert.Equal(t, string(verdict.OutcomeRefused), wire["outcome"]) + assert.Equal(t, string(verdict.ReasonDestructiveChange), wire["reason"]) + assert.Equal(t, string(verdict.ClassByDesign), wire["class"]) + assert.NotContains(t, wire, "format_version", "the refusal is a verdict, not a plan report") + + acknowledged := newMigrateCmd(url, alter) + acknowledged.AllowDestructive = schema + ".orders" + report := dryRunPlanOf(t, acknowledged) + require.Len(t, report.Statements, 1) + assert.True(t, report.Statements[0].Destructive) + assert.Equal(t, router.DispositionExecute, report.Disposition) + assert.Equal(t, plan.SourceAlter, report.Source) + + assert.True(t, noteColumnExists(t, pool, schema), "a dry run executes nothing") +} + +// An unqualified table is acknowledged by the schema the session's +// search_path resolves it to. Two schemas hold an orders table, and the +// search_path puts the second one first: naming the other schema's orders +// is a usage error from the dry run and the apply alike, with neither +// table changed, and naming the resolved one drops the column from that +// table only. +func TestMigrateAcknowledgesTheTableTheSearchPathResolves(t *testing.T) { + url, pool, other := newOrdersTable(t) + resolved := testutil.NewSchema(t, pool) + _, err := pool.Exec(t.Context(), fmt.Sprintf(` + CREATE TABLE %s.orders ( + id bigint PRIMARY KEY, + note text + )`, resolved)) + require.NoError(t, err) + u, err := neturl.Parse(url) + require.NoError(t, err) + q := u.Query() + q.Set("options", "-csearch_path="+resolved+","+other) + u.RawQuery = q.Encode() + const alter = "ALTER TABLE orders DROP COLUMN note" + + for _, dryRun := range []bool{true, false} { + cmd := newMigrateCmd(u.String(), alter) + cmd.DryRun = dryRun + cmd.AllowDestructive = other + ".orders" + var out strings.Builder + err := cmd.run(t.Context(), &out) + require.ErrorIs(t, err, migrate.ErrAllowDestructiveMismatch, "dry run %v", dryRun) + assert.Contains(t, err.Error(), resolved+".orders", "the error names the table the search_path resolves") + assert.Empty(t, out.String()) + } + assert.True(t, noteColumnExists(t, pool, other), "the other schema's table is untouched") + assert.True(t, noteColumnExists(t, pool, resolved), "a mismatched acknowledgement executes nothing") + + cmd := newMigrateCmd(u.String(), alter) + cmd.AllowDestructive = resolved + ".orders" + cmd.JSON = true + var out strings.Builder + require.NoError(t, cmd.run(t.Context(), &out)) + var v verdict.Verdict + require.NoError(t, json.Unmarshal([]byte(out.String()), &v)) + assert.Equal(t, verdict.OutcomeExecuted, v.Outcome) + assert.True(t, v.AllowedDestructive) + assert.False(t, noteColumnExists(t, pool, resolved), "the resolved table lost the column") + assert.True(t, noteColumnExists(t, pool, other), "the other schema's table kept it") +} + +// A dry run checks the acknowledgement the way the apply does: one that +// names another table is a usage error with nothing printed. +func TestMigrateDryRunRejectsAMismatchedDestructiveAcknowledgement(t *testing.T) { + url, _, schema := newOrdersTable(t) + + cmd := newMigrateCmd(url, fmt.Sprintf("ALTER TABLE %s.orders DROP COLUMN note", schema)) + cmd.DryRun = true + cmd.AllowDestructive = "public.orders" + var out strings.Builder + err := cmd.run(t.Context(), &out) + require.ErrorIs(t, err, migrate.ErrAllowDestructiveMismatch) + assert.Empty(t, out.String()) +} diff --git a/internal/cli/allow_destructive_test.go b/internal/cli/allow_destructive_test.go new file mode 100644 index 00000000..1518c40d --- /dev/null +++ b/internal/cli/allow_destructive_test.go @@ -0,0 +1,63 @@ +package cli + +import ( + "testing" + + "github.com/alecthomas/kong" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// --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. +func TestAllowDestructiveFlagGrammar(t *testing.T) { + parse := func(args ...string) error { + c := New("test") + k, err := kong.New(c, kong.Vars{"version": "test"}) + require.NoError(t, err) + _, err = k.Parse(append([]string{ + "migrate", + "--url", "postgres://user@localhost:5432/app", + "--alter", "ALTER TABLE public.orders DROP COLUMN note", + "--allow-destructive", "public.orders", + }, args...)) + return err + } + + t.Run("stands alone", func(t *testing.T) { + require.NoError(t, parse()) + }) + + t.Run("combines with --force", func(t *testing.T) { + require.NoError(t, parse("--force", "public.orders")) + }) + + t.Run("combines with --dry-run", func(t *testing.T) { + require.NoError(t, parse("--dry-run")) + }) + + t.Run("combines with --restart", func(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("rejects --desired", func(t *testing.T) { + c := &MigrateCmd{Desired: "orders.sql", AllowDestructive: "public.orders"} + err := c.validateInput() + require.Error(t, err) + assert.Contains(t, err.Error(), "--allow-destructive cannot be combined with --desired") + }) + + t.Run("wires the acknowledgement into the engine policy", func(t *testing.T) { + c := parseMigrate(t, "--allow-destructive", "public.orders") + assert.Equal(t, "public.orders", c.options(nil).AllowDestructive) + }) +} diff --git a/internal/cli/cli.go b/internal/cli/cli.go index 3c989cc4..7360ab8d 100644 --- a/internal/cli/cli.go +++ b/internal/cli/cli.go @@ -113,6 +113,7 @@ type MigrateCmd struct { 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"` 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"` @@ -140,7 +141,12 @@ type MigrateCmd struct { // they have no meaning on a dry run either. --restart acknowledges // discarding a copy-and-swap leftover, so it has no meaning on a dry run // or alongside the acknowledgements that steer a statement away from -// copy-and-swap. +// 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. func (c *MigrateCmd) Validate(kctx *kong.Context) error { if err := c.validateInput(); err != nil { return err @@ -154,6 +160,9 @@ 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/color_test.go b/internal/cli/color_test.go index 1edaec70..f25fff55 100644 --- a/internal/cli/color_test.go +++ b/internal/cli/color_test.go @@ -181,6 +181,7 @@ func fullVerdict(t *testing.T) verdict.Verdict { DependentViews: []string{`"public"."v"`}, ExecutedSQL: []string{`ALTER TABLE "t" ADD CONSTRAINT "c" CHECK (x > 0) NOT VALID`}, Forced: true, + AllowedDestructive: true, BlockingPassthrough: true, LockTimeout: "3s", StatementTimeout: "10m", diff --git a/internal/cli/docs_test.go b/internal/cli/docs_test.go index 4fb43b9b..6e9578ee 100644 --- a/internal/cli/docs_test.go +++ b/internal/cli/docs_test.go @@ -13,6 +13,7 @@ import ( "github.com/stretchr/testify/require" "github.com/block/pg-sprite/pkg/lint" + "github.com/block/pg-sprite/pkg/migrate" "github.com/block/pg-sprite/pkg/plan" "github.com/block/pg-sprite/pkg/planner" "github.com/block/pg-sprite/pkg/preflight" @@ -75,7 +76,7 @@ func TestCLIOutputExamplesMatchPipelineOutput(t *testing.T) { raw, err := os.ReadFile(cliOutputExamplesDoc) require.NoError(t, err) blocks := regexp.MustCompile("(?s)```console\n\\$ pg-sprite [^\n]*--json\n(.*?)```").FindAllStringSubmatch(string(raw), -1) - require.Len(t, blocks, 12, "the doc publishes twelve captured --json outputs") + require.Len(t, blocks, 13, "the doc publishes thirteen captured --json outputs") metadataOnly := alterReport(t, "ALTER TABLE users ADD COLUMN note text", "public", "users", usersFacts()) @@ -132,6 +133,18 @@ func TestCLIOutputExamplesMatchPipelineOutput(t *testing.T) { destructive := alterReport(t, "ALTER TABLE users DROP COLUMN email", "public", "users", usersFacts()) + // Without the acknowledgement the dry run ends in the apply's refusal + // verdict instead of the plan report above. + dropEmail, err := statement.ParseOne("ALTER TABLE users DROP COLUMN email") + require.NoError(t, err) + dropEmailCanonical, err := statement.Canonical(dropEmail.SQL()) + require.NoError(t, err) + dropEmailClassified, err := planner.Classify(dropEmailCanonical, usersFacts()) + require.NoError(t, err) + destructiveRefused, refused, err := migrate.AdmitDestructive(dropEmail, "public", + router.Route([]planner.Plan{dropEmailClassified}).Statements[0], "") + require.NoError(t, err) + require.True(t, refused) lintReport, err := lint.Check("CREATE INDEX users_email_idx ON users (email);\n" + "ALTER TABLE users ADD CONSTRAINT users_email_key UNIQUE (email);\n") @@ -156,7 +169,7 @@ func TestCLIOutputExamplesMatchPipelineOutput(t *testing.T) { diff.Fingerprint = plan.Fingerprint(diff.Statements) want := []any{metadataOnly, saferIdiom, executed, acceptedBlocking, rewriteRequired, - copyAndSwapRouted, executedViaCopyAndSwap, backendUnavailable, partitioned, destructive, + copyAndSwapRouted, executedViaCopyAndSwap, backendUnavailable, partitioned, destructiveRefused, destructive, lintReport, diff} for i, w := range want { marshaled, err := json.Marshal(w) diff --git a/internal/cli/dryrun.go b/internal/cli/dryrun.go index fdc18f9e..9b7359af 100644 --- a/internal/cli/dryrun.go +++ b/internal/cli/dryrun.go @@ -21,7 +21,8 @@ import ( // plan and never executes anything. The statement-type gate runs first, // exactly as it does on apply, so a gated kind dry-runs to the same refusal // verdict (and refusal exit code) the apply would end in; so does the -// executor's shape admission of the routed steps, once they are known, when +// destructive-change gate, unless --allow-destructive names the table, and +// the executor's shape admission of the routed steps, once they are known, when // no earlier apply check refuses first. The checks that need the apply's // live facts — the size guard and the privilege preflight, which the apply // runs before admission — are not run. Introspecting the @@ -62,6 +63,17 @@ func (c *MigrateCmd) runDryRun(ctx context.Context, out io.Writer) error { routed := router.Route([]planner.Plan{classified}) logger.Debug("statement routed", "route", string(classified.Route), "disposition", string(routed.Disposition)) + // The apply refuses a destructive statement before any route runs it, + // so without the acknowledgement the dry run ends in that same refusal + // verdict; with it, the plan below is the one the acknowledged apply + // takes. + v, refused, err := migrate.AdmitDestructive(st, facts.Schema, routed.Statements[0], c.AllowDestructive) + if err != nil { + return err + } + if refused { + return c.emit(out, v) + } report := plan.NewReport(plan.SourceAlter) report.Schema = facts.Schema diff --git a/internal/cli/dryrun_integration_test.go b/internal/cli/dryrun_integration_test.go index ccb76a53..d9411f1b 100644 --- a/internal/cli/dryrun_integration_test.go +++ b/internal/cli/dryrun_integration_test.go @@ -26,7 +26,13 @@ import ( // code. func dryRunPlan(t *testing.T, url, alter string) plan.Report { t.Helper() - cmd := newMigrateCmd(url, alter) + return dryRunPlanOf(t, newMigrateCmd(url, alter)) +} + +// dryRunPlanOf is dryRunPlan for a command the caller configured, such as +// one carrying --allow-destructive. +func dryRunPlanOf(t *testing.T, cmd *MigrateCmd) plan.Report { + t.Helper() cmd.DryRun = true cmd.JSON = true var out strings.Builder @@ -282,6 +288,8 @@ func TestMigrateDryRunInlineConstraintIsRewriteRequired(t *testing.T) { // destructive whether a human submits it directly (the riskier door) or the // diff derives it from a reviewed desired-state file. Destructive is derived // from the classifier's decisions, so the agreement holds by construction. +// The imperative dry run reports the plan only under --allow-destructive; +// without it the dry run ends in the apply's destructive-change refusal. func TestDryRunAndDiffAgreeOnDestructive(t *testing.T) { url := testutil.StartPostgres(t) pool, err := dbconn.NewPool(t.Context(), dbconn.Config{URL: url}) @@ -292,7 +300,9 @@ func TestDryRunAndDiffAgreeOnDestructive(t *testing.T) { "CREATE TABLE %s.t (id int PRIMARY KEY, doomed text)", schema)) require.NoError(t, err) - alterReport := dryRunPlan(t, url, fmt.Sprintf("ALTER TABLE %s.t DROP COLUMN doomed", schema)) + alter := newMigrateCmd(url, fmt.Sprintf("ALTER TABLE %s.t DROP COLUMN doomed", schema)) + alter.AllowDestructive = schema + ".t" + alterReport := dryRunPlanOf(t, alter) require.Len(t, alterReport.Statements, 1) alterSt := alterReport.Statements[0] assert.True(t, alterSt.Destructive, "the submitted DROP COLUMN must be marked destructive") diff --git a/internal/cli/migrate.go b/internal/cli/migrate.go index b7025a00..c5f37061 100644 --- a/internal/cli/migrate.go +++ b/internal/cli/migrate.go @@ -81,6 +81,7 @@ func (c *MigrateCmd) options(logger *slog.Logger) migrate.Options { return migrate.Options{ Force: c.Force, AcceptBlocking: c.AcceptBlocking, + AllowDestructive: c.AllowDestructive, MaxTableSizeBytes: int64(c.MaxTableSize), Budget: executor.SequenceBudget{ Brief: executor.Budget{LockTimeout: c.LockTimeout, StatementTimeout: c.StatementTimeout}, diff --git a/internal/cli/migrate_desired.go b/internal/cli/migrate_desired.go index 8746ee0a..a0d010f6 100644 --- a/internal/cli/migrate_desired.go +++ b/internal/cli/migrate_desired.go @@ -30,6 +30,9 @@ func (c *MigrateCmd) validateInput() error { if c.Restart != "" { return errors.New("--restart cannot be combined with --desired") } + if c.AllowDestructive != "" { + return errors.New("--allow-destructive cannot be combined with --desired: desired-state execution refuses a destructive plan whole") + } } return nil } diff --git a/internal/cli/verdict_text.go b/internal/cli/verdict_text.go index ff9f0703..dc2f48e4 100644 --- a/internal/cli/verdict_text.go +++ b/internal/cli/verdict_text.go @@ -48,6 +48,9 @@ func writeVerdictText(out io.Writer, pal palette, v verdict.Verdict) error { if v.Forced { fmt.Fprintf(&b, "\n %s the submitted form ran as-is (force acknowledged)", pal.bold("forced:")) } + if v.AllowedDestructive { + fmt.Fprintf(&b, "\n %s the statement's data loss was acknowledged (allow-destructive)", pal.bold("allowed:")) + } if v.FailedStep > 0 { fmt.Fprintf(&b, "\n %s step %d: %s", pal.bold("failed at:"), v.FailedStep, v.FailedStepSQL) } diff --git a/pkg/capabilities/capabilities.yaml b/pkg/capabilities/capabilities.yaml index 20f7fdd8..068ffda1 100644 --- a/pkg/capabilities/capabilities.yaml +++ b/pkg/capabilities/capabilities.yaml @@ -80,7 +80,7 @@ rows: front_doors: migrate: supported diff: supported - reason_notes: "Metadata-only; flagged **destructive** in the plan report. Refused with `replica-identity` when a replica identity index the drop reaches keys on the column: the table's own, or one on a partition or inheritance child below it unless the statement says `ONLY`. Refused with `dependent-view`, naming the views, when a view or materialized view reads the column, on a table the drop reaches. Refused with `cascading-drop` when the statement says `CASCADE`, which would also drop the views that read the column" + reason_notes: "Metadata-only; flagged **destructive** in the plan report. `migrate` refuses it with `destructive-change` unless `--allow-destructive` names the table (`migrate --desired` refuses a destructive plan whole); dropping a column and adding it back in one statement is refused with the `alter-column-type-in-place` guidance. Refused with `replica-identity` when a replica identity index the drop reaches keys on the column: the table's own, or one on a partition or inheritance child below it unless the statement says `ONLY`. Refused with `dependent-view`, naming the views, when a view or materialized view reads the column, on a table the drop reaches. Refused with `cascading-drop` when the statement says `CASCADE`, which would also drop the views that read the column" - id: alter-column-type-binary-coercible-proven-against-live-column-facts area: "column_changes" operation: "`ALTER COLUMN TYPE`, binary-coercible (proven against live column facts)" @@ -113,7 +113,7 @@ rows: front_doors: migrate: supported diff: supported - reason_notes: "Metadata-only" + reason_notes: "Metadata-only. `DROP NOT NULL` is flagged **destructive**: `migrate` refuses it with `destructive-change` unless `--allow-destructive` names the table" - id: set-not-null area: "column_changes" operation: "`SET NOT NULL`" @@ -218,7 +218,7 @@ rows: front_doors: migrate: supported diff: supported - reason_notes: "Metadata-only; flagged **destructive**. Refused with `replica-identity` when the constraint owns the table's replica identity index, or the index a partition's is attached under. Refused with `cascading-drop` when the statement says `CASCADE`, which would also drop other tables' foreign keys on the constraint" + reason_notes: "Metadata-only; flagged **destructive**. `migrate` refuses it with `destructive-change` unless `--allow-destructive` names the table. Refused with `replica-identity` when the constraint owns the table's replica identity index, or the index a partition's is attached under. Refused with `cascading-drop` when the statement says `CASCADE`, which would also drop other tables' foreign keys on the constraint" - id: create-unique-index-on-a-plain-table-including-partial-expression-covering-include-gin-gist-brin area: "indexes" operation: "`CREATE [UNIQUE] INDEX` on a plain table — including partial, expression, covering (`INCLUDE`), GIN/GiST/BRIN" diff --git a/pkg/capabilities/front_door_integration_test.go b/pkg/capabilities/front_door_integration_test.go index cf0bd2b2..d170f43d 100644 --- a/pkg/capabilities/front_door_integration_test.go +++ b/pkg/capabilities/front_door_integration_test.go @@ -31,6 +31,11 @@ type migrateClaim struct { // fixture server has no logical decoding) and its executed outcome is // the route's own token. copyAndSwap bool + // destructive marks a row whose statements discard live data or + // structure: migrate runs them only under the destructive + // acknowledgement, which the run names for the fixture's accounts + // table, the table every such row's statements change. + destructive bool } // migrateClaims maps every capability row whose imperative `migrate` door is @@ -66,7 +71,8 @@ var migrateClaims = map[string]migrateClaim{ id bigint PRIMARY KEY, legacy_id int )`, - statements: []string{`ALTER TABLE %[1]s.accounts DROP COLUMN legacy_id`}, + statements: []string{`ALTER TABLE %[1]s.accounts DROP COLUMN legacy_id`}, + destructive: true, }, "alter-column-type-binary-coercible-proven-against-live-column-facts": { setup: ` @@ -112,6 +118,7 @@ var migrateClaims = map[string]migrateClaim{ `ALTER TABLE %[1]s.accounts ALTER COLUMN nickname DROP DEFAULT`, `ALTER TABLE %[1]s.accounts ALTER COLUMN nickname DROP NOT NULL`, }, + destructive: true, }, "set-not-null": { setup: ` @@ -180,7 +187,8 @@ var migrateClaims = map[string]migrateClaim{ balance int, CONSTRAINT balance_nonnegative CHECK (balance >= 0) )`, - statements: []string{`ALTER TABLE %[1]s.accounts DROP CONSTRAINT balance_nonnegative`}, + statements: []string{`ALTER TABLE %[1]s.accounts DROP CONSTRAINT balance_nonnegative`}, + destructive: true, }, "create-unique-index-on-a-plain-table-including-partial-expression-covering-include-gin-gist-brin": { setup: ` @@ -246,6 +254,9 @@ func TestMigrateSupportedRowsExecute(t *testing.T) { require.NoError(t, err) opts := migrate.DefaultOptions() + if claim.destructive { + opts.AllowDestructive = schema + ".accounts" + } executed := verdict.OutcomeExecuted if claim.copyAndSwap { opts.CopySwap = migrate.CopySwapOptions{ diff --git a/pkg/migrate/allow_destructive.go b/pkg/migrate/allow_destructive.go new file mode 100644 index 00000000..eadf2ba7 --- /dev/null +++ b/pkg/migrate/allow_destructive.go @@ -0,0 +1,139 @@ +package migrate + +import ( + "errors" + "fmt" + "log/slog" + "strings" + + "github.com/block/pg-sprite/pkg/router" + "github.com/block/pg-sprite/pkg/statement" + "github.com/block/pg-sprite/pkg/suggest" + "github.com/block/pg-sprite/pkg/verdict" +) + +// This file is the imperative front door's destructive-change gate: a +// statement whose plan discards live data or structure runs only when the +// operator's acknowledgement names its table. The core never checks +// destructiveness (SAFETY.md), so this gate and the planner's Destructive +// derivation are the whole guard on this path. + +var ( + // ErrAllowDestructiveMismatch means the destructive acknowledgement + // named a table other than the statement's resolved target. Nothing has + // executed. + ErrAllowDestructiveMismatch = errors.New("the destructive acknowledgement must name the resolved target table") + // ErrAllowDestructiveNotSupported is returned by [RunDesired] when + // Options.AllowDestructive is set: desired-state execution refuses a + // destructive plan whole and has no acknowledgement for it. + ErrAllowDestructiveNotSupported = errors.New( + "the destructive acknowledgement applies to the imperative front door only; desired-state execution refuses a destructive plan whole") +) + +// checkAllowDestructiveAck validates the destructive acknowledgement +// against the resolved schema-qualified target table, the same typed +// acknowledgement the force override takes. A mismatch is a usage error: +// nothing has executed. +func checkAllowDestructiveAck(table, ack string) error { + if ack == table { + return nil + } + return fmt.Errorf("%w %q, got %q; nothing was executed", ErrAllowDestructiveMismatch, table, ack) +} + +// destructiveOperations returns the labels of the routed statement's +// operations that would discard live data or structure, in statement order, +// for every disposition a route could run: executed as routed, or forced +// past a rewrite-required or backend-unavailable refusal. It is empty when +// the statement discards nothing, and for a planner refusal, which runs +// nothing and which no acknowledgement lifts, so that refusal keeps its own +// reason. +func destructiveOperations(rs router.Statement) []string { + if rs.Disposition == router.DispositionRefuse { + return nil + } + var ops []string + for _, d := range rs.Decisions { + if d.Destructive { + ops = append(ops, d.Operation) + } + } + return ops +} + +// destructiveChangeVerdict is the refusal for a statement that discards +// live data or structure without the destructive acknowledgement. It shares +// its reason and class with the desired-state admission refusal, so both +// front doors report one contract. A statement that drops a column and adds +// it back under the same name carries the guidance to change the column in +// place, which keeps its values. +func destructiveChangeVerdict(st statement.Statement, ops []string) (verdict.Verdict, error) { + detail := fmt.Sprintf("the statement discards live data or structure (%s); pg-sprite runs no destructive "+ + "statement unless the destructive acknowledgement names its table — review what is lost, then rerun "+ + "with --allow-destructive %s", + strings.Join(ops, ", "), qualified(st)) + v := verdict.Verdict{Statement: st.SQL(), Table: qualified(st)} + parsed, err := statement.ParseOps(st.SQL()) + if err != nil { + return verdict.Verdict{}, fmt.Errorf("parse destructive statement on %s: %w", qualified(st), err) + } + if column, ok := suggest.ReAddedColumn(parsed); ok { + v.Guidance = suggest.GuidanceAlterColumnTypeInPlace + detail += fmt.Sprintf("; the statement drops %s and adds it back empty — to keep its values, %s", + column, manualPath(v.Guidance)) + } + v.Detail = detail + return v.WithRefusal(destructiveChangeRefusal()), nil +} + +// AdmitDestructive is the destructive-change gate a dry run applies to the +// statement it routed, with the result [Run] reaches for the same statement +// and acknowledgement: the refusal verdict when the routed statement rs +// discards live data or structure and ack is empty, or the mismatch error +// when ack does not name the resolved target table. The boolean is false +// when the statement discards nothing or ack names its table. +// +// schema is the schema st's table resolved to on the session's search_path, +// as the dry run's live facts read it; a qualified st ignores it. An +// unqualified st's verdict names the table and statement the way [Run]'s +// does, qualified. +func AdmitDestructive(st statement.Statement, schema string, rs router.Statement, ack string) (verdict.Verdict, bool, error) { + unqualified := st.Schema() == "" && st.Table() != "" + if unqualified && schema == "" { + return verdict.Verdict{}, false, fmt.Errorf("admit destructive change for %s: %w", st.Table(), errAdmissionSchemaUnknown) + } + table := qualified(st) + if unqualified { + table = schema + "." + st.Table() + } + if ack != "" { + return verdict.Verdict{}, false, checkAllowDestructiveAck(table, ack) + } + ops := destructiveOperations(rs) + if len(ops) == 0 { + return verdict.Verdict{}, false, nil + } + target := st + if unqualified { + resolved, err := resolveForVerdict(st, schema) + if err != nil { + return verdict.Verdict{}, false, err + } + target = resolved + } + v, err := destructiveChangeVerdict(target, ops) + if err != nil { + return verdict.Verdict{}, false, err + } + 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) { + audit.Warn("destructive change acknowledged; the statement will discard live data or structure", + "table", qualified(st), + "kind", st.Kind().String()) +} diff --git a/pkg/migrate/allow_destructive_integration_test.go b/pkg/migrate/allow_destructive_integration_test.go new file mode 100644 index 00000000..80e57094 --- /dev/null +++ b/pkg/migrate/allow_destructive_integration_test.go @@ -0,0 +1,389 @@ +package migrate_test + +import ( + "encoding/json" + "strings" + "testing" + + "github.com/jackc/pgx/v5/pgxpool" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/block/pg-sprite/internal/testutil" + "github.com/block/pg-sprite/pkg/dbconn" + "github.com/block/pg-sprite/pkg/migrate" + "github.com/block/pg-sprite/pkg/suggest" + "github.com/block/pg-sprite/pkg/verdict" +) + +// destructiveOptions is runOptions with the destructive acknowledgement +// naming table, for a test whose subject is something other than the +// destructive-change gate but whose statement drops something. +func destructiveOptions(table string) migrate.Options { + opts := runOptions() + opts.AllowDestructive = table + return opts +} + +// destructiveFixture is one database with a schema of its own holding the +// orders table every destructive-change test drops something from. +type destructiveFixture struct { + cfg dbconn.Config + pool *pgxpool.Pool + schema string +} + +// newDestructiveFixture creates the orders table: a column, a CHECK +// constraint, and a NOT NULL for each destructive operation kind to +// discard, and rows whose values a drop would lose. +func newDestructiveFixture(t *testing.T) destructiveFixture { + t.Helper() + cfg := dbconn.Config{URL: testutil.StartPostgres(t)} + pool, err := dbconn.NewPool(t.Context(), cfg) + require.NoError(t, err) + t.Cleanup(pool.Close) + f := destructiveFixture{cfg: cfg, pool: pool, schema: testutil.NewSchema(t, pool)} + f.exec(t, ` + CREATE TABLE {schema}.orders ( + id bigint PRIMARY KEY, + qty integer NOT NULL CONSTRAINT orders_qty_positive CHECK (qty > 0), + note text NOT NULL + )`) + f.exec(t, `CREATE INDEX orders_qty_idx ON {schema}.orders (qty)`) + f.exec(t, ` + INSERT INTO {schema}.orders (id, qty, note) + SELECT g, g, 'note ' || g + FROM generate_series(1, 500) g`) + return f +} + +// inSchema substitutes every {schema} in sql with the fixture's schema. +func (f destructiveFixture) inSchema(sql string) string { + return strings.ReplaceAll(sql, "{schema}", f.schema) +} + +func (f destructiveFixture) exec(t *testing.T, sql string) { + t.Helper() + _, err := f.pool.Exec(t.Context(), f.inSchema(sql)) + require.NoError(t, err) +} + +// run drives the imperative front door for sql, every {schema} substituted. +func (f destructiveFixture) run(t *testing.T, sql string, opts migrate.Options) (verdict.Verdict, error) { + t.Helper() + return migrate.Run(t.Context(), f.pool, parseOne(t, f.inSchema(sql)), opts) +} + +// table is the acknowledgement that names the fixture's orders table. +func (f destructiveFixture) table() string { return f.schema + ".orders" } + +// copySwapOptions is a quiesced copy-and-swap policy over the fixture's +// database with room on disk, so a statement routed to that backend can run. +func (f destructiveFixture) copySwapOptions() migrate.Options { + opts := runOptions() + opts.CopySwap = migrate.CopySwapOptions{ + Connection: f.cfg, + FreeDiskBytes: testutil.UnlimitedDisk, + Quiesced: true, + } + return opts +} + +func (f destructiveFixture) hasColumn(t *testing.T, column string) bool { + t.Helper() + var exists bool + require.NoError(t, f.pool.QueryRow(t.Context(), ` + SELECT EXISTS ( + SELECT 1 FROM pg_attribute + WHERE attrelid = ($1 || '.orders')::regclass + AND attname = $2 + AND NOT attisdropped)`, + f.schema, column).Scan(&exists)) + return exists +} + +func (f destructiveFixture) hasConstraint(t *testing.T, name string) bool { + t.Helper() + var exists bool + require.NoError(t, f.pool.QueryRow(t.Context(), ` + SELECT EXISTS ( + SELECT 1 FROM pg_constraint + WHERE conrelid = ($1 || '.orders')::regclass + AND conname = $2)`, + f.schema, name).Scan(&exists)) + return exists +} + +func (f destructiveFixture) notNull(t *testing.T, column string) bool { + t.Helper() + var notNull bool + require.NoError(t, f.pool.QueryRow(t.Context(), ` + SELECT attnotnull FROM pg_attribute + WHERE attrelid = ($1 || '.orders')::regclass + AND attname = $2`, + f.schema, column).Scan(¬Null)) + return notNull +} + +func (f destructiveFixture) columnType(t *testing.T, column string) string { + t.Helper() + var typ string + require.NoError(t, f.pool.QueryRow(t.Context(), ` + SELECT data_type FROM information_schema.columns + WHERE table_schema = $1 + AND table_name = 'orders' + AND column_name = $2`, + f.schema, column).Scan(&typ)) + return typ +} + +// assertDestructiveRefusal pins the refusal contract both front doors share: +// reason destructive-change, class by-design, a nil error, and the table +// named in the detail's acknowledgement. Nothing the gate refuses executes, +// so each caller also asserts the durable state the statement would have +// changed. +func assertDestructiveRefusal(t *testing.T, f destructiveFixture, v verdict.Verdict, err error) { + t.Helper() + require.NoError(t, err, "a refusal is a verdict, not an error") + assert.Equal(t, verdict.OutcomeRefused, v.Outcome) + assert.Equal(t, verdict.ReasonDestructiveChange, v.Reason) + assert.Equal(t, verdict.ClassByDesign, v.Class) + assert.Empty(t, v.Owner) + assert.Equal(t, f.table(), v.Table) + assert.False(t, v.AllowedDestructive) + assert.Contains(t, v.Detail, "--allow-destructive "+f.table()) + refusal, err := v.Refusal() + require.NoError(t, err, "the verdict carries its typed refusal proof") + assert.False(t, verdict.AcceptedBlockingEligible(refusal), "accept-blocking cannot run a destructive statement") +} + +// A DROP COLUMN discards every value the column holds, so without the +// acknowledgement it is refused before anything runs, although PostgreSQL +// would drop the column as an instant catalog change. +func TestRunRefusesAnUnacknowledgedDropColumn(t *testing.T) { + f := newDestructiveFixture(t) + + v, err := f.run(t, `ALTER TABLE {schema}.orders DROP COLUMN note`, runOptions()) + + assertDestructiveRefusal(t, f, v, err) + assert.Empty(t, v.Guidance, "nothing is added back, so there is no in-place change to point at") + assert.True(t, f.hasColumn(t, "note"), "the column and its values are still there") +} + +// A DROP CONSTRAINT discards a guarantee the schema was providing, so it +// is refused without the acknowledgement like a dropped column. +func TestRunRefusesAnUnacknowledgedDropConstraint(t *testing.T) { + f := newDestructiveFixture(t) + + v, err := f.run(t, `ALTER TABLE {schema}.orders DROP CONSTRAINT orders_qty_positive`, runOptions()) + + assertDestructiveRefusal(t, f, v, err) + assert.True(t, f.hasConstraint(t, "orders_qty_positive"), "the constraint is still there") +} + +// DROP NOT NULL discards the same guarantee its constraint form would, so +// it is refused without the acknowledgement too. +func TestRunRefusesAnUnacknowledgedDropNotNull(t *testing.T) { + f := newDestructiveFixture(t) + + v, err := f.run(t, `ALTER TABLE {schema}.orders ALTER COLUMN note DROP NOT NULL`, runOptions()) + + assertDestructiveRefusal(t, f, v, err) + assert.True(t, f.notNull(t, "note"), "the column is still NOT NULL") +} + +// With the acknowledgement naming the table, the DROP COLUMN runs its +// normal native path, and the verdict records the acknowledgement. +func TestRunExecutesAnAcknowledgedDropColumn(t *testing.T) { + f := newDestructiveFixture(t) + opts := runOptions() + opts.AllowDestructive = f.table() + + v, err := f.run(t, `ALTER TABLE {schema}.orders DROP COLUMN note`, opts) + + require.NoError(t, err) + assert.Equal(t, verdict.OutcomeExecuted, v.Outcome) + assert.True(t, v.AllowedDestructive) + assert.False(t, v.Forced, "the destructive acknowledgement is not an override of the routing") + assert.False(t, f.hasColumn(t, "note")) + + raw, err := v.JSON() + require.NoError(t, err) + var wire map[string]any + require.NoError(t, json.Unmarshal([]byte(raw), &wire)) + assert.Equal(t, true, wire["allowed_destructive"], "the acknowledgement is part of the JSON contract") +} + +// An acknowledged DROP CONSTRAINT runs and records the acknowledgement. +func TestRunExecutesAnAcknowledgedDropConstraint(t *testing.T) { + f := newDestructiveFixture(t) + opts := runOptions() + opts.AllowDestructive = f.table() + + v, err := f.run(t, `ALTER TABLE {schema}.orders DROP CONSTRAINT orders_qty_positive`, opts) + + require.NoError(t, err) + assert.Equal(t, verdict.OutcomeExecuted, v.Outcome) + assert.True(t, v.AllowedDestructive) + assert.False(t, f.hasConstraint(t, "orders_qty_positive")) +} + +// An acknowledged DROP NOT NULL runs and records the acknowledgement. +func TestRunExecutesAnAcknowledgedDropNotNull(t *testing.T) { + f := newDestructiveFixture(t) + opts := runOptions() + opts.AllowDestructive = f.table() + + v, err := f.run(t, `ALTER TABLE {schema}.orders ALTER COLUMN note DROP NOT NULL`, opts) + + require.NoError(t, err) + assert.Equal(t, verdict.OutcomeExecuted, v.Outcome) + assert.True(t, v.AllowedDestructive) + assert.False(t, f.notNull(t, "note")) +} + +// A statement that discards nothing runs as it always did with the +// acknowledgement present, and its verdict claims no acknowledged loss. +func TestRunIgnoresTheAcknowledgementForANonDestructiveStatement(t *testing.T) { + f := newDestructiveFixture(t) + opts := runOptions() + opts.AllowDestructive = f.table() + + v, err := f.run(t, `ALTER TABLE {schema}.orders ADD COLUMN sku text`, opts) + + require.NoError(t, err) + assert.Equal(t, verdict.OutcomeExecuted, v.Outcome) + assert.False(t, v.AllowedDestructive) + assert.True(t, f.hasColumn(t, "sku")) +} + +// An acknowledgement that names another table is a usage error with no +// verdict, and nothing executes: the typed acknowledgement exists so a +// copied command cannot discard a different table's data. +func TestRunRejectsAMismatchedDestructiveAcknowledgement(t *testing.T) { + f := newDestructiveFixture(t) + opts := runOptions() + opts.AllowDestructive = "public.orders" + + v, err := f.run(t, `ALTER TABLE {schema}.orders DROP COLUMN note`, opts) + + require.ErrorIs(t, err, migrate.ErrAllowDestructiveMismatch) + assert.Contains(t, err.Error(), f.table(), "the error names the table the acknowledgement must name") + assert.Equal(t, verdict.Verdict{}, v, "a usage error carries no verdict") + assert.True(t, f.hasColumn(t, "note"), "nothing was executed") +} + +// The gate covers every route: a statement that drops a column and +// retypes another routes to copy-and-swap, and without the acknowledgement +// it is refused before any shadow table is built. +func TestRunRefusesAnUnacknowledgedDropColumnRoutedToCopyAndSwap(t *testing.T) { + f := newDestructiveFixture(t) + const alter = ` + ALTER TABLE {schema}.orders + DROP COLUMN note, + ALTER COLUMN qty TYPE bigint` + + v, err := f.run(t, alter, f.copySwapOptions()) + + assertDestructiveRefusal(t, f, v, err) + assert.True(t, f.hasColumn(t, "note"), "the column is still there") + assert.Equal(t, "integer", f.columnType(t, "qty"), "the retype did not run either") + var tables int + require.NoError(t, f.pool.QueryRow(t.Context(), ` + SELECT count(*) FROM pg_class c + JOIN pg_namespace n ON n.oid = c.relnamespace + WHERE n.nspname = $1 + AND c.relkind = 'r'`, + f.schema).Scan(&tables)) + assert.Equal(t, 1, tables, "no shadow table was built") +} + +// With the acknowledgement the same statement runs through copy-and-swap: +// the destructive acknowledgement lifts the gate and nothing else. +func TestRunExecutesAnAcknowledgedDropColumnThroughCopyAndSwap(t *testing.T) { + f := newDestructiveFixture(t) + opts := f.copySwapOptions() + opts.AllowDestructive = f.table() + const alter = ` + ALTER TABLE {schema}.orders + DROP COLUMN note, + ALTER COLUMN qty TYPE bigint` + + v, err := f.run(t, alter, opts) + + require.NoError(t, err) + assert.Equal(t, verdict.OutcomeExecutedViaCopyAndSwap, v.Outcome) + assert.True(t, v.AllowedDestructive) + assert.False(t, f.hasColumn(t, "note")) + assert.Equal(t, "bigint", f.columnType(t, "qty")) +} + +// --force and the destructive acknowledgement accept different risks. A +// forced run of a type rewrite that also drops a column is still refused +// for the data loss, and runs once both are named. +func TestRunNeedsBothForceAndTheDestructiveAcknowledgement(t *testing.T) { + f := newDestructiveFixture(t) + const alter = ` + ALTER TABLE {schema}.orders + DROP COLUMN note, + ALTER COLUMN qty TYPE bigint` + + forced := runOptions() + forced.Force = f.table() + v, err := f.run(t, alter, forced) + assertDestructiveRefusal(t, f, v, err) + assert.True(t, f.hasColumn(t, "note"), "force alone does not acknowledge the loss") + + both := runOptions() + both.Force = f.table() + both.AllowDestructive = f.table() + v, err = f.run(t, alter, both) + require.NoError(t, err) + assert.Equal(t, verdict.OutcomeExecuted, v.Outcome, "the forced form runs natively, not through copy-and-swap") + assert.True(t, v.Forced) + assert.True(t, v.AllowedDestructive) + assert.False(t, f.hasColumn(t, "note")) + assert.Equal(t, "bigint", f.columnType(t, "qty")) +} + +// Dropping a column and adding it back under the same name in one ALTER +// discards every value and leaves the column empty, which is rarely what a +// change of type means. The refusal carries the typed guidance to change +// the column in place, which keeps the values. +func TestRunGuidesADropAndReAddToAnInPlaceTypeChange(t *testing.T) { + f := newDestructiveFixture(t) + const alter = ` + ALTER TABLE {schema}.orders + DROP COLUMN qty, + ADD COLUMN qty bigint` + + v, err := f.run(t, alter, runOptions()) + + assertDestructiveRefusal(t, f, v, err) + assert.Equal(t, suggest.GuidanceAlterColumnTypeInPlace, v.Guidance) + assert.Contains(t, v.Detail, "ALTER COLUMN ... TYPE ... USING") + var total int64 + require.NoError(t, f.pool.QueryRow(t.Context(), + "SELECT sum(qty) FROM "+f.schema+".orders").Scan(&total)) + assert.EqualValues(t, 500*501/2, total, "every qty value is still there") +} + +// The destructive acknowledgement does not reach a DROP INDEX: the +// statement-kind gate refuses it first in favor of DROP INDEX +// CONCURRENTLY, and only --accept-blocking governs that refusal. +func TestRunAllowDestructiveDoesNotLiftADropIndexRefusal(t *testing.T) { + f := newDestructiveFixture(t) + opts := runOptions() + opts.AllowDestructive = f.table() + + v, err := f.run(t, `DROP INDEX {schema}.orders_qty_idx`, opts) + + require.NoError(t, err) + assert.Equal(t, verdict.OutcomeRefused, v.Outcome) + assert.Equal(t, verdict.ReasonIndexStatement, v.Reason) + assert.False(t, v.AllowedDestructive) + var exists bool + require.NoError(t, f.pool.QueryRow(t.Context(), + "SELECT to_regclass($1) IS NOT NULL", f.schema+".orders_qty_idx").Scan(&exists)) + assert.True(t, exists, "the index is still there") +} diff --git a/pkg/migrate/allow_destructive_test.go b/pkg/migrate/allow_destructive_test.go new file mode 100644 index 00000000..d6ce689e --- /dev/null +++ b/pkg/migrate/allow_destructive_test.go @@ -0,0 +1,144 @@ +package migrate + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/block/pg-sprite/pkg/planner" + "github.com/block/pg-sprite/pkg/router" + "github.com/block/pg-sprite/pkg/statement" + "github.com/block/pg-sprite/pkg/suggest" + "github.com/block/pg-sprite/pkg/verdict" +) + +// routeOne classifies and routes sql without live facts, the way a dry run +// against a table it could not introspect would. +func routeOne(t *testing.T, sql string) (statement.Statement, router.Statement) { + t.Helper() + st, err := statement.ParseOne(sql) + require.NoError(t, err) + canonical, err := statement.Canonical(st.SQL()) + require.NoError(t, err) + classified, err := planner.Classify(canonical, planner.Facts{}) + require.NoError(t, err) + 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. +func TestRunDesiredRejectsAllowDestructive(t *testing.T) { + opts := DefaultOptions() + opts.AllowDestructive = "public.t" + + res, err := RunDesired(t.Context(), nil, DesiredRequest{}, opts) + require.ErrorIs(t, err, ErrAllowDestructiveNotSupported) + assert.Equal(t, DesiredResult{}, res) +} + +// The dry run's gate decides a DROP COLUMN exactly as Run does: refused +// without the acknowledgement, admitted with one naming the table, and a +// usage error with one naming another table. +func TestAdmitDestructive(t *testing.T) { + st, rs := routeOne(t, "ALTER TABLE app.orders DROP COLUMN note") + + t.Run("refuses without the acknowledgement", func(t *testing.T) { + v, refused, err := AdmitDestructive(st, "app", rs, "") + require.NoError(t, err) + require.True(t, refused) + assert.Equal(t, verdict.ReasonDestructiveChange, v.Reason) + assert.Equal(t, verdict.ClassByDesign, v.Class) + assert.Equal(t, "app.orders", v.Table) + }) + + t.Run("admits the acknowledged statement", func(t *testing.T) { + _, refused, err := AdmitDestructive(st, "app", rs, "app.orders") + require.NoError(t, err) + assert.False(t, refused) + }) + + t.Run("rejects an acknowledgement naming another table", func(t *testing.T) { + _, refused, err := AdmitDestructive(st, "app", rs, "public.orders") + require.ErrorIs(t, err, ErrAllowDestructiveMismatch) + assert.False(t, refused) + }) + + t.Run("qualifies an unqualified table with the resolved schema", func(t *testing.T) { + bare, bareRS := routeOne(t, "ALTER TABLE orders DROP COLUMN note") + v, refused, err := AdmitDestructive(bare, "app", bareRS, "") + require.NoError(t, err) + require.True(t, refused) + assert.Equal(t, "app.orders", v.Table) + _, _, err = AdmitDestructive(bare, "app", bareRS, "app.orders") + require.NoError(t, err) + }) + + // The deparser cannot re-emit every unqualified statement qualified (a + // rename, for one), so the gate qualifies only a statement it refuses + // and leaves the rest to the dry run's own report. + t.Run("leaves an unqualified statement that discards nothing untouched", func(t *testing.T) { + rename, renameRS := routeOne(t, "ALTER TABLE users RENAME COLUMN name TO full_name") + _, refused, err := AdmitDestructive(rename, "public", renameRS, "") + require.NoError(t, err) + assert.False(t, refused) + }) + + t.Run("admits a statement that discards nothing", func(t *testing.T) { + add, addRS := routeOne(t, "ALTER TABLE app.orders ADD COLUMN sku text") + _, refused, err := AdmitDestructive(add, "app", addRS, "") + require.NoError(t, err) + assert.False(t, refused) + }) +} + +// A planner refusal runs nothing and no acknowledgement lifts it, so the +// destructive gate leaves it alone and it keeps its own reason. +func TestDestructiveGateLeavesPlannerRefusalsAlone(t *testing.T) { + rs := router.Statement{ + Plan: planner.Plan{ + Decisions: []planner.Decision{{Operation: "DROP COLUMN ref", Destructive: true}}, + }, + Disposition: router.DispositionRefuse, + } + assert.Empty(t, destructiveOperations(rs)) +} + +// Dropping a column and adding it back under the same name carries the +// guidance to change the column in place; a drop that adds nothing back, +// or adds a different column, carries none. +func TestDestructiveChangeVerdictGuidance(t *testing.T) { + reAdd, err := statement.ParseOne(` + ALTER TABLE app.orders + DROP COLUMN qty, + ADD COLUMN qty bigint`) + require.NoError(t, err) + v, err := destructiveChangeVerdict(reAdd, []string{"DROP COLUMN qty"}) + require.NoError(t, err) + assert.Equal(t, suggest.GuidanceAlterColumnTypeInPlace, v.Guidance) + + other, err := statement.ParseOne(` + ALTER TABLE app.orders + DROP COLUMN qty, + ADD COLUMN quantity bigint`) + require.NoError(t, err) + v, err = destructiveChangeVerdict(other, []string{"DROP COLUMN qty"}) + require.NoError(t, err) + assert.Empty(t, v.Guidance) +} diff --git a/pkg/migrate/cascading_drop_integration_test.go b/pkg/migrate/cascading_drop_integration_test.go index 1f89651e..6d739f30 100644 --- a/pkg/migrate/cascading_drop_integration_test.go +++ b/pkg/migrate/cascading_drop_integration_test.go @@ -87,7 +87,7 @@ func TestRunRefusesACascadingDrop(t *testing.T) { t.Run("a drop without CASCADE on a column nothing depends on", func(t *testing.T) { schema := seed(t) - v, err := migrate.Run(t.Context(), pool, parseOne(t, fmt.Sprintf("ALTER TABLE %s.users DROP COLUMN note", schema)), runOptions()) + v, err := migrate.Run(t.Context(), pool, parseOne(t, fmt.Sprintf("ALTER TABLE %s.users DROP COLUMN note", schema)), destructiveOptions(schema+".users")) require.NoError(t, err) assert.Equal(t, verdict.OutcomeExecuted, v.Outcome, "detail: %s", v.Detail) assert.False(t, columnExists(t, pool, schema, "users", "note")) diff --git a/pkg/migrate/dependent_view_integration_test.go b/pkg/migrate/dependent_view_integration_test.go index 4b52f436..89781883 100644 --- a/pkg/migrate/dependent_view_integration_test.go +++ b/pkg/migrate/dependent_view_integration_test.go @@ -101,7 +101,11 @@ func TestRunRefusesChangingAColumnAViewReads(t *testing.T) { // default as app-breaking; the view gate is what this case proves // does not fire. forced bool - check func(t *testing.T, schema string) + // destructive acknowledges the dropped column's data loss, which + // migrate refuses by default; the view gate is what this case + // proves does not fire. + destructive bool + check func(t *testing.T, schema string) }{ { name: "renaming the column the views read", @@ -112,8 +116,9 @@ func TestRunRefusesChangingAColumnAViewReads(t *testing.T) { }, }, { - name: "dropping a column no view reads", - statement: "ALTER TABLE %s.users DROP COLUMN note", + name: "dropping a column no view reads", + statement: "ALTER TABLE %s.users DROP COLUMN note", + destructive: true, check: func(t *testing.T, schema string) { assert.False(t, columnExists(t, pool, schema, "users", "note")) }, @@ -125,6 +130,9 @@ func TestRunRefusesChangingAColumnAViewReads(t *testing.T) { if tc.forced { opts.Force = schema + ".users" } + if tc.destructive { + opts.AllowDestructive = schema + ".users" + } v, err := migrate.Run(t.Context(), pool, parseOne(t, fmt.Sprintf(tc.statement, schema)), opts) require.NoError(t, err) assert.Equal(t, verdict.OutcomeExecuted, v.Outcome, "detail: %s", v.Detail) @@ -172,7 +180,7 @@ func TestRunRefusesChangingAColumnAChildsViewReads(t *testing.T) { t.Run("dropping under ONLY", func(t *testing.T) { schema := seed(t) - v, err := migrate.Run(t.Context(), pool, parseOne(t, fmt.Sprintf("ALTER TABLE ONLY %s.users DROP COLUMN email", schema)), runOptions()) + v, err := migrate.Run(t.Context(), pool, parseOne(t, fmt.Sprintf("ALTER TABLE ONLY %s.users DROP COLUMN email", schema)), destructiveOptions(schema+".users")) require.NoError(t, err) assert.Equal(t, verdict.OutcomeExecuted, v.Outcome, "detail: %s", v.Detail) assert.False(t, columnExists(t, pool, schema, "users", "email")) @@ -204,7 +212,7 @@ func TestRunDropsAColumnAChildDeclaresWhileAViewReadsTheChildsCopy(t *testing.T) t.Run("the drop runs", func(t *testing.T) { schema := seed(t) - v, err := migrate.Run(t.Context(), pool, parseOne(t, fmt.Sprintf("ALTER TABLE %s.users DROP COLUMN email", schema)), runOptions()) + v, err := migrate.Run(t.Context(), pool, parseOne(t, fmt.Sprintf("ALTER TABLE %s.users DROP COLUMN email", schema)), destructiveOptions(schema+".users")) require.NoError(t, err) assert.Equal(t, verdict.OutcomeExecuted, v.Outcome, "detail: %s", v.Detail) assert.False(t, columnExists(t, pool, schema, "users", "email")) diff --git a/pkg/migrate/desired.go b/pkg/migrate/desired.go index 93fda2c4..8421b93b 100644 --- a/pkg/migrate/desired.go +++ b/pkg/migrate/desired.go @@ -183,9 +183,10 @@ func (r DesiredResult) refused(ref verdict.Refusal, detail string) DesiredResult // the operational error. An error with a zero result means the pipeline // stopped before planning or executing anything. // -// Options.Force is rejected: the declarative front door never runs a -// submitted form blind. A destructive or force-worthy change belongs on -// the imperative front door where the operator states it explicitly. +// Options.Force and Options.AllowDestructive are rejected: the declarative +// front door never runs a submitted form blind and refuses a destructive +// plan whole. A destructive or force-worthy change belongs on the +// imperative front door where the operator states it explicitly. // // Every planned statement runs back through [Run], so Run's note on the // pool's search_path applies to the pool passed here. @@ -198,6 +199,9 @@ func RunDesired(ctx context.Context, pool *pgxpool.Pool, req DesiredRequest, opt if opts.Force != "" { return DesiredResult{}, ErrForceNotSupported } + if opts.AllowDestructive != "" { + return DesiredResult{}, ErrAllowDestructiveNotSupported + } report, err := diffplan.Plan(ctx, pool, diffplan.Request{Schema: req.Schema, Desired: req.Desired}) if ref, ok := desiredShapeRefusal(err); ok { return DesiredResult{}.refused(ref, fmt.Sprintf("%v; nothing was planned or executed", err)), nil diff --git a/pkg/migrate/migrate.go b/pkg/migrate/migrate.go index 8a6f877a..d26a88ba 100644 --- a/pkg/migrate/migrate.go +++ b/pkg/migrate/migrate.go @@ -85,6 +85,18 @@ type Options struct { // one acknowledgement names it. 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 + // 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. + AllowDestructive string + // MaxTableSizeBytes is the threshold above which a blind bounded // attempt of the submitted form is refused, measured as the table's // full on-disk footprint: heap, indexes, and TOAST, all partitions. @@ -124,8 +136,9 @@ type Options struct { // the same budgets, size guard, and retry policy the CLI's flag defaults // wire. These are defaults to tune, not a recommendation — a large table // needs a more generous concurrent-build and validate bound, a hot table a -// tighter lock budget. Force, Logger, and Audit stay zero: overriding -// safety and receiving diagnostics are always deliberate choices. +// tighter lock budget. Force, AllowDestructive, Logger, and Audit stay +// zero: overriding safety and receiving diagnostics are always deliberate +// choices. func DefaultOptions() Options { return Options{ MaxTableSizeBytes: 1 << 30, @@ -169,6 +182,9 @@ 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") } @@ -207,6 +223,11 @@ func (o Options) retry() executor.RetryPolicy { // [verdict.OutcomeExecutedViaCopyAndSwap]; a failure on that path leaves a // 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. +// // 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 // statement text. Run's own catalog reads — the resolve and the @@ -256,6 +277,11 @@ func run(ctx context.Context, pool *pgxpool.Pool, st statement.Statement, opts O return verdict.Verdict{}, err } } + if opts.AllowDestructive != "" { + if err := checkAllowDestructiveAck(qualified(st), opts.AllowDestructive); err != nil { + return verdict.Verdict{}, err + } + } facts, err := LiveFacts(ctx, pool, st) if err != nil { @@ -278,9 +304,34 @@ func run(ctx context.Context, pool *pgxpool.Pool, st statement.Statement, opts O if err := checkRestartRoute(st, rs); err != nil { return verdict.Verdict{}, err } + } + // INV: RF-10 — a statement that discards live data or structure is + // refused before any route runs it, unless the acknowledgement names + // its table; every disposition below is reached only past this gate. + allowedDestructive := false + if destructive := destructiveOperations(rs); len(destructive) > 0 { + if opts.AllowDestructive == "" { + return destructiveChangeVerdict(st, destructive) + } + allowedDestructive = true + auditAllowDestructive(opts.audit(), st) + } + if opts.CopySwap.Restart != "" { auditRestart(opts.audit(), st) } + v, err := dispatch(ctx, pool, st, canonical, rs, opts, adopts, validates, logger) + if allowedDestructive && v.Outcome != "" { + v.AllowedDestructive = true + } + return v, err +} + +// dispatch runs a routed statement that is past the front door's +// acknowledgement gates down the path its disposition names. +func dispatch(ctx context.Context, pool *pgxpool.Pool, st statement.Statement, canonical string, + rs router.Statement, opts Options, adopts *schemadiff.IndexAdoption, validates *schemadiff.ConstraintValidation, + logger *slog.Logger) (verdict.Verdict, error) { switch rs.Disposition { case router.DispositionExecute: if rs.Backend == router.BackendCopyAndSwap { diff --git a/pkg/migrate/no_rewrite_integration_test.go b/pkg/migrate/no_rewrite_integration_test.go index 094f0bd3..184023f6 100644 --- a/pkg/migrate/no_rewrite_integration_test.go +++ b/pkg/migrate/no_rewrite_integration_test.go @@ -28,6 +28,10 @@ type noRewriteCase struct { // having executed nothing. minMajor int belowMinReason verdict.Reason + // destructive marks a statement that discards live data or structure, + // which runs only under the destructive acknowledgement; the row's + // claim is about the rewrite, so the case acknowledges the loss. + destructive bool } // noRewriteFixture is the table most cases start from: rows in it, so a @@ -100,13 +104,14 @@ var noRewriteCases = map[string][]noRewriteCase{ }, }, "drop-column": { - {name: "drop column", setup: noRewriteFixture, statement: "ALTER TABLE %[1]s.t DROP COLUMN note"}, + {name: "drop column", setup: noRewriteFixture, statement: "ALTER TABLE %[1]s.t DROP COLUMN note", destructive: true}, }, "drop-constraint": { { - name: "drop check", - setup: append(slices.Clone(noRewriteFixture), "ALTER TABLE %[1]s.t ADD CONSTRAINT t_qty_check CHECK (qty > 0)"), - statement: "ALTER TABLE %[1]s.t DROP CONSTRAINT t_qty_check", + name: "drop check", + setup: append(slices.Clone(noRewriteFixture), "ALTER TABLE %[1]s.t ADD CONSTRAINT t_qty_check CHECK (qty > 0)"), + statement: "ALTER TABLE %[1]s.t DROP CONSTRAINT t_qty_check", + destructive: true, }, }, "plain-tables-their-indexes": { @@ -115,7 +120,7 @@ var noRewriteCases = map[string][]noRewriteCase{ "set-default-drop-default-drop-not-null": { {name: "set default", setup: noRewriteFixture, statement: "ALTER TABLE %[1]s.t ALTER COLUMN qty SET DEFAULT 1"}, {name: "drop default", setup: noRewriteFixture, statement: "ALTER TABLE %[1]s.t ALTER COLUMN note DROP DEFAULT"}, - {name: "drop not null", setup: noRewriteFixture, statement: "ALTER TABLE %[1]s.t ALTER COLUMN email DROP NOT NULL"}, + {name: "drop not null", setup: noRewriteFixture, statement: "ALTER TABLE %[1]s.t ALTER COLUMN email DROP NOT NULL", destructive: true}, }, "add-check-add-foreign-key-imperative": { {name: "add check", setup: noRewriteFixture, statement: "ALTER TABLE %[1]s.t ADD CONSTRAINT t_qty_check CHECK (qty > 0)"}, @@ -217,7 +222,11 @@ func TestNativeRowsRunWithoutARewrite(t *testing.T) { before := heapFilenodes(t, pool, schema) sql := fmt.Sprintf(tc.statement, schema) - v, err := migrate.Run(t.Context(), pool, parseOne(t, sql), runOptions()) + opts := runOptions() + if tc.destructive { + opts.AllowDestructive = schema + ".t" + } + v, err := migrate.Run(t.Context(), pool, parseOne(t, sql), opts) require.NoError(t, err) switch { case major < tc.minMajor: diff --git a/pkg/migrate/refusal_registry.go b/pkg/migrate/refusal_registry.go index 877ddd27..eca4c2a4 100644 --- a/pkg/migrate/refusal_registry.go +++ b/pkg/migrate/refusal_registry.go @@ -62,8 +62,10 @@ func budgetExceededRefusal() verdict.Refusal { return verdict.Environmental(verdict.ReasonBudgetExceeded) } -// destructiveChangeRefusal: desired-state execution never infers permission -// to discard live structure; the deliberate path is the imperative statement. +// destructiveChangeRefusal: neither front door infers permission to discard +// live data or structure. Desired-state admission refuses a destructive plan +// whole; the imperative gate refuses a destructive statement unless the +// destructive acknowledgement names its table. func destructiveChangeRefusal() verdict.Refusal { return plan.DestructiveChangeRefusal() } diff --git a/pkg/migrate/replica_identity_integration_test.go b/pkg/migrate/replica_identity_integration_test.go index b006d5c4..376727ca 100644 --- a/pkg/migrate/replica_identity_integration_test.go +++ b/pkg/migrate/replica_identity_integration_test.go @@ -113,7 +113,7 @@ func TestRunRefusesDroppingTheReplicaIdentity(t *testing.T) { assert.False(t, indexExists(t, pool, schema, "accounts_note_key")) v, err = migrate.Run(t.Context(), pool, - parseOne(t, fmt.Sprintf("ALTER TABLE %s.accounts DROP COLUMN note", schema)), runOptions()) + parseOne(t, fmt.Sprintf("ALTER TABLE %s.accounts DROP COLUMN note", schema)), destructiveOptions(schema+".accounts")) require.NoError(t, err) assert.Equal(t, verdict.OutcomeExecuted, v.Outcome, "%s", v.Detail) assert.False(t, columnExists(t, pool, schema, "accounts", "note")) diff --git a/pkg/plan/plan.go b/pkg/plan/plan.go index f9b98587..89d3660a 100644 --- a/pkg/plan/plan.go +++ b/pkg/plan/plan.go @@ -37,10 +37,12 @@ import ( // the cascading-drop planner reason. Version 12 routes an app-breaking // rename to the rewrite-required disposition and added the // expand-contract-column and coordinated-table-rename guidance codes. -// Version 13 classifies the rename of an index that backs a constraint as -// app-breaking-rename and added the coordinated-constraint-rename guidance -// code. -const FormatVersion = 13 +// Version 14 added the alter-column-type-in-place guidance code, which a +// destructive-change refusal verdict carries; no report emits it, but the +// report embeds the whole guidance vocabulary. Version 13 classifies the +// rename of an index that backs a constraint as app-breaking-rename and +// added the coordinated-constraint-rename guidance code. +const FormatVersion = 14 // Source identifies which front door derived the plan. type Source string diff --git a/pkg/plan/plan_test.go b/pkg/plan/plan_test.go index 72d05250..9c661e0f 100644 --- a/pkg/plan/plan_test.go +++ b/pkg/plan/plan_test.go @@ -534,7 +534,7 @@ func TestReportJSONShape(t *testing.T) { raw, err := json.Marshal(r) require.NoError(t, err) assert.JSONEq(t, fmt.Sprintf(`{ - "format_version": 13, + "format_version": 14, "source": "diff", "schema": "public", "table": "t", @@ -592,7 +592,7 @@ func TestReportJSONOmitsUnsetOptionalFields(t *testing.T) { raw, err := json.Marshal(r) require.NoError(t, err) assert.JSONEq(t, `{ - "format_version": 13, + "format_version": 14, "source": "alter", "disposition": "execute", "fingerprint": "sha256:e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855", diff --git a/pkg/plan/refusal.go b/pkg/plan/refusal.go index 4721c25a..f1d24c55 100644 --- a/pkg/plan/refusal.go +++ b/pkg/plan/refusal.go @@ -90,8 +90,10 @@ func ForeignKeyChangeRefusal() verdict.Refusal { return verdict.CapabilityBoundary(verdict.ReasonDeclarativeForeignKey) } -// DestructiveChangeRefusal classifies desired-state admission of a plan that -// would discard live structure; the operator must choose an explicit statement. +// DestructiveChangeRefusal classifies a change that would discard live data +// or structure: desired-state admission of a destructive plan, and an +// imperative destructive statement without the destructive acknowledgement. +// Both front doors share it, so they report one contract. func DestructiveChangeRefusal() verdict.Refusal { return verdict.ByDesign(verdict.ReasonDestructiveChange) } diff --git a/pkg/suggest/readded_column.go b/pkg/suggest/readded_column.go new file mode 100644 index 00000000..fecc6480 --- /dev/null +++ b/pkg/suggest/readded_column.go @@ -0,0 +1,24 @@ +package suggest + +import "github.com/block/pg-sprite/pkg/statement" + +// ReAddedColumn reports the first column one statement both drops and adds +// back under the same name, in statement order: the drop-and-re-add shape +// that discards the column's values when the intent is usually a change of +// type. ok is false when no dropped column is added back. A destructive-change +// refusal carries [GuidanceAlterColumnTypeInPlace] for it, so the operator +// sees how to keep the values before acknowledging their loss. +func ReAddedColumn(ops []statement.Op) (column string, ok bool) { + dropped := map[string]bool{} + for _, op := range ops { + if op.Kind == statement.OpDropColumn { + dropped[op.Column] = true + } + } + for _, op := range ops { + if op.Kind == statement.OpAddColumn && dropped[op.Column] { + return op.Column, true + } + } + return "", false +} diff --git a/pkg/suggest/readded_column_test.go b/pkg/suggest/readded_column_test.go new file mode 100644 index 00000000..3d59c937 --- /dev/null +++ b/pkg/suggest/readded_column_test.go @@ -0,0 +1,49 @@ +package suggest_test + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/block/pg-sprite/pkg/statement" + "github.com/block/pg-sprite/pkg/suggest" +) + +func parseOps(t *testing.T, sql string) []statement.Op { + t.Helper() + ops, err := statement.ParseOps(sql) + require.NoError(t, err) + return ops +} + +// One ALTER that drops a column and adds it back under the same name is +// the drop-and-re-add shape, in either order. +func TestReAddedColumnFindsADropAndReAdd(t *testing.T) { + column, ok := suggest.ReAddedColumn(parseOps(t, ` + ALTER TABLE orders + DROP COLUMN qty, + ADD COLUMN qty bigint`)) + require.True(t, ok) + assert.Equal(t, "qty", column) + + column, ok = suggest.ReAddedColumn(parseOps(t, ` + ALTER TABLE orders + ADD COLUMN note text, + DROP COLUMN note`)) + require.True(t, ok, "PostgreSQL refuses this order, but the shape is the same and so is the advice") + assert.Equal(t, "note", column) +} + +// A drop with nothing added back, or with a different column added, is +// not the shape. +func TestReAddedColumnIgnoresOtherDrops(t *testing.T) { + _, ok := suggest.ReAddedColumn(parseOps(t, `ALTER TABLE orders DROP COLUMN qty`)) + assert.False(t, ok) + + _, ok = suggest.ReAddedColumn(parseOps(t, ` + ALTER TABLE orders + DROP COLUMN qty, + ADD COLUMN quantity bigint`)) + assert.False(t, ok) +} diff --git a/pkg/suggest/suggest.go b/pkg/suggest/suggest.go index ab6b3898..fc95ea28 100644 --- a/pkg/suggest/suggest.go +++ b/pkg/suggest/suggest.go @@ -19,11 +19,11 @@ import ( // FormatVersion identifies the report contract // (docs/suggest-report.md). A consumer must reject a report whose version // it does not understand instead of guessing at the field semantics. -// Version 4 added coordinated-constraint-rename to the Guidance -// vocabulary; version 3 added expand-contract-column and -// coordinated-table-rename; version 2 added name-constraint-then-validate -// and unique-index-then-constraint. -const FormatVersion = 4 +// Version 5 added alter-column-type-in-place to the Guidance vocabulary; +// version 4 added coordinated-constraint-rename; version 3 added +// expand-contract-column and coordinated-table-rename; version 2 added +// name-constraint-then-validate and unique-index-then-constraint. +const FormatVersion = 5 // Caveat is a typed condition attached to a recommendation; automation // branches on it, never on prose. The caveats are independent — no caveat @@ -143,6 +143,15 @@ const ( // constraint break the instant it commits. Coordinate the rename // with the application deploy that adopts the new constraint name. GuidanceCoordinatedConstraintRename Guidance = "coordinated-constraint-rename" + // GuidanceAlterColumnTypeInPlace: one ALTER TABLE that drops a column + // and adds a column of the same name. The drop discards every value + // the column holds and the re-added column starts empty, which is + // rarely what a change of type means. Change the column in place with + // ALTER COLUMN ... TYPE ... USING ..., which converts the values + // instead of discarding them. Carried by a destructive-change refusal, + // never by a suggestion: the advisory surface covers blocking idioms, + // and this statement is refused for its data loss. + GuidanceAlterColumnTypeInPlace Guidance = "alter-column-type-in-place" ) // Guidances returns the closed set of Guidance values. It is part of the @@ -160,6 +169,7 @@ func Guidances() []Guidance { GuidanceExpandContractColumn, GuidanceCoordinatedTableRename, GuidanceCoordinatedConstraintRename, + GuidanceAlterColumnTypeInPlace, } } @@ -192,6 +202,9 @@ func ManualPath(g Guidance) string { case GuidanceCoordinatedConstraintRename: return "the index backs a constraint, and renaming it renames the constraint: coordinate the rename with the " + "application deploy that stops naming the old constraint (ON CONFLICT ON CONSTRAINT, error handling)" + case GuidanceAlterColumnTypeInPlace: + return "change the column in place with ALTER TABLE ... ALTER COLUMN ... TYPE ... USING ..., which converts its values; " + + "dropping and re-adding it discards every value it holds" } return string(g) } diff --git a/pkg/suggest/suggest_test.go b/pkg/suggest/suggest_test.go index 777a80d9..fc4f6570 100644 --- a/pkg/suggest/suggest_test.go +++ b/pkg/suggest/suggest_test.go @@ -383,7 +383,7 @@ func TestReportJSONShape(t *testing.T) { recommended, err := json.Marshal(report.Suggestions[0].Recommended) require.NoError(t, err) assert.JSONEq(t, `{ - "format_version": 4, + "format_version": 5, "suggestions": [ { "statement": 1, @@ -414,7 +414,7 @@ func TestReportJSONGuidanceShape(t *testing.T) { operation, err := json.Marshal(report.Suggestions[0].Operation) require.NoError(t, err) assert.JSONEq(t, `{ - "format_version": 4, + "format_version": 5, "suggestions": [ { "statement": 1, @@ -435,7 +435,7 @@ func TestReportJSONCleanSuggestionsAreEmptyArray(t *testing.T) { raw, err := json.Marshal(report) require.NoError(t, err) assert.JSONEq(t, `{ - "format_version": 4, + "format_version": 5, "suggestions": [] }`, string(raw)) } diff --git a/pkg/verdict/verdict.go b/pkg/verdict/verdict.go index c6574c13..eaaa3142 100644 --- a/pkg/verdict/verdict.go +++ b/pkg/verdict/verdict.go @@ -105,11 +105,12 @@ const ( // that has no carrying path for the operation in this build (a // copy-and-swap rewrite the shadow-table engine does not carry yet). ReasonBackendUnavailable Reason = "backend-unavailable" - // ReasonDestructiveChange: the desired-state plan discards live - // structure — a dropped column, constraint, index, or NOT NULL — and - // desired-state execution runs no destructive statement without an - // explicit path for it. The imperative front door remains the way to - // run a reviewed destructive statement deliberately. + // ReasonDestructiveChange: the change discards live data or structure — + // a dropped column, constraint, index, or NOT NULL. The imperative + // front door runs such a statement only when the destructive + // acknowledgement names its table; desired-state execution refuses a + // destructive plan whole and has no acknowledgement for it, so the + // imperative statement is the way to run a reviewed destructive change. ReasonDestructiveChange Reason = "destructive-change" // ReasonPlanFingerprintMismatch: the plan recomputed at execution time // does not carry the fingerprint the caller pinned, so the plan a @@ -540,8 +541,10 @@ type Verdict struct { // Guidance is the typed manual path for a rewrite-required refusal, // drawn from the suggest contract's Guidance vocabulary // (docs/suggest-report.md), the same value the plan report's statement - // carries. Detail states the steps for a human; Guidance is the key an - // automated caller branches on. Empty for every other verdict. + // carries, or for a destructive-change refusal whose statement drops a + // column and adds it back under the same name. Detail states the steps + // for a human; Guidance is the key an automated caller branches on. + // Empty for every other verdict. Guidance suggest.Guidance `json:"guidance,omitempty"` // DependentViews names the views and materialized views that read the // column a refused statement retypes or drops, each schema-qualified @@ -563,6 +566,12 @@ type Verdict struct { // strategy refusal. It is the machine-readable audit record of the // override. Forced bool `json:"forced,omitempty"` + // AllowedDestructive reports that --allow-destructive acknowledged the + // data the statement discards: a statement the destructive-change gate + // would refuse went on through the engine's other gates instead. It is + // the machine-readable audit record of the acknowledgement, set on + // every verdict the acknowledged statement ends in. + AllowedDestructive bool `json:"allowed_destructive,omitempty"` // BlockingPassthrough marks execution of an operator-accepted refusal. BlockingPassthrough bool `json:"blocking_passthrough,omitempty"` // LockTimeout is the explicit lock-acquisition budget. @@ -645,6 +654,9 @@ func (v Verdict) String() string { if v.Forced { b.WriteString("\n forced: the submitted form ran as-is (force acknowledged)") } + if v.AllowedDestructive { + b.WriteString("\n allowed: the statement's data loss was acknowledged (allow-destructive)") + } if v.FailedStep > 0 { fmt.Fprintf(&b, "\n failed at: step %d: %s", v.FailedStep, v.FailedStepSQL) } diff --git a/replay/buzz/README.md b/replay/buzz/README.md index f8202791..e98f8699 100644 --- a/replay/buzz/README.md +++ b/replay/buzz/README.md @@ -21,11 +21,13 @@ authoritative. See the [refusal class contract](../../docs/refusal-classes.md). ## Boundary facts the curation surfaced -Curating the manifest against real verdicts (not predictions) established two +Curating the manifest against real verdicts (not predictions) established three behaviors worth knowing when reading the results: - `CREATE INDEX IF NOT EXISTS` is refused by design — a name-only no-op cannot prove the existing index is valid or even the requested one. -- Buzz's unnamed `ADD FOREIGN KEY` is refused (`not-native-safe-rewrite-required`) - while the same constraint written with `CONSTRAINT ` executes via the safer - `NOT VALID` + `VALIDATE` sequence — the rewrite needs a name to reference. +- Buzz's unnamed `ADD FOREIGN KEY` executes via the safer `NOT VALID` + `VALIDATE` + sequence, under the name the server would have chosen. +- Every `DROP COLUMN`, `DROP CONSTRAINT`, and `DROP NOT NULL` is refused by design + (`destructive-change`): the replay never passes `--allow-destructive`, so it pins + the refusal and advances the history through psql. diff --git a/replay/buzz/assessment.tsv b/replay/buzz/assessment.tsv index e9526dfa..5b8aa640 100644 --- a/replay/buzz/assessment.tsv +++ b/replay/buzz/assessment.tsv @@ -27,7 +27,10 @@ 0002 29-29 execute 0003 12-12 execute 0004 21-21 refuse:unsupported-partitioned-parent:capability-boundary -0005 25-25 execute +# A drop discards data, so migrate refuses it until --allow-destructive names +# the table (RF-10); the replay never acknowledges, so every drop pins the +# by-design destructive-change refusal and advances through psql. +0005 25-25 refuse:destructive-change:by-design 0005 26-30 refuse:backend-unavailable:capability-boundary 0005 33-33 refuse:unsupported-partitioned-parent:capability-boundary 0006 15-50 refuse:unsupported-statement:no-online-safety-problem:declarative-front-door @@ -112,10 +115,12 @@ 0029 253-261 refuse:unsupported-statement:no-online-safety-problem:declarative-front-door 0029 263-575 psql 0030 5-5 psql -0030 7-22 refuse:not-native-safe-rewrite-required:capability-boundary +# The multi-operation ALTER drops two constraints; the destructive gate refuses +# it before the rewrite-required routing is reached. +0030 7-22 refuse:destructive-change:by-design 0030 27-29 execute -0030 31-31 execute -0030 32-32 execute +0030 31-31 refuse:destructive-change:by-design +0030 32-32 refuse:destructive-change:by-design 0030 33-34 execute 0030 38-48 psql 0031 3-3 psql