fix: detect and apply GENERATED ALWAYS AS changes on existing columns (#591)

[tianzhou]

Summary

columnsEqual compared name, type, nullability, default, max length, identity and comment, but never the generation clause. Changing the expression of an existing GENERATED ALWAYS AS (...) column, or switching a column between plain and generated, produced an empty plan, so every existing database silently kept the old expression.

The generation clause (IsGenerated, GeneratedKind, GeneratedExpr) is now part of the comparison, and the change is applied with the narrowest DDL PostgreSQL offers:

Change	DDL
expression change, PostgreSQL 17+	ALTER COLUMN ... SET EXPRESSION AS (...)
STORED -> plain	ALTER COLUMN ... DROP EXPRESSION (a following SET DEFAULT etc. applies as usual)
plain -> generated, STORED <-> VIRTUAL, VIRTUAL -> plain, expression change on PostgreSQL 14-16	DROP COLUMN + ADD COLUMN

PostgreSQL has no ALTER form for the last row (DROP EXPRESSION is rejected for VIRTUAL columns, verified on 18). Since a generated column holds no data of its own, re-creating it loses nothing. The indexes and constraints that DROP COLUMN takes with it are re-created from the desired state, and foreign keys bound to a replaced unique/PK constraint go through the existing #439 pre-drop/post-add path.

To pick the right form, the plan command now passes the target's major version into the diff via the new diff.GenerateMigrationForTarget; GenerateMigration and GenerateMigrationWithOptions are unchanged (version 0 = assume a current server).

Fixes #591

Test plan

• testdata/diff/create_table/issue_591_alter_generated_column (PG18, one apply cycle) covers, grouped by scenario: a STORED expression change with a dependent index; STORED -> plain with a new default; plain -> STORED with a check constraint, unique constraint bound by an FK, and an expression/partial index; VIRTUAL expression change, VIRTUAL -> plain, STORED -> VIRTUAL; and a re-created column with dependent views (including one whose old definition reads it), a policy, a trigger, column and view grants, an expression EXCLUDE constraint, a standalone unique index bound by FKs (existing and newly added), and a same-named index moving onto the column. The core scenarios produced an empty plan before the fix.
• internal/diff/generated_column_test.go: version gate (SET EXPRESSION on 0/17/18, DROP + ADD + index re-creation on 14/16) and the expression/column reference matcher.
• Verified end to end on embedded PostgreSQL 16 and 18 with a populated row: migration applies, values are recomputed, and a second plan is empty.

PGSCHEMA_TEST_FILTER="create_table/issue_591_" go test ./internal/diff -run TestDiffFromFiles
PGSCHEMA_TEST_FILTER="create_table/issue_591_" go test ./cmd -run TestPlanAndApply
go test ./internal/diff -run 'TestGeneratedExpressionChange_VersionGate|TestExprReferencesAnyColumn'
go test ./internal/diff   # full diff suite, no regressions

🤖 Generated with Claude Code

[greptile-apps]

Greptile Summary

This PR detects generated-column definition changes, gates PostgreSQL-version-specific alteration syntax, and recreates columns and selected dependents when no in-place DDL exists. The recreation path is incomplete for existing data and several dependent schema properties:

• VIRTUAL-to-plain recreation discards existing computed values.
• Unchanged column grants and dependent views are not preserved.
• Same-named index transitions and quoted identifiers can leave indexes missing or stale.

Confidence Score: 0/5

This PR is not safe to merge because generated-column recreation can lose existing values and privileges, fail on unchanged dependent views, and leave indexes missing or stale.

Five independent migration failures remain in the new recreation path: destructive VIRTUAL-to-plain conversion, omitted column grants, unhandled unchanged views, incorrect same-named index handling, and false-negative dependency matching for quoted identifiers.

Files Needing Attention: internal/diff/column.go, internal/diff/table.go, internal/diff/diff.go

Important Files Changed

Filename	Overview
internal/diff/column.go	Adds generation-clause comparison and version-aware alteration/recreation selection, but VIRTUAL-to-plain recreation does not preserve existing computed values.
internal/diff/table.go	Recreates generated columns and selected constraints/indexes, but misses grants, unchanged views, an asymmetric index transition, and quoted identifiers containing embedded quotes.
internal/diff/diff.go	Propagates the target version into table diffing, while global privilege and view lifecycle logic remains disconnected from recreated columns.
cmd/plan/plan.go	Extracts the inspected target major version and passes it into migration generation.
internal/diff/generated_column_test.go	Covers version-gated expression changes and basic expression matching but omits populated VIRTUAL-to-plain transitions and uncovered dependent-object cases.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
  A[Generated clause changed] --> B{Supported in-place ALTER?}
  B -->|Yes| C[SET or DROP EXPRESSION]
  B -->|No| D[DROP COLUMN]
  D --> E[ADD COLUMN]
  E --> F[Restore selected constraints and indexes]
  D -. currently uncovered .-> G[Preserve existing virtual values]
  D -. currently uncovered .-> H[Pre-drop and recreate unchanged views]
  E -. currently uncovered .-> I[Restore unchanged column grants]
  F -. matcher/name gaps .-> J[Missing or stale indexes]

Loading

_{Reviews (1): Last reviewed commit: "fix: detect and apply GENERATED ALWAYS A..." | Re-trigger Greptile}

[Copilot]

🟡 Changes recommended

Five critical dependency, index, identifier, and privilege-handling issues must be resolved before approval.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Adds detection and migration support for changes to existing generated columns, including PostgreSQL version-specific DDL and dependency recreation.

Changes:

• Compares generated-column kind and expression during schema diffing.
• Uses SET/DROP EXPRESSION where supported; otherwise recreates columns and dependencies.
• Adds STORED/VIRTUAL fixtures and version-gating tests.
• Introduces target-version-aware migration generation.

Five critical issues remain in internal/diff/table.go:

• Recreated columns do not account for unchanged dependent views and transitive dependents.
• Index recreation checks the new index instead of the old index, potentially preserving stale definitions.
• Foreign keys backed by standalone unique indexes are not included in dependency recreation.
• Quoted identifiers containing embedded quotes are not matched correctly.
• Column-level privileges are lost when columns are dropped and re-added.

File summaries

File	Description
testdata/diff/create_table/issue_591_alter_generated_virtual/plan.txt	Expected textual VIRTUAL-column plan.
testdata/diff/create_table/issue_591_alter_generated_virtual/plan.sql	Expected SQL VIRTUAL-column plan.
testdata/diff/create_table/issue_591_alter_generated_virtual/plan.json	Expected JSON VIRTUAL-column plan.
testdata/diff/create_table/issue_591_alter_generated_virtual/old.sql	Original VIRTUAL-column schema.
testdata/diff/create_table/issue_591_alter_generated_virtual/new.sql	Desired VIRTUAL-column schema.
testdata/diff/create_table/issue_591_alter_generated_virtual/diff.sql	Expected VIRTUAL-column migration DDL.
testdata/diff/create_table/issue_591_alter_generated_column/plan.txt	Expected textual STORED-column plan.
testdata/diff/create_table/issue_591_alter_generated_column/plan.sql	Expected SQL STORED-column plan.
testdata/diff/create_table/issue_591_alter_generated_column/plan.json	Expected JSON STORED-column plan.
testdata/diff/create_table/issue_591_alter_generated_column/old.sql	Original STORED-column schema.
testdata/diff/create_table/issue_591_alter_generated_column/new.sql	Desired STORED-column schema.
testdata/diff/create_table/issue_591_alter_generated_column/diff.sql	Expected STORED-column migration DDL.
internal/diff/table.go	Recreates generated columns and dependent schema objects.
internal/diff/generated_column_test.go	Tests version gating and expression matching.
internal/diff/diff.go	Adds target-version-aware migration generation.
internal/diff/column.go	Detects and emits generated-column changes.
cmd/plan/plan.go	Passes the target PostgreSQL version into diff generation.

Review details

• Files reviewed: 17/17 changed files
• Comments generated: 5
• Review effort level: Balanced

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[tianzhou]

Follow-up in 7ab3aa6 addressing the review and the CI failure.

CI (dependency/table_fk_to_generated_column idempotency): now that the generation expression is compared, a pre-existing normalization gap surfaced. pg_get_expr qualifies a same-schema function depending on the inspecting session's search_path, so the current state read public.calc_priority() while the desired state (normalized through the temp-schema rename) read calc_priority(), and the second plan re-emitted SET EXPRESSION AS. The IR normalizer now strips same-schema qualifiers from GeneratedExpr, as it already does for defaults and index expressions. That case's diff.sql changes accordingly; dump output is unaffected (the only dump golden with a qualified call in a generated expression is a cross-schema one).

Review findings, all fixed except one:

• Unchanged views reading a re-created column go through the existing pre-drop/recreate cycle with transitive dependents.
• Column grants touching a re-created column are re-issued after ADD COLUMN.
• FKs bound to a standalone unique index on the column take the Dropping constraint cascade #439 pre-drop/post-add path.
• The implicit-drop decision for a same-named index inspects the old definition.
• Quoted identifiers with embedded quotes match in their "a""b" form.
• Not changed: VIRTUAL -> plain. PostgreSQL refuses DROP EXPRESSION on VIRTUAL columns because nothing is stored, so replacement is the only schema-level path; the plan shows it as drop + add, and backfilling the last computed values would be a data migration outside the schema plan.

New case create_table/issue_591_recreate_generated_dependents exercises all of the fixed scenarios in one apply cycle. Full diff suite, the three issue cases plus the dependency case through TestPlanAndApply (apply + idempotency), and ./internal/plan pass locally.

[Copilot]

🟡 Changes recommended

Two critical dependency-handling defects and three moderate object-recreation defects remain unresolved.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (1)

internal/diff/diff.go:1015

• When dependsOnRecreated is the only change, this viewDiff contains no trigger additions. The pre-drop removes every INSTEAD OF trigger on the view, and the root-view recreation path only recreates entries in AddedTriggers, so unchanged desired triggers silently disappear. Populate the recreation diff with all desired triggers (including disabled state/comments), as is already done for transitively recreated views.

					diff.modifiedViews = append(diff.modifiedViews, &viewDiff{
						Old:              oldView,
						New:              newView,
						RequiresRecreate: true,
					})

• Files reviewed: 25/25 changed files
• Comments generated: 4
• Review effort level: Balanced

[tianzhou]

Follow-up in 1a7365b for the second review round.

• Views: dependency detection uses the old definition, so a view whose new definition no longer reads the column is still pre-dropped.
• Policies and triggers: expressions naming a re-created column (USING / WITH CHECK, WHEN, UPDATE OF) block DROP COLUMN with 2BP01, verified by probe on PG18. Policies are dropped ahead of the column and triggers in the drop phase; both are created again from the desired state.
• View grants: privileges on recreated views (including transitive dependents) are re-issued after recreation.
• $ is treated as an identifier character in the expression matcher.

On the suppressed comment about INSTEAD OF triggers: the recreate path already creates every desired trigger from diff.New.Triggers after CREATE VIEW (see generateModifyViewsSQL), and it restores the comment and materialized-view indexes the same way, so nothing is lost there.

create_table/issue_591_recreate_generated_dependents now exercises all of these in one apply cycle; apply and idempotency pass on PG18 along with the full diff suite.

[Copilot]

🟡 Changes recommended

Multiple critical dependency-handling defects can produce failed or incorrect migrations.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

• Files reviewed: 25/25 changed files
• Comments generated: 6
• Review effort level: Balanced

[tianzhou]

Follow-up in 68ef6b7 for the third review round.

Fixed

• Indexes attributed to a re-created column are dropped explicitly (DROP INDEX IF EXISTS, a no-op when the column drop already removed them) and re-created, so a false positive of the textual dependency check can no longer leave a stale index behind. a::b is recognized as a type cast.
• Grouped column grants: the old grant stays in the comparison so removals on surviving columns are revoked; the desired grant on the re-created column is re-issued after the column is back.

Not changed, with reasons on the threads

• Dependent-view closure from desired definitions: existing behaviour of every view recreation; a change to that machinery belongs in its own PR.
• Partition children: diffTables already emits per-child column DDL for any column change on main (verified with a plain DROP COLUMN case), so this is a general gap to track separately.
• SQL-standard routines depending on a column: affects every DROP COLUMN the engine emits; fails loudly at apply; no routine dependency tracking exists today.
• Ordinal position: pgschema ignores column order everywhere already; the plan shows the replacement explicitly.

Apply and idempotency pass on PG18 for the issue cases and the dependency case, along with the full diff suite.

[Copilot]

🟡 Changes recommended

Quoted table names containing embedded quotes can evade view dependency detection and make column recreation fail.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

• Files reviewed: 25/25 changed files
• Comments generated: 1
• Review effort level: Balanced

[tianzhou]

Follow-up in 02eb0eb for the fourth review round.

The quoted-table-name miss was in containsIdentifier, which predates this PR and backs every view-dependency check in the engine. Instead of patching one call site, both textual matchers (containsIdentifier for relations, exprReferencesAnyColumn for columns) now share a single identifierRegexp in internal/diff/identifier_match.go:

• bare and quoted spellings of every name segment, with embedded quotes doubled as the deparsers render them;
• schema.name matched segment by segment, so quoted schemas and names work and other.public.users does not match public.users;
• identifier boundaries that include $; string literals stripped before matching;
• column mode additionally excludes function calls (name() and type casts (::name);
• compiled patterns cached.

Future textual dependency checks should build on identifierRegexp rather than a local regex, so the quoting and boundary rules stay in one place. The full diff suite passes with the view-dependency paths now on the shared matcher.

[Copilot]

🟡 Changes recommended

Generated-column rewrites still risk failed or destructive migrations, stale stored values, and missed dependencies for valid quoted identifiers.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (2)

Previously missed (2) — in code that hasn't changed since the last review.

internal/diff/column.go:74

• Generated-column rewrites run during generateModifyTablesSQL, before generateModifyFunctionsSQL. If this expression calls an immutable function whose body is also changed in the same plan, SET EXPRESSION (and the pre-17 drop/add path) computes stored values using the old function; the function is replaced afterward, and a second plan reports no drift although existing rows are stale. Schedule the relevant function modification before the column rewrite, or force another recomputation afterward.
  ir/normalize.go:198
• StripSchemaPrefixFromBody only recognizes the literal unquoted prefix schema., but pg_get_expr quotes mixed-case, reserved, or otherwise non-simple schema names (for example, "My Schema".calc(a)). The current-state expression therefore keeps that qualifier while the temporary-schema expression is stripped, producing a perpetual generated-column diff and repeated rewrites for supported quoted schema names. Normalize the quoted quote_ident(schema). form as well, with proper embedded-quote handling.

• Files reviewed: 28/28 changed files
• Comments generated: 3
• Review effort level: Balanced

[tianzhou]

Follow-up in eb21036 for the fifth review round.

Fixed

• Quoted schema names (suppressed note on ir/normalize.go): StripSchemaPrefixFromBody now also strips the quote_ident form ("My Schema".calc(a) -> calc(a)), so a generated column in such a schema no longer compares different on every plan. Index expressions and function bodies get the same normalization; TestStripSchemaPrefixFromBody_QuotedSchema covers it.
• A column named a.b is matched as one identifier again in containsIdentifier, with schema.name matching as the fallback.
• viewDependsOnRecreatedColumn reads schema and table from the table diffs instead of parsing a joined key.

Not changed

• VIRTUAL -> plain with NOT NULL: same as adding any NOT NULL column without a default, which pgschema already emits and documents as a two-plan workflow; the plan shows the replacement.
• Function body changed in the same plan as a generated expression that calls it (suppressed note on column.go): stored values computed before the function change are no staler than today, where a function change alone never recomputes them and PostgreSQL does not either. Reordering functions ahead of tables would break routines that reference columns added in the same plan.

Full diff suite, dump suite, ir tests, and apply/idempotency for the issue cases pass locally.

[Copilot]

🟡 Changes recommended

Generated-expression normalization and dependency ordering can emit incorrect or failing migrations and omit dependent constraints.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

• Files reviewed: 29/29 changed files
• Comments generated: 4
• Review effort level: Balanced

[tianzhou]

Follow-up in 8d4ed70 for the sixth review round.

Fixed

• Expression-only EXCLUDE constraints on a re-created column are detected from the definition text and dropped before / re-added after the column.
• Foreign keys newly added to existing tables that target a rebuilt unique index are deferred to the post-add step (normal and inline emission suppressed).
• StripSchemaPrefixFromBody no longer rewrites text inside double-quoted identifiers.

Not changed

• Ordering of generated-expression rewrites against function drops and modifications: this is the engine's phase model (drops, creates, then modifications) and affects column defaults the same way today; it needs its own PR.

Full diff suite, dump suite, ir tests, and apply/idempotency for the issue cases pass locally.

[Copilot]

🟡 Changes recommended

Newly added unique-index FK dependencies and column privileges on recreated views can still produce failed migrations or lost grants.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

internal/diff/diff.go:1514

• Recreated views are excluded from the old object-privilege set above, but their column privileges still compare equal here. Since the inspector includes views/materialized views in ColumnPrivileges and DROP VIEW removes their column ACLs, an unchanged GRANT SELECT (col) ON view is silently lost. Mark privileges on every relation in recreatedViewKeys for re-grant as well.

• Files reviewed: 29/29 changed files
• Comments generated: 2
• Review effort level: Balanced

[tianzhou]

Follow-up in 601595b for the seventh review round. All three points fixed:

• FKs (added on existing tables, or existing FKs whose new definition changes) that target a standalone unique index created by this migration, rebuilt or new, are deferred to the post-add step.
• Column grants on views that are dropped and created again are re-issued after recreation (the suppressed note on diff.go), matching the object-level grant handling; the dependents case keeps GRANT SELECT (id) ON big_orders through the recreation.

Full diff suite, plan tests, and apply/idempotency for the issue cases pass locally; no existing goldens changed from widening the FK deferral.

[Copilot]

🟡 Changes recommended

Identifier matching can mistake keywords or case-distinct quoted identifiers for recreated columns, triggering unrelated drop-and-recreate operations.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

• Files reviewed: 29/29 changed files
• Comments generated: 1
• Review effort level: Balanced

[tianzhou]

Follow-up in eca9dec for the eighth review round: the shared identifier matcher only emits a bare branch for names that have a valid unquoted spelling and matches the quoted branch exactly, so reserved-word or case-distinct quoted columns no longer trigger unrelated recreations. Full diff suite and apply/idempotency for the issue cases pass locally.

[Copilot]

🔵 Needs a closer look

Destructive column recreation and cross-object dependency handling warrant final human validation despite extensive regression coverage.

Review details

• Files reviewed: 29/29 changed files
• Comments generated: 1
• Review effort level: Balanced

[tianzhou]

Folded the three issue_591 diff cases into create_table/issue_591_alter_generated_column in cd59747 (one embedded-postgres apply cycle instead of three; scenarios are grouped with comments in old.sql/new.sql). Same coverage; diff, apply, and idempotency pass locally.