fix: DROP function before CREATE when return type or param names change (#326)

[tianzhou]

Summary

• When a function's return type or parameter names change (but input parameter types stay the same), PostgreSQL rejects CREATE OR REPLACE FUNCTION because it cannot alter these properties
• pgschema now detects these incompatible changes via functionRequiresRecreate() and generates DROP FUNCTION followed by CREATE FUNCTION instead
• Also refactored generateDropFunctionSQL into a shared helper to eliminate duplication

Fixes #326

Test plan

• PGSCHEMA_TEST_FILTER="create_function/issue_326" go test -v ./internal/diff -run TestDiffFromFiles — 3 new test cases (return type change, param type change, param name change)
• PGSCHEMA_TEST_FILTER="create_function/" go test -v ./internal/diff -run TestDiffFromFiles — all 8 function diff tests pass
• PGSCHEMA_TEST_FILTER="create_function/" go test -v ./cmd -run TestPlanAndApply — all 8 function integration tests pass
• PGSCHEMA_TEST_FILTER="dependency/" go test -v ./internal/diff -run TestDiffFromFiles — all 13 dependency tests pass

🤖 Generated with Claude Code

[greptile-apps]

Greptile Summary

This PR fixes PostgreSQL compatibility when modifying functions whose return type or parameter names change. PostgreSQL does not allow CREATE OR REPLACE FUNCTION to alter these properties, requiring DROP FUNCTION followed by CREATE FUNCTION instead.

Key Changes:

• Added functionRequiresRecreate() to detect when return type or parameter names change
• Modified generateModifyFunctionsSQL() to generate DROP+CREATE sequence for incompatible changes
• Refactored generateDropFunctionSQL() into a shared helper to eliminate code duplication

Test Coverage:

• Three new test cases covering return type change, parameter name change, and combined changes
• All existing function diff tests (8 total) and integration tests pass
• Dependency tests (13 total) continue to pass

Confidence Score: 5/5

• Safe to merge - well-tested fix for PostgreSQL compatibility issue
• The implementation correctly handles PostgreSQL's CREATE OR REPLACE limitations, includes comprehensive test coverage (3 new test cases + existing tests), and properly refactors duplicated code. The logic is sound and follows existing patterns in the codebase.
• No files require special attention

Important Files Changed

Filename	Overview
internal/diff/function.go	Added functionRequiresRecreate() to detect return type/param name changes and refactored DROP SQL generation into shared helper
testdata/diff/create_function/issue_326_return_type_change/diff.sql	Test case validating DROP+CREATE when return type changes from text to integer
testdata/diff/create_function/issue_326_param_name_change/diff.sql	Test case validating DROP+CREATE when parameter name changes from old_name to new_name
testdata/diff/create_function/issue_326_param_type_change/diff.sql	Test case validating DROP+CREATE when both parameter type and name change

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[Function Modification Detected] --> B{Only Comment Changed?}
    B -->|Yes| C[Generate COMMENT ON FUNCTION]
    B -->|No| D{Only Attributes Changed?<br/>LEAKPROOF/PARALLEL}
    D -->|Yes| E[Generate ALTER FUNCTION for attributes]
    D -->|No| F{Requires Recreate?<br/>Return type or param names changed}
    F -->|Yes| G[Generate DROP FUNCTION]
    G --> H[Generate CREATE FUNCTION]
    H --> I[Check if comment also changed]
    F -->|No| J[Generate CREATE OR REPLACE FUNCTION]
    J --> K[Check if comment also changed]
    E --> L[Check if comment also changed]
    I -->|Changed| M[Generate COMMENT]
    K -->|Changed| M
    L -->|Changed| M

Loading

_{Last reviewed commit: ccb9399}

[Copilot]

Pull request overview

Updates function diffing/migration generation to handle PostgreSQL’s limitation where CREATE OR REPLACE FUNCTION cannot change certain function signature properties (e.g., return type, parameter names), by emitting DROP FUNCTION followed by CREATE FUNCTION when needed.

Changes:

• Add functionRequiresRecreate() and use it in function modification planning to emit DROP FUNCTION IF EXISTS + CREATE OR REPLACE FUNCTION.
• Refactor function drop SQL generation into shared generateDropFunctionSQL() helper.
• Add new regression fixtures for issue #326 covering return type change, parameter type change, and parameter name change.

Reviewed changes

Copilot reviewed 19 out of 19 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
internal/diff/function.go	Adds recreate detection and emits DROP+CREATE for incompatible function modifications; refactors DROP SQL generation into a helper.
testdata/diff/create_function/issue_326_return_type_change/plan.txt	New expected human plan output for return type change case.
testdata/diff/create_function/issue_326_return_type_change/plan.sql	New expected SQL plan for return type change case.
testdata/diff/create_function/issue_326_return_type_change/plan.json	New expected JSON plan for return type change case.
testdata/diff/create_function/issue_326_return_type_change/old.sql	Old schema fixture for return type change.
testdata/diff/create_function/issue_326_return_type_change/new.sql	New schema fixture for return type change.
testdata/diff/create_function/issue_326_return_type_change/diff.sql	Expected migration SQL for return type change.
testdata/diff/create_function/issue_326_param_type_change/plan.txt	New expected human plan output for parameter type change case.
testdata/diff/create_function/issue_326_param_type_change/plan.sql	New expected SQL plan for parameter type change case.
testdata/diff/create_function/issue_326_param_type_change/plan.json	New expected JSON plan for parameter type change case.
testdata/diff/create_function/issue_326_param_type_change/old.sql	Old schema fixture for parameter type change.
testdata/diff/create_function/issue_326_param_type_change/new.sql	New schema fixture for parameter type change.
testdata/diff/create_function/issue_326_param_type_change/diff.sql	Expected migration SQL for parameter type change.
testdata/diff/create_function/issue_326_param_name_change/plan.txt	New expected human plan output for parameter name change case.
testdata/diff/create_function/issue_326_param_name_change/plan.sql	New expected SQL plan for parameter name change case.
testdata/diff/create_function/issue_326_param_name_change/plan.json	New expected JSON plan for parameter name change case.
testdata/diff/create_function/issue_326_param_name_change/old.sql	Old schema fixture for parameter name change.
testdata/diff/create_function/issue_326_param_name_change/new.sql	New schema fixture for parameter name change.
testdata/diff/create_function/issue_326_param_name_change/diff.sql	Expected migration SQL for parameter name change.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

functionRequiresRecreate currently only compares parameter names (and count) after filtering out TABLE params. If an OUT parameter’s type or mode changes (which does not affect Function.GetArguments() and therefore can still show up as a “modify”), this will fall through to CREATE OR REPLACE FUNCTION and PostgreSQL will reject it. Consider extending this check to also compare OUT/INOUT parameter mode and data type (at least for non-input parameters), not just .Name.

Suggested change

	for i := range oldParams {
	if oldParams[i].Name != newParams[i].Name {
	return true
	}
	for i := range oldParams {
	// Name changes always require a DROP+CREATE
	if oldParams[i].Name != newParams[i].Name {
	return true
	}
	// For OUT/INOUT parameters, a change in mode or data type (or other
	// signature-relevant attributes) also requires DROP+CREATE, since
	// PostgreSQL does not allow CREATE OR REPLACE FUNCTION to alter them.
	if (oldParams[i].Mode == "OUT" || oldParams[i].Mode == "INOUT") ||
	(newParams[i].Mode == "OUT" || newParams[i].Mode == "INOUT") {
	if !parameterEqual(oldParams[i], newParams[i]) {
	return true
	}
	}

[Copilot]

Dropping and recreating a function inside the modify phase can fail if any existing objects depend on it (e.g., views or triggers), since PostgreSQL will block DROP FUNCTION ... under RESTRICT (the default). There doesn’t appear to be corresponding logic here to pre-drop/recreate dependent objects for this “recreate” path (unlike the drop phase which handles triggers for dropped functions). Consider extending dependency handling (or explicitly using CASCADE with controlled recreation) so this migration path is reliable when dependencies exist.

[Copilot]

Pull request overview

Copilot reviewed 19 out of 19 changed files in this pull request and generated 1 comment.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

The DROP+CREATE path for incompatible function changes can still fail at runtime if any existing objects depend on the function (e.g., views, policies, triggers on unchanged tables). PostgreSQL will reject DROP FUNCTION with dependent objects present, and this branch runs during the modify phase without any dependency pre-drop/recreate handling. Consider adding dependency handling similar to view recreation (drop/recreate dependents around the function recreation), or reordering so dependents are updated/dropped before the function is dropped; adding a regression test with a view depending on the function would help prevent plan failures.