diff --git a/README.md b/README.md index ae51d66..c8119b6 100644 --- a/README.md +++ b/README.md @@ -103,6 +103,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 eef03f0..6000c86 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 64d6dd3..4734d92 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 0bf64db..811cf4d 100644 --- a/docs/capabilities.md +++ b/docs/capabilities.md @@ -209,8 +209,8 @@ 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` | -| `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, 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 | | `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 95f6217..7c32846 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 5b806e1..5e2b3e3 100644 --- a/docs/limitations.md +++ b/docs/limitations.md @@ -18,6 +18,7 @@ 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. Without `ONLY`, a type change reaches the column on every partition and inheritance child too, and a drop reaches every partition and each inheritance child that inherits the column from this one parent without declaring it; a child that declares the column or inherits it from a second parent keeps its copy. The planner reads the views on every copy the change reaches from the catalog and refuses the change with `dependent-view`, naming each view in the plan and in the verdict's `dependent_views`, 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/lint-report.md b/docs/lint-report.md index b709626..ef4a7a9 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 b54ccdc..de0bc90 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 ecc590a..2f78141 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, on the table or, without `ONLY`, on a partition or inheritance child below it whose copy of the column the change reaches: every copy for a type change, and for a drop only the copies the server drops, which leaves out a child that declares the column itself or inherits it from a second parent. 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`) @@ -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 34382c2..a3132af 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. @@ -509,6 +510,29 @@ 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. A +type change without `ONLY` reaches the column on every partition and inheritance +child too, so a view on one of them counts. A drop without `ONLY` reaches every +partition, but only the inheritance children that inherit the column from this one +parent without declaring it: a child that declares the column, or inherits it from +a second parent, keeps its copy, so a view on that copy does not count. The plan +names each view in `decisions[].dependent_views`, and a refused run's verdict in +`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. + ### `unsupported-operation` | Verdict | Lock | Scan / rewrite | Dry-run exit | diff --git a/internal/cli/color_test.go b/internal/cli/color_test.go index 4caa493..1edaec7 100644 --- a/internal/cli/color_test.go +++ b/internal/cli/color_test.go @@ -178,6 +178,7 @@ func fullVerdict(t *testing.T) verdict.Verdict { Detail: "every field populated for the renderer parity lock", SaferIdiom: "DROP INDEX CONCURRENTLY", Guidance: suggest.GuidanceUniqueIndexThenConstraint, + DependentViews: []string{`"public"."v"`}, ExecutedSQL: []string{`ALTER TABLE "t" ADD CONSTRAINT "c" CHECK (x > 0) NOT VALID`}, Forced: true, BlockingPassthrough: true, diff --git a/internal/cli/dryrun_text.go b/internal/cli/dryrun_text.go index fc67f7a..85a31b7 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,21 +337,26 @@ 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 op := refusedOperation(ps); op != "" { - return "the planner knows no safe path for " + op + d, ok := firstRefusal(ps) + if !ok { + return "the planner knows no safe path for this statement" + } + if d.Reason == planner.ReasonDependentView { + 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" } - return "the planner knows no safe path for this statement" + return "the planner knows no safe path for " + d.Operation } -// 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 { +// firstRefusal returns the first decision the planner refused — the same +// decision the run path's refusal verdict explains in its detail. +func firstRefusal(ps plan.Statement) (planner.Decision, bool) { for _, d := range ps.Decisions { if d.Route == planner.RouteRefuse { - return d.Operation + return d, true } } - return "" + return planner.Decision{}, false } // stickyWriter accumulates the first write error so the renderer reads as diff --git a/internal/cli/dryrun_text_test.go b/internal/cli/dryrun_text_test.go index ae2fc44..9d3ec9d 100644 --- a/internal/cli/dryrun_text_test.go +++ b/internal/cli/dryrun_text_test.go @@ -295,6 +295,72 @@ 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") +} + +// The refusal sentence explains the first refused operation, the same one +// the run's verdict explains, even when a later operation is refused +// because views read its column; that operation's views still appear in +// its own diagnostic. +func TestDryRunTextRefusalLeadsWithTheFirstRefusedOperation(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 ref, DROP COLUMN email", + Route: planner.RouteRefuse, + Disposition: router.DispositionRefuse, + Reason: verdict.ReasonUnsupportedStatement, + Class: verdict.ClassCapabilityBoundary, + Decisions: []planner.Decision{ + {Operation: "DROP COLUMN ref", Destructive: true, Route: planner.RouteRefuse, Reason: planner.ReasonReplicaIdentity}, + { + Operation: "DROP COLUMN email", + Destructive: true, + Route: planner.RouteRefuse, + Reason: planner.ReasonDependentView, + DependentViews: []string{`"app"."active_users"`}, + }, + }, + }) + + 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 — the planner knows no safe path for DROP COLUMN ref") + assert.NotContains(t, text, "refused — DROP COLUMN email changes a column read by") + assert.Contains(t, text, "DROP COLUMN email — changes a column a view reads") + assert.Contains(t, text, "help: drop or redefine those views first, then re-plan") +} + // 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 34bac6f..69f99d8 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, 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)" @@ -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 0000000..939ef57 --- /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 6263004..d4fd754 100644 --- a/pkg/diffplan/diffplan.go +++ b/pkg/diffplan/diffplan.go @@ -20,6 +20,7 @@ import ( "errors" "fmt" "maps" + "slices" "github.com/jackc/pgx/v5/pgxpool" @@ -99,6 +100,13 @@ 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 retypesOrDropsColumn(changes) { + views, err := preflight.ColumnDependentViews(ctx, pool, req.Schema, ds.Table()) + if err != nil { + return plan.Report{}, err + } + facts.DependentViews, facts.DropDependentViews, facts.OwnDependentViews = views.Tree, views.Drop, views.Own + } } report.TableExists = &tableExists if report.Statements, report.Disposition, err = classifyChanges(changes, facts); err != nil { @@ -159,6 +167,14 @@ func addColumnProofs(ctx context.Context, pool *pgxpool.Pool, schema, table stri return proofs, nil } +// retypesOrDropsColumn reports whether any change retypes or drops a +// column, the two changes a view reading the column blocks. +func retypesOrDropsColumn(changes []schemadiff.Change) bool { + return slices.ContainsFunc(changes, func(ch schemadiff.Change) bool { + return ch.Kind == schemadiff.ChangeAlterType || ch.Kind == schemadiff.ChangeDropColumn + }) +} + // classifyChanges routes every derived change through the shared // classify-and-route pipeline. facts sharpen type-change classification; // the zero value is valid and strictly more conservative. diff --git a/pkg/lint/lint.go b/pkg/lint/lint.go index e4466e9..6bdb6a6 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 0e09f78..f9736aa 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 0000000..eadb26f --- /dev/null +++ b/pkg/migrate/dependent_view_integration_test.go @@ -0,0 +1,260 @@ +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.Equal(t, []string{ + fmt.Sprintf(`"%s"."Mail Report"`, schema), + fmt.Sprintf(`"%s"."active_users"`, schema), + }, v.DependentViews) + 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 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", + 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) + }) + } +} + +// 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.Equal(t, []string{fmt.Sprintf(`"%s"."admin_mail"`, schema)}, v.DependentViews) + 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")) + }) +} + +// A recursive drop leaves a child's copy of the column in place when the +// child declares the column itself, so a view on that copy does not block +// the drop and the run goes ahead: the parent loses the column, the child +// and its view keep theirs. A type change still reaches the child's copy, +// so the same view refuses it. +func TestRunDropsAColumnAChildDeclaresWhileAViewReadsTheChildsCopy(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 (email varchar(50), 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 + } + + t.Run("the drop runs", func(t *testing.T) { + schema := seed(t) + v, err := migrate.Run(t.Context(), pool, parseOne(t, fmt.Sprintf("ALTER TABLE %s.users DROP COLUMN email", schema)), runOptions()) + 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")) + }) + + t.Run("a type change is refused", func(t *testing.T) { + schema := seed(t) + v, err := migrate.Run(t.Context(), pool, parseOne(t, fmt.Sprintf("ALTER TABLE %s.users ALTER COLUMN email TYPE varchar(100)", schema)), runOptions()) + require.NoError(t, err) + assert.Equal(t, verdict.OutcomeRefused, v.Outcome) + assert.Equal(t, []string{fmt.Sprintf(`"%s"."admin_mail"`, schema)}, v.DependentViews) + assert.Equal(t, "character varying(50)", columnType(t, pool, schema, "admins", "email")) + }) +} + +// The views are read only for a statement that retypes or drops a column, +// the two changes they block; any other ALTER TABLE plans without that +// catalog read. +func TestLiveFactsReadsDependentViewsOnlyForARetypeOrDrop(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 text); + CREATE VIEW %[1]s.active_users AS SELECT id, email FROM %[1]s.users`, schema)) + require.NoError(t, err) + want := []string{fmt.Sprintf(`"%s"."active_users"`, schema)} + + for _, sql := range []string{ + "ALTER TABLE %s.users ADD COLUMN note text", + "ALTER TABLE %s.users ALTER COLUMN email SET NOT NULL", + "ALTER TABLE %s.users RENAME COLUMN email TO mail", + } { + facts, err := migrate.LiveFacts(t.Context(), pool, parseOne(t, fmt.Sprintf(sql, schema))) + require.NoError(t, err, sql) + assert.Nil(t, facts.Classifier.DependentViews, sql) + assert.Nil(t, facts.Classifier.DropDependentViews, sql) + assert.Nil(t, facts.Classifier.OwnDependentViews, sql) + } + for _, sql := range []string{ + "ALTER TABLE %s.users ALTER COLUMN email TYPE varchar(100)", + "ALTER TABLE %s.users ADD COLUMN note text, DROP COLUMN email", + } { + facts, err := migrate.LiveFacts(t.Context(), pool, parseOne(t, fmt.Sprintf(sql, schema))) + require.NoError(t, err, sql) + assert.Equal(t, want, facts.Classifier.DependentViews["email"], sql) + assert.Equal(t, want, facts.Classifier.DropDependentViews["email"], sql) + assert.Equal(t, want, facts.Classifier.OwnDependentViews["email"], sql) + } +} diff --git a/pkg/migrate/facts.go b/pkg/migrate/facts.go index 3386bb8..00555e2 100644 --- a/pkg/migrate/facts.go +++ b/pkg/migrate/facts.go @@ -43,12 +43,14 @@ 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). For one that -// adds an unnamed CHECK or FOREIGN KEY it also reads the constraint names -// the schema already uses, so the safer sequence names the constraint as -// the server would. +// LiveFacts introspects the statement's target table for classifier facts. +// For an ALTER TABLE that retypes or drops a column 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). For one that adds an unnamed CHECK or +// FOREIGN KEY it also reads the constraint names the schema already uses, +// so the safer sequence names the constraint as the server would. func LiveFacts(ctx context.Context, pool *pgxpool.Pool, st statement.Statement) (Facts, error) { if st.Table() == "" { @@ -75,6 +77,17 @@ func LiveFacts(ctx context.Context, pool *pgxpool.Pool, if err != nil { return Facts{}, err } + changes, err := retypesOrDropsColumn(st) + if err != nil { + return Facts{}, err + } + if changes { + views, err := preflight.ColumnDependentViews(ctx, pool, schema, st.Table()) + if err != nil { + return Facts{}, err + } + classifier.DependentViews, classifier.DropDependentViews, classifier.OwnDependentViews = views.Tree, views.Drop, views.Own + } if adds { if classifier.AddColumnProofs, err = preflight.AddColumnRewrites(ctx, pool, schema, st.Table(), st.SQL()); err != nil { return Facts{}, err @@ -153,6 +166,22 @@ func addsColumn(st statement.Statement) (bool, error) { return slices.ContainsFunc(ops, func(op statement.Op) bool { return op.Kind == statement.OpAddColumn }), nil } +// retypesOrDropsColumn reports whether st is an ALTER TABLE that changes a +// column's type or drops a column, the two changes a view reading the +// column blocks. +func retypesOrDropsColumn(st statement.Statement) (bool, error) { + if st.Kind() != statement.KindAlterTable { + return false, nil + } + ops, err := statement.ParseOps(st.SQL()) + if err != nil { + return false, fmt.Errorf("read the operations of %q: %w", st.SQL(), err) + } + return slices.ContainsFunc(ops, func(op statement.Op) bool { + return op.Kind == statement.OpAlterColumnType || op.Kind == statement.OpDropColumn + }), nil +} + // addsUnnamedValidatableConstraint reports whether st is an ALTER TABLE // adding a CHECK or FOREIGN KEY it does not name. func addsUnnamedValidatableConstraint(st statement.Statement) (bool, error) { diff --git a/pkg/migrate/verdicts.go b/pkg/migrate/verdicts.go index 7fb020a..ecad274 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" @@ -157,6 +158,11 @@ 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.DependentViews = d.DependentViews + 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 f224c51..d2a60c2 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 acd357d..b1869c4 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 9 against this test. +// omissions. A consumer pins format_version 10 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": 9, + "format_version": 10, "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": 9, + "format_version": 10, "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 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 8f67b80..3cc0c11 100644 --- a/pkg/planner/planner.go +++ b/pkg/planner/planner.go @@ -83,6 +83,7 @@ func Reasons() []Reason { ReasonRelocation, ReasonPartitionParentLock, ReasonReplicaIdentity, + ReasonDependentView, ReasonUnsupportedOperation, } } @@ -155,6 +156,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" @@ -225,6 +232,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 @@ -252,9 +263,10 @@ type Plan struct { // they are trusted as stated: the CLI fills them by introspecting the // target database, and a library caller may supply facts it already holds — // but they must describe the database the change will run on, because a -// wrong fact can upgrade a rewrite to native. Missing facts are always -// safe: the zero value is valid and classifies strictly more -// conservatively (every type change becomes copy-and-swap). +// wrong fact can upgrade a rewrite to native. Missing facts never let an +// unsafe change succeed: the zero value is valid, routes every type change +// to copy-and-swap, and only defers some refusals, such as a column a view +// reads, to the server's own refusal at execution. type Facts struct { // ColumnTypes maps a column name to its live type as rendered by // PostgreSQL's format_type (e.g. "character varying(50)"). @@ -276,6 +288,20 @@ 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, on the table or on any table below it, which is what a + // recursive ALTER COLUMN TYPE changes + // (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 + // DropDependentViews is DependentViews for the tables whose copy of + // the column a recursive DROP COLUMN drops: a child that declares the + // column itself or inherits it from a second parent keeps its copy. + DropDependentViews map[string][]string + // OwnDependentViews is DependentViews for the table alone, which is + // all an ALTER TABLE ONLY changes. + OwnDependentViews map[string][]string // ConstraintNames are the names of the constraints already in the // table's schema, which the server passes over when it names an // unnamed CHECK or FOREIGN KEY. Without them the safer sequence states @@ -302,6 +328,28 @@ func FactsFrom(live schemadiff.Model) Facts { return Facts{ColumnTypes: types, ColumnCollations: collations, 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, 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, and a recursive drop +// counts only the copies of the column it drops. +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: + return facts.OwnDependentViews[op.Column] + case op.Kind == statement.OpDropColumn: + return facts.DropDependentViews[op.Column] + default: + return facts.DependentViews[op.Column] + } +} + // 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 { @@ -355,6 +403,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 c9f4cef..a5ac839 100644 --- a/pkg/planner/planner_test.go +++ b/pkg/planner/planner_test.go @@ -127,6 +127,109 @@ 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"`}, + }, + DropDependentViews: 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)", + "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) +} + +// 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 recursive drop counts only the copies +// of the column it drops: a view on a child's own declared copy blocks a +// type change, which reaches that copy, and not a drop, which leaves it. +func TestClassifyJudgesDependentViewsByWhatTheChangeReaches(t *testing.T) { + facts := planner.Facts{ + ColumnTypes: map[string]string{"email": "character varying(50)", "code": "text", "tag": "text"}, + DependentViews: map[string][]string{ + "email": {`"app"."active_users"`}, "code": {`"app"."child_codes"`}, "tag": {`"app"."child_tags"`}, + }, + DropDependentViews: 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) + + plan, err = planner.Classify("ALTER TABLE t DROP COLUMN tag", facts) + require.NoError(t, err) + assert.Equal(t, planner.ReasonMetadataOnly, plan.Decisions[0].Reason) + assert.Empty(t, plan.Decisions[0].DependentViews) + + plan, err = planner.Classify("ALTER TABLE t ALTER COLUMN tag TYPE varchar(10)", facts) + require.NoError(t, err) + assert.Equal(t, planner.ReasonDependentView, plan.Decisions[0].Reason) + assert.Equal(t, []string{`"app"."child_tags"`}, plan.Decisions[0].DependentViews) +} + // facts mirrors a live table whose column types exercise both sides of the // binary-coercible rules. var facts = planner.Facts{ColumnTypes: map[string]string{ @@ -601,6 +704,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 0000000..bfc2193 --- /dev/null +++ b/pkg/preflight/dependent_views.go @@ -0,0 +1,117 @@ +package preflight + +import ( + "context" + "fmt" + + "github.com/jackc/pgx/v5" + "github.com/jackc/pgx/v5/pgxpool" +) + +// 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 every map. +type DependentViews struct { + // Tree covers the table and every table below it, partitions and + // inheritance children alike: what a recursive ALTER COLUMN TYPE + // changes. + Tree map[string][]string + // Drop covers the table and the tables below it whose copy of the + // column a recursive DROP COLUMN drops: what that drop changes. + Drop 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. +// A recursive type change reaches the column on every table below, so a +// view on any of them blocks it. A recursive drop reaches less: it drops a +// child's copy only when the child inherits the column from this one +// parent alone and does not also declare it, and descends from there, so a +// child that declared the column, merged it on INHERIT, or inherits it +// from a second parent keeps its copy and its views. Partitions never +// declare a column of their own, so a drop reaches every partition. 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() + // INV: CO-9 — every relation, function, and operator is + // pg_catalog-qualified. + rows, err := pool.Query(ctx, `WITH RECURSIVE tree(oid, depth) AS ( + SELECT pg_catalog.to_regclass(pg_catalog.quote_ident($1) OPERATOR(pg_catalog.||) '.' + OPERATOR(pg_catalog.||) pg_catalog.quote_ident($2))::pg_catalog.oid, 0 + UNION + SELECT i.inhrelid, tree.depth OPERATOR(pg_catalog.+) 1 + FROM tree + JOIN pg_catalog.pg_inherits i ON i.inhparent OPERATOR(pg_catalog.=) tree.oid + ), + dropped(oid, attname) AS ( + SELECT ra.attrelid, ra.attname + FROM tree + JOIN pg_catalog.pg_attribute ra ON ra.attrelid OPERATOR(pg_catalog.=) tree.oid + WHERE tree.depth OPERATOR(pg_catalog.=) 0 + AND ra.attnum OPERATOR(pg_catalog.>) 0 AND NOT ra.attisdropped + UNION + SELECT ca.attrelid, ca.attname + FROM dropped + JOIN pg_catalog.pg_inherits i ON i.inhparent OPERATOR(pg_catalog.=) dropped.oid + JOIN pg_catalog.pg_attribute ca ON ca.attrelid OPERATOR(pg_catalog.=) i.inhrelid + AND ca.attname OPERATOR(pg_catalog.=) dropped.attname + WHERE NOT ca.attisdropped AND NOT ca.attislocal + AND ca.attinhcount OPERATOR(pg_catalog.=) 1 + ) + SELECT a.attname::pg_catalog.text, n.nspname::pg_catalog.text, v.relname::pg_catalog.text, + pg_catalog.bool_or(tree.depth OPERATOR(pg_catalog.=) 0), + pg_catalog.bool_or(EXISTS (SELECT 1 FROM dropped + WHERE dropped.oid OPERATOR(pg_catalog.=) tree.oid + AND dropped.attname OPERATOR(pg_catalog.=) a.attname)) + FROM tree + JOIN pg_catalog.pg_depend d ON d.refobjid OPERATOR(pg_catalog.=) tree.oid + JOIN pg_catalog.pg_rewrite r ON r.oid OPERATOR(pg_catalog.=) d.objid + JOIN pg_catalog.pg_class v ON v.oid OPERATOR(pg_catalog.=) r.ev_class + JOIN pg_catalog.pg_namespace n ON n.oid OPERATOR(pg_catalog.=) v.relnamespace + JOIN pg_catalog.pg_attribute a ON a.attrelid OPERATOR(pg_catalog.=) d.refobjid + AND a.attnum OPERATOR(pg_catalog.=) d.refobjsubid + WHERE d.classid OPERATOR(pg_catalog.=) 'pg_catalog.pg_rewrite'::pg_catalog.regclass + AND d.refclassid OPERATOR(pg_catalog.=) 'pg_catalog.pg_class'::pg_catalog.regclass + AND d.refobjsubid OPERATOR(pg_catalog.>) 0 + AND v.oid OPERATOR(pg_catalog.<>) d.refobjid + AND v.relkind OPERATOR(pg_catalog.=) ANY ('{v,m}'::pg_catalog."char"[]) + AND EXISTS (SELECT 1 FROM dropped + WHERE dropped.oid OPERATOR(pg_catalog.=) (SELECT oid FROM tree WHERE depth OPERATOR(pg_catalog.=) 0) + AND dropped.attname OPERATOR(pg_catalog.=) a.attname) + GROUP BY a.attname, n.nspname, v.relname + ORDER BY a.attname, n.nspname, v.relname`, schema, table) + if err != nil { + return DependentViews{}, fmt.Errorf("read the views that depend on the columns of %s: %w", name, err) + } + views := DependentViews{Tree: map[string][]string{}, Drop: map[string][]string{}, Own: map[string][]string{}} + for rows.Next() { + var column, viewSchema, view string + var own, drop bool + if err := rows.Scan(&column, &viewSchema, &view, &own, &drop); err != nil { + rows.Close() + 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 drop { + views.Drop[column] = append(views.Drop[column], qualified) + } + if own { + views.Own[column] = append(views.Own[column], qualified) + } + } + if err := rows.Err(); err != nil { + 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 new file mode 100644 index 0000000..d55ad21 --- /dev/null +++ b/pkg/preflight/dependent_views_integration_test.go @@ -0,0 +1,142 @@ +package preflight_test + +import ( + "context" + "fmt" + "testing" + + "github.com/jackc/pgx/v5/pgconn" + "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. 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}) + 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; + 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") + 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.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) + assert.Equal(t, views.Tree, views.Drop, "admins inherits every column from users alone, so a drop reaches it") +} + +// A recursive DROP COLUMN drops a child's copy only when the child +// inherits the column from this one parent and does not declare it, and +// descends only through the copies it drops; a type change reaches every +// copy. The server is the oracle: with the views Drop names removed the +// drop succeeds, so no other view blocks it, and each view Drop names +// blocks it on its own. +func TestColumnDependentViewsForADrop(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 text); + CREATE TABLE %[1]s.inherited () INHERITS (%[1]s.users); + CREATE TABLE %[1]s.below_inherited () INHERITS (%[1]s.inherited); + CREATE TABLE %[1]s.declared (email text) INHERITS (%[1]s.users); + CREATE TABLE %[1]s.below_declared () INHERITS (%[1]s.declared); + CREATE TABLE %[1]s.other_parent (email text); + CREATE TABLE %[1]s.two_parents () INHERITS (%[1]s.users, %[1]s.other_parent); + CREATE TABLE %[1]s.attached (id bigint NOT NULL, email text); + ALTER TABLE %[1]s.attached INHERIT %[1]s.users; + CREATE VIEW %[1]s.v_users AS SELECT email FROM ONLY %[1]s.users; + CREATE VIEW %[1]s.v_inherited AS SELECT email FROM ONLY %[1]s.inherited; + CREATE VIEW %[1]s.v_below_inherited AS SELECT email FROM %[1]s.below_inherited; + CREATE VIEW %[1]s.v_declared AS SELECT email FROM ONLY %[1]s.declared; + CREATE VIEW %[1]s.v_below_declared AS SELECT email FROM %[1]s.below_declared; + CREATE VIEW %[1]s.v_two_parents AS SELECT email FROM %[1]s.two_parents; + CREATE VIEW %[1]s.v_attached AS SELECT email FROM %[1]s.attached; + CREATE TABLE %[1]s.events (id bigint, email text) PARTITION BY RANGE (id); + CREATE TABLE %[1]s.events_1 PARTITION OF %[1]s.events FOR VALUES FROM (0) TO (10); + CREATE VIEW %[1]s.v_events_1 AS SELECT email FROM %[1]s.events_1`, schema)) + require.NoError(t, err) + qualified := func(names ...string) []string { + out := make([]string, len(names)) + for i, name := range names { + out[i] = fmt.Sprintf(`"%s"."%s"`, schema, name) + } + return out + } + + views, err := preflight.ColumnDependentViews(t.Context(), pool, schema, "users") + require.NoError(t, err) + assert.Equal(t, qualified("v_attached", "v_below_declared", "v_below_inherited", "v_declared", + "v_inherited", "v_two_parents", "v_users"), views.Tree["email"]) + assert.Equal(t, qualified("v_below_inherited", "v_inherited", "v_users"), views.Drop["email"]) + assert.Equal(t, qualified("v_users"), views.Own["email"]) + + dropFails := func(t *testing.T, keep string) error { + t.Helper() + tx, err := pool.Begin(t.Context()) + require.NoError(t, err) + defer func() { _ = tx.Rollback(context.WithoutCancel(t.Context())) }() + for _, view := range views.Drop["email"] { + if view != keep { + _, err := tx.Exec(t.Context(), "DROP VIEW "+view) + require.NoError(t, err) + } + } + _, err = tx.Exec(t.Context(), fmt.Sprintf("ALTER TABLE %s.users DROP COLUMN email", schema)) + return err + } + require.NoError(t, dropFails(t, ""), "no view outside Drop blocks the drop") + for _, view := range views.Drop["email"] { + var pgErr *pgconn.PgError + require.ErrorAs(t, dropFails(t, view), &pgErr, view) + assert.Equal(t, "2BP01", pgErr.Code, view) + } + + partitioned, err := preflight.ColumnDependentViews(t.Context(), pool, schema, "events") + require.NoError(t, err) + assert.Equal(t, qualified("v_events_1"), partitioned.Drop["email"], "a partition never declares a column") +} + +// 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 +} diff --git a/pkg/statement/ops.go b/pkg/statement/ops.go index 66f51dc..1307a72 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 @@ -398,7 +402,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 8fea838..60df78a 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)", diff --git a/pkg/verdict/verdict.go b/pkg/verdict/verdict.go index 69dffd6..350d816 100644 --- a/pkg/verdict/verdict.go +++ b/pkg/verdict/verdict.go @@ -535,6 +535,12 @@ type Verdict struct { // carries. Detail states the steps for a human; Guidance is the key an // automated caller branches on. Empty for every other verdict. Guidance suggest.Guidance `json:"guidance,omitempty"` + // DependentViews names the views and materialized views that read the + // column a refused statement retypes or drops, each schema-qualified + // with every identifier quoted, sorted: the objects to drop or redefine + // before the change can run. The same list the plan report's + // dependent-view decision carries. Empty for every other verdict. + DependentViews []string `json:"dependent_views,omitempty"` // ExecutedSQL is the ordered SQL the engine actually ran and committed. // On an executed verdict it is the substituted safer native sequence // (empty when the submitted form ran as-is — a non-empty value is what