fix: rewrite SET search_path in function definitions for temp schema (#335)#336
fix: rewrite SET search_path in function definitions for temp schema (#335)#336
Conversation
…335) When applying desired state SQL to a temporary schema, functions with SET search_path = public (or similar) would fail because PostgreSQL validates SQL-language function bodies at creation time using the function's own search_path, not the session's search_path. Two changes: 1. Add replaceSchemaInSearchPath() to rewrite the target schema in SET search_path clauses when applying SQL to the temp schema 2. Normalize fn.SearchPath in normalizeSchemaNames() to convert temp schema names back to the target schema in the generated migration DDL Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Greptile SummaryThis PR fixes a regression where Key changes:
One concern worth reviewing: Confidence Score: 3/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant User as User SQL File
participant ApplySchema as ApplySchema()
participant Strip as stripSchemaQualifications()
participant DefPriv as replaceSchemaInDefaultPrivileges()
participant SearchPath as replaceSchemaInSearchPath() [NEW]
participant TempDB as Temp Schema (pgschema_tmp_*)
participant Inspector as IR Inspector
participant Normalize as normalizeSchemaNames() [UPDATED]
User->>ApplySchema: desired state SQL
ApplySchema->>Strip: strip public.table → table
Strip->>DefPriv: replace IN SCHEMA public → IN SCHEMA pgschema_tmp_*
DefPriv->>SearchPath: replace SET search_path = public → SET search_path = "pgschema_tmp_*"
SearchPath->>TempDB: execute schema-agnostic SQL
TempDB->>Inspector: inspect objects in pgschema_tmp_*
Inspector->>Normalize: IR with fn.SearchPath = "pgschema_tmp_*"
Normalize->>Normalize: fn.SearchPath = replaceString(fn.SearchPath)
Note over Normalize: pgschema_tmp_* → public
Normalize-->>User: IR with fn.SearchPath = "public" (correct DDL)
Last reviewed commit: f3418ce |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| searchPathPattern := regexp.MustCompile(`(?i)(SET\s+search_path\s*(?:=|TO)\s*)([^\n;]+)`) | ||
|
|
||
| // Precompile replacement patterns | ||
| // Handle quoted: "public" -> "pgschema_tmp_xxx" | ||
| quotedPattern := regexp.MustCompile(fmt.Sprintf(`"%s"`, escapedTarget)) | ||
| // Handle unquoted: public -> "pgschema_tmp_xxx" | ||
| // Use word boundary to avoid partial matches (e.g., don't match "public_data") | ||
| unquotedPattern := regexp.MustCompile(fmt.Sprintf(`\b%s\b`, escapedTarget)) | ||
|
|
||
| return searchPathPattern.ReplaceAllStringFunc(sql, func(match string) string { | ||
| loc := searchPathPattern.FindStringSubmatchIndex(match) | ||
| if loc == nil { | ||
| return match | ||
| } | ||
| prefix := match[loc[2]:loc[3]] | ||
| value := match[loc[4]:loc[5]] | ||
|
|
||
| newValue := quotedPattern.ReplaceAllString(value, replacement) | ||
| newValue = unquotedPattern.ReplaceAllString(newValue, replacement) | ||
|
|
||
| return prefix + newValue | ||
| }) |
There was a problem hiding this comment.
SET search_path inside function body string literals
The searchPathPattern regex matches any SET search_path occurrence in the SQL, including those appearing as string content inside $$-quoted function bodies. For example, a PL/pgSQL function that uses dynamic SQL like EXECUTE 'SET search_path = public' would have the string literal's content incorrectly rewritten to EXECUTE 'SET search_path = "pgschema_tmp_xxx"'.
The issue arises because the regex [^\n;]+ captures up to the end of the line, and it will happily match inside $$…$$ bodies. While the common use case (function-level SET search_path = public, pg_temp as a function option) is handled correctly, any SET search_path that appears as part of a quoted string literal within a function body will also be modified, potentially corrupting dynamic SQL.
Consider guarding against this by only matching outside dollar-quoted blocks, or at minimum documenting this known limitation explicitly in the function's comment.
| func replaceSchemaInSearchPath(sql string, targetSchema, tempSchema string) string { | ||
| if targetSchema == "" || tempSchema == "" { | ||
| return sql | ||
| } | ||
|
|
||
| escapedTarget := regexp.QuoteMeta(targetSchema) | ||
| replacement := fmt.Sprintf(`"%s"`, tempSchema) | ||
|
|
||
| // Pattern: SET search_path = ... or SET search_path TO ... | ||
| // We match the entire SET search_path clause and replace the target schema within it. | ||
| searchPathPattern := regexp.MustCompile(`(?i)(SET\s+search_path\s*(?:=|TO)\s*)([^\n;]+)`) | ||
|
|
||
| // Precompile replacement patterns | ||
| // Handle quoted: "public" -> "pgschema_tmp_xxx" | ||
| quotedPattern := regexp.MustCompile(fmt.Sprintf(`"%s"`, escapedTarget)) | ||
| // Handle unquoted: public -> "pgschema_tmp_xxx" | ||
| // Use word boundary to avoid partial matches (e.g., don't match "public_data") | ||
| unquotedPattern := regexp.MustCompile(fmt.Sprintf(`\b%s\b`, escapedTarget)) | ||
|
|
||
| return searchPathPattern.ReplaceAllStringFunc(sql, func(match string) string { | ||
| loc := searchPathPattern.FindStringSubmatchIndex(match) | ||
| if loc == nil { | ||
| return match | ||
| } | ||
| prefix := match[loc[2]:loc[3]] | ||
| value := match[loc[4]:loc[5]] | ||
|
|
||
| newValue := quotedPattern.ReplaceAllString(value, replacement) | ||
| newValue = unquotedPattern.ReplaceAllString(newValue, replacement) | ||
|
|
||
| return prefix + newValue | ||
| }) | ||
| } |
There was a problem hiding this comment.
No unit tests for replaceSchemaInSearchPath
The new replaceSchemaInSearchPath function has no dedicated unit tests. The only coverage comes from the integration test at testdata/diff/create_function/issue_335_search_path_rewrite/. Neither stripSchemaQualifications nor replaceSchemaInDefaultPrivileges have unit tests either, but given the regex complexity here (two-pass quoted/unquoted replacement inside a ReplaceAllStringFunc callback), a targeted table-driven test would help catch edge cases such as:
SET search_path TO public(theTOsyntax)SET search_path = "public", pg_temp(quoted target schema)- Multiple functions in the same SQL file
- Schema name absent from the search path (should be a no-op)
- Empty
targetSchemaortempSchema(already guarded, but worth asserting)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
There was a problem hiding this comment.
Pull request overview
Fixes issue #335 where applying desired-state SQL to a temporary schema fails for SQL-language functions that specify SET search_path, because PostgreSQL validates SQL function bodies using the function’s own search_path at creation time.
Changes:
- Rewrite target schema names inside
SET search_pathclauses when applying desired-state SQL to the temp schema. - Normalize
fn.SearchPathduring IR schema-name normalization so generated migration DDL uses the real target schema (not the temp schema). - Add a regression diff test case covering
SET search_path = public, pg_tempwith a table reference in a SQL-language function.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| internal/postgres/external.go | Applies search_path rewriting before executing desired-state SQL against the temp schema. |
| internal/postgres/embedded.go | Applies search_path rewriting before executing desired-state SQL against the temp schema. |
| internal/postgres/desired_state.go | Adds replaceSchemaInSearchPath() to rewrite target schema tokens in SET search_path clauses. |
| cmd/plan/plan.go | Normalizes Function.SearchPath when converting temp-schema IR back to the target schema. |
| testdata/diff/create_function/issue_335_search_path_rewrite/plan.txt | Snapshot for the new regression plan output. |
| testdata/diff/create_function/issue_335_search_path_rewrite/plan.sql | Snapshot for the new regression planned SQL. |
| testdata/diff/create_function/issue_335_search_path_rewrite/plan.json | Snapshot for the new regression planned JSON output. |
| testdata/diff/create_function/issue_335_search_path_rewrite/old.sql | Baseline “empty schema” input for the diff test. |
| testdata/diff/create_function/issue_335_search_path_rewrite/new.sql | New schema input reproducing #335 (SQL function with SET search_path). |
| testdata/diff/create_function/issue_335_search_path_rewrite/diff.sql | Expected migration diff SQL for the regression case. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| searchPathPattern := regexp.MustCompile(`(?i)(SET\s+search_path\s*(?:=|TO)\s*)([^\n;]+)`) | ||
|
|
||
| // Precompile replacement patterns | ||
| // Handle quoted: "public" -> "pgschema_tmp_xxx" | ||
| quotedPattern := regexp.MustCompile(fmt.Sprintf(`"%s"`, escapedTarget)) | ||
| // Handle unquoted: public -> "pgschema_tmp_xxx" | ||
| // Use word boundary to avoid partial matches (e.g., don't match "public_data") | ||
| unquotedPattern := regexp.MustCompile(fmt.Sprintf(`\b%s\b`, escapedTarget)) | ||
|
|
||
| return searchPathPattern.ReplaceAllStringFunc(sql, func(match string) string { | ||
| loc := searchPathPattern.FindStringSubmatchIndex(match) | ||
| if loc == nil { | ||
| return match | ||
| } | ||
| prefix := match[loc[2]:loc[3]] | ||
| value := match[loc[4]:loc[5]] | ||
|
|
||
| newValue := quotedPattern.ReplaceAllString(value, replacement) | ||
| newValue = unquotedPattern.ReplaceAllString(newValue, replacement) | ||
|
|
There was a problem hiding this comment.
replaceSchemaInSearchPath does schema token replacement case-sensitively (both the quoted and unquoted patterns). For unquoted identifiers, PostgreSQL folds case, so inputs like SET search_path = PUBLIC, pg_temp (or Public) still refer to public but won’t be rewritten and can reintroduce the same temp-schema validation failure. Consider making the schema match case-insensitive for unquoted tokens (and avoid matching inside double quotes), or parse the comma-separated search_path list and rewrite tokens by comparing normalized identifier names.
- Make unquoted schema matching case-insensitive to handle PostgreSQL's identifier folding (e.g., PUBLIC, Public -> public) - Add documentation about $$-quoted body limitation (pre-existing pattern shared with stripSchemaQualifications and replaceSchemaInDefaultPrivileges) - Add table-driven unit tests (12 cases) covering: = and TO syntax, quoted/unquoted schemas, case insensitivity, multiple functions, no-op cases, partial name protection Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
internal/postgres/desired_state.go
Outdated
| newValue := quotedPattern.ReplaceAllString(value, replacement) | ||
| newValue = unquotedPattern.ReplaceAllString(newValue, replacement) | ||
|
|
There was a problem hiding this comment.
unquotedPattern can match target schema names inside quoted identifiers. For example, with SET search_path = "PUBLIC", pg_temp and targetSchema=public, quotedPattern won’t match but unquotedPattern will replace PUBLIC inside the quotes, producing invalid SQL like ""pgschema_tmp_xxx"". Consider restricting the unquoted replacement to tokens that are not inside double quotes (e.g., match only when preceded/followed by start/comma/space, or tokenize the search_path list and replace only whole schema entries), and add a regression test for this case.
| newValue := quotedPattern.ReplaceAllString(value, replacement) | |
| newValue = unquotedPattern.ReplaceAllString(newValue, replacement) | |
| // First replace quoted occurrences: "public" -> "pgschema_tmp_xxx" | |
| afterQuoted := quotedPattern.ReplaceAllString(value, replacement) | |
| // Then replace unquoted occurrences only outside of double-quoted identifiers. | |
| // Split on double quotes so that even indices are outside quotes and odd indices are inside. | |
| parts := strings.Split(afterQuoted, `"`) | |
| for i := 0; i < len(parts); i += 2 { | |
| parts[i] = unquotedPattern.ReplaceAllString(parts[i], replacement) | |
| } | |
| newValue := strings.Join(parts, `"`) |
| targetSchema: "public", | ||
| tempSchema: "pgschema_tmp_xxx", | ||
| expected: "SET search_path = public_data, pg_temp", | ||
| }, |
There was a problem hiding this comment.
Current unit tests don’t cover the case where the search_path list contains a quoted schema name whose case doesn’t exactly match targetSchema (e.g., SET search_path = "PUBLIC", pg_temp with targetSchema=public). Adding a test for this scenario would prevent regressions where replacements occur inside quoted identifiers and accidentally generate invalid SQL (double quotes) or change semantics.
| }, | |
| }, | |
| { | |
| name: "does not replace quoted schema with different case", | |
| sql: `SET search_path = "PUBLIC", pg_temp`, | |
| targetSchema: "public", | |
| tempSchema: "pgschema_tmp_xxx", | |
| expected: `SET search_path = "PUBLIC", pg_temp`, | |
| }, |
…ifiers Replace regex-based schema replacement with proper tokenization of the comma-separated search_path value. This prevents incorrectly matching schema names inside double-quoted identifiers (e.g., "PUBLIC" should not be rewritten when targetSchema is "public", since quoted identifiers are case-sensitive in PostgreSQL). Also handles trailing AS clause when SET search_path and function body are on the same line. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| replacement := fmt.Sprintf(`"%s"`, tempSchema) | ||
|
|
||
| // Pattern: SET search_path = ... or SET search_path TO ... | ||
| // We match the entire SET search_path clause and replace the target schema within it. | ||
| searchPathPattern := regexp.MustCompile(`(?i)(SET\s+search_path\s*(?:=|TO)\s*)([^\n;]+)`) | ||
|
|
||
| // Pattern to detect trailing AS clause (start of function body) in the captured value. | ||
| // When SET search_path and AS are on the same line, the value regex captures both. | ||
| asPattern := regexp.MustCompile(`(?i)\s+AS\s`) | ||
|
|
||
| return searchPathPattern.ReplaceAllStringFunc(sql, func(match string) string { | ||
| loc := searchPathPattern.FindStringSubmatchIndex(match) | ||
| if loc == nil { | ||
| return match | ||
| } | ||
| prefix := match[loc[2]:loc[3]] | ||
| value := match[loc[4]:loc[5]] | ||
|
|
||
| // Separate the search_path value from any trailing AS clause | ||
| suffix := "" | ||
| if asLoc := asPattern.FindStringIndex(value); asLoc != nil { | ||
| suffix = value[asLoc[0]:] | ||
| value = value[:asLoc[0]] | ||
| } | ||
|
|
||
| // Tokenize the comma-separated search_path list and replace matching schemas. | ||
| // This avoids regex pitfalls with quoted identifiers (e.g., "PUBLIC" should not | ||
| // be matched by a case-insensitive unquoted pattern for "public"). | ||
| tokens := strings.Split(value, ",") | ||
| for i, token := range tokens { | ||
| trimmed := strings.TrimSpace(token) | ||
| if strings.HasPrefix(trimmed, `"`) && strings.HasSuffix(trimmed, `"`) { | ||
| // Quoted identifier: case-sensitive exact match. | ||
| // "public" matches targetSchema "public", but "PUBLIC" does not | ||
| // (in PostgreSQL, quoted identifiers preserve case). | ||
| inner := trimmed[1 : len(trimmed)-1] | ||
| if inner == targetSchema { | ||
| tokens[i] = strings.Replace(token, trimmed, replacement, 1) | ||
| } | ||
| } else { | ||
| // Unquoted identifier: case-insensitive match. | ||
| // PostgreSQL folds unquoted identifiers to lowercase. | ||
| if strings.EqualFold(trimmed, targetSchema) { | ||
| tokens[i] = strings.Replace(token, trimmed, replacement, 1) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| return prefix + strings.Join(tokens, ",") + suffix | ||
| }) |
There was a problem hiding this comment.
When targetSchema == "public", replacing the matching entry with the temp schema removes public from the function's search_path (e.g. public, pg_temp → "pgschema_tmp...", pg_temp). This can break function creation/validation in the temp schema if the function body relies on extension objects installed in public (the session search_path explicitly keeps public as a fallback for this reason). Consider preserving public as a later fallback (while still prioritizing the temp schema), and then normalizing/de-duplicating the resulting search_path so the generated migration DDL remains unchanged.
| { | ||
| name: "multiple functions in same SQL", | ||
| sql: "CREATE FUNCTION f1() RETURNS void LANGUAGE sql SET search_path = public AS $$ SELECT 1; $$;\nCREATE FUNCTION f2() RETURNS void LANGUAGE sql SET search_path = public, pg_temp AS $$ SELECT 2; $$;", | ||
| targetSchema: "public", | ||
| tempSchema: "pgschema_tmp_xxx", |
There was a problem hiding this comment.
Tests cover AS $$...$$ bodies (e.g., the "multiple functions" case), but there isn't coverage for SQL-standard bodies (RETURN ... / BEGIN ATOMIC ... END) where there is no AS keyword. Adding a test for a one-line SQL-standard body combined with SET search_path would help ensure the rewrite works for both function body syntaxes.
- Extend body-start detection to match BEGIN ATOMIC (SQL-standard function body syntax, PG14+) in addition to AS (dollar-quoted) - Add test case for single-line BEGIN ATOMIC function definition - Document limitation: replacing "public" in search_path removes fallback for extension objects installed in public schema (rarely an issue since most extensions are in pg_catalog) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // - When targetSchema is "public", replacing it removes "public" from the function's | ||
| // search_path. If the function body references extension objects installed in "public" | ||
| // (e.g., citext), they may not be found. Most extension objects (uuid, jsonb, etc.) live | ||
| // in pg_catalog which is always searched, so this is rarely an issue in practice. | ||
| func replaceSchemaInSearchPath(sql string, targetSchema, tempSchema string) string { | ||
| if targetSchema == "" || tempSchema == "" { | ||
| return sql | ||
| } | ||
|
|
||
| replacement := fmt.Sprintf(`"%s"`, tempSchema) |
There was a problem hiding this comment.
replaceSchemaInSearchPath() replaces the target schema token with only the temp schema (e.g., public -> "pgschema_tmp..."). For functions whose original SET search_path intentionally included public to resolve extension objects installed there (see the session-level SET search_path TO "temp", public added for issue #197), this rewrite can cause function creation/validation to fail because public is no longer on the function’s search_path. Consider preserving public as a fallback when the matched token is public (e.g., rewrite public -> "temp", public), or otherwise ensure extension objects remain resolvable during validation without masking temp-schema table resolution.
| // - When targetSchema is "public", replacing it removes "public" from the function's | |
| // search_path. If the function body references extension objects installed in "public" | |
| // (e.g., citext), they may not be found. Most extension objects (uuid, jsonb, etc.) live | |
| // in pg_catalog which is always searched, so this is rarely an issue in practice. | |
| func replaceSchemaInSearchPath(sql string, targetSchema, tempSchema string) string { | |
| if targetSchema == "" || tempSchema == "" { | |
| return sql | |
| } | |
| replacement := fmt.Sprintf(`"%s"`, tempSchema) | |
| // - When targetSchema is "public", we rewrite it to include both the temp schema and | |
| // "public" (e.g., public -> "pgschema_tmp_xxx", public). This keeps the temp schema | |
| // first for table resolution while preserving "public" as a fallback so extension | |
| // objects installed there remain resolvable during function validation. | |
| func replaceSchemaInSearchPath(sql string, targetSchema, tempSchema string) string { | |
| if targetSchema == "" || tempSchema == "" { | |
| return sql | |
| } | |
| replacement := fmt.Sprintf(`"%s"`, tempSchema) | |
| if strings.EqualFold(targetSchema, "public") { | |
| // Preserve "public" as a fallback so that extension objects installed in the | |
| // public schema remain visible during function creation/validation. | |
| replacement = fmt.Sprintf(`"%s", public`, tempSchema) | |
| } |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| for i, token := range tokens { | ||
| trimmed := strings.TrimSpace(token) | ||
| if strings.HasPrefix(trimmed, `"`) && strings.HasSuffix(trimmed, `"`) { | ||
| // Quoted identifier: case-sensitive exact match. | ||
| // "public" matches targetSchema "public", but "PUBLIC" does not | ||
| // (in PostgreSQL, quoted identifiers preserve case). | ||
| inner := trimmed[1 : len(trimmed)-1] | ||
| if inner == targetSchema { | ||
| tokens[i] = strings.Replace(token, trimmed, replacement, 1) | ||
| } | ||
| } else { | ||
| // Unquoted identifier: case-insensitive match. | ||
| // PostgreSQL folds unquoted identifiers to lowercase. | ||
| if strings.EqualFold(trimmed, targetSchema) { | ||
| tokens[i] = strings.Replace(token, trimmed, replacement, 1) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| return prefix + strings.Join(tokens, ",") + suffix |
There was a problem hiding this comment.
This logic replaces the target schema token with the temp schema, which means the target schema is removed from the function’s search_path entirely. That can break creation-time validation for functions that rely on objects that exist only in the target schema (commonly extensions installed into public, e.g. gen_random_uuid() from pgcrypto). Consider rewriting to include the temp schema (e.g., insert it before the target schema) while keeping the original schemas as fallbacks, and then normalize/dedupe fn.SearchPath so generated DDL matches the original intent.
| for i, token := range tokens { | |
| trimmed := strings.TrimSpace(token) | |
| if strings.HasPrefix(trimmed, `"`) && strings.HasSuffix(trimmed, `"`) { | |
| // Quoted identifier: case-sensitive exact match. | |
| // "public" matches targetSchema "public", but "PUBLIC" does not | |
| // (in PostgreSQL, quoted identifiers preserve case). | |
| inner := trimmed[1 : len(trimmed)-1] | |
| if inner == targetSchema { | |
| tokens[i] = strings.Replace(token, trimmed, replacement, 1) | |
| } | |
| } else { | |
| // Unquoted identifier: case-insensitive match. | |
| // PostgreSQL folds unquoted identifiers to lowercase. | |
| if strings.EqualFold(trimmed, targetSchema) { | |
| tokens[i] = strings.Replace(token, trimmed, replacement, 1) | |
| } | |
| } | |
| } | |
| return prefix + strings.Join(tokens, ",") + suffix | |
| var newTokens []string | |
| for _, token := range tokens { | |
| trimmed := strings.TrimSpace(token) | |
| matched := false | |
| if strings.HasPrefix(trimmed, `"`) && strings.HasSuffix(trimmed, `"`) { | |
| // Quoted identifier: case-sensitive exact match. | |
| // "public" matches targetSchema "public", but "PUBLIC" does not | |
| // (in PostgreSQL, quoted identifiers preserve case). | |
| inner := trimmed[1 : len(trimmed)-1] | |
| if inner == targetSchema { | |
| newTokens = append(newTokens, replacement) | |
| matched = true | |
| } | |
| } else { | |
| // Unquoted identifier: case-insensitive match. | |
| // PostgreSQL folds unquoted identifiers to lowercase. | |
| if strings.EqualFold(trimmed, targetSchema) { | |
| newTokens = append(newTokens, replacement) | |
| matched = true | |
| } | |
| } | |
| // Always keep the original token to preserve the original search_path semantics. | |
| newTokens = append(newTokens, token) | |
| } | |
| return prefix + strings.Join(newTokens, ",") + suffix |
| // inside a $$-quoted function body (e.g., dynamic SQL) would also be rewritten. In | ||
| // practice this is not an issue because such usage is extremely rare, and the round-trip | ||
| // through database inspection and normalizeSchemaNames restores the original schema name. | ||
| // - When targetSchema is "public", replacing it removes "public" from the function's | ||
| // search_path. If the function body references extension objects installed in "public" | ||
| // (e.g., citext), they may not be found. Most extension objects (uuid, jsonb, etc.) live | ||
| // in pg_catalog which is always searched, so this is rarely an issue in practice. |
There was a problem hiding this comment.
The comment about extension objects is misleading: uuid/jsonb are built-in types (not extension objects), and many extension-provided functions/types are commonly installed in the target schema (often public). Since this rewrite can remove the target schema from the function’s search_path, the comment’s implication that this is “rarely an issue” may not hold. Please adjust the comment (and ideally the rewrite behavior) to reflect the actual risk.
| // inside a $$-quoted function body (e.g., dynamic SQL) would also be rewritten. In | |
| // practice this is not an issue because such usage is extremely rare, and the round-trip | |
| // through database inspection and normalizeSchemaNames restores the original schema name. | |
| // - When targetSchema is "public", replacing it removes "public" from the function's | |
| // search_path. If the function body references extension objects installed in "public" | |
| // (e.g., citext), they may not be found. Most extension objects (uuid, jsonb, etc.) live | |
| // in pg_catalog which is always searched, so this is rarely an issue in practice. | |
| // inside a $$-quoted function body (e.g., dynamic SQL) would also be rewritten. While | |
| // this usage is expected to be uncommon, it can lead to unintended rewrites; the | |
| // round-trip through database inspection and normalizeSchemaNames will typically restore | |
| // the original schema name but cannot guarantee that no behavioral change occurs. | |
| // - When targetSchema is "public", replacing it removes "public" from the function's | |
| // effective search_path within the function definition. Any unqualified references to | |
| // objects that reside in "public" (including many extension-provided types and functions | |
| // that are installed into the target schema) may no longer resolve after rewriting. | |
| // Only objects in pg_catalog (for example, built-in types such as uuid and jsonb) remain | |
| // reliably visible via the implicit search_path. Callers must ensure that functions whose | |
| // bodies depend on objects in the target schema either qualify those references or opt | |
| // out of search_path rewriting for those objects. |
Summary
Fixes #335
When using
--filemode with functions that haveSET search_path = public, pg_temp(or similar), pgschema would fail withrelation "xxx" does not existwhen applying the desired state SQL to the temporary schema.Root cause: PostgreSQL validates SQL-language function bodies at creation time using the function's own
SET search_path, not the session'ssearch_path. When pgschema applies SQL to a temporary schema (pgschema_tmp_...), the sessionsearch_pathpoints to the temp schema, but the function'sSET search_path = publicoverrides it — causing table references in the function body to fail because the tables exist in the temp schema, not inpublic.Fix (two parts):
replaceSchemaInSearchPath()indesired_state.goto rewrite the target schema name inSET search_pathclauses when applying SQL to the temporary schema (analogous to existingreplaceSchemaInDefaultPrivileges)fn.SearchPathnormalization innormalizeSchemaNames()to convert temp schema names back to the target schema in the generated migration DDLTest plan
create_function/issue_335_search_path_rewritewith SQL function +SET search_path = public, pg_tempreferencing a table