fix: preserve all function SET clauses from pg_proc.proconfig - #529
Conversation
Greptile SummaryThis PR extends function introspection and IR comparison to preserve every
Confidence Score: 4/5The arbitrary function-setting renderer should be fixed before merging because valid values containing whitespace or other literal-sensitive characters generate invalid migration SQL. Full proconfig preservation reaches the new renderer correctly, but the renderer treats catalog values as reusable SQL tokens, breaking valid function settings that require quoting. Files Needing Attention: internal/diff/function.go Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart LR
A["pg_proc.proconfig text[]"] --> B["Inspector: Function.SetConfig"]
B --> C["Temporary-schema normalization"]
C --> D["Order-insensitive function diff"]
D --> E["CREATE FUNCTION renderer"]
E --> F["SET key = value clauses"]
Reviews (1): Last reviewed commit: "fix: preserve all function SET clauses f..." | Re-trigger Greptile |
There was a problem hiding this comment.
Pull request overview
This PR fixes function-local configuration loss by preserving and re-rendering all pg_proc.proconfig entries (not just search_path) through the IR → diff/plan → apply pipeline, addressing issue #526 where pgschema apply could silently drop function SET clauses.
Changes:
- Replaces
Function.SearchPathwithFunction.SetConfig []stringand updates introspection SQL to read the fullproconfigarray. - Updates function diff/rendering to compare
SetConfigorder-insensitively and emit deterministicSETclauses. - Adds an integration regression test for plan → apply → plan convergence with multiple (and edge-case)
SETclauses. - Additionally introduces
can_run_in_transactionin plan JSON steps and updates migrate fixtures accordingly.
Reviewed changes
Copilot reviewed 12 out of 13 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| testdata/diff/migrate/v1/plan.json | Updates expected plan JSON to include can_run_in_transaction. |
| testdata/diff/migrate/v2/plan.json | Updates expected plan JSON to include can_run_in_transaction. |
| testdata/diff/migrate/v3/plan.json | Updates expected plan JSON to include can_run_in_transaction. |
| testdata/diff/migrate/v4/plan.json | Updates expected plan JSON to include can_run_in_transaction (including CONCURRENTLY=false cases). |
| testdata/diff/migrate/v5/plan.json | Updates expected plan JSON to include can_run_in_transaction. |
| ir/queries/queries.sql | Introspects full pg_proc.proconfig via COALESCE(p.proconfig, '{}') AS set_config. |
| ir/queries/queries.sql.go | Regenerates sqlc output to return set_config []string and scan via pq.Array. |
| ir/ir.go | Updates function IR to store SetConfig []string instead of SearchPath. |
| ir/inspector.go | Populates Function.SetConfig from inspector query results. |
| internal/diff/function.go | Renders and compares SetConfig deterministically; introduces setConfigEqual. |
| cmd/plan/plan.go | Normalizes schema references inside SetConfig entries during plan generation. |
| internal/plan/plan.go | Adds Step.CanRunInTransaction to plan JSON + backward-compat handling in FromJSON. |
| cmd/issue_526_integration_test.go | Adds integration test ensuring SET clauses survive plan → apply → plan. |
Files not reviewed (1)
- ir/queries/queries.sql.go: Generated file
Suppressed comments (1)
internal/plan/plan.go:44
- This PR is scoped/titled around preserving function SET clauses, but it also introduces a new
can_run_in_transactionfield in plan JSON and updates multiple migrate plan fixtures accordingly. That’s a public JSON format change and likely warrants either (a) being called out explicitly in the PR title/description, or (b) moving to a separate PR to keep the change focused.
// Step represents a single execution step with SQL and optional directive
type Step struct {
SQL string `json:"sql"`
Directive *Directive `json:"directive,omitempty"`
CanRunInTransaction bool `json:"can_run_in_transaction"`
// Metadata for summary generation
Type string `json:"type,omitempty"` // e.g., "table", "index"
Operation string `json:"operation,omitempty"` // e.g., "create", "alter", "drop"
Path string `json:"path,omitempty"` // e.g., "public.users"
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Fixed in e9f00e8: non-search_path SET values are now rendered through The existing |
tianzhou
left a comment
There was a problem hiding this comment.
Fixed in e9f00e8: the non-search_path SET values in generateFunctionSQL now go through quoteString().
Values like DateStyle=ISO, MDY now render as SET DateStyle = 'ISO, MDY'. The quoteString utility (already used elsewhere in diff.go for COMMENT ON) wraps the value in single quotes and doubles any internal single quotes — same approach pg_dump uses for the non-search_path case.
Previously, only search_path was extracted from pg_proc.proconfig and
preserved in the function IR. All other SET clauses (TimeZone, DateStyle,
statement_timeout, etc.) were silently dropped during introspection,
causing plan→apply→plan divergence and lost runtime semantics.
Changes:
- ir/ir.go: Replace Function.SearchPath (string) with SetConfig ([]string)
containing raw 'key=value' entries from proconfig
- ir/queries/queries.sql: Replace search_path-only subquery with
COALESCE(p.proconfig, '{}') to capture all entries
- ir/queries/queries.sql.go: Regenerated with sqlc
- ir/inspector.go: Read full SetConfig array instead of just SearchPath
- internal/diff/function.go: Render all SET clauses in sorted order,
with special handling for search_path empty-string case; compare
SetConfig slices in all three function diff helpers
- cmd/plan/plan.go: Schema-normalize all SetConfig entries
- cmd/issue_526_integration_test.go: Round-trip regression tests
Fixes #526
The proconfig array stores values without quotes (e.g. DateStyle=ISO, MDY), but valid migration SQL requires them as quoted string literals (e.g. SET DateStyle = 'ISO, MDY'). Non-search_path settings are now rendered through quoteString() so whitespace and special characters don't produce invalid SQL.
e9f00e8 to
210a0bb
Compare
Summary
Fixes #526 —
pgschema applywas silently dropping every function-localSETclause exceptsearch_path.Root cause
The function IR had a single
SearchPathfield. The introspection SQL extracted onlysearch_pathfrompg_proc.proconfig, and rendering emitted onlySET search_path.Changes
ir/ir.go)SearchPath stringwithSetConfig []string— stores rawkey=valueentriesir/queries/queries.sql)WHERE cfg LIKE 'search_path=%'toCOALESCE(p.proconfig, '{}')ir/inspector.go)SetConfigarrayinternal/diff/function.go)quoteString()cmd/plan/plan.go)SetConfigentriesRendering
search_pathkeeps existing special behavior (unquoted for multi-schema paths,''for empty)SET <key> = '<value>'viaquoteString()(handles whitespace, embedded quotes)Edge cases
proconfig:COALESCEproduces{}, handled gracefullysetConfigEqualsorts before comparingquoteStringwraps in single quotes and doubles internal quotesreplaceSchemaInSearchPathunaffected — it operates on raw SQL and only matchesSET search_path