feat(planner): refuse retyping or dropping a column a view reads - #223

🤖 Review findings - created by Kiran's code review agent - for pg-sprite/pull/223, e35b869.

Verdict: 6 findings — 0 blocking, 2 non-blocking (a recursive DROP COLUMN is wrongly refused when a child's column survives the drop), 4 suggestions.

Non-blocking

1. A recursive DROP COLUMN is refused over views on child columns that PostgreSQL would keep. See pkg/preflight/dependent_views.go:40-42. The tree query (JOIN pg_catalog.pg_inherits i ON i.inhparent = tree.oid) counts every child's same-named column and never checks attislocal or attinhcount. PostgreSQL's ATExecDropColumn only drops a child's copy when attinhcount == 1 && !attislocal; otherwise it keeps the column. Example: CREATE TABLE admins (email text, level int) INHERITS (users) with a view admin_mail reading admins.email. Then ALTER TABLE users DROP COLUMN email succeeds on the server, but viewsReadingColumn returns Tree["email"] and the planner refuses it with ReasonDependentView.

   • This fails closed: it is a false refusal, not an unsafe run, and ONLY works around it.
   • Partitions are not affected, because their columns are never local.
   • ALTER COLUMN TYPE is not affected, because it always recurses.

2. The docs and tests say the drop reaches every child, which is wrong for merged or multiply-inherited children. The function comment at pkg/preflight/dependent_views.go:29-30 says "A recursive change reaches the column on every table below". docs/limitations.md:21 and plan-report.md say the same. Both are wrong for DROP COLUMN on a child that declares the column locally, attaches later with ALTER TABLE ... INHERIT, or inherits it from two parents. TestRunRefusesChangingAColumnAChildsViewReads only covers a child that inherits email without declaring it, so no test pins the false-refusal case.

General suggestions

3. Dry-run can name a different refusal cause than the real run. See internal/cli/dryrun_text.go:340. if d, ok := dependentViewRefusal(ps); ok { picks a dependent-view decision wherever it sits in the statement. The run path's routeRefusalVerdict (pkg/migrate/verdicts.go:151-165) reports the first refused decision and breaks. Example: for ALTER TABLE t DROP COLUMN ref, DROP COLUMN email (ref in the replica identity index, a view reading email), the run reports replica identity but dry-run leads with the view. That contradicts the "mirroring the run path" doc comment. Every diagnostic still prints, so only the leading sentence differs.

4. The Facts doc still promises that missing facts are always more conservative. pkg/planner/planner.go:265-267 still says the zero value "classifies strictly more conservatively". A nil DependentViews map (pkg/planner/planner.go:282-287) instead lets a retype or drop of a column a view reads classify as native or destructive. Possible rewording: "missing facts never make an unsafe change succeed; some only defer a refusal to the server".

5. The dependent-view query runs for every ALTER TABLE, even ones that never use it. See pkg/migrate/facts.go:79. if st.Kind() == statement.KindAlterTable { runs the recursive catalog query for every ALTER TABLE, including ADD COLUMN, SET NOT NULL and ADD CONSTRAINT, where viewsReadingColumn returns nil. The neighbouring fact reads are gated on the ops that need them (addsColumn, addsUnnamedValidatableConstraint). pkg/diffplan/diffplan.go:102 has the same issue. A timeout on this unused query aborts the plan.

6. On the run path, the dependent views appear only in prose. See pkg/migrate/verdicts.go:161. The migrate.Run refusal is the generic CapabilityBoundary(ReasonUnsupportedStatement), and the view names go only into v.Detail; the typed dependent_views field exists only in the plan report. AGENTS.md asks for a typed field before behaviour shows only in prose, and the new integration tests assert on Detail wording. This follows the existing replica-identity precedent.

The one thing that could have broken, verified

Could --force, or an op the check exempts, let a retype or drop of a column a view reads reach execution?

• No. DispositionRefuse ignores --force (pkg/migrate/migrate.go:292-296).
• viewsReadingColumn exempts only DROP COLUMN ... CASCADE and RENAME. RESTRICT parses as Cascade=false, and ops_test covers it.
• ONLY switches the check to the table's own views rather than skipping it.
• The remaining gaps (missing facts, a view created after planning) leave the server's own refusal as the backstop.

Verified correct

• Every site that handles ReasonReplicaIdentity also handles ReasonDependentView: Reasons(), impactText, the routeRefusalVerdict detail, and the pinned vocabulary test.
• The dependent-view check runs after the replica-identity check and before the per-kind switch, so a refused decision keeps the Destructive flag from destructiveOp.
• Child columns are matched by name, not attnum, so partitions with different attnums are handled.
• The relkind IN ('v','m') filter excludes rules on ordinary tables, and v.oid <> d.refobjid excludes self-references.
• ORDER BY sorts on name-typed columns (C collation), so the order is bytewise and matches the sortedQualified test helper.
• View names go through pgx.Identifier.Sanitize, as the repo convention requires.
• A missing table gives a NULL from to_regclass and no rows, and LiveFacts returns earlier on ErrTableNotFound anyway.
• The removed and changed lines are only format-version literals, doc wording, and the OpDropColumn construction, which now also records Cascade.

No local builds or tests were run; this review is source inspection only. CI at this head is fully green (16/16).

This review was generated by Claude Code (claude-opus-5-5).

🤖 Thanks, all six are addressed in 48ce870.

1. False refusal on a recursive drop. ColumnDependentViews now also returns Drop, the views on the column copies a recursive DROP COLUMN actually drops. A recursive CTE starts from the root's columns and descends only through children whose copy has attinhcount = 1 AND NOT attislocal, which is what ATExecDropColumn does. viewsReadingColumn reads DropDependentViews for a recursive drop, Own for ONLY, and Tree for a type change. Your admins example now runs. TestRunDropsAColumnAChildDeclaresWhileAViewReadsTheChildsCopy pins it, and a mutation that reads Tree for the drop fails that test.
2. Docs and tests on drop reach. The function comment, limitations.md, plan-report.md and the online DDL reference now say that a type change reaches every table below, while a drop skips a child that declared the column, merged it on INHERIT, or inherits it from a second parent. TestColumnDependentViewsForADrop uses the server as the oracle: inherited, declared, attached, two-parent and partition children, plus a grandchild under each of the first two. It asserts Drop holds exactly the views a real DROP COLUMN ... CASCADE removes.
3. Dry-run cause. refuseText now leads with the first refused decision (firstRefusal), the same one routeRefusalVerdict reports, and names views only when that decision is the dependent-view one. TestDryRunTextRefusalLeadsWithTheFirstRefusedOperation covers your replica identity plus view example.
4. Facts doc. It now reads: "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."
5. Unused query. Both migrate and diffplan now read dependent views only when the statement retypes or drops a column (retypesOrDropsColumn). TestLiveFactsReadsDependentViewsOnlyForARetypeOrDrop pins the gate.
6. Typed field on the run path. The verdict now carries dependent_views, the same sorted, quoted list as the plan report's decision. The integration tests assert on that field instead of the Detail wording, and the renderer parity fixture covers it.

Addressed by Claude Code

🤖 Reviewed at head 48ce870607aadc32a78677e2b9910b652d9c13fd on Jash's behalf, scoped to the delta above #222.

Verdict: one correctness gap remains in recursive DROP COLUMN dependency discovery. PostgreSQL still rejects the unsafe drop; the gap makes the dry-run miss a blocker that this change intends to identify.

1. [P2, blocker] Account for multiple inheritance paths from the same dropped ancestor. In pkg/preflight/dependent_views.go:68–69, dropped excludes every child whose initial attinhcount exceeds one. Consider root(email text), two children branch_a and branch_b inheriting root, and leaf inheriting both branches without declaring email locally. A view selecting leaf.email blocks ALTER TABLE root DROP COLUMN email: PostgreSQL's ATExecDropColumn decrements leaf's inheritance count through the first branch, then schedules its column for deletion through the second. This query sees the original count of two and omits the view from Drop, so the planner reports a native drop instead of dependent-view. The existing two-parent fixture has an independent surviving parent and therefore does not exercise this case. This touches core preflight; the observed risk is a missed planning refusal and subsequent server error, not data corruption.

Action item

1. Model whether all inherited sources of a nonlocal column are removed by the recursive drop, including converging paths. Add a diamond-inheritance regression with a dependent view, asserting both its inclusion in Drop and PostgreSQL's refusal; update the documentation's blanket claim that a second parent always preserves the column.

OSS and integration: naming the blocking views and explaining the remedy improves standalone usability. Both front doors populate the same typed facts, the verdict now exposes dependent_views, and plan/lint versions 10/4 make the contract change explicit for consumers. No orchestrator-specific dependency is introduced.

Validation: inspected the full delta and exact-head surrounding source, including classification, both fact-loading paths, renderers, tests, and the PostgreSQL deletion algorithm. The previous review's local-column, redundant-query, first-refusal, and typed-verdict concerns are addressed. CI is successful across the reported checks, including PostgreSQL 14–18. I did not run local builds or database tests; the diamond finding is source-derived and needs the regression above.

Review generated by Codex.

🤖 Thanks, you're right about the diamond. Fixed in 50c24fd.

• Fix: the attinhcount = 1 filter is gone. ColumnDependentViews now reads every table in the hierarchy in one statement: each table's parents, and each column's attislocal, attinhcount and the views that read it. droppedCopies then replays ATExecDropColumn's walk. Each dropped table visits its children, and a child's copy is dropped on the visit that brings its count to its last parent, unless the child declares the column. Otherwise the visit only lowers the count. Since the recursive CTE can't ask whether every parent was dropped, this decision now lives in Go.
• Regression test: TestColumnDependentViewsForADrop adds your diamond (branch_a, branch_b, diamond) and an uneven one, where uneven inherits from below_inherited (two levels down) and from branch_b. It asserts that v_diamond and v_uneven are in Drop. The server is the oracle: the real DROP COLUMN succeeds once every view in Drop is removed, and keeping any single one, v_diamond included, fails with 2BP01. Swapping the replay back to the one-parent rule makes the test fail on v_diamond.
• Docs: the function comment, limitations.md, plan-report.md, the online DDL reference and the DropDependentViews doc no longer say a second parent always preserves the column. They now say a child keeps its copy when it declares the column or also inherits it from a table outside the drop, and loses it when it inherits along two paths from the dropped table.

Addressed by Claude Code