Repository navigation
fix: re-emit COMMENT when a function or procedure is dropped and recreated (#677) - #678
Conversation
…eated (#677) When a function change requires DROP + CREATE (return type, OUT params, parameter names) or a procedure body changes (always DROP + CREATE), the plan only emitted COMMENT ON when the comment text differed. Since DROP discards the comment, an unchanged comment was lost after apply and the next plan reported a spurious diff. Emit COMMENT ON after every DROP + CREATE whenever the new routine has a comment, so a single apply converges. Fixes #677 Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
There was a problem hiding this comment.
🟢 Approval recommended
The review reports no active issues with the changes to preserve function and procedure comments across recreation.
No open issues 🎉
What changed in this PR
Fixes #677 by preserving function and procedure comments when migrations drop and recreate those routines, preventing a comment-only diff on the next plan.
Changes:
- Re-emits non-empty comments after function and procedure recreation, even when the comment text is unchanged.
- Extends existing regression fixtures for function return-type changes and procedure signature/body changes.
- Updates expected migration SQL and plan outputs. Privilege restoration remains outside this PR’s scope.
| File | Description |
|---|---|
internal/diff/function.go |
Restores comments after function recreation. |
internal/diff/procedure.go |
Restores comments after procedure recreation. |
testdata/diff/create_function/issue_326_return_type_change/old.sql |
Adds the initial function comment. |
testdata/diff/create_function/issue_326_return_type_change/new.sql |
Preserves the comment across a return-type change. |
testdata/diff/create_function/issue_326_return_type_change/diff.sql |
Expects comment restoration in migration SQL. |
testdata/diff/create_function/issue_326_return_type_change/plan.sql |
Includes comment restoration in the SQL plan. |
testdata/diff/create_function/issue_326_return_type_change/plan.json |
Updates the fingerprint and comment step. |
testdata/diff/create_function/issue_326_return_type_change/plan.txt |
Updates the plan summary and displayed SQL. |
testdata/diff/create_procedure/alter_procedure/old.sql |
Adds comments and the initial body-change fixture. |
testdata/diff/create_procedure/alter_procedure/new.sql |
Keeps comments across signature and body changes. |
testdata/diff/create_procedure/alter_procedure/diff.sql |
Expects recreation and comment restoration. |
testdata/diff/create_procedure/alter_procedure/plan.sql |
Includes procedure changes and restored comments. |
testdata/diff/create_procedure/alter_procedure/plan.json |
Updates the fingerprint and procedure steps. |
testdata/diff/create_procedure/alter_procedure/plan.txt |
Updates the plan summary and displayed SQL. |
Review effort: Balanced
[Low impact] The PR appears safe to merge; it restores comments after recreation without changing comment-only edits. SummaryRestores non-empty comments after functions or procedures are dropped and recreated, even when the comment text has not changed.
Acknowledged deferral: Reviews (1) · Last reviewed commit: "fix: re-emit COMMENT when a function or ..." · Reviewed by Greptile |
Summary
When a function change is planned as
DROP FUNCTION+CREATE FUNCTION(return type, OUT parameter, or parameter-name change), or a procedure body changes (procedures always go throughDROP+CREATE), the plan only emittedCOMMENT ONwhen the comment text differed between old and new.DROPdiscards the comment, so an unchanged comment was silently lost after apply and the next plan showed a one-line diff adding it back.The DROP + CREATE branches in
internal/diff/function.goandinternal/diff/procedure.gonow re-emitCOMMENT ONwhenever the new routine has a non-empty comment, so one apply converges.Not covered here:
GRANTs on a recreated function are lost the same way (privileges are diffed independently old-vs-new, so an unchanged grant is not re-granted). That also applies to recreated views and materialized views, so it is a separate cross-cutting fix.Fixes #677
Test plan
Folded into existing cases rather than adding new embedded-postgres cycles:
testdata/diff/create_function/issue_326_return_type_change: added an unchanged comment on the function whose return type changes.testdata/diff/create_procedure/alter_procedure: added an unchanged comment on the signature-changing procedure (already handled via drop+add) and a newlog_eventprocedure with a body-only change and an unchanged comment (the affected DROP + CREATE modify path).Both failed before the fix (no
COMMENT ONafter theCREATE) and pass after:The
TestPlanAndApplyruns include the idempotency check (second plan after apply must be empty).🤖 Generated with Claude Code