From e8ac4dc384d8bb754fcaf5e5092b4af587b5089e Mon Sep 17 00:00:00 2001 From: Armand Parajon Date: Sat, 17 Oct 2026 15:44:32 -0400 Subject: [PATCH 1/2] feat(planner): refuse a drop with CASCADE CASCADE on DROP COLUMN, DROP CONSTRAINT, or DROP INDEX also drops whatever depends on the target: a view that reads the column, or another table's foreign key on a dropped primary key. Those objects are not in the statement, so the plan cannot show them and the one-table lock does not cover them. The ALTER TABLE forms were classified as plain metadata-only drops and ran natively. The planner now refuses every such drop with the new cascading-drop reason, read from the statement alone, so lint reports it too. The migrate verdict tells the operator to drop each dependent object explicitly and resubmit without CASCADE, which PostgreSQL then refuses while anything still depends on the target. DROP INDEX ... CASCADE no longer gets a DROP INDEX CONCURRENTLY ... CASCADE suggestion, which the server rejects. The plan report moves to format_version 11 and the lint report to 5. Co-Authored-By: Claude Opus 5.5 --- demo/tour.sh | 8 +- docs/capabilities.md | 6 +- docs/cli-output-examples.md | 16 ++-- docs/limitations.md | 3 +- docs/lint-report.md | 12 ++- docs/optimistic-attempt.md | 1 + docs/plan-report.md | 11 ++- docs/postgres-online-ddl-reference.md | 26 ++++- internal/cli/dryrun_text.go | 2 + pkg/capabilities/capabilities.yaml | 6 +- pkg/lint/lint.go | 5 +- pkg/lint/lint_test.go | 4 +- .../cascading_drop_integration_test.go | 96 +++++++++++++++++++ .../dependent_view_integration_test.go | 12 --- pkg/migrate/verdicts.go | 5 + pkg/plan/plan.go | 5 +- pkg/plan/plan_test.go | 8 +- pkg/planner/planner.go | 21 ++-- pkg/planner/planner_test.go | 69 ++++++++++--- pkg/statement/ops.go | 15 ++- pkg/statement/ops_test.go | 10 ++ 21 files changed, 264 insertions(+), 77 deletions(-) create mode 100644 pkg/migrate/cascading_drop_integration_test.go diff --git a/demo/tour.sh b/demo/tour.sh index 6000c86..f922b0f 100755 --- a/demo/tour.sh +++ b/demo/tour.sh @@ -72,7 +72,7 @@ dry_run() { else assert_eq "dry-run refusal exit of [$sql]" 2 "$status" fi - assert_eq "plan format_version of [$sql]" 10 "$(jq -r '.format_version' <<<"$out")" + assert_eq "plan format_version of [$sql]" 11 "$(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")" @@ -180,7 +180,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]" 10 "$(jq -r '.format_version' <<<"$out")" + assert_eq "diff format_version of [$desired]" 11 "$(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"*) ;; @@ -208,7 +208,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]" 10 "$(jq -r '.format_version' <<<"$out")" + assert_eq "diff format_version of [$desired]" 11 "$(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")" @@ -243,7 +243,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]" 10 "$(jq -r '.format_version' <<<"$out")" + assert_eq "diff format_version of [$desired]" 11 "$(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 diff --git a/docs/capabilities.md b/docs/capabilities.md index 811cf4d..c687869 100644 --- a/docs/capabilities.md +++ b/docs/capabilities.md @@ -209,7 +209,7 @@ or the only online form is a direct statement the operator runs). | `ADD COLUMN ... GENERATED ... STORED` | 🟡 | copy-and-swap | Yes | Table rewrite; copy-and-swap route. The copy engine must **recompute, never copy,** generated columns 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, unless the statement says `CASCADE` | +| `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 | | `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; copy-and-swap route, refused today. A `USING` expression will be admitted only when the copy can reproduce it: immutable, naming only the retyped column, not the primary key (see limitations) | | `SET DEFAULT` / `DROP DEFAULT` / `DROP NOT NULL` | ✅ | native, as-is | Yes | Metadata-only | @@ -228,7 +228,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 | +| `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 | ### Indexes @@ -237,7 +237,7 @@ or the only online form is a direct statement the operator runs). | Operation | Status | Engine path | Online-safety problem? | Behavior and why | | --- | --- | --- | --- | --- | | `CREATE [UNIQUE] INDEX` on a plain table — including partial, expression, covering (`INCLUDE`), GIN/GiST/BRIN | ✅ | native, safer sequence | Yes | Executed as (or rewritten to) `CREATE INDEX CONCURRENTLY`, with validity verification, typed invalid-index outcomes, and a proven recovery for abandoned leftovers (`RebuildAbandonedIndex`, or `DropAbandonedIndex` to remove the leftover without rebuilding; both library-only; [runbook](invalid-index-recovery.md)) | -| `DROP INDEX` | ❌ | — | Yes — the online form is a direct `CONCURRENTLY` statement that `migrate` does not drive | `migrate` never runs this statement online-safely: the plain form is refused and names `DROP INDEX CONCURRENTLY`, and the concurrent form is refused as already safe — run it directly against the database. A single-index plain `DROP INDEX` without `CASCADE` can be run as-is under bounded budgets with `--accept-blocking SCHEMA.TABLE` (exit 3, never 0); `--accept-blocking` checks the replica identity under the table's lock and refuses the drop as `unsupported-statement`, naming the identity index. `diff` reports an index to remove as a **destructive** `DROP INDEX` and hints to run it through `migrate --alter`, which ends in this `index-statement` refusal — run `DROP INDEX CONCURRENTLY` directly instead. `diff` refuses dropping the table's replica identity index, or one a partition's is attached under, with `replica-identity` | +| `DROP INDEX` | ❌ | — | Yes — the online form is a direct `CONCURRENTLY` statement that `migrate` does not drive | `migrate` never runs this statement online-safely: the plain form is refused and names `DROP INDEX CONCURRENTLY`, and the concurrent form is refused as already safe — run it directly against the database. A single-index plain `DROP INDEX` without `CASCADE` can be run as-is under bounded budgets with `--accept-blocking SCHEMA.TABLE` (exit 3, never 0); `--accept-blocking` checks the replica identity under the table's lock and refuses the drop as `unsupported-statement`, naming the identity index. `diff` reports an index to remove as a **destructive** `DROP INDEX` and hints to run it through `migrate --alter`, which ends in this `index-statement` refusal — run `DROP INDEX CONCURRENTLY` directly instead. `diff` refuses dropping the table's replica identity index, or one a partition's is attached under, with `replica-identity`. `plan` and `lint` refuse `DROP INDEX … CASCADE` with `cascading-drop`, since `DROP INDEX CONCURRENTLY` does not accept `CASCADE` | | `REINDEX` | ❌ | — | Yes — the online form is a direct `CONCURRENTLY` statement that `migrate` does not drive | `migrate` never runs this statement online-safely: the plain form is refused and names `REINDEX ... CONCURRENTLY`, and the concurrent form is refused as already safe — run it directly against the database. A plain `REINDEX INDEX` or `REINDEX TABLE` can be run as-is under bounded budgets with `--accept-blocking SCHEMA.TABLE` (exit 3, never 0). `REINDEX SCHEMA`, `DATABASE`, and `SYSTEM` stay refused. A desired schema cannot express a reindex, so `diff` never produces one | | Index build on a **partitioned parent** | 🟡 | native, planned flow | Yes | PostgreSQL has no parent-level `CONCURRENTLY`; the blocking form is refused by policy (`--force` does not bypass it, and `--accept-blocking` does not reach it yet). The partition-aware flow — `CREATE INDEX ON ONLY` → per-partition CIC → `ATTACH PARTITION`, with crash-resume per leaf — is planned | | `ADD CONSTRAINT ... USING INDEX` on a partitioned parent | ❌ | — | Yes — unsolvable today | PostgreSQL does not support adopting an index on a partitioned parent in any supported version; refused before execution | diff --git a/docs/cli-output-examples.md b/docs/cli-output-examples.md index 7c32846..95c1d47 100644 --- a/docs/cli-output-examples.md +++ b/docs/cli-output-examples.md @@ -147,7 +147,7 @@ itself. ```console $ pg-sprite migrate --alter 'ALTER TABLE users ADD COLUMN note text' --dry-run --json { - "format_version": 10, + "format_version": 11, "source": "alter", "schema": "public", "table": "users", @@ -188,7 +188,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": 10, + "format_version": 11, "source": "alter", "schema": "public", "table": "users", @@ -290,7 +290,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": 10, + "format_version": 11, "source": "alter", "schema": "public", "table": "users", @@ -327,7 +327,7 @@ implemented yet. ```console $ pg-sprite migrate --alter 'ALTER TABLE users ALTER COLUMN id TYPE text' --dry-run --json { - "format_version": 10, + "format_version": 11, "source": "alter", "schema": "public", "table": "users", @@ -364,7 +364,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": 10, + "format_version": 11, "source": "alter", "schema": "public", "table": "events", @@ -404,7 +404,7 @@ the reviewer or orchestrator to gate on; `migrate` itself does not block it. ```console $ pg-sprite migrate --alter 'ALTER TABLE users DROP COLUMN email' --dry-run --json { - "format_version": 10, + "format_version": 11, "source": "alter", "schema": "public", "table": "users", @@ -453,7 +453,7 @@ ALTER TABLE users ADD CONSTRAINT users_email_key UNIQUE (email); ```console $ pg-sprite lint /tmp/changes.sql --json { - "format_version": 4, + "format_version": 5, "postgres_versions": "14-18", "findings": [ { @@ -531,7 +531,7 @@ CREATE TABLE users ( ```console $ pg-sprite diff --desired /tmp/users.sql --json { - "format_version": 10, + "format_version": 11, "source": "diff", "schema": "public", "table": "users", diff --git a/docs/limitations.md b/docs/limitations.md index 0ba5894..227f450 100644 --- a/docs/limitations.md +++ b/docs/limitations.md @@ -18,7 +18,8 @@ not escape hatches: | `ATTACH PARTITION` | Without `--force`, refused with `not-native-safe-rewrite-required`, nothing executed: the engine does not yet construct the validated `CHECK` that lets the attach skip its scan. Pre-prove the bound yourself, then run the attach directly. `--force SCHEMA.TABLE` runs the blocking `ATTACH PARTITION` as submitted under bounded budgets, taking the blocking lock. | | `DETACH PARTITION` and `DETACH PARTITION ... CONCURRENTLY` | Without `--force`, refused with `unsupported-statement` before anything executes: the plain form is rewritten to `CONCURRENTLY`, and the executor does not drive that step because a cancelled concurrent detach leaves a detach-pending state it does not yet own recovering. Run `DETACH PARTITION ... CONCURRENTLY` directly. `--force SCHEMA.TABLE` runs a plain `DETACH PARTITION` as submitted under bounded budgets, taking the blocking lock; `DETACH ... CONCURRENTLY` stays refused under `--force`. `migrate --dry-run` reports the rewrite as a step to run and exits 0 while the real run refuses (exit 2), so the dry-run exit code is not a submit gate for a detach. | | Dropping what a replica identity index is built on | `REPLICA IDENTITY USING INDEX` outlives its index, so dropping the index, the primary key or unique constraint that owns it, or a column it covers leaves the table with no usable identity, and every `UPDATE` and `DELETE` on it fails while a publication publishes them. The planner refuses such a drop with `replica-identity`, on the table and on what the drop reaches below it: a `DROP COLUMN` reaches partitions and inheritance children unless the statement says `ONLY`, and a `DROP CONSTRAINT` reaches partitions, whose indexes are attached under the parent's. The executor reads the identity again under the lock the drop takes, so an identity that moved onto the dropped object after planning still refuses the run before anything commits. `migrate --accept-blocking` makes the same check before a `DROP INDEX`. Point `REPLICA IDENTITY` at another unique index first, or `DEFAULT` if a usable primary key remains after the drop, then re-plan. | -| Retyping or dropping a column a view reads | PostgreSQL refuses `ALTER COLUMN TYPE` and `DROP COLUMN` on a column a view or materialized view reads, even for a type change it would relabel in place, and a change without `ONLY` reaches the column on every partition and inheritance child too. The planner reads those views from the catalog, the children's included, and refuses the change with `dependent-view`, naming each view, instead of letting the statement fail at the server. `DROP COLUMN … CASCADE` is not refused: the server drops the views with the column, and the drop is flagged destructive. pg-sprite does not drop and recreate the views for you. Drop or redefine them first, then re-plan. | +| Retyping or dropping a column a view reads | PostgreSQL refuses `ALTER COLUMN TYPE` and `DROP COLUMN` on a column a view or materialized view reads, even for a type change it would relabel in place, and a change without `ONLY` reaches the column on every partition and inheritance child too. The planner reads those views from the catalog, the children's included, and refuses the change with `dependent-view`, naming each view, instead of letting the statement fail at the server. `DROP COLUMN … CASCADE`, which would drop the views with the column, is refused with `cascading-drop`. pg-sprite does not drop and recreate the views for you. Drop or redefine them first, then re-plan. | +| Dropping with `CASCADE` | `CASCADE` on `DROP COLUMN`, `DROP CONSTRAINT`, or `DROP INDEX` also drops whatever depends on the target: a view that reads the column, or another table's foreign key on the constraint. Those objects are not in the statement, so the plan cannot show them and the table's lock does not cover them. The planner refuses every such drop with `cascading-drop`. Drop each dependent object as its own change, then resubmit without `CASCADE`; PostgreSQL then refuses the drop while anything still depends on the target. | | `ADD COLUMN` on a hot standby, or by a role without `TEMPORARY` | Whether adding a column rewrites the table is the server's answer, read by adding the column to an empty temporary table. A server that accepts no writes, or a role whose `TEMPORARY` on the database was revoked, cannot create that table, so the plan fails with `ErrAddColumnProbeUnavailable` before anything runs. Plan against the primary, or grant `TEMPORARY` on the database to the engine's role. | | Copy-and-swap | The copy-and-swap backend is not yet available. Statements that require it route to `refuse`; pg-sprite never falls through to a blocking rewrite. | | `ALTER COLUMN TYPE ... USING ` the copy cannot reproduce | The copy-and-swap route is still closed (above); when it opens, 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. | diff --git a/docs/lint-report.md b/docs/lint-report.md index ef4a7a9..25ad12f 100644 --- a/docs/lint-report.md +++ b/docs/lint-report.md @@ -15,11 +15,13 @@ and the closed vocabularies below (codes, severities, and the embedded planner r 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 the `dependent-view` reason to the embedded -Reasons vocabulary. Version 3 added the `concurrent-in-transaction` code. Version 2 added the -`replica-identity` reason. The linter itself never reports either reason, because each is judged -from live facts (the table's replica identity, the views that read a column) the linter does not -have. +The current version is **5**: version 5 added the `cascading-drop` reason to the embedded +Reasons vocabulary. Version 4 added the `dependent-view` reason. Version 3 added the +`concurrent-in-transaction` code. Version 2 added the `replica-identity` reason. The linter +never reports `replica-identity` or `dependent-view`, because each is judged from live facts +(the table's replica identity, the views that read a column) the linter does not have. It does +report `cascading-drop`, which it reads from the statement, on an `unsupported-operation` +finding. ### Relationship to the plan report diff --git a/docs/optimistic-attempt.md b/docs/optimistic-attempt.md index de0bc90..af208b0 100644 --- a/docs/optimistic-attempt.md +++ b/docs/optimistic-attempt.md @@ -327,6 +327,7 @@ against `sizeGuardApplies`): | `relocation` | n/a — same | | `replica-identity` | n/a — refused at plan time | | `dependent-view` | n/a — refused at plan time | +| `cascading-drop` | n/a — refused at plan time | | `unsupported-operation` | n/a — refused at plan time | To check your own DDL, ask the planner — the classification is in the dry-run diff --git a/docs/plan-report.md b/docs/plan-report.md index cc1f339..abb9ee7 100644 --- a/docs/plan-report.md +++ b/docs/plan-report.md @@ -17,7 +17,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 **10**: 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 **11**: 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); @@ -125,7 +125,8 @@ consumer rendering either into a shared surface must clamp and escape them. | `relocation` | SET TABLESPACE moves the heap — a rewrite-scale copy. | | `partition-parent-lock` | Creating a partition (`CREATE TABLE … PARTITION OF`): a brief `ACCESS EXCLUSIVE` on the parent, no scan. | | `replica-identity` | The operation drops what a `REPLICA IDENTITY USING INDEX` of the table, or of a partition or inheritance child below it, is built on: the index, the constraint that owns it, or one of its columns, `INCLUDE` columns too. The setting outlives the index, so every `UPDATE` and `DELETE` on the table fails while a publication publishes them. Refused; move the identity first (`ALTER TABLE … REPLICA IDENTITY`), then re-plan. | -| `dependent-view` | The operation retypes or drops a column that a view or materialized view reads, on the table or, without `ONLY`, on a partition or inheritance child below it. PostgreSQL refuses this while the view exists, even for a type change it would relabel. Refused; `dependent_views` names the views. Drop or redefine them first, then re-plan. `DROP COLUMN … CASCADE`, which drops the views with the column, is not refused. | +| `dependent-view` | The operation retypes or drops a column that a view or materialized view reads, on the table or, without `ONLY`, on a partition or inheritance child below it. PostgreSQL refuses this while the view exists, even for a type change it would relabel. Refused; `dependent_views` names the views. Drop or redefine them first, then re-plan. `DROP COLUMN … CASCADE`, which would drop the views with the column, is refused as `cascading-drop`. | +| `cascading-drop` | A `DROP COLUMN`, `DROP CONSTRAINT`, or `DROP INDEX` with `CASCADE`, which also drops whatever depends on the target, such as a view that reads the column or another table's foreign key on the constraint. Those objects are outside the statement, so the plan cannot show them. Refused; drop each dependent object as its own change, then resubmit without `CASCADE`. | | `unsupported-operation` | The planner does not recognize the operation or knows no safe path for it. | ### Target-dependent refusal reasons (`reason`, `statements[].reason`) @@ -303,7 +304,7 @@ in `pkg/plan` — if the code drifts from this page, CI fails. ```json { - "format_version": 10, + "format_version": 11, "source": "alter", "schema": "app", "table": "orders", @@ -346,7 +347,7 @@ A desired state that drops an index and adds a column with a constant default: ```json { - "format_version": 10, + "format_version": 11, "source": "diff", "schema": "app", "table": "orders", @@ -413,7 +414,7 @@ A desired state for a table that does not exist yet, whose `CREATE TABLE` carrie ```json { - "format_version": 10, + "format_version": 11, "source": "diff", "schema": "app", "table": "gadgets", diff --git a/docs/postgres-online-ddl-reference.md b/docs/postgres-online-ddl-reference.md index 9a0e0f8..effe49b 100644 --- a/docs/postgres-online-ddl-reference.md +++ b/docs/postgres-online-ddl-reference.md @@ -55,7 +55,8 @@ the lock queue below applies to every query on the parent) and `app-breaking-ren catalog change is instant; the running application code that still uses the old name is what breaks). `replica-identity` is a refusal: the operation would drop what the table's replica identity index keys on. `dependent-view` is a refusal too: the operation retypes or -drops a column a view reads, which PostgreSQL refuses. `unsupported-operation` means the planner does not recognize the operation or +drops a column a view reads, which PostgreSQL refuses. `cascading-drop` is a refusal as well: +the drop's `CASCADE` would also drop objects outside the statement. `unsupported-operation` means the planner does not recognize the operation or knows no safe path for it. The exclusion-constraint carve-out above is a known full-scan cost with no online path, not an operation with no cost bucket. @@ -516,13 +517,30 @@ would otherwise relabel without a rewrite, so the statement could only fail. A change without `ONLY` reaches the column on every partition and inheritance child too, so a view on one of them counts. The plan names each view in `decisions[].dependent_views`. Drop or redefine the views first, then re-plan. A -rename is not refused, because a view follows the column, and neither is -`DROP COLUMN … CASCADE`, which the server runs by dropping the views with the -column; that drop is flagged destructive. +rename is not refused, because a view follows the column. `DROP COLUMN … CASCADE`, +which the server runs by dropping the views with the column, is refused as +`cascading-drop` instead. The planner reads the views from live facts, so the linter, which has none, never reports it, and the server's own refusal remains the backstop for a view created after planning. +### `cascading-drop` + +| Verdict | Lock | Scan / rewrite | Dry-run exit | +|---|---|---|---| +| refused — never executed | — | — | 2 | + +The operation is a `DROP COLUMN`, `DROP CONSTRAINT`, or `DROP INDEX` with `CASCADE`. +PostgreSQL then also drops whatever depends on the target: a view or materialized +view that reads the column, or another table's foreign key on a dropped primary key +or unique constraint. Those objects are not named in the statement, so the plan +cannot show them, and dropping a foreign key locks the table that holds it, which the +one-table lock does not cover. `DROP INDEX CONCURRENTLY` does not accept `CASCADE` +either, so there is no online form to suggest. Drop each dependent object as its own +change, then resubmit without `CASCADE`: PostgreSQL then refuses the drop while +anything still depends on the target. The refusal is read from the statement alone, +so the linter reports it too. + ### `unsupported-operation` | Verdict | Lock | Scan / rewrite | Dry-run exit | diff --git a/internal/cli/dryrun_text.go b/internal/cli/dryrun_text.go index a6669a2..6f585ef 100644 --- a/internal/cli/dryrun_text.go +++ b/internal/cli/dryrun_text.go @@ -295,6 +295,8 @@ func impactText(r planner.Reason) string { return "drops what the table's replica identity keys on; UPDATE and DELETE on the table fail while a publication publishes them" case planner.ReasonDependentView: return "changes a column a view reads; PostgreSQL refuses this until the view is dropped or redefined" + case planner.ReasonCascadingDrop: + return "CASCADE also drops the views and other tables' foreign keys that depend on it; drop those explicitly and resubmit without CASCADE" case planner.ReasonUnsupportedOperation: return "pg-sprite does not recognize this operation and will not run it" } diff --git a/pkg/capabilities/capabilities.yaml b/pkg/capabilities/capabilities.yaml index 69f99d8..c95023d 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, unless the statement says `CASCADE`" + 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" - 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)" @@ -217,7 +217,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" + 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" - 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" @@ -241,7 +241,7 @@ rows: migrate: refused diff: supported refusal_reason: index-statement - reason_notes: "`migrate` never runs this statement online-safely: the plain form is refused and names `DROP INDEX CONCURRENTLY`, and the concurrent form is refused as already safe — run it directly against the database. A single-index plain `DROP INDEX` without `CASCADE` can be run as-is under bounded budgets with `--accept-blocking SCHEMA.TABLE` (exit 3, never 0); `--accept-blocking` checks the replica identity under the table's lock and refuses the drop as `unsupported-statement`, naming the identity index. `diff` reports an index to remove as a **destructive** `DROP INDEX` and hints to run it through `migrate --alter`, which ends in this `index-statement` refusal — run `DROP INDEX CONCURRENTLY` directly instead. `diff` refuses dropping the table's replica identity index, or one a partition's is attached under, with `replica-identity`" + reason_notes: "`migrate` never runs this statement online-safely: the plain form is refused and names `DROP INDEX CONCURRENTLY`, and the concurrent form is refused as already safe — run it directly against the database. A single-index plain `DROP INDEX` without `CASCADE` can be run as-is under bounded budgets with `--accept-blocking SCHEMA.TABLE` (exit 3, never 0); `--accept-blocking` checks the replica identity under the table's lock and refuses the drop as `unsupported-statement`, naming the identity index. `diff` reports an index to remove as a **destructive** `DROP INDEX` and hints to run it through `migrate --alter`, which ends in this `index-statement` refusal — run `DROP INDEX CONCURRENTLY` directly instead. `diff` refuses dropping the table's replica identity index, or one a partition's is attached under, with `replica-identity`. `plan` and `lint` refuse `DROP INDEX … CASCADE` with `cascading-drop`, since `DROP INDEX CONCURRENTLY` does not accept `CASCADE`" - id: reindex area: "indexes" operation: "`REINDEX`" diff --git a/pkg/lint/lint.go b/pkg/lint/lint.go index 6bdb6a6..984bdfe 100644 --- a/pkg/lint/lint.go +++ b/pkg/lint/lint.go @@ -19,8 +19,9 @@ import ( // field semantics. Version 2 added the replica-identity planner reason to // the embedded Reasons vocabulary; version 3 added the // concurrent-in-transaction code; version 4 added the dependent-view -// planner reason to the embedded Reasons vocabulary. -const FormatVersion = 4 +// planner reason to the embedded Reasons vocabulary; version 5 added the +// cascading-drop planner reason to it. +const FormatVersion = 5 // Severity ranks a finding by what the engine would do with it. type Severity string diff --git a/pkg/lint/lint_test.go b/pkg/lint/lint_test.go index f9736aa..8313400 100644 --- a/pkg/lint/lint_test.go +++ b/pkg/lint/lint_test.go @@ -196,7 +196,7 @@ func TestReportJSONShape(t *testing.T) { suggestion, err := json.Marshal(report.Findings[0].Suggestion) require.NoError(t, err) assert.JSONEq(t, `{ - "format_version": 4, + "format_version": 5, "postgres_versions": "14-18", "findings": [ { @@ -223,7 +223,7 @@ func TestReportJSONCleanFindingsAreEmptyArray(t *testing.T) { raw, err := json.Marshal(report) require.NoError(t, err) assert.JSONEq(t, `{ - "format_version": 4, + "format_version": 5, "postgres_versions": "14-18", "findings": [], "errors": 0, diff --git a/pkg/migrate/cascading_drop_integration_test.go b/pkg/migrate/cascading_drop_integration_test.go new file mode 100644 index 0000000..1f89651 --- /dev/null +++ b/pkg/migrate/cascading_drop_integration_test.go @@ -0,0 +1,96 @@ +package migrate_test + +import ( + "fmt" + "testing" + + "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/verdict" +) + +// A drop with CASCADE also drops whatever depends on its target: here a view +// that reads the column, and another table's foreign key on the primary +// key. Neither is named in the statement, so the run is refused at plan +// time and tells the operator to drop the dependents explicitly. Nothing +// reaches the server: the column, the constraint, the view, and the other +// table's foreign key are all still there. The same drops without CASCADE, +// on a target nothing depends on, still run. +func TestRunRefusesACascadingDrop(t *testing.T) { + url := testutil.StartPostgres(t) + pool, err := dbconn.NewPool(t.Context(), dbconn.Config{URL: url}) + require.NoError(t, err) + defer pool.Close() + + seed := func(t *testing.T) string { + t.Helper() + schema := testutil.NewSchema(t, pool) + _, err := pool.Exec(t.Context(), fmt.Sprintf(` + CREATE TABLE %[1]s.users (id bigint PRIMARY KEY, email text, note text); + CREATE VIEW %[1]s.active_users AS SELECT id, email FROM %[1]s.users; + CREATE TABLE %[1]s.orders (id bigint PRIMARY KEY, user_id bigint REFERENCES %[1]s.users (id))`, schema)) + require.NoError(t, err) + return schema + } + dependentsRemain := func(t *testing.T, schema string) { + t.Helper() + assert.True(t, columnExists(t, pool, schema, "users", "email")) + var views, keys int + require.NoError(t, pool.QueryRow(t.Context(), + `SELECT count(*) FROM pg_catalog.pg_class c JOIN pg_catalog.pg_namespace n ON n.oid = c.relnamespace + WHERE n.nspname OPERATOR(pg_catalog.=) $1 AND c.relname OPERATOR(pg_catalog.=) 'active_users'`, schema).Scan(&views)) + assert.Equal(t, 1, views, "the view that reads the column") + require.NoError(t, pool.QueryRow(t.Context(), + `SELECT count(*) FROM pg_catalog.pg_constraint + WHERE conrelid OPERATOR(pg_catalog.=) pg_catalog.to_regclass(pg_catalog.quote_ident($1) || '.orders') + AND contype OPERATOR(pg_catalog.=) 'f'`, schema).Scan(&keys)) + assert.Equal(t, 1, keys, "the other table's foreign key on the primary key") + require.NoError(t, pool.QueryRow(t.Context(), + `SELECT count(*) FROM pg_catalog.pg_constraint + WHERE conrelid OPERATOR(pg_catalog.=) pg_catalog.to_regclass(pg_catalog.quote_ident($1) || '.users') + AND contype OPERATOR(pg_catalog.=) 'p'`, schema).Scan(&keys)) + assert.Equal(t, 1, keys, "the primary key") + } + + for _, tc := range []struct { + name string + statement string + operation string + }{ + { + name: "a column a view reads", + statement: "ALTER TABLE %s.users DROP COLUMN email CASCADE", + operation: "DROP COLUMN email", + }, + { + name: "a primary key another table's foreign key references", + statement: "ALTER TABLE %s.users DROP CONSTRAINT users_pkey CASCADE", + operation: "DROP CONSTRAINT users_pkey", + }, + } { + t.Run(tc.name, func(t *testing.T) { + schema := seed(t) + v, err := migrate.Run(t.Context(), pool, parseOne(t, fmt.Sprintf(tc.statement, schema)), runOptions()) + require.NoError(t, err) + assert.Equal(t, verdict.OutcomeRefused, v.Outcome) + assert.Equal(t, verdict.ReasonUnsupportedStatement, v.Reason) + assert.Equal(t, verdict.ClassCapabilityBoundary, v.Class) + assert.Contains(t, v.Detail, tc.operation+" uses CASCADE, which also drops the objects that depend on it") + assert.Contains(t, v.Detail, "drop each dependent object explicitly, then resubmit without CASCADE") + dependentsRemain(t, schema) + }) + } + + 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()) + require.NoError(t, err) + assert.Equal(t, verdict.OutcomeExecuted, v.Outcome, "detail: %s", v.Detail) + assert.False(t, columnExists(t, pool, schema, "users", "note")) + dependentsRemain(t, schema) + }) +} diff --git a/pkg/migrate/dependent_view_integration_test.go b/pkg/migrate/dependent_view_integration_test.go index 271a2eb..0004545 100644 --- a/pkg/migrate/dependent_view_integration_test.go +++ b/pkg/migrate/dependent_view_integration_test.go @@ -99,18 +99,6 @@ func TestRunRefusesChangingAColumnAViewReads(t *testing.T) { assert.True(t, columnExists(t, pool, schema, "users", "mail")) }, }, - { - name: "dropping the column with CASCADE, which drops the views", - statement: "ALTER TABLE %s.users DROP COLUMN email CASCADE", - check: func(t *testing.T, schema string) { - assert.False(t, columnExists(t, pool, schema, "users", "email")) - var views int - require.NoError(t, pool.QueryRow(t.Context(), - `SELECT count(*) FROM pg_catalog.pg_class c JOIN pg_catalog.pg_namespace n ON n.oid = c.relnamespace - WHERE n.nspname = $1 AND c.relkind IN ('v', 'm')`, schema).Scan(&views)) - assert.Zero(t, views, "CASCADE drops the views with the column") - }, - }, { name: "dropping a column no view reads", statement: "ALTER TABLE %s.users DROP COLUMN note", diff --git a/pkg/migrate/verdicts.go b/pkg/migrate/verdicts.go index 9b52047..0452dbb 100644 --- a/pkg/migrate/verdicts.go +++ b/pkg/migrate/verdicts.go @@ -162,6 +162,11 @@ func routeRefusalVerdict(st statement.Statement, rs router.Statement) verdict.Ve v.Detail = fmt.Sprintf("%s changes a column read by %s, and PostgreSQL refuses to retype or drop a column while a view reads it; "+ "drop or redefine those views first, then re-plan", d.Operation, strings.Join(d.DependentViews, ", ")) } + if d.Reason == planner.ReasonCascadingDrop { + v.Detail = fmt.Sprintf("%s uses CASCADE, which also drops the objects that depend on it, such as views or other tables' foreign keys, "+ + "outside this statement's table; drop each dependent object explicitly, then resubmit without CASCADE, "+ + "and PostgreSQL then refuses the drop if anything still depends on it", d.Operation) + } break } return v diff --git a/pkg/plan/plan.go b/pkg/plan/plan.go index d2a60c2..82b0c22 100644 --- a/pkg/plan/plan.go +++ b/pkg/plan/plan.go @@ -33,8 +33,9 @@ import ( // set-generated-expression change kind. Version 8 added the replica-identity // planner reason. Version 9 added the validate-constraint and // validate-foreign-key change kinds. Version 10 added the dependent-view -// planner reason and the dependent_views decision field. -const FormatVersion = 10 +// planner reason and the dependent_views decision field. Version 11 added +// the cascading-drop planner reason. +const FormatVersion = 11 // 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 b1869c4..d43631f 100644 --- a/pkg/plan/plan_test.go +++ b/pkg/plan/plan_test.go @@ -454,7 +454,7 @@ func TestDiscloseGreenfieldExecutionRequiresAbsentTable(t *testing.T) { } // The JSON shape is the adapter-facing contract: exact keys, exact -// omissions. A consumer pins format_version 10 against this test. +// omissions. A consumer pins format_version 11 against this test. func TestReportJSONShape(t *testing.T) { exists := true r := plan.Report{ @@ -510,7 +510,7 @@ func TestReportJSONShape(t *testing.T) { raw, err := json.Marshal(r) require.NoError(t, err) assert.JSONEq(t, fmt.Sprintf(`{ - "format_version": 10, + "format_version": 11, "source": "diff", "schema": "public", "table": "t", @@ -568,7 +568,7 @@ func TestReportJSONOmitsUnsetOptionalFields(t *testing.T) { raw, err := json.Marshal(r) require.NoError(t, err) assert.JSONEq(t, `{ - "format_version": 10, + "format_version": 11, "source": "alter", "disposition": "execute", "fingerprint": "sha256:e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855", @@ -652,7 +652,7 @@ func TestFingerprintCoversExecutionNotExplanation(t *testing.T) { } // Sources is the closed vocabulary a consumer branches on; the set is -// pinned to format_version 10. +// pinned to format_version 11. func TestSourcesVocabularyPinned(t *testing.T) { assert.Equal(t, []plan.Source{plan.SourceAlter, plan.SourceDiff}, plan.Sources()) } diff --git a/pkg/planner/planner.go b/pkg/planner/planner.go index fe9a418..b4674c2 100644 --- a/pkg/planner/planner.go +++ b/pkg/planner/planner.go @@ -84,6 +84,7 @@ func Reasons() []Reason { ReasonPartitionParentLock, ReasonReplicaIdentity, ReasonDependentView, + ReasonCascadingDrop, ReasonUnsupportedOperation, } } @@ -161,6 +162,14 @@ const ( // nothing, so the statement could only fail. The decision names the // views; change or drop them first, then re-plan. ReasonDependentView Reason = "dependent-view" + // ReasonCascadingDrop: a drop with CASCADE, which also drops whatever + // depends on what it drops: a view that reads a dropped column, or + // another table's foreign key on a dropped key. Those objects are not + // in the statement, so neither the plan nor the table's lock covers + // them. Drop each dependent object as its own change, then resubmit + // without CASCADE, which PostgreSQL refuses while anything still + // depends on the target. + ReasonCascadingDrop Reason = "cascading-drop" // ReasonUnsupportedOperation: the planner does not recognize the // operation or knows no safe path for it. ReasonUnsupportedOperation Reason = "unsupported-operation" @@ -311,14 +320,10 @@ func FactsFrom(live schemadiff.Model) Facts { // viewsReadingColumn returns the views that read the column op retypes or // drops: the two column changes PostgreSQL refuses while a view reads the -// column. A rename is not one of them, since a view follows the column, and -// neither is a DROP COLUMN ... CASCADE, which the server accepts by dropping -// the views with it; that drop stays destructive. Under ONLY the change -// stops at the table, so only its own views count. +// column. A rename is not one of them, since a view follows the column. +// Under ONLY the change stops at the table, so only its own views count. func viewsReadingColumn(op statement.Op, facts Facts) []string { switch { - case op.Kind == statement.OpDropColumn && op.Cascade: - return nil case op.Kind != statement.OpAlterColumnType && op.Kind != statement.OpDropColumn: return nil case op.Only: @@ -381,6 +386,10 @@ func classifyOp(op statement.Op, st statement.Statement, facts Facts, sql string d.Route, d.Reason = RouteRefuse, ReasonReplicaIdentity return d } + if op.Cascade { + d.Route, d.Reason = RouteRefuse, ReasonCascadingDrop + return d + } if views := viewsReadingColumn(op, facts); len(views) > 0 { d.Route, d.Reason, d.DependentViews = RouteRefuse, ReasonDependentView, views return d diff --git a/pkg/planner/planner_test.go b/pkg/planner/planner_test.go index 9d7e613..cd1df84 100644 --- a/pkg/planner/planner_test.go +++ b/pkg/planner/planner_test.go @@ -171,23 +171,14 @@ func TestClassifyRefusesChangingAColumnAViewReads(t *testing.T) { assert.Equal(t, planner.ReasonBinaryCoercible, plan.Decisions[0].Reason) } -// A DROP COLUMN ... CASCADE drops the views with the column, which the -// server accepts, so it is not refused; it stays destructive. A view on a -// table below counts against a recursive change and not against one under -// ONLY, which stops at the table. +// A view on a table below counts against a recursive change and not +// against one under ONLY, which stops at the table. func TestClassifyJudgesDependentViewsByWhatTheChangeReaches(t *testing.T) { facts := planner.Facts{ ColumnTypes: map[string]string{"email": "character varying(50)", "code": "text"}, DependentViews: map[string][]string{"email": {`"app"."active_users"`}, "code": {`"app"."child_codes"`}}, OwnDependentViews: map[string][]string{"email": {`"app"."active_users"`}}, } - plan, err := planner.Classify("ALTER TABLE t DROP COLUMN email CASCADE", facts) - require.NoError(t, err) - require.Len(t, plan.Decisions, 1) - assert.Equal(t, planner.ReasonMetadataOnly, plan.Decisions[0].Reason) - assert.True(t, plan.Decisions[0].Destructive) - assert.Empty(t, plan.Decisions[0].DependentViews) - for sql, want := range map[string][]string{ "ALTER TABLE t DROP COLUMN email RESTRICT": {`"app"."active_users"`}, "ALTER TABLE ONLY t DROP COLUMN email": {`"app"."active_users"`}, @@ -201,9 +192,62 @@ func TestClassifyJudgesDependentViewsByWhatTheChangeReaches(t *testing.T) { assert.Equal(t, want, plan.Decisions[0].DependentViews, sql) } - plan, err = planner.Classify("ALTER TABLE ONLY t DROP COLUMN code", facts) + plan, err := planner.Classify("ALTER TABLE ONLY t DROP COLUMN code", facts) + require.NoError(t, err) + assert.Equal(t, planner.ReasonMetadataOnly, plan.Decisions[0].Reason) +} + +// CASCADE also drops whatever depends on the target, objects the +// statement does not name, so every drop with it is refused, from the +// statement alone and whatever the live facts say, while the default +// RESTRICT classifies as usual. A drop the replica identity guard refuses +// keeps that more specific reason. +func TestClassifyRefusesACascadingDrop(t *testing.T) { + views := planner.Facts{ + ColumnTypes: map[string]string{"email": "character varying(50)"}, + DependentViews: map[string][]string{"email": {`"app"."active_users"`}}, + OwnDependentViews: map[string][]string{"email": {`"app"."active_users"`}}, + } + for _, facts := range []planner.Facts{{}, views} { + for _, sql := range []string{ + "ALTER TABLE t DROP COLUMN email CASCADE", + "ALTER TABLE ONLY t DROP COLUMN email CASCADE", + "ALTER TABLE t DROP COLUMN IF EXISTS email CASCADE", + "ALTER TABLE t DROP CONSTRAINT t_pkey CASCADE", + "ALTER TABLE t DROP CONSTRAINT IF EXISTS t_pkey CASCADE", + "DROP INDEX t_email_idx CASCADE", + "DROP INDEX app.t_email_idx, app.t_other_idx CASCADE", + } { + plan, err := planner.Classify(sql, facts) + require.NoError(t, err, sql) + require.Len(t, plan.Decisions, 1, sql) + d := plan.Decisions[0] + assert.Equal(t, planner.RouteRefuse, d.Route, sql) + assert.Equal(t, planner.ReasonCascadingDrop, d.Reason, sql) + assert.True(t, d.Destructive, sql) + assert.Empty(t, d.SaferSQL, "DROP INDEX CONCURRENTLY does not accept CASCADE: %s", sql) + assert.Empty(t, d.DependentViews, sql) + assert.Equal(t, planner.RouteRefuse, plan.Route, sql) + } + } + + plan, err := planner.Classify("ALTER TABLE t ADD COLUMN note text, DROP CONSTRAINT t_pkey CASCADE", planner.Facts{}) require.NoError(t, err) + require.Len(t, plan.Decisions, 2) assert.Equal(t, planner.ReasonMetadataOnly, plan.Decisions[0].Reason) + assert.Equal(t, planner.ReasonCascadingDrop, plan.Decisions[1].Reason) + assert.Equal(t, planner.RouteRefuse, plan.Route) + + for sql, reason := range map[string]planner.Reason{ + "ALTER TABLE t DROP CONSTRAINT t_pkey": planner.ReasonMetadataOnly, + "ALTER TABLE t DROP CONSTRAINT t_pkey RESTRICT": planner.ReasonMetadataOnly, + "DROP INDEX t_email_idx RESTRICT": planner.ReasonSaferIdiom, + } { + plan, err := planner.Classify(sql, planner.Facts{}) + require.NoError(t, err, sql) + require.Len(t, plan.Decisions, 1, sql) + assert.Equal(t, reason, plan.Decisions[0].Reason, sql) + } } // facts mirrors a live table whose column types exercise both sides of the @@ -681,6 +725,7 @@ func TestReasonsVocabularyPinned(t *testing.T) { planner.ReasonPartitionParentLock, planner.ReasonReplicaIdentity, planner.ReasonDependentView, + planner.ReasonCascadingDrop, planner.ReasonUnsupportedOperation, }, planner.Reasons()) } diff --git a/pkg/statement/ops.go b/pkg/statement/ops.go index b4caf44..ae16b42 100644 --- a/pkg/statement/ops.go +++ b/pkg/statement/ops.go @@ -94,8 +94,9 @@ type Op struct { // operation stops at the table and does not recurse into the tables // below it. Only bool - // Cascade reports a DROP COLUMN ... CASCADE, which also drops the - // objects that depend on the column, views among them, where the + // Cascade reports a drop with CASCADE (DROP COLUMN, DROP CONSTRAINT, + // or DROP INDEX), which also drops the objects that depend on what it + // drops, such as a view or another table's foreign key, where the // default RESTRICT refuses while any exist. Cascade bool // Indexes are the unqualified names of the indexes an OpDropIndex @@ -365,7 +366,13 @@ func ParseOps(sql string) ([]Op, error) { if drop.GetRemoveType() != pganalyze.ObjectType_OBJECT_INDEX { return []Op{{Kind: OpUnrecognized}}, nil } - return []Op{{Kind: OpDropIndex, Name: dropIndexNames(drop), Indexes: droppedIndexes(drop), Concurrent: drop.GetConcurrent()}}, nil + return []Op{{ + Kind: OpDropIndex, + Name: dropIndexNames(drop), + Indexes: droppedIndexes(drop), + Concurrent: drop.GetConcurrent(), + Cascade: drop.GetBehavior() == pganalyze.DropBehavior_DROP_CASCADE, + }}, nil case node.GetReindexStmt() != nil: re := node.GetReindexStmt() return []Op{{ @@ -432,7 +439,7 @@ func alterTableOp(cmd *pganalyze.AlterTableCmd) Op { case pganalyze.AlterTableType_AT_ValidateConstraint: return Op{Kind: OpValidateConstraint, Name: cmd.GetName()} case pganalyze.AlterTableType_AT_DropConstraint: - return Op{Kind: OpDropConstraint, Name: cmd.GetName()} + return Op{Kind: OpDropConstraint, Name: cmd.GetName(), Cascade: cmd.GetBehavior() == pganalyze.DropBehavior_DROP_CASCADE} case pganalyze.AlterTableType_AT_AttachPartition: return Op{Kind: OpAttachPartition} case pganalyze.AlterTableType_AT_DetachPartition: diff --git a/pkg/statement/ops_test.go b/pkg/statement/ops_test.go index 9a0ee44..97fa3ee 100644 --- a/pkg/statement/ops_test.go +++ b/pkg/statement/ops_test.go @@ -268,6 +268,11 @@ func TestParseOpsShapes(t *testing.T) { sql: "ALTER TABLE t DROP CONSTRAINT c", want: statement.Op{Kind: statement.OpDropConstraint, Name: "c"}, }, + { + name: "drop constraint cascade", + sql: "ALTER TABLE t DROP CONSTRAINT c CASCADE", + want: statement.Op{Kind: statement.OpDropConstraint, Name: "c", Cascade: true}, + }, { name: "attach partition", sql: "ALTER TABLE t ATTACH PARTITION p FOR VALUES FROM (1) TO (10)", @@ -308,6 +313,11 @@ func TestParseOpsShapes(t *testing.T) { sql: "DROP INDEX CONCURRENTLY i", want: statement.Op{Kind: statement.OpDropIndex, Name: "i", Indexes: []string{"i"}, Concurrent: true}, }, + { + name: "drop index cascade", + sql: "DROP INDEX i CASCADE", + want: statement.Op{Kind: statement.OpDropIndex, Name: "i", Indexes: []string{"i"}, Cascade: true}, + }, { name: "reindex", sql: "REINDEX INDEX i", From 03e40c41828f0ff15dcdf112c3b4b3d8f5701b1a Mon Sep 17 00:00:00 2001 From: Armand Parajon Date: Sat, 17 Oct 2026 15:52:28 -0400 Subject: [PATCH 2/2] demo: expect lint format_version 5 Co-Authored-By: Claude Opus 5.5 --- demo/tour.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/demo/tour.sh b/demo/tour.sh index f922b0f..1879262 100755 --- a/demo/tour.sh +++ b/demo/tour.sh @@ -326,7 +326,7 @@ run_offline() { if [ "$CHECK" = 1 ]; then out=$("$PGS" lint --json risky.sql) || status=$? assert_eq "lint exit" 1 "$status" - assert_eq "lint format_version" 4 "$(jq -r '.format_version' <<<"$out")" + assert_eq "lint format_version" 5 "$(jq -r '.format_version' <<<"$out")" assert_eq "lint errors" 1 "$(jq -r '.errors' <<<"$out")" else "$PGS" lint risky.sql || echo "(exit $? — error findings gate, by design)"