From c0bff628267d27d9a9b3ea63c7b627d86d405b59 Mon Sep 17 00:00:00 2001 From: Armand Parajon Date: Sat, 17 Oct 2026 02:44:09 -0400 Subject: [PATCH 1/2] planner: refuse retyping or dropping a column a view reads PostgreSQL refuses ALTER COLUMN TYPE and DROP COLUMN on a column that a view or materialized view reads, even for a type change it would otherwise relabel without a rewrite. The planner classified both as native, so the statement reached the server and failed there with an untyped error that did not say which views were in the way. Both front doors now read, for the target table, which views and materialized views pg_depend records as reading each column, and the planner refuses a type change or drop of such a column with the new dependent-view reason. The decision names the views in a new dependent_views field, and the refusal detail and dry-run text tell the operator to drop or redefine them first. A rename is still allowed, since a view follows the column. The server's own refusal remains the backstop for a view created after planning. The new reason grows the closed Reasons vocabulary and the decision gains a field, so the plan report moves to format version 10 and the lint report, which embeds the reasons, to version 4. The linter has no live facts and never reports the reason. Co-Authored-By: Claude Opus 5.5 --- README.md | 2 + demo/tour.sh | 10 +- docs/binary-coercible-type-changes.md | 4 +- docs/capabilities.md | 4 +- docs/cli-output-examples.md | 16 +-- docs/limitations.md | 1 + docs/lint-report.md | 7 +- docs/optimistic-attempt.md | 1 + docs/plan-report.md | 10 +- docs/postgres-online-ddl-reference.md | 18 ++- internal/cli/dryrun_text.go | 21 ++++ internal/cli/dryrun_text_test.go | 31 +++++ pkg/capabilities/capabilities.yaml | 4 +- .../dependent_view_integration_test.go | 57 +++++++++ pkg/diffplan/diffplan.go | 3 + pkg/lint/lint.go | 5 +- pkg/lint/lint_test.go | 4 +- .../dependent_view_integration_test.go | 118 ++++++++++++++++++ pkg/migrate/facts.go | 13 +- pkg/migrate/verdicts.go | 5 + pkg/plan/plan.go | 5 +- pkg/plan/plan_test.go | 8 +- pkg/planner/planner.go | 32 +++++ pkg/planner/planner_test.go | 48 +++++++ pkg/preflight/dependent_views.go | 52 ++++++++ .../dependent_views_integration_test.go | 56 +++++++++ 26 files changed, 496 insertions(+), 39 deletions(-) create mode 100644 pkg/diffplan/dependent_view_integration_test.go create mode 100644 pkg/migrate/dependent_view_integration_test.go create mode 100644 pkg/preflight/dependent_views.go create mode 100644 pkg/preflight/dependent_views_integration_test.go diff --git a/README.md b/README.md index 28049431..f16420cf 100644 --- a/README.md +++ b/README.md @@ -100,6 +100,8 @@ refusal — never a silently wrong or incomplete result: - **Dropping what a replica identity index is built on is refused** — the index, the constraint that owns it, or a column it covers; move `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 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 diff --git a/demo/tour.sh b/demo/tour.sh index eef03f0b..6000c86f 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]" 9 "$(jq -r '.format_version' <<<"$out")" + assert_eq "plan format_version of [$sql]" 10 "$(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]" 9 "$(jq -r '.format_version' <<<"$out")" + assert_eq "diff format_version of [$desired]" 10 "$(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]" 9 "$(jq -r '.format_version' <<<"$out")" + assert_eq "diff format_version of [$desired]" 10 "$(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]" 9 "$(jq -r '.format_version' <<<"$out")" + assert_eq "diff format_version of [$desired]" 10 "$(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 @@ -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" 3 "$(jq -r '.format_version' <<<"$out")" + assert_eq "lint format_version" 4 "$(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)" diff --git a/docs/binary-coercible-type-changes.md b/docs/binary-coercible-type-changes.md index 648d9997..fb301a9f 100644 --- a/docs/binary-coercible-type-changes.md +++ b/docs/binary-coercible-type-changes.md @@ -362,7 +362,9 @@ statement rolls back; the time and lock are spent regardless). statement. - **A view or rule referencing the column blocks the statement outright** — `cannot alter type of a column used by a view or rule` — whether or not the change would - have been a relabel. The view has to be dropped and recreated around the change. + have been a relabel. The view has to be dropped and recreated around the change. The planner + reads which views and materialized views read the column and refuses the change as + `dependent-view`, naming them, before the statement reaches the server. - **A stored generated column whose expression references the altered column blocks the statement the same way** — `cannot alter type of a column used by a generated column` (`ERRCODE_FEATURE_NOT_SUPPORTED`), again regardless of whether the change would have been a diff --git a/docs/capabilities.md b/docs/capabilities.md index ae8bf45c..3db6bcd3 100644 --- a/docs/capabilities.md +++ b/docs/capabilities.md @@ -203,8 +203,8 @@ review the object warrants) · | `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` | -| `ALTER COLUMN TYPE`, binary-coercible (proven against live column facts) | ✅ | native, as-is | Yes | Catalog relabel, e.g. `varchar(50)` → `varchar(100)`, `varchar` → `text`; PostgreSQL itself refuses the change when a view, rule, or `STORED` generated column depends on the column — see [binary-coercible-type-changes.md](binary-coercible-type-changes.md#no-rewrite-is-not-no-cost) | +| `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 | +| `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 | | `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 | diff --git a/docs/cli-output-examples.md b/docs/cli-output-examples.md index 95f6217c..7c32846f 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": 9, + "format_version": 10, "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": 9, + "format_version": 10, "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": 9, + "format_version": 10, "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": 9, + "format_version": 10, "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": 9, + "format_version": 10, "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": 9, + "format_version": 10, "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": 3, + "format_version": 4, "postgres_versions": "14-18", "findings": [ { @@ -531,7 +531,7 @@ CREATE TABLE users ( ```console $ pg-sprite diff --desired /tmp/users.sql --json { - "format_version": 9, + "format_version": 10, "source": "diff", "schema": "public", "table": "users", diff --git a/docs/limitations.md b/docs/limitations.md index 9f44f6c0..42a522da 100644 --- a/docs/limitations.md +++ b/docs/limitations.md @@ -15,6 +15,7 @@ not escape hatches: | `ADD PRIMARY KEY` / `ADD UNIQUE` with a storage parameter the online sequence cannot render | The online sequence builds the key's index with `CREATE UNIQUE INDEX CONCURRENTLY` and carries the key's `WITH (...)` parameters onto it, each name quoted and each value a quoted literal. A parameter it cannot render faithfully, one whose value is not a plain word, number, or string, contains a backslash, or whose name is namespaced, would be dropped or changed, so no sequence is built. Running the statement as written builds the index under `ACCESS EXCLUSIVE`, so it is refused as `rewrite-required` with the `unique-index-then-constraint` guidance. Build the index concurrently with the parameter, then attach it with `ADD CONSTRAINT ... USING INDEX`. | | `ADD FOREIGN KEY ... NOT VALID` on a partitioned parent | PostgreSQL does not support this before version 18, so pg-sprite refuses it on versions 14–17. It is supported on version 18 and later. | | 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. The planner reads those views from the catalog and refuses the change with `dependent-view`, naming each view, instead of letting the statement fail at the server. pg-sprite does not drop and recreate the views for you. Drop or redefine them first, then re-plan. | | `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 b7096264..ef4a7a91 100644 --- a/docs/lint-report.md +++ b/docs/lint-report.md @@ -15,9 +15,10 @@ 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 **3**: version 3 added the `concurrent-in-transaction` code. Version 2 -added the `replica-identity` reason to the embedded Reasons vocabulary. The linter itself never -reports that reason, because it is judged from live replica identity facts the linter does not +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. ### Relationship to the plan report diff --git a/docs/optimistic-attempt.md b/docs/optimistic-attempt.md index b54ccdcf..de0bc90c 100644 --- a/docs/optimistic-attempt.md +++ b/docs/optimistic-attempt.md @@ -326,6 +326,7 @@ against `sizeGuardApplies`): | `type-rewrite` | n/a — same | | `relocation` | n/a — same | | `replica-identity` | n/a — refused at plan time | +| `dependent-view` | 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 bae76dde..d4dfbe8b 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 **9**: 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 **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` 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); @@ -90,6 +90,7 @@ consumer rendering either into a shared surface must clamp and escape them. | `unverified` | bool | when true | The planner failed closed to this route for lack of live facts — the route is what the engine would do, not a proven property of the change. With facts (a live introspection or a supplied column type) the same operation may classify as native. Absent means the decision is proven. | | `safer_sql` | array | safer-idiom only | The ordered safer native sequence, when the planner could construct it. A safer form of the submitted operation, not a semantic equivalent: it converges on the same declared end state with different locking, transactionality, and failure modes. | | `safer_sql_execution` | string | with `safer_sql` | The typed execution contract for `safer_sql` (see Execution contracts). Present exactly when `safer_sql` is. | +| `dependent_views` | array | dependent-view only | The views and materialized views that read the column, each schema-qualified with every identifier quoted, sorted. Present exactly when `reason` is `dependent-view`. Catalog identifiers: a consumer rendering them into a shared surface must clamp and escape them. | ## Closed vocabularies @@ -124,6 +125,7 @@ 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, which PostgreSQL refuses 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. | | `unsupported-operation` | The planner does not recognize the operation or knows no safe path for it. | ### Target-dependent refusal reasons (`reason`, `statements[].reason`) @@ -301,7 +303,7 @@ in `pkg/plan` — if the code drifts from this page, CI fails. ```json { - "format_version": 9, + "format_version": 10, "source": "alter", "schema": "app", "table": "orders", @@ -344,7 +346,7 @@ A desired state that drops an index and adds a column with a constant default: ```json { - "format_version": 9, + "format_version": 10, "source": "diff", "schema": "app", "table": "orders", @@ -411,7 +413,7 @@ A desired state for a table that does not exist yet, whose `CREATE TABLE` carrie ```json { - "format_version": 9, + "format_version": 10, "source": "diff", "schema": "app", "table": "gadgets", diff --git a/docs/postgres-online-ddl-reference.md b/docs/postgres-online-ddl-reference.md index cb48fe81..d5b023a5 100644 --- a/docs/postgres-online-ddl-reference.md +++ b/docs/postgres-online-ddl-reference.md @@ -54,7 +54,8 @@ The remaining reasons are first-bucket by cost but carry a warning the cost alon the lock queue below applies to every query on the parent) and `app-breaking-rename` (the 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. `unsupported-operation` means the planner does not recognize the operation or +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 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. @@ -503,6 +504,21 @@ unique index, or `REPLICA IDENTITY DEFAULT` when a primary key remains), then re The planner judges this from live facts, so the linter, which has none, never reports it. +### `dependent-view` + +| Verdict | Lock | Scan / rewrite | Dry-run exit | +|---|---|---|---| +| refused — never executed | — | — | 2 | + +The operation changes the type of, or drops, a column that a view or materialized +view reads. PostgreSQL refuses both while the view exists, even a type change it +would otherwise relabel without a rewrite, so the statement could only fail. 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. +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. + ### `unsupported-operation` | Verdict | Lock | Scan / rewrite | Dry-run exit | diff --git a/internal/cli/dryrun_text.go b/internal/cli/dryrun_text.go index fc67f7a6..a6669a24 100644 --- a/internal/cli/dryrun_text.go +++ b/internal/cli/dryrun_text.go @@ -77,6 +77,9 @@ func writeStatementDiagnostics(w *stickyWriter, n int, ps plan.Statement, runner w.diag(decisionSeverity(ps, d), string(d.Reason), fmt.Sprintf("%s — %s", d.Operation, impactText(d.Reason))) codes = appendCode(codes, string(d.Reason)) + if len(d.DependentViews) > 0 { + w.diag("help", "", "drop or redefine those views first, then re-plan") + } } if ps.Destructive { w.diag("warning", "destructive", "this change discards live data or structure") @@ -290,6 +293,8 @@ func impactText(r planner.Reason) string { return "takes a brief exclusive lock on the partitioned parent, briefly blocking access to every partition" case planner.ReasonReplicaIdentity: 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.ReasonUnsupportedOperation: return "pg-sprite does not recognize this operation and will not run it" } @@ -332,12 +337,28 @@ func refuseText(ps plan.Statement, r verdict.Reason) string { "SET EXPRESSION needs PostgreSQL 17 or later; before that, the only path is to drop and re-add the column, " + "which also drops its indexes" } + if d, ok := dependentViewRefusal(ps); ok { + return d.Operation + " changes a column read by " + strings.Join(d.DependentViews, ", ") + + ", and PostgreSQL refuses to retype or drop a column while a view reads it" + } if op := refusedOperation(ps); op != "" { return "the planner knows no safe path for " + op } return "the planner knows no safe path for this statement" } +// dependentViewRefusal returns the decision that refused the statement +// because views read the column it changes, the one refusal whose views +// the text names. +func dependentViewRefusal(ps plan.Statement) (planner.Decision, bool) { + for _, d := range ps.Decisions { + if d.Route == planner.RouteRefuse && d.Reason == planner.ReasonDependentView { + return d, true + } + } + return planner.Decision{}, false +} + // refusedOperation names the first operation the planner refused — the // same operation the run path's refusal verdict names in its detail. func refusedOperation(ps plan.Statement) string { diff --git a/internal/cli/dryrun_text_test.go b/internal/cli/dryrun_text_test.go index ae2fc446..75cfc732 100644 --- a/internal/cli/dryrun_text_test.go +++ b/internal/cli/dryrun_text_test.go @@ -295,6 +295,37 @@ func TestDryRunTextPlannerRefusalNamesOperation(t *testing.T) { assert.NotContains(t, text, "error[refuse]") } +// A refusal because views read the changed column names those views in +// the refusal itself, rather than the generic no-safe-path sentence, and +// the help line says what to do about them. +func TestDryRunTextDependentViewRefusalNamesTheViews(t *testing.T) { + report := plan.NewReport(plan.SourceAlter) + report.Schema, report.Table, report.ServerVersion = "app", "users", "16.10" + report.Disposition = router.DispositionRefuse + report.Statements = append(report.Statements, plan.Statement{ + SQL: "ALTER TABLE app.users DROP COLUMN email", + Route: planner.RouteRefuse, + Disposition: router.DispositionRefuse, + Reason: verdict.ReasonUnsupportedStatement, + Class: verdict.ClassCapabilityBoundary, + Decisions: []planner.Decision{{ + Operation: "DROP COLUMN email", + Destructive: true, + Route: planner.RouteRefuse, + Reason: planner.ReasonDependentView, + DependentViews: []string{`"app"."active_users"`, `"reports"."mail"`}, + }}, + }) + + var out strings.Builder + require.NoError(t, writeDryRunText(&out, palette{}, report)) + text := strings.Join(strings.Fields(out.String()), " ") + assert.Contains(t, text, `error[unsupported-statement]: refused — DROP COLUMN email changes a column read by "app"."active_users", "reports"."mail", and PostgreSQL refuses to retype or drop a column while a view reads it`) + assert.Contains(t, text, "help: drop or redefine those views first, then re-plan") + assert.Contains(t, text, onlineDDLReferenceURL+"#dependent-view") + assert.NotContains(t, text, "knows no safe path") +} + // A refused generated-expression change says why it cannot run, rather // than the generic no-safe-path sentence, so the reader knows the drift is // real and what applying it would cost. diff --git a/pkg/capabilities/capabilities.yaml b/pkg/capabilities/capabilities.yaml index 0e5b7152..6a41a422 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`" + 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" - 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)" @@ -91,7 +91,7 @@ rows: front_doors: migrate: supported diff: supported - reason_notes: "Catalog relabel, e.g. `varchar(50)` → `varchar(100)`, `varchar` → `text`; PostgreSQL itself refuses the change when a view, rule, or `STORED` generated column depends on the column — see [binary-coercible-type-changes.md](binary-coercible-type-changes.md#no-rewrite-is-not-no-cost)" + reason_notes: "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)" - id: alter-column-type-general-or-with-using area: "column_changes" operation: "`ALTER COLUMN TYPE`, general (or with `USING`)" diff --git a/pkg/diffplan/dependent_view_integration_test.go b/pkg/diffplan/dependent_view_integration_test.go new file mode 100644 index 00000000..939ef57b --- /dev/null +++ b/pkg/diffplan/dependent_view_integration_test.go @@ -0,0 +1,57 @@ +package diffplan_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/diffplan" + "github.com/block/pg-sprite/pkg/planner" + "github.com/block/pg-sprite/pkg/router" + "github.com/block/pg-sprite/pkg/verdict" +) + +// A desired file that widens a column a view reads plans the type change +// as a refused statement naming the view, since PostgreSQL would refuse it +// while the view exists. The rest of the file stays planned as usual: an +// unrelated column add still routes natively. +func TestPlanRefusesChangingAColumnAViewReads(t *testing.T) { + url := testutil.StartPostgres(t) + pool, err := dbconn.NewPool(t.Context(), dbconn.Config{URL: url}) + require.NoError(t, err) + defer pool.Close() + + schema := testutil.NewSchema(t, pool) + _, err = pool.Exec(t.Context(), fmt.Sprintf(` + CREATE TABLE %[1]s.users (id bigint PRIMARY KEY, email varchar(50)); + CREATE VIEW %[1]s.active_users AS SELECT id, email FROM %[1]s.users`, schema)) + require.NoError(t, err) + + report, err := diffplan.Plan(t.Context(), pool, diffplan.Request{Schema: schema, + Desired: parseDesired(t, "CREATE TABLE users (id bigint PRIMARY KEY, email varchar(100), note text);")}) + require.NoError(t, err) + assert.Equal(t, router.DispositionRefuse, report.Disposition) + + var found, addPlanned bool + for _, st := range report.Statements { + switch st.SQL { + case fmt.Sprintf("ALTER TABLE %s.users ALTER COLUMN email TYPE varchar(100)", schema): + found = true + assert.Equal(t, planner.RouteRefuse, st.Route) + assert.Equal(t, verdict.ReasonUnsupportedStatement, st.Reason) + assert.Empty(t, st.ExecSQL) + require.Len(t, st.Decisions, 1) + assert.Equal(t, planner.ReasonDependentView, st.Decisions[0].Reason) + assert.Equal(t, []string{fmt.Sprintf(`"%s"."active_users"`, schema)}, st.Decisions[0].DependentViews) + case fmt.Sprintf("ALTER TABLE %s.users ADD COLUMN note text", schema): + addPlanned = true + assert.Equal(t, planner.RouteNative, st.Route) + } + } + assert.True(t, found, "the type change is planned and refused, got %v", report.Statements) + assert.True(t, addPlanned, "the unrelated column add is still planned, got %v", report.Statements) +} diff --git a/pkg/diffplan/diffplan.go b/pkg/diffplan/diffplan.go index 62630048..8f630ea8 100644 --- a/pkg/diffplan/diffplan.go +++ b/pkg/diffplan/diffplan.go @@ -99,6 +99,9 @@ func Plan(ctx context.Context, pool *pgxpool.Pool, req Request) (plan.Report, er if facts.AddColumnProofs, err = addColumnProofs(ctx, pool, req.Schema, ds.Table(), changes); err != nil { return plan.Report{}, err } + if facts.DependentViews, err = preflight.ColumnDependentViews(ctx, pool, req.Schema, ds.Table()); err != nil { + return plan.Report{}, err + } } report.TableExists = &tableExists if report.Statements, report.Disposition, err = classifyChanges(changes, facts); err != nil { diff --git a/pkg/lint/lint.go b/pkg/lint/lint.go index da802a67..b954572e 100644 --- a/pkg/lint/lint.go +++ b/pkg/lint/lint.go @@ -18,8 +18,9 @@ import ( // report whose version it does not understand instead of guessing at the // field semantics. Version 2 added the replica-identity planner reason to // the embedded Reasons vocabulary; version 3 added the -// concurrent-in-transaction code. -const FormatVersion = 3 +// concurrent-in-transaction code; version 4 added the dependent-view +// planner reason to the embedded Reasons vocabulary. +const FormatVersion = 4 // 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 655daad3..15e2952b 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": 3, + "format_version": 4, "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": 3, + "format_version": 4, "postgres_versions": "14-18", "findings": [], "errors": 0, diff --git a/pkg/migrate/dependent_view_integration_test.go b/pkg/migrate/dependent_view_integration_test.go new file mode 100644 index 00000000..385e335c --- /dev/null +++ b/pkg/migrate/dependent_view_integration_test.go @@ -0,0 +1,118 @@ +package migrate_test + +import ( + "fmt" + "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/planner" + "github.com/block/pg-sprite/pkg/verdict" +) + +// columnType is the column's live type as format_type renders it. +func columnType(t *testing.T, pool *pgxpool.Pool, schema, table, column string) string { + t.Helper() + var typ string + require.NoError(t, pool.QueryRow(t.Context(), + `SELECT pg_catalog.format_type(a.atttypid, a.atttypmod) + FROM pg_catalog.pg_attribute a + WHERE a.attrelid = pg_catalog.to_regclass(pg_catalog.quote_ident($1) || '.' || pg_catalog.quote_ident($2)) + AND a.attname = $3 AND NOT a.attisdropped`, + schema, table, column).Scan(&typ)) + return typ +} + +// A column a view or materialized view reads cannot be retyped or dropped +// while the view exists; PostgreSQL refuses even a type change it would +// otherwise relabel without a rewrite. The run is refused at plan time with +// the views named, so the operator learns what to change before anything +// reaches the server, and the column is left as it was. A rename, which the +// views follow, and changes to a column no view reads still run. +func TestRunRefusesChangingAColumnAViewReads(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 varchar(50), note text); + CREATE VIEW %[1]s.active_users AS SELECT id, email FROM %[1]s.users; + CREATE MATERIALIZED VIEW %[1]s."Mail Report" AS SELECT email FROM %[1]s.users`, schema)) + require.NoError(t, err) + return schema + } + + for _, tc := range []struct { + name string + statement string + }{ + {name: "a type change the server would relabel", statement: "ALTER TABLE %s.users ALTER COLUMN email TYPE varchar(100)"}, + {name: "a type change to text", statement: "ALTER TABLE %s.users ALTER COLUMN email TYPE text"}, + {name: "a column drop", statement: "ALTER TABLE %s.users DROP COLUMN email"}, + } { + 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, fmt.Sprintf(`a column read by "%s"."Mail Report", "%s"."active_users"`, schema, schema)) + assert.Contains(t, v.Detail, "drop or redefine those views first, then re-plan") + assert.Equal(t, "character varying(50)", columnType(t, pool, schema, "users", "email")) + }) + } + + t.Run("the plan names the views", func(t *testing.T) { + schema := seed(t) + st := parseOne(t, fmt.Sprintf("ALTER TABLE %s.users ALTER COLUMN email TYPE varchar(100)", schema)) + facts, err := migrate.LiveFacts(t.Context(), pool, st) + require.NoError(t, err) + plan, err := planner.Classify(st.SQL(), facts.Classifier) + require.NoError(t, err) + require.Len(t, plan.Decisions, 1) + assert.Equal(t, planner.ReasonDependentView, plan.Decisions[0].Reason) + assert.Equal(t, []string{ + fmt.Sprintf(`"%s"."Mail Report"`, schema), + fmt.Sprintf(`"%s"."active_users"`, schema), + }, plan.Decisions[0].DependentViews) + }) + + for _, tc := range []struct { + name string + statement string + check func(t *testing.T, schema string) + }{ + { + name: "renaming the column the views read", + statement: "ALTER TABLE %s.users RENAME COLUMN email TO mail", + check: func(t *testing.T, schema string) { + assert.True(t, columnExists(t, pool, schema, "users", "mail")) + }, + }, + { + name: "dropping a column no view reads", + statement: "ALTER TABLE %s.users DROP COLUMN note", + check: func(t *testing.T, schema string) { + assert.False(t, columnExists(t, pool, schema, "users", "note")) + }, + }, + } { + 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.OutcomeExecuted, v.Outcome, "detail: %s", v.Detail) + tc.check(t, schema) + }) + } +} diff --git a/pkg/migrate/facts.go b/pkg/migrate/facts.go index de5b8afe..eb8c7c65 100644 --- a/pkg/migrate/facts.go +++ b/pkg/migrate/facts.go @@ -34,9 +34,11 @@ type Facts struct { TableExists *bool } -// LiveFacts introspects the statement's target table for classifier facts, -// and for an ALTER TABLE that adds columns, has the server prove whether -// each one rewrites the table (preflight.AddColumnRewrites). +// LiveFacts introspects the statement's target table for classifier facts. +// For an ALTER TABLE it reads which views depend on each column +// (preflight.ColumnDependentViews), and for one that adds columns, has the +// server prove whether each one rewrites the table +// (preflight.AddColumnRewrites). func LiveFacts(ctx context.Context, pool *pgxpool.Pool, st statement.Statement) (Facts, error) { if st.Table() == "" { @@ -59,6 +61,11 @@ func LiveFacts(ctx context.Context, pool *pgxpool.Pool, if err != nil { return Facts{}, err } + if st.Kind() == statement.KindAlterTable { + if classifier.DependentViews, err = preflight.ColumnDependentViews(ctx, pool, ResolvedSchema(st), st.Table()); err != nil { + return Facts{}, err + } + } if adds { if classifier.AddColumnProofs, err = preflight.AddColumnRewrites(ctx, pool, ResolvedSchema(st), st.Table(), st.SQL()); err != nil { return Facts{}, err diff --git a/pkg/migrate/verdicts.go b/pkg/migrate/verdicts.go index 42848fc8..6df281e2 100644 --- a/pkg/migrate/verdicts.go +++ b/pkg/migrate/verdicts.go @@ -3,6 +3,7 @@ package migrate import ( "errors" "fmt" + "strings" "github.com/block/pg-sprite/pkg/executor" "github.com/block/pg-sprite/pkg/plan" @@ -146,6 +147,10 @@ func routeRefusalVerdict(st statement.Statement, rs router.Statement) verdict.Ve "which makes UPDATE and DELETE fail while a publication publishes them; "+ "point REPLICA IDENTITY at another unique index first (or DEFAULT, if a usable primary key remains after the drop), then re-plan", d.Operation) } + if d.Reason == planner.ReasonDependentView { + 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, ", ")) + } break } return v diff --git a/pkg/plan/plan.go b/pkg/plan/plan.go index f224c518..d2a60c2d 100644 --- a/pkg/plan/plan.go +++ b/pkg/plan/plan.go @@ -32,8 +32,9 @@ import ( // foreign-key create-shape cause. Version 7 added the // 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. -const FormatVersion = 9 +// validate-foreign-key change kinds. Version 10 added the dependent-view +// planner reason and the dependent_views decision field. +const FormatVersion = 10 // 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 234f42c9..261dd67f 100644 --- a/pkg/plan/plan_test.go +++ b/pkg/plan/plan_test.go @@ -473,7 +473,7 @@ func TestDiscloseGreenfieldExecutionRequiresAbsentTable(t *testing.T) { } // The JSON shape is the adapter-facing contract: exact keys, exact -// omissions. A consumer pins format_version 9 against this test. +// omissions. A consumer pins format_version 10 against this test. func TestReportJSONShape(t *testing.T) { exists := true r := plan.Report{ @@ -529,7 +529,7 @@ func TestReportJSONShape(t *testing.T) { raw, err := json.Marshal(r) require.NoError(t, err) assert.JSONEq(t, fmt.Sprintf(`{ - "format_version": 9, + "format_version": 10, "source": "diff", "schema": "public", "table": "t", @@ -587,7 +587,7 @@ func TestReportJSONOmitsUnsetOptionalFields(t *testing.T) { raw, err := json.Marshal(r) require.NoError(t, err) assert.JSONEq(t, `{ - "format_version": 9, + "format_version": 10, "source": "alter", "disposition": "execute", "fingerprint": "sha256:e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855", @@ -671,7 +671,7 @@ func TestFingerprintCoversExecutionNotExplanation(t *testing.T) { } // Sources is the closed vocabulary a consumer branches on; the set is -// pinned to format_version 9. +// pinned to format_version 10. 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 972101c0..062328b3 100644 --- a/pkg/planner/planner.go +++ b/pkg/planner/planner.go @@ -83,6 +83,7 @@ func Reasons() []Reason { ReasonRelocation, ReasonPartitionParentLock, ReasonReplicaIdentity, + ReasonDependentView, ReasonUnsupportedOperation, } } @@ -154,6 +155,12 @@ const ( // Move the identity first (ALTER TABLE ... REPLICA IDENTITY), then // re-plan. ReasonReplicaIdentity Reason = "replica-identity" + // ReasonDependentView: the operation changes the type of, or drops, a + // column that a view or materialized view reads. PostgreSQL refuses + // both while the view exists, even a type change that rewrites + // nothing, so the statement could only fail. The decision names the + // views; change or drop them first, then re-plan. + ReasonDependentView Reason = "dependent-view" // ReasonUnsupportedOperation: the planner does not recognize the // operation or knows no safe path for it. ReasonUnsupportedOperation Reason = "unsupported-operation" @@ -224,6 +231,10 @@ type Decision struct { // of prose — it is what tells a consumer the sequence must not be // wrapped in a transaction block. SaferSQLExecution Execution `json:"safer_sql_execution,omitempty"` + // DependentViews names the views and materialized views that read the + // column, each schema-qualified with every identifier quoted, present + // exactly for dependent-view decisions. + DependentViews []string `json:"dependent_views,omitempty"` } // ExecutableAsSubmitted reports whether the operation's submitted form is @@ -268,6 +279,11 @@ type Facts struct { // constant; the proof leaves out only an ADD COLUMN IF NOT EXISTS for a // column the table already has, which the server skips. AddColumnProofs map[string]statement.AddColumnProof + // DependentViews maps a column to the views and materialized views that + // read it (preflight.ColumnDependentViews). A column absent from the map + // has none, or the facts were not read, as in an offline lint; either + // way the server's own refusal at execution remains the backstop. + DependentViews map[string][]string } // FactsFrom extracts the facts a live introspection model provides: the @@ -283,6 +299,18 @@ func FactsFrom(live schemadiff.Model) Facts { return Facts{ColumnTypes: types, ReplicaIdentity: live.ReplicaIdentity} } +// 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. +func viewsReadingColumn(op statement.Op, facts Facts) []string { + switch op.Kind { + case statement.OpAlterColumnType, statement.OpDropColumn: + return facts.DependentViews[op.Column] + default: + return nil + } +} + // addColumnProof returns what the server proved about adding op's // column, or "" when no proof was run. func addColumnProof(op statement.Op, facts Facts) statement.AddColumnProof { @@ -336,6 +364,10 @@ func classifyOp(op statement.Op, st statement.Statement, facts Facts, sql string d.Route, d.Reason = RouteRefuse, ReasonReplicaIdentity return d } + if views := viewsReadingColumn(op, facts); len(views) > 0 { + d.Route, d.Reason, d.DependentViews = RouteRefuse, ReasonDependentView, views + return d + } switch op.Kind { case statement.OpCreateTable: if op.PartitionOf { diff --git a/pkg/planner/planner_test.go b/pkg/planner/planner_test.go index 0e8e875a..b54b6af3 100644 --- a/pkg/planner/planner_test.go +++ b/pkg/planner/planner_test.go @@ -121,6 +121,53 @@ func TestClassifyJudgesAnOnlyDropByTheTablesOwnIdentity(t *testing.T) { } } +// PostgreSQL refuses to retype or drop a column a view reads, so either +// change on such a column is refused with the views named, even a type +// change that would otherwise relabel in place. A rename, which the view +// follows, and changes to columns no view reads classify as usual. +func TestClassifyRefusesChangingAColumnAViewReads(t *testing.T) { + views := planner.Facts{ + ColumnTypes: map[string]string{"email": "character varying(50)", "note": "text"}, + DependentViews: map[string][]string{ + "email": {`"app"."active_users"`, `"reports"."emails"`}, + }, + } + for _, sql := range []string{ + "ALTER TABLE t ALTER COLUMN email TYPE varchar(100)", + "ALTER TABLE t ALTER COLUMN email TYPE text", + "ALTER TABLE t ALTER COLUMN email TYPE bigint USING email::bigint", + "ALTER TABLE t DROP COLUMN email", + "ALTER TABLE ONLY t DROP COLUMN email", + } { + plan, err := planner.Classify(sql, views) + 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.ReasonDependentView, d.Reason, sql) + assert.Equal(t, []string{`"app"."active_users"`, `"reports"."emails"`}, d.DependentViews, sql) + assert.Equal(t, planner.RouteRefuse, plan.Route, sql) + } + + for sql, reason := range map[string]planner.Reason{ + "ALTER TABLE t RENAME COLUMN email TO mail": planner.ReasonAppBreakingRename, + "ALTER TABLE t ALTER COLUMN email SET DEFAULT ''": planner.ReasonMetadataOnly, + "ALTER TABLE t DROP COLUMN note": planner.ReasonMetadataOnly, + "ALTER TABLE t ALTER COLUMN note TYPE text": planner.ReasonBinaryCoercible, + } { + plan, err := planner.Classify(sql, views) + require.NoError(t, err, sql) + require.Len(t, plan.Decisions, 1, sql) + assert.Equal(t, reason, plan.Decisions[0].Reason, sql) + assert.Empty(t, plan.Decisions[0].DependentViews, sql) + } + + plan, err := planner.Classify("ALTER TABLE t ALTER COLUMN email TYPE varchar(100)", + planner.Facts{ColumnTypes: views.ColumnTypes}) + require.NoError(t, err) + assert.Equal(t, planner.ReasonBinaryCoercible, plan.Decisions[0].Reason) +} + // facts mirrors a live table whose column types exercise both sides of the // binary-coercible rules. var facts = planner.Facts{ColumnTypes: map[string]string{ @@ -594,6 +641,7 @@ func TestReasonsVocabularyPinned(t *testing.T) { planner.ReasonRelocation, planner.ReasonPartitionParentLock, planner.ReasonReplicaIdentity, + planner.ReasonDependentView, planner.ReasonUnsupportedOperation, }, planner.Reasons()) } diff --git a/pkg/preflight/dependent_views.go b/pkg/preflight/dependent_views.go new file mode 100644 index 00000000..58e632aa --- /dev/null +++ b/pkg/preflight/dependent_views.go @@ -0,0 +1,52 @@ +package preflight + +import ( + "context" + "fmt" + + "github.com/jackc/pgx/v5" + "github.com/jackc/pgx/v5/pgxpool" +) + +// ColumnDependentViews maps each column of schema.table that a view or +// materialized view reads to those views, each schema-qualified with every +// identifier quoted, sorted. A column no view reads is absent. +// +// PostgreSQL refuses to change the type of a column a view reads, even a +// relabel that rewrites nothing, and refuses to drop one without CASCADE. +// The dependency is the one pg_depend records from the view's rewrite rule +// to the column, so a view that reads the table without naming the column +// does not appear. The read is a planning fact: the server's own refusal at +// execution stays the backstop for a view created after it. +func ColumnDependentViews(ctx context.Context, pool *pgxpool.Pool, schema, table string) (map[string][]string, error) { + rows, err := pool.Query(ctx, `SELECT a.attname::text, n.nspname::text, v.relname::text + FROM pg_catalog.pg_depend d + JOIN pg_catalog.pg_rewrite r ON r.oid = d.objid + JOIN pg_catalog.pg_class v ON v.oid = r.ev_class + JOIN pg_catalog.pg_namespace n ON n.oid = v.relnamespace + JOIN pg_catalog.pg_attribute a ON a.attrelid = d.refobjid AND a.attnum = d.refobjsubid + WHERE d.classid = 'pg_catalog.pg_rewrite'::pg_catalog.regclass + AND d.refclassid = 'pg_catalog.pg_class'::pg_catalog.regclass + AND d.refobjid = pg_catalog.to_regclass(pg_catalog.quote_ident($1) || '.' || pg_catalog.quote_ident($2)) + AND d.refobjsubid > 0 + AND v.oid <> d.refobjid + AND v.relkind IN ('v', 'm') + GROUP BY a.attname, n.nspname, v.relname + ORDER BY a.attname, n.nspname, v.relname`, schema, table) + if err != nil { + return nil, fmt.Errorf("read the views that depend on the columns of %s: %w", pgx.Identifier{schema, table}.Sanitize(), err) + } + views := map[string][]string{} + for rows.Next() { + var column, viewSchema, view string + if err := rows.Scan(&column, &viewSchema, &view); err != nil { + rows.Close() + return nil, fmt.Errorf("read the views that depend on the columns of %s: %w", pgx.Identifier{schema, table}.Sanitize(), err) + } + views[column] = append(views[column], pgx.Identifier{viewSchema, view}.Sanitize()) + } + if err := rows.Err(); err != nil { + return nil, fmt.Errorf("read the views that depend on the columns of %s: %w", pgx.Identifier{schema, table}.Sanitize(), err) + } + return views, nil +} diff --git a/pkg/preflight/dependent_views_integration_test.go b/pkg/preflight/dependent_views_integration_test.go new file mode 100644 index 00000000..c49cf04e --- /dev/null +++ b/pkg/preflight/dependent_views_integration_test.go @@ -0,0 +1,56 @@ +package preflight_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/preflight" +) + +// ColumnDependentViews maps each column to the views and materialized views +// that read it, from any schema. A view that reads the table without naming +// a column, or reads another table, does not count against the column, and +// a column no view reads is absent. +func TestColumnDependentViews(t *testing.T) { + url := testutil.StartPostgres(t) + pool, err := dbconn.NewPool(t.Context(), dbconn.Config{URL: url}) + require.NoError(t, err) + defer pool.Close() + + schema := testutil.NewSchema(t, pool) + other := testutil.NewSchema(t, pool) + _, err = pool.Exec(t.Context(), fmt.Sprintf(` + CREATE TABLE %[1]s.users (id bigint PRIMARY KEY, email text, note text, unread text); + CREATE TABLE %[1]s.teams (id bigint PRIMARY KEY, email text); + CREATE VIEW %[1]s.contacts AS SELECT id, email FROM %[1]s.users; + CREATE MATERIALIZED VIEW %[2]s."Notes" AS SELECT note FROM %[1]s.users WHERE email IS NOT NULL; + CREATE VIEW %[1]s.user_count AS SELECT pg_catalog.count(*) FROM %[1]s.users; + CREATE VIEW %[1]s.team_mail AS SELECT email FROM %[1]s.teams`, schema, other)) + require.NoError(t, err) + + views, err := preflight.ColumnDependentViews(t.Context(), pool, schema, "users") + require.NoError(t, err) + assert.Equal(t, map[string][]string{ + "id": {fmt.Sprintf(`"%s"."contacts"`, schema)}, + "email": sortedQualified([2]string{schema, "contacts"}, [2]string{other, "Notes"}), + "note": {fmt.Sprintf(`"%s"."Notes"`, other)}, + }, views) +} + +// sortedQualified renders the views as ColumnDependentViews orders them: by +// schema, then name, bytewise. +func sortedQualified(views ...[2]string) []string { + if views[0][0] > views[1][0] { + views[0], views[1] = views[1], views[0] + } + out := make([]string, len(views)) + for i, v := range views { + out[i] = fmt.Sprintf(`"%s"."%s"`, v[0], v[1]) + } + return out +} From 7f5473a076fe0cd3dec766f37b5b149ee151cf59 Mon Sep 17 00:00:00 2001 From: Armand Parajon Date: Sat, 17 Oct 2026 11:52:47 -0400 Subject: [PATCH 2/2] fix(planner): judge dependent views by what the change reaches A DROP COLUMN ... CASCADE drops the views that read the column, which the server accepts, so it is no longer refused; it stays destructive. The parser records CASCADE on the operation. A change without ONLY reaches the column on every partition and inheritance child, so a view on one of them blocks it too. The lookup now walks the inheritance tree and reports, per column of the table, the views on the whole tree and on the table alone; a change under ONLY is judged by the table's own. Co-Authored-By: Claude Opus 5.5 --- docs/capabilities.md | 2 +- docs/limitations.md | 2 +- docs/plan-report.md | 2 +- docs/postgres-online-ddl-reference.md | 10 ++- pkg/capabilities/capabilities.yaml | 2 +- pkg/diffplan/diffplan.go | 4 +- .../dependent_view_integration_test.go | 57 +++++++++++++++++ pkg/migrate/facts.go | 4 +- pkg/planner/planner.go | 27 +++++--- pkg/planner/planner_test.go | 38 +++++++++++ pkg/preflight/dependent_views.go | 64 ++++++++++++++----- .../dependent_views_integration_test.go | 14 +++- pkg/statement/ops.go | 6 +- pkg/statement/ops_test.go | 10 +++ 14 files changed, 204 insertions(+), 38 deletions(-) diff --git a/docs/capabilities.md b/docs/capabilities.md index 3db6bcd3..09803591 100644 --- a/docs/capabilities.md +++ b/docs/capabilities.md @@ -203,7 +203,7 @@ review the object warrants) · | `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 | +| `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` | | `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 | diff --git a/docs/limitations.md b/docs/limitations.md index 42a522da..84d78cfa 100644 --- a/docs/limitations.md +++ b/docs/limitations.md @@ -15,7 +15,7 @@ not escape hatches: | `ADD PRIMARY KEY` / `ADD UNIQUE` with a storage parameter the online sequence cannot render | The online sequence builds the key's index with `CREATE UNIQUE INDEX CONCURRENTLY` and carries the key's `WITH (...)` parameters onto it, each name quoted and each value a quoted literal. A parameter it cannot render faithfully, one whose value is not a plain word, number, or string, contains a backslash, or whose name is namespaced, would be dropped or changed, so no sequence is built. Running the statement as written builds the index under `ACCESS EXCLUSIVE`, so it is refused as `rewrite-required` with the `unique-index-then-constraint` guidance. Build the index concurrently with the parameter, then attach it with `ADD CONSTRAINT ... USING INDEX`. | | `ADD FOREIGN KEY ... NOT VALID` on a partitioned parent | PostgreSQL does not support this before version 18, so pg-sprite refuses it on versions 14–17. It is supported on version 18 and later. | | 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. The planner reads those views from the catalog and refuses the change with `dependent-view`, naming each view, instead of letting the statement fail at the server. 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` 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. | | `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/plan-report.md b/docs/plan-report.md index d4dfbe8b..229d132d 100644 --- a/docs/plan-report.md +++ b/docs/plan-report.md @@ -125,7 +125,7 @@ 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, which PostgreSQL refuses 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. | +| `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. | | `unsupported-operation` | The planner does not recognize the operation or knows no safe path for it. | ### Target-dependent refusal reasons (`reason`, `statements[].reason`) diff --git a/docs/postgres-online-ddl-reference.md b/docs/postgres-online-ddl-reference.md index d5b023a5..9a0e0f8b 100644 --- a/docs/postgres-online-ddl-reference.md +++ b/docs/postgres-online-ddl-reference.md @@ -512,9 +512,13 @@ reports it. The operation changes the type of, or drops, a column that a view or materialized view reads. PostgreSQL refuses both while the view exists, even a type change it -would otherwise relabel without a rewrite, so the statement could only fail. 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. +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. 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. diff --git a/pkg/capabilities/capabilities.yaml b/pkg/capabilities/capabilities.yaml index 6a41a422..1472c70b 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" + 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`" - 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)" diff --git a/pkg/diffplan/diffplan.go b/pkg/diffplan/diffplan.go index 8f630ea8..e772ad3e 100644 --- a/pkg/diffplan/diffplan.go +++ b/pkg/diffplan/diffplan.go @@ -99,9 +99,11 @@ func Plan(ctx context.Context, pool *pgxpool.Pool, req Request) (plan.Report, er if facts.AddColumnProofs, err = addColumnProofs(ctx, pool, req.Schema, ds.Table(), changes); err != nil { return plan.Report{}, err } - if facts.DependentViews, err = preflight.ColumnDependentViews(ctx, pool, req.Schema, ds.Table()); err != nil { + views, err := preflight.ColumnDependentViews(ctx, pool, req.Schema, ds.Table()) + if err != nil { return plan.Report{}, err } + facts.DependentViews, facts.OwnDependentViews = views.Tree, views.Own } report.TableExists = &tableExists if report.Statements, report.Disposition, err = classifyChanges(changes, facts); err != nil { diff --git a/pkg/migrate/dependent_view_integration_test.go b/pkg/migrate/dependent_view_integration_test.go index 385e335c..271a2eb8 100644 --- a/pkg/migrate/dependent_view_integration_test.go +++ b/pkg/migrate/dependent_view_integration_test.go @@ -99,6 +99,18 @@ 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", @@ -116,3 +128,48 @@ func TestRunRefusesChangingAColumnAViewReads(t *testing.T) { }) } } + +// A recursive change reaches the column on every table below, so a view on +// an inheritance child refuses the parent's change with the child's view +// named, while the same drop under ONLY, which leaves the child's column in +// place, runs. +func TestRunRefusesChangingAColumnAChildsViewReads(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 varchar(50)); + CREATE TABLE %[1]s.admins (level int) INHERITS (%[1]s.users); + CREATE VIEW %[1]s.admin_mail AS SELECT email, level FROM %[1]s.admins`, schema)) + require.NoError(t, err) + return schema + } + + for _, statement := range []string{ + "ALTER TABLE %s.users ALTER COLUMN email TYPE varchar(100)", + "ALTER TABLE %s.users DROP COLUMN email", + } { + t.Run(statement, func(t *testing.T) { + schema := seed(t) + v, err := migrate.Run(t.Context(), pool, parseOne(t, fmt.Sprintf(statement, schema)), runOptions()) + require.NoError(t, err) + assert.Equal(t, verdict.OutcomeRefused, v.Outcome) + assert.Contains(t, v.Detail, fmt.Sprintf(`a column read by "%s"."admin_mail"`, schema)) + assert.Equal(t, "character varying(50)", columnType(t, pool, schema, "admins", "email")) + }) + } + + 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()) + require.NoError(t, err) + assert.Equal(t, verdict.OutcomeExecuted, v.Outcome, "detail: %s", v.Detail) + assert.False(t, columnExists(t, pool, schema, "users", "email")) + assert.True(t, columnExists(t, pool, schema, "admins", "email")) + }) +} diff --git a/pkg/migrate/facts.go b/pkg/migrate/facts.go index eb8c7c65..2d55d5a5 100644 --- a/pkg/migrate/facts.go +++ b/pkg/migrate/facts.go @@ -62,9 +62,11 @@ func LiveFacts(ctx context.Context, pool *pgxpool.Pool, return Facts{}, err } if st.Kind() == statement.KindAlterTable { - if classifier.DependentViews, err = preflight.ColumnDependentViews(ctx, pool, ResolvedSchema(st), st.Table()); err != nil { + views, err := preflight.ColumnDependentViews(ctx, pool, ResolvedSchema(st), st.Table()) + if err != nil { return Facts{}, err } + classifier.DependentViews, classifier.OwnDependentViews = views.Tree, views.Own } if adds { if classifier.AddColumnProofs, err = preflight.AddColumnRewrites(ctx, pool, ResolvedSchema(st), st.Table(), st.SQL()); err != nil { diff --git a/pkg/planner/planner.go b/pkg/planner/planner.go index 062328b3..4e48bdd3 100644 --- a/pkg/planner/planner.go +++ b/pkg/planner/planner.go @@ -280,10 +280,14 @@ type Facts struct { // column the table already has, which the server skips. AddColumnProofs map[string]statement.AddColumnProof // DependentViews maps a column to the views and materialized views that - // read it (preflight.ColumnDependentViews). A column absent from the map - // has none, or the facts were not read, as in an offline lint; either - // way the server's own refusal at execution remains the backstop. + // read it, on the table or on any table below it + // (preflight.ColumnDependentViews). A column absent from the map has + // none, or the facts were not read, as in an offline lint; either way + // the server's own refusal at execution remains the backstop. DependentViews map[string][]string + // OwnDependentViews is DependentViews for the table alone, which is + // all an ALTER TABLE ONLY changes. + OwnDependentViews map[string][]string } // FactsFrom extracts the facts a live introspection model provides: the @@ -301,13 +305,20 @@ 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. +// 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. func viewsReadingColumn(op statement.Op, facts Facts) []string { - switch op.Kind { - case statement.OpAlterColumnType, statement.OpDropColumn: - return facts.DependentViews[op.Column] - default: + switch { + case op.Kind == statement.OpDropColumn && op.Cascade: return nil + case op.Kind != statement.OpAlterColumnType && op.Kind != statement.OpDropColumn: + return nil + case op.Only: + return facts.OwnDependentViews[op.Column] + default: + return facts.DependentViews[op.Column] } } diff --git a/pkg/planner/planner_test.go b/pkg/planner/planner_test.go index b54b6af3..4ff22719 100644 --- a/pkg/planner/planner_test.go +++ b/pkg/planner/planner_test.go @@ -131,6 +131,9 @@ func TestClassifyRefusesChangingAColumnAViewReads(t *testing.T) { DependentViews: map[string][]string{ "email": {`"app"."active_users"`, `"reports"."emails"`}, }, + OwnDependentViews: map[string][]string{ + "email": {`"app"."active_users"`, `"reports"."emails"`}, + }, } for _, sql := range []string{ "ALTER TABLE t ALTER COLUMN email TYPE varchar(100)", @@ -168,6 +171,41 @@ 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. +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"`}, + "ALTER TABLE t DROP COLUMN code": {`"app"."child_codes"`}, + "ALTER TABLE t ALTER COLUMN code TYPE varchar(10)": {`"app"."child_codes"`}, + } { + plan, err := planner.Classify(sql, facts) + require.NoError(t, err, sql) + require.Len(t, plan.Decisions, 1, sql) + assert.Equal(t, planner.ReasonDependentView, plan.Decisions[0].Reason, sql) + assert.Equal(t, want, plan.Decisions[0].DependentViews, sql) + } + + 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) +} + // facts mirrors a live table whose column types exercise both sides of the // binary-coercible rules. var facts = planner.Facts{ColumnTypes: map[string]string{ diff --git a/pkg/preflight/dependent_views.go b/pkg/preflight/dependent_views.go index 58e632aa..78902c23 100644 --- a/pkg/preflight/dependent_views.go +++ b/pkg/preflight/dependent_views.go @@ -8,45 +8,75 @@ import ( "github.com/jackc/pgx/v5/pgxpool" ) -// ColumnDependentViews maps each column of schema.table that a view or -// materialized view reads to those views, each schema-qualified with every -// identifier quoted, sorted. A column no view reads is absent. +// DependentViews names, per column, the views and materialized views that +// read it, each schema-qualified with every identifier quoted, sorted. A +// column no view reads is absent from both maps. +type DependentViews struct { + // Tree covers the table and every table below it, partitions and + // inheritance children alike: what a recursive ALTER TABLE changes. + Tree map[string][]string + // Own covers the table alone: what an ALTER TABLE ONLY changes. + Own map[string][]string +} + +// ColumnDependentViews reads which views and materialized views read each +// column of schema.table, and of the same-named column of every table +// below it. A column only a table below has is left out: no change to the +// table reaches it. // // PostgreSQL refuses to change the type of a column a view reads, even a // relabel that rewrites nothing, and refuses to drop one without CASCADE. -// The dependency is the one pg_depend records from the view's rewrite rule -// to the column, so a view that reads the table without naming the column -// does not appear. The read is a planning fact: the server's own refusal at -// execution stays the backstop for a view created after it. -func ColumnDependentViews(ctx context.Context, pool *pgxpool.Pool, schema, table string) (map[string][]string, error) { - rows, err := pool.Query(ctx, `SELECT a.attname::text, n.nspname::text, v.relname::text - FROM pg_catalog.pg_depend d +// A recursive change reaches the column on every table below, so a view on +// a child blocks it as surely as one on the table. The dependency is the +// one pg_depend records from the view's rewrite rule to the column, so a +// view that reads the table without naming the column does not appear. The +// read is a planning fact: the server's own refusal at execution stays the +// backstop for a view created after it. +func ColumnDependentViews(ctx context.Context, pool *pgxpool.Pool, schema, table string) (DependentViews, error) { + name := pgx.Identifier{schema, table}.Sanitize() + rows, err := pool.Query(ctx, `WITH RECURSIVE tree(oid, depth) AS ( + SELECT pg_catalog.to_regclass(pg_catalog.quote_ident($1) || '.' || pg_catalog.quote_ident($2))::oid, 0 + UNION + SELECT i.inhrelid, tree.depth + 1 + FROM tree + JOIN pg_catalog.pg_inherits i ON i.inhparent = tree.oid + ) + SELECT a.attname::text, n.nspname::text, v.relname::text, pg_catalog.bool_or(tree.depth = 0) + FROM tree + JOIN pg_catalog.pg_depend d ON d.refobjid = tree.oid JOIN pg_catalog.pg_rewrite r ON r.oid = d.objid JOIN pg_catalog.pg_class v ON v.oid = r.ev_class JOIN pg_catalog.pg_namespace n ON n.oid = v.relnamespace JOIN pg_catalog.pg_attribute a ON a.attrelid = d.refobjid AND a.attnum = d.refobjsubid WHERE d.classid = 'pg_catalog.pg_rewrite'::pg_catalog.regclass AND d.refclassid = 'pg_catalog.pg_class'::pg_catalog.regclass - AND d.refobjid = pg_catalog.to_regclass(pg_catalog.quote_ident($1) || '.' || pg_catalog.quote_ident($2)) AND d.refobjsubid > 0 AND v.oid <> d.refobjid AND v.relkind IN ('v', 'm') + AND EXISTS (SELECT 1 FROM pg_catalog.pg_attribute ra + WHERE ra.attrelid = (SELECT oid FROM tree WHERE depth = 0) + AND ra.attname = a.attname AND ra.attnum > 0 AND NOT ra.attisdropped) GROUP BY a.attname, n.nspname, v.relname ORDER BY a.attname, n.nspname, v.relname`, schema, table) if err != nil { - return nil, fmt.Errorf("read the views that depend on the columns of %s: %w", pgx.Identifier{schema, table}.Sanitize(), err) + return DependentViews{}, fmt.Errorf("read the views that depend on the columns of %s: %w", name, err) } - views := map[string][]string{} + views := DependentViews{Tree: map[string][]string{}, Own: map[string][]string{}} for rows.Next() { var column, viewSchema, view string - if err := rows.Scan(&column, &viewSchema, &view); err != nil { + var own bool + if err := rows.Scan(&column, &viewSchema, &view, &own); err != nil { rows.Close() - return nil, fmt.Errorf("read the views that depend on the columns of %s: %w", pgx.Identifier{schema, table}.Sanitize(), err) + return DependentViews{}, fmt.Errorf("read the views that depend on the columns of %s: %w", name, err) + } + qualified := pgx.Identifier{viewSchema, view}.Sanitize() + views.Tree[column] = append(views.Tree[column], qualified) + if own { + views.Own[column] = append(views.Own[column], qualified) } - views[column] = append(views[column], pgx.Identifier{viewSchema, view}.Sanitize()) } if err := rows.Err(); err != nil { - return nil, fmt.Errorf("read the views that depend on the columns of %s: %w", pgx.Identifier{schema, table}.Sanitize(), err) + return DependentViews{}, fmt.Errorf("read the views that depend on the columns of %s: %w", name, err) } return views, nil } diff --git a/pkg/preflight/dependent_views_integration_test.go b/pkg/preflight/dependent_views_integration_test.go index c49cf04e..2c555903 100644 --- a/pkg/preflight/dependent_views_integration_test.go +++ b/pkg/preflight/dependent_views_integration_test.go @@ -15,7 +15,8 @@ import ( // ColumnDependentViews maps each column to the views and materialized views // that read it, from any schema. A view that reads the table without naming // a column, or reads another table, does not count against the column, and -// a column no view reads is absent. +// a column no view reads is absent. A view on an inheritance child counts +// for the tree, not for the table alone. func TestColumnDependentViews(t *testing.T) { url := testutil.StartPostgres(t) pool, err := dbconn.NewPool(t.Context(), dbconn.Config{URL: url}) @@ -30,7 +31,9 @@ func TestColumnDependentViews(t *testing.T) { CREATE VIEW %[1]s.contacts AS SELECT id, email FROM %[1]s.users; CREATE MATERIALIZED VIEW %[2]s."Notes" AS SELECT note FROM %[1]s.users WHERE email IS NOT NULL; CREATE VIEW %[1]s.user_count AS SELECT pg_catalog.count(*) FROM %[1]s.users; - CREATE VIEW %[1]s.team_mail AS SELECT email FROM %[1]s.teams`, schema, other)) + CREATE VIEW %[1]s.team_mail AS SELECT email FROM %[1]s.teams; + CREATE TABLE %[1]s.admins (level int) INHERITS (%[1]s.users); + CREATE VIEW %[1]s.admin_notes AS SELECT note, level FROM %[1]s.admins`, schema, other)) require.NoError(t, err) views, err := preflight.ColumnDependentViews(t.Context(), pool, schema, "users") @@ -39,7 +42,12 @@ func TestColumnDependentViews(t *testing.T) { "id": {fmt.Sprintf(`"%s"."contacts"`, schema)}, "email": sortedQualified([2]string{schema, "contacts"}, [2]string{other, "Notes"}), "note": {fmt.Sprintf(`"%s"."Notes"`, other)}, - }, views) + }, views.Own) + assert.Equal(t, map[string][]string{ + "id": {fmt.Sprintf(`"%s"."contacts"`, schema)}, + "email": sortedQualified([2]string{schema, "contacts"}, [2]string{other, "Notes"}), + "note": sortedQualified([2]string{schema, "admin_notes"}, [2]string{other, "Notes"}), + }, views.Tree) } // sortedQualified renders the views as ColumnDependentViews orders them: by diff --git a/pkg/statement/ops.go b/pkg/statement/ops.go index ef984af4..b4caf444 100644 --- a/pkg/statement/ops.go +++ b/pkg/statement/ops.go @@ -94,6 +94,10 @@ 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 + // default RESTRICT refuses while any exist. + Cascade bool // Indexes are the unqualified names of the indexes an OpDropIndex // drops, in statement order, so a classifier can match each one // without parsing Name's display form, or the one index an @@ -388,7 +392,7 @@ func alterTableOp(cmd *pganalyze.AlterTableCmd) Op { case pganalyze.AlterTableType_AT_AddColumn: return addColumnOp(cmd.GetDef().GetColumnDef()) case pganalyze.AlterTableType_AT_DropColumn: - return Op{Kind: OpDropColumn, Column: cmd.GetName()} + return Op{Kind: OpDropColumn, Column: cmd.GetName(), Cascade: cmd.GetBehavior() == pganalyze.DropBehavior_DROP_CASCADE} case pganalyze.AlterTableType_AT_AlterColumnType: def := cmd.GetDef().GetColumnDef() name, mods := typeRef(def.GetTypeName()) diff --git a/pkg/statement/ops_test.go b/pkg/statement/ops_test.go index 8b21b389..8842bf0b 100644 --- a/pkg/statement/ops_test.go +++ b/pkg/statement/ops_test.go @@ -106,6 +106,16 @@ func TestParseOpsShapes(t *testing.T) { sql: "ALTER TABLE t DROP COLUMN age", want: statement.Op{Kind: statement.OpDropColumn, Column: "age"}, }, + { + name: "drop column cascade", + sql: "ALTER TABLE t DROP COLUMN age CASCADE", + want: statement.Op{Kind: statement.OpDropColumn, Column: "age", Cascade: true}, + }, + { + name: "drop column restrict", + sql: "ALTER TABLE t DROP COLUMN age RESTRICT", + want: statement.Op{Kind: statement.OpDropColumn, Column: "age"}, + }, { name: "alter type with mods", sql: "ALTER TABLE t ALTER COLUMN name TYPE varchar(100)",