Conversation
…399) Disable function body validation (SET check_function_bodies = off) when applying desired state SQL to temporary schemas. This prevents type-identity mismatches where parameter types are stripped to the temp schema but body references still point to the original schema. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Greptile SummaryThis PR fixes a The fix adds Confidence Score: 5/5Safe to merge — the fix is minimal, well-scoped, and correct for both database providers. The fix uses a well-known PostgreSQL GUC (check_function_bodies = off) applied only to the dedicated, short-lived connection used for IR extraction. Disabling body validation does not affect IR quality (the inspector reads from pg_catalog, not by executing functions), and any real errors in function bodies will still be caught when the generated migration is applied to the target database. The fix is symmetric across both providers, accompanied by a targeted regression test, and the existing test for issue #354 continues to pass. No P0 or P1 issues found. No files require special attention. Important Files Changed
Sequence DiagramsequenceDiagram
participant User as User SQL Files
participant AP as ApplySchema
participant PG as Temp PostgreSQL Schema
participant IR as IR Inspector
User->>AP: SQL with public.role_type params and public.role_caps in body
AP->>PG: SET search_path TO pgschema_tmp_xxx, public
AP->>PG: SET check_function_bodies = off (NEW #399)
AP->>AP: stripSchemaQualifications() - params stripped, body preserved (#354)
AP->>PG: CREATE FUNCTION role_has_cap(p_role role_type...) AS $$ ... FROM public.role_caps ... $$
Note over PG: Body NOT validated at creation time (check_function_bodies = off)
PG-->>AP: Function created successfully
AP->>IR: Inspect pg_catalog for function definition
IR-->>AP: IR with body intact
Reviews (1): Last reviewed commit: "fix: plan fails with schema-qualified re..." | Re-trigger Greptile |
There was a problem hiding this comment.
Pull request overview
Fixes pgschema plan failures when SQL-language function bodies contain schema-qualified references by disabling function body validation while applying desired-state SQL to the temporary planning schema.
Changes:
- Set
check_function_bodies = offduring desired-state application to temp schemas (embedded + external plan DB paths). - Document the rationale/requirement alongside schema-qualification stripping logic.
- Add a regression test fixture reproducing issue #399 (schema-qualified reference inside
$$function body).
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/postgres/external.go | Disables function body validation prior to executing rewritten desired-state SQL in the external plan DB. |
| internal/postgres/embedded.go | Disables function body validation prior to executing rewritten desired-state SQL in the embedded planner DB. |
| internal/postgres/desired_state.go | Documents the interaction between body-preservation and function-body validation (issues #354 / #399). |
| testdata/diff/create_function/issue_399_schema_qualified_body/plan.txt | Expected human-readable plan output for the new regression case. |
| testdata/diff/create_function/issue_399_schema_qualified_body/plan.sql | Expected SQL plan step output for the new regression case. |
| testdata/diff/create_function/issue_399_schema_qualified_body/plan.json | Expected JSON plan output for the new regression case. |
| testdata/diff/create_function/issue_399_schema_qualified_body/old.sql | Baseline SQL state for the regression fixture. |
| testdata/diff/create_function/issue_399_schema_qualified_body/new.sql | Desired SQL state reproducing the schema-qualified body reference bug. |
| testdata/diff/create_function/issue_399_schema_qualified_body/diff.sql | Expected diff SQL for the regression fixture. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Disable function body validation to avoid type-identity mismatches (issue #399). | ||
| // Schema qualifications inside dollar-quoted function bodies are preserved (issue #354), | ||
| // but parameter types are stripped. For SQL-language functions, PostgreSQL validates the | ||
| // body at creation time, which can fail when body references use the original schema's | ||
| // types while parameters reference the temporary schema's types. | ||
| if _, err := util.ExecContextWithLogging(ctx, conn, "SET check_function_bodies = off", "disable function body validation for desired state"); err != nil { | ||
| return fmt.Errorf("failed to disable check_function_bodies: %w", err) | ||
| } |
There was a problem hiding this comment.
SET check_function_bodies = off is session-scoped. Since this uses db.Conn() (a pooled connection) and conn.Close() returns it to the pool, the setting can leak into later uses of the same underlying connection. To avoid cross-call side effects, wrap the desired-state application in an explicit transaction and use SET LOCAL check_function_bodies = off, or ensure the setting is reset (e.g., RESET check_function_bodies) via defer, including on error paths.
There was a problem hiding this comment.
The *sql.DB pool here is private to the ExternalDatabase instance, which is short-lived (created for plan generation, then Stop()'d). No other code shares this pool. The same session-scoped SET search_path on the line right above also has no cleanup/reset — adding a defer for check_function_bodies but not for search_path would be inconsistent. Both are fine as-is given the instance lifecycle.
| // types while parameters reference the temporary schema's types. | ||
| if _, err := util.ExecContextWithLogging(ctx, conn, "SET check_function_bodies = off", "disable function body validation for desired state"); err != nil { | ||
| return fmt.Errorf("failed to disable check_function_bodies: %w", err) | ||
| } |
There was a problem hiding this comment.
SET check_function_bodies = off persists on the session. Because this uses db.Conn() (connection pool) and then conn.Close(), that connection may be reused later with check_function_bodies still disabled. Consider using BEGIN; SET LOCAL check_function_bodies = off; ... COMMIT; around the desired-state SQL, or resetting the GUC (RESET check_function_bodies) in a defer so pooled connections are returned to a clean state even on failures.
| } | |
| } | |
| defer func() { | |
| _, _ = conn.ExecContext(context.Background(), "RESET check_function_bodies") | |
| }() |
There was a problem hiding this comment.
Same as the other comment — the EmbeddedPostgres instance owns its own private *sql.DB connected to a temporary embedded PostgreSQL instance that gets destroyed after plan generation. The connection pool is never shared. The existing SET search_path two lines above follows the same pattern with no cleanup, so adding a defer only for this setting would be inconsistent.
Summary
$$-delimited body (e.g.,FROM public.role_caps),pgschema planfails withoperator does not existbecause parameter types are stripped to the temporary schema while body references still point to the original schemaSET check_function_bodies = off) when applying desired state SQL to temporary schemas during plan generation — the body is only needed for IR extraction, not executionFixes #399
Test plan
testdata/diff/create_function/issue_399_schema_qualified_body/reproducing the bugissue_354_empty_search_path(no regression)go test ./...)🤖 Generated with Claude Code