Conversation
PostgreSQL stores SET search_path = '' as search_path="" in proconfig. The extracted value is literal "" (two double-quote chars), which was rendered verbatim as invalid SQL. Now correctly renders as ''. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Greptile SummaryThis PR fixes a specific, well-understood PostgreSQL serialization quirk: when a function is created with Key observations:
Confidence Score: 5/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant PG as PostgreSQL (pg_proc.proconfig)
participant Inspector as ir/Inspector
participant IR as ir.Function.SearchPath
participant DiffGen as generateFunctionSQL
participant SQL as Generated SQL
PG->>Inspector: search_path="" (proconfig entry for SET search_path = '')
Inspector->>IR: SearchPath = `""`
IR->>DiffGen: function.SearchPath == `""`?
alt SearchPath is `""`
DiffGen->>SQL: SET search_path = '' ✅
else SearchPath is non-empty (e.g. pg_catalog, public)
DiffGen->>SQL: SET search_path = pg_catalog, public ✅
else SearchPath is ""
Note over DiffGen,SQL: Before fix: SET search_path = "" ❌ (invalid SQL)
end
Last reviewed commit: e46e036 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
248
to
256
| if function.SearchPath != "" { | ||
| stmt.WriteString(fmt.Sprintf("\nSET search_path = %s", function.SearchPath)) | ||
| // PostgreSQL stores SET search_path = '' as search_path="" in proconfig. | ||
| // The extracted value is "" (two double-quote chars). Render as '' (single-quoted empty string). | ||
| if function.SearchPath == `""` { | ||
| stmt.WriteString("\nSET search_path = ''") | ||
| } else { | ||
| stmt.WriteString(fmt.Sprintf("\nSET search_path = %s", function.SearchPath)) | ||
| } | ||
| } |
There was a problem hiding this comment.
Consider also covering partial "" in a composite path
The current check only fires when the entire SearchPath value equals "" (i.e., search_path="" was the only proconfig entry, representing a fully empty path). If PostgreSQL ever stores a composite value such as pg_catalog, "" — where the empty element appears alongside real schemas — the literal "" in that string would fall through to the else branch and be rendered verbatim as a double-quoted identifier, rather than ''.
In practice this is very unlikely, but since the fix already has a comment explaining the proconfig format, it might be worth a brief note here that only the whole-value case is handled:
Suggested change
| if function.SearchPath != "" { | |
| stmt.WriteString(fmt.Sprintf("\nSET search_path = %s", function.SearchPath)) | |
| // PostgreSQL stores SET search_path = '' as search_path="" in proconfig. | |
| // The extracted value is "" (two double-quote chars). Render as '' (single-quoted empty string). | |
| if function.SearchPath == `""` { | |
| stmt.WriteString("\nSET search_path = ''") | |
| } else { | |
| stmt.WriteString(fmt.Sprintf("\nSET search_path = %s", function.SearchPath)) | |
| } | |
| } | |
| if function.SearchPath != "" { | |
| // PostgreSQL stores SET search_path = '' as search_path="" in proconfig. | |
| // The extracted value is "" (two double-quote chars). Render as '' (single-quoted empty string). | |
| // Note: only the whole-value empty case is handled; mixed paths (e.g. pg_catalog, "") are not expected. | |
| if function.SearchPath == `""` { | |
| stmt.WriteString("\nSET search_path = ''") | |
| } else { | |
| stmt.WriteString(fmt.Sprintf("\nSET search_path = %s", function.SearchPath)) | |
| } | |
| } |
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes function DDL rendering for SET search_path = '' by translating PostgreSQL’s pg_proc.proconfig encoding (search_path="") into valid SQL (SET search_path = '') during diff/plan generation.
Changes:
- Special-case rendering of
function.SearchPath == "\"\""to emitSET search_path = ''in generated function SQL. - Added a new golden diff fixture covering empty
search_pathfor functions (create_function/issue_354_empty_search_path).
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| internal/diff/function.go | Adjusts function SQL generation to render empty search_path correctly. |
| testdata/diff/create_function/issue_354_empty_search_path/plan.txt | Adds expected human plan output for the new empty search_path case. |
| testdata/diff/create_function/issue_354_empty_search_path/plan.sql | Adds expected SQL plan output for the new empty search_path case. |
| testdata/diff/create_function/issue_354_empty_search_path/plan.json | Adds expected JSON plan output for the new empty search_path case. |
| testdata/diff/create_function/issue_354_empty_search_path/old.sql | Adds old-state SQL for the golden diff fixture. |
| testdata/diff/create_function/issue_354_empty_search_path/new.sql | Adds new-state SQL for the golden diff fixture. |
| testdata/diff/create_function/issue_354_empty_search_path/diff.sql | Adds expected diff SQL for the golden diff fixture. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
Comment on lines
245
to
+252
| // Add SET search_path if specified | ||
| // Note: Output without outer quotes to handle multi-schema paths correctly | ||
| // e.g., "SET search_path = pg_catalog, public" not "SET search_path = 'pg_catalog, public'" | ||
| if function.SearchPath != "" { | ||
| stmt.WriteString(fmt.Sprintf("\nSET search_path = %s", function.SearchPath)) | ||
| // PostgreSQL stores SET search_path = '' as search_path="" in proconfig. | ||
| // The extracted value is "" (two double-quote chars). Render as '' (single-quoted empty string). | ||
| if function.SearchPath == `""` { | ||
| stmt.WriteString("\nSET search_path = ''") |
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes function DDL rendering for the special case where PostgreSQL stores SET search_path = '' as search_path="" in pg_proc.proconfig, ensuring emitted SQL uses valid '' instead of invalid "".
Changes:
- Special-case
Function.SearchPath == "\"\""when generating function SQL to emitSET search_path = ''. - Add a new function diff fixture covering empty
search_pathrendering (issue_354_empty_search_path).
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| internal/diff/function.go | Renders the search_path="" stored form as SET search_path = '' to avoid invalid SQL. |
| testdata/diff/create_function/issue_354_empty_search_path/plan.txt | Adds expected plan output for the empty search_path function case. |
| testdata/diff/create_function/issue_354_empty_search_path/plan.sql | Adds expected generated SQL for the empty search_path function case. |
| testdata/diff/create_function/issue_354_empty_search_path/plan.json | Adds expected JSON plan output verifying the emitted SQL contains SET search_path = ''. |
| testdata/diff/create_function/issue_354_empty_search_path/old.sql | Adds the “before” schema for the new diff fixture. |
| testdata/diff/create_function/issue_354_empty_search_path/new.sql | Adds the “after” schema including a function with SET search_path = ''. |
| testdata/diff/create_function/issue_354_empty_search_path/diff.sql | Adds the expected diff SQL output for the new fixture. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
tianzhou
changed the title
5 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
SET search_path = ''was rendered asSET search_path = ""(invalid SQL). PostgreSQL stores this assearch_path=""inpg_proc.proconfig; the extracted literal""is now correctly rendered as''.public.test) were unconditionally stripped from function/procedure bodies during normalization. This broke functions with restrictivesearch_pathsettings ('',pg_temp, etc.) where qualified references are required. Fix: move stripping from normalization time to comparison time, and make SQL preprocessing skip dollar-quoted blocks.Fixes #354
Test plan
create_function/issue_354_empty_search_pathcovering both bugssplitDollarQuotedSegments(7 cases including parameter references, nested tags)dependency/table_to_function,issue_252,issue_320)TestPlanAndApply)🤖 Generated with Claude Code