fix: recreate non-view dependents around a function DROP + CREATE (#601) - #622
christophostertag wants to merge 1 commit into
Conversation
…plex#601) A function change that CREATE OR REPLACE cannot apply is planned as DROP FUNCTION + CREATE FUNCTION (pgplex#326). pgplex#619 moved the calling views through that cycle; the other objects PostgreSQL records a dependency for still made the DROP fail with SQLSTATE 2BP01. Column defaults, CHECK and EXCLUDE constraints, expression and partial indexes, policies, trigger WHEN conditions and domain defaults and CHECK constraints whose desired definition calls a recreated function are now held: the regular table and domain diff leaves them alone, their current version is dropped right before the function and the desired version is created right after it. Drops, recreation and restores share one transaction: index changes of kept materialized views, which the plan builds CONCURRENTLY, wait until the restores are done, and a test checks every diff case for a transaction boundary inside that span or a change to a held object outside it. A rebuilt index is created without CONCURRENTLY so the table is never without it, and a CHECK constraint on an existing table is added NOT VALID and validated in a transaction of its own afterwards. New tables and domains are created without the held objects, which follow the function. Objects naming a column the diff re-creates (pgplex#591) keep their early drop. The regular column diff leaves a held default as it is. Defaults on partitioned tables are changed with ALTER TABLE ONLY so partitions keep defaults of their own, and partitions leave the CHECK constraints and indexes they inherit to their parent. A materialized view whose index calls the function is recreated like one whose query does. Rejected at plan time, with the column and function named: a generated column calling a recreated function, a column added or re-created (pgplex#591) whose default - its own or its domain type's - calls one, since its existing rows would be filled before the function exists again, and a column whose type changes while its new default calls one. A CHECK constraint the desired state declares NOT VALID is now added NOT VALID without VALIDATE; the online rewrite validated it anyway, which failed with SQLSTATE 23514 when existing rows violate it and otherwise re-planned the constraint on every run. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Dependency handling and NOT VALID preservation still have correctness gaps in supported edge cases.
Get a fresh assessment by requesting another Copilot review.
Review effort: Balanced
Findings: 1
Open (4)
What changed in this PR
Extends function DROP/CREATE migrations to safely recreate dependent schema objects and preserve NOT VALID CHECK semantics.
Changes:
- Holds defaults, constraints, indexes, policies, triggers, domains, and materialized-view indexes around function recreation.
- Rejects unsupported generated-column and default-dependent migrations.
- Adds transaction-grouping and PostgreSQL integration fixtures.
| File | Description |
|---|---|
cmd/plan/plan.go |
Validates unsafe function recreations. |
internal/diff/diff.go |
Integrates dependent holding and ordering. |
internal/diff/function_dependents.go |
Implements dependent collection, recreation, and validation. |
internal/diff/function_dependents_test.go |
Tests recreation validation. |
internal/diff/table.go |
Preserves invalid CHECK state. |
internal/diff/view.go |
Supports deferred materialized-view indexes. |
internal/plan/held_dependents_test.go |
Tests transaction grouping. |
internal/plan/plan.go |
Propagates held-dependent metadata. |
internal/plan/rewrite.go |
Adjusts online rewrites and validation isolation. |
testdata/diff/online/add_check_not_valid/diff.sql |
Expected canonical CHECK DDL. |
testdata/diff/online/add_check_not_valid/new.sql |
Desired invalid CHECK fixture. |
testdata/diff/online/add_check_not_valid/old.sql |
Legacy-row fixture. |
testdata/diff/online/add_check_not_valid/plan.json |
Expected JSON plan. |
testdata/diff/online/add_check_not_valid/plan.sql |
Expected SQL plan. |
testdata/diff/online/add_check_not_valid/plan.txt |
Expected text plan. |
testdata/diff/dependency/issue_601_function_recreate_changed_dependents/diff.sql |
Expected changed-dependent DDL. |
testdata/diff/dependency/issue_601_function_recreate_changed_dependents/new.sql |
Desired changed dependents. |
testdata/diff/dependency/issue_601_function_recreate_changed_dependents/old.sql |
Initial changed-dependent schema. |
testdata/diff/dependency/issue_601_function_recreate_changed_dependents/plan.json |
Expected JSON plan. |
testdata/diff/dependency/issue_601_function_recreate_changed_dependents/plan.sql |
Expected SQL plan. |
testdata/diff/dependency/issue_601_function_recreate_changed_dependents/plan.txt |
Expected text plan. |
testdata/diff/dependency/issue_601_function_recreate_domain_exclude_not_valid/diff.sql |
Expected domain/exclusion DDL. |
testdata/diff/dependency/issue_601_function_recreate_domain_exclude_not_valid/new.sql |
Desired domain dependencies. |
testdata/diff/dependency/issue_601_function_recreate_domain_exclude_not_valid/old.sql |
Initial domain dependencies. |
testdata/diff/dependency/issue_601_function_recreate_domain_exclude_not_valid/plan.json |
Expected JSON plan. |
testdata/diff/dependency/issue_601_function_recreate_domain_exclude_not_valid/plan.sql |
Expected SQL plan. |
testdata/diff/dependency/issue_601_function_recreate_domain_exclude_not_valid/plan.txt |
Expected text plan. |
testdata/diff/dependency/issue_601_function_recreate_partitioned_table/diff.sql |
Expected partitioned-table DDL. |
testdata/diff/dependency/issue_601_function_recreate_partitioned_table/new.sql |
Desired partition dependencies. |
testdata/diff/dependency/issue_601_function_recreate_partitioned_table/old.sql |
Initial partition dependencies. |
testdata/diff/dependency/issue_601_function_recreate_partitioned_table/plan.json |
Expected JSON plan. |
testdata/diff/dependency/issue_601_function_recreate_partitioned_table/plan.sql |
Expected SQL plan. |
testdata/diff/dependency/issue_601_function_recreate_partitioned_table/plan.txt |
Expected text plan. |
testdata/diff/dependency/issue_601_function_recreate_trigger_with_online_steps/diff.sql |
Expected trigger/online DDL. |
testdata/diff/dependency/issue_601_function_recreate_trigger_with_online_steps/new.sql |
Desired trigger dependencies. |
testdata/diff/dependency/issue_601_function_recreate_trigger_with_online_steps/old.sql |
Initial trigger dependencies. |
testdata/diff/dependency/issue_601_function_recreate_trigger_with_online_steps/plan.json |
Expected JSON plan. |
testdata/diff/dependency/issue_601_function_recreate_trigger_with_online_steps/plan.sql |
Expected SQL plan. |
testdata/diff/dependency/issue_601_function_recreate_trigger_with_online_steps/plan.txt |
Expected text plan. |
testdata/diff/dependency/issue_601_function_recreate_unchanged_dependents/diff.sql |
Expected unchanged-dependent DDL. |
testdata/diff/dependency/issue_601_function_recreate_unchanged_dependents/new.sql |
Desired unchanged dependents. |
testdata/diff/dependency/issue_601_function_recreate_unchanged_dependents/old.sql |
Initial unchanged dependents. |
testdata/diff/dependency/issue_601_function_recreate_unchanged_dependents/plan.json |
Expected JSON plan. |
testdata/diff/dependency/issue_601_function_recreate_unchanged_dependents/plan.sql |
Expected SQL plan. |
testdata/diff/dependency/issue_601_function_recreate_unchanged_dependents/plan.txt |
Expected text plan. |
testdata/diff/dependency/issue_601_function_recreate_with_enum_value/diff.sql |
Expected enum-ordering DDL. |
testdata/diff/dependency/issue_601_function_recreate_with_enum_value/new.sql |
Desired enum dependency state. |
testdata/diff/dependency/issue_601_function_recreate_with_enum_value/old.sql |
Initial enum dependency state. |
testdata/diff/dependency/issue_601_function_recreate_with_enum_value/plan.json |
Expected JSON plan. |
testdata/diff/dependency/issue_601_function_recreate_with_enum_value/plan.sql |
Expected SQL plan. |
testdata/diff/dependency/issue_601_function_recreate_with_enum_value/plan.txt |
Expected text plan. |
testdata/diff/dependency/issue_601_function_recreate_with_matview_index_change/diff.sql |
Expected materialized-view index DDL. |
testdata/diff/dependency/issue_601_function_recreate_with_matview_index_change/new.sql |
Desired materialized-view index state. |
testdata/diff/dependency/issue_601_function_recreate_with_matview_index_change/old.sql |
Initial materialized-view state. |
testdata/diff/dependency/issue_601_function_recreate_with_matview_index_change/plan.json |
Expected JSON plan. |
testdata/diff/dependency/issue_601_function_recreate_with_matview_index_change/plan.sql |
Expected SQL plan. |
testdata/diff/dependency/issue_601_function_recreate_with_matview_index_change/plan.txt |
Expected text plan. |
testdata/diff/dependency/issue_601_function_recreate_with_table_online_steps/diff.sql |
Expected table-online DDL. |
testdata/diff/dependency/issue_601_function_recreate_with_table_online_steps/new.sql |
Desired table-online state. |
testdata/diff/dependency/issue_601_function_recreate_with_table_online_steps/old.sql |
Initial table-online state. |
testdata/diff/dependency/issue_601_function_recreate_with_table_online_steps/plan.json |
Expected JSON plan. |
testdata/diff/dependency/issue_601_function_recreate_with_table_online_steps/plan.sql |
Expected SQL plan. |
testdata/diff/dependency/issue_601_function_recreate_with_table_online_steps/plan.txt |
Expected text plan. |
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| td.AddedConstraints = removeByName(td.AddedConstraints, constraints, func(c *ir.Constraint) string { return c.Name }) | ||
| td.ModifiedConstraints = removeByName(td.ModifiedConstraints, constraints, func(c *ConstraintDiff) string { return c.New.Name }) | ||
| earlyConstraints := make(map[string]bool) | ||
| for _, c := range td.DroppedConstraints { | ||
| if constraints[c.Name] && exclusionReferencesColumns(c, td.RecreatedColumns) { | ||
| earlyConstraints[c.Name] = true | ||
| } | ||
| } | ||
| td.DroppedConstraints = removeByName(td.DroppedConstraints, subtract(constraints, earlyConstraints), func(c *ir.Constraint) string { return c.Name }) |
| oldTable := oldTables[key] | ||
| if newTable.IsExternal || (oldTable != nil && oldTable.IsExternal) { | ||
| continue |
| domains := make(map[string]*ir.Type) | ||
| for _, dbSchema := range newIR.Schemas { | ||
| for _, typ := range dbSchema.Types { | ||
| if typ.Kind == ir.TypeKindDomain { | ||
| domains[strings.ToLower(typ.Schema+"."+typ.Name)] = typ | ||
| } |
| if !constraint.IsValid { | ||
| suffix += " NOT VALID" | ||
| } |


Summary
Follow-up to #619. A function change that
CREATE OR REPLACEcannot apply (return type, parameter names, OUT parameters) is planned asDROP FUNCTION+CREATE FUNCTION(#326). #619 moved calling views through that cycle; the other objects PostgreSQL records a dependency for still made apply fail withcannot drop function ... because other objects depend on it (SQLSTATE 2BP01), e.g. a columnDEFAULT lim()whenlim()changes fromintegertobigint— the limitation #619 left open.Changes
internal/diff/function_dependents.go): column defaults, CHECK and EXCLUDE constraints, expression and partial indexes, policies, triggerWHENconditions, and domain defaults/CHECK constraints whose desired definition calls a recreated function are taken out of the regular table/domain diff. Their current version is dropped right before the function and the desired version is created right after it, in the same transaction. Index changes of kept materialized views (builtCONCURRENTLY, i.e. in transactions of their own) wait until the restores are done, and the regular diff leaves held objects alone (a held column default keeps its current value until the drop right before the function), so drops, recreation and restores share one transaction — with the one exception of GENERATED ALWAYS AS expression changes are ignored by plan #591's early drops noted below;TestHeldDependentsShareOneTransactionchecks, for every diff test case, that the held steps form one transaction and that nothing else changes a held object outside it. Unchanged, modified and new dependents are handled alike; objects whose old definition calls the function but whose new one does not stay with the regular diff. An EXCLUDE constraint, policy or trigger naming a column the diff re-creates (GENERATED ALWAYS AS expression changes are ignored by plan #591) is still dropped before that column.CONCURRENTLY(the table is never without it, e.g. a unique expression index), a CHECK on an existing table is re-addedNOT VALIDand validated in its own transaction once everything is back, and an index that did not exist before keeps the usual concurrent rewrite.ALTER TABLE ONLY, so partitions keep defaults of their own; partitions leave the CHECK constraints and indexes they inherit to their parent.ValidateFunctionRecreations, called fromGeneratePlan): a generated column that calls a recreated function (keeping it would require dropping and re-adding the column; on PG14 theDROP FUNCTIONeven drops it silently), or a column added to an existing table — or re-created by GENERATED ALWAYS AS expression changes are ignored by plan #591, e.g. VIRTUAL to plain — whose default (its own, or its domain type's) calls one: its existing rows would be filled before the function exists again, by the old function or with NULL. A column whose type or collation changes while its new default calls a recreated function is refused as well (change the type or collation in a separate step). The error names the column and the function.No
CASCADEis used; drops areRESTRICT/IF EXISTS.Lock note: the drops take
ACCESS EXCLUSIVEon the affected tables until the transaction commits, and a rebuilt index is built under that lock.VALIDATE CONSTRAINTruns afterwards withSHARE UPDATE EXCLUSIVE.Behavior change: CHECK constraints declared NOT VALID
Needed so a restored
NOT VALIDCHECK keeps its state, and a bug on its own: a desiredALTER TABLE ... ADD CONSTRAINT ... CHECK (...) NOT VALIDon an existing table was rewritten toADD ... NOT VALID+VALIDATE CONSTRAINT. With rows violating it, apply failed withSQLSTATE 23514; without, the constraint ended up validated and every later plan re-emittedDROP CONSTRAINT/ADD ... NOT VALID/VALIDATE. It is now addedNOT VALIDwithoutVALIDATE(new caseonline/add_check_not_valid). Foreign keys declaredNOT VALIDhave the same problem and are left unchanged here.Known limitations
RETURN/BEGIN ATOMIC) calling the function, expression partition keys (not modeled by the IR), aggregates using it as a support function, objects outside the managed schema.pgschema dumpomits them and changing them plans as no change), so a re-created policy/constraint loses a comment set outside pgschema, as with any policy/constraint pgschema re-creates today.DROP COLUMN, so it is outside the single-transaction span.Refs #601, follows #619.
Test plan
New cases in
testdata/diff/dependency/:issue_601_function_recreate_unchanged_dependents,…_changed_dependents(modified, added, new table),…_domain_exclude_not_valid,…_partitioned_table,…_trigger_with_online_steps,…_with_matview_index_changeand…_with_table_online_steps(other objects' online steps stay outside the function's transaction; default-only and SET NOT NULL changes keep their default until then),…_with_enum_value(enumADD VALUEcommits before it); plusonline/add_check_not_valid,TestValidateFunctionRecreations*andTestHeldDependentsShareOneTransaction.Confirmed red before the fix (2BP01, and 23514 for the NOT VALID case). Full
go test ./...passes locally (PG18). Also verified with the CLI (external plan database, saved-plan apply, repeat plan empty) on PostgreSQL 14–18, including runtime behavior: defaults use the new function and partition defaults survive, CHECK/EXCLUDE/unique index/policy are enforced (policy for a non-owner role), triggers fire on the new condition, unrelated objects keep their OIDs.🤖 Generated with Claude Code