fix: preserve schema qualifiers in function/procedure bodies (#354)

[tianzhou]

Summary

• Schema qualifiers (e.g., public.test) were unconditionally stripped from function/procedure bodies during normalization. This broke functions with restrictive search_path settings (SET search_path = '', pg_temp, etc.) where qualified references are required.
• Move stripping from normalization time to comparison time (definitionsEqualIgnoringSchema in diff package), so the IR preserves the original body for correct DDL generation
• Make stripSchemaQualifications skip dollar-quoted blocks so function bodies applied to embedded postgres preserve their qualifiers
• Fix dollar-quote regex to match PostgreSQL's grammar (prevents $1$ false positives on parameter references)

Fixes #354 (Bug 2: schema qualifiers stripped from function bodies)

Depends on #357 (Bug 1: empty search_path rendering)

Test plan

• Updated diff test create_function/issue_354_empty_search_path — now expects public.test preserved in body
• Added 7 unit tests for splitDollarQuotedSegments (parameter references, nested tags, unterminated quotes)
• Updated existing test expectations for preserved qualifiers (dependency/table_to_function, issue_252, issue_320)
• All function/procedure diff tests pass
• All dump tests pass

🤖 Generated with Claude Code

[greptile-apps]

Greptile Summary

This PR fixes a bug (#354) where schema qualifiers (e.g., public.test) were unconditionally stripped from function/procedure bodies during IR normalization, causing incorrect DDL generation for functions that restrict their search_path (e.g., SET search_path = ''). The fix has two parts:

• Move stripping from normalization-time to comparison-time: normalizeFunction and normalizeProcedure no longer call StripSchemaPrefixFromBody. Instead, definitionsEqualIgnoringSchema (called by all equality helpers in function.go and procedure.go) strips both sides just before comparison. This preserves qualifiers in the IR for correct DDL output while still treating public.test and test as equal when diffing.
• Dollar-quote awareness in stripSchemaQualifications: The desired_state.go function that rewrites user SQL before feeding it to embedded Postgres now splits the SQL into dollar-quoted and non-quoted segments, preserving function body contents verbatim. A new dollarQuoteRe regex correctly excludes $1/$2 positional parameter references from matching as dollar-quote tags.

Key changes by file:

• ir/normalize.go — strips removed from function/procedure normalization; stripSchemaPrefixFromBody exported as StripSchemaPrefixFromBody
• internal/diff/function.go / procedure.go — all definition-equality checks use new definitionsEqualIgnoringSchema helper
• internal/postgres/desired_state.go — stripSchemaQualifications made dollar-quote-aware via splitDollarQuotedSegments; empty search_path rendered as SET search_path = '' instead of SET search_path = ""
• Test fixtures for issue_252, issue_320, dependency/table_to_function, and new issue_354_empty_search_path updated to reflect preserved qualifiers

Confidence Score: 4/5

• Safe to merge with one minor documentation inconsistency to address.
• The core logic change is well-designed: moving schema-stripping from normalization to comparison time is the correct architectural fix. The splitDollarQuotedSegments implementation is correct — the regex excludes parameter references, handles nested tags via exact-string tag matching, and the loop always advances pos. All callers of definitionsEqualIgnoringSchema pre-check old.Schema == new.Schema, so using old.Schema as the strip argument is safe. The test coverage is comprehensive. The only finding is a stale sentence in the replaceSchemaInSearchPath limitations comment that now incorrectly claims stripSchemaQualifications lacks dollar-quote awareness.
• No files require special attention beyond the stale comment in internal/postgres/desired_state.go at line 266.

Important Files Changed

Filename	Overview
internal/diff/function.go	Replaces direct Definition string comparison with definitionsEqualIgnoringSchema across all three equality helpers and adds the helper itself. Logic is sound: all callers already verify old.Schema == new.Schema before the definition check, so using old.Schema in the helper is safe.
internal/diff/procedure.go	Two equality helpers updated to use definitionsEqualIgnoringSchema. Consistent with function.go; schema equality is pre-checked in both callers.
internal/postgres/desired_state.go	Adds dollar-quote-aware splitting to stripSchemaQualifications so function bodies are preserved verbatim. The splitDollarQuotedSegments regex correctly excludes $1/$2 parameter references. One stale comment in replaceSchemaInSearchPath's limitations block still describes stripSchemaQualifications as lacking dollar-quote awareness.
internal/postgres/desired_state_test.go	Seven new TestSplitDollarQuotedSegments cases cover the main scenarios including parameter references, named tags, multiple blocks, and unterminated quotes. Coverage is good.
ir/normalize.go	Removes schema-qualifier stripping from normalizeFunction and normalizeProcedure; renames helper to exported StripSchemaPrefixFromBody. Views still strip at normalization time (intentional — views lack SET search_path concerns).
testdata/dump/issue_320_plpgsql_reserved_keyword_type/pgschema.sql	Updated to preserve public.user (unquoted) in the dump. PostgreSQL accepts schema-qualified references without quoting even for reserved keywords, so this round-trips correctly.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[User SQL / pgschema dump] --> B[stripSchemaQualifications]
    B --> C{splitDollarQuotedSegments}
    C -->|Non-dollar-quoted segments| D[stripSchemaQualificationsFromText\nStrips schema.object patterns]
    C -->|Dollar-quoted blocks $$...$$| E[Preserved verbatim\nfunction body kept intact]
    D --> F[Assembled SQL for embedded Postgres]
    E --> F

    F --> G[Embedded Postgres inspection\nnormalizeIR]
    G --> H[normalizeFunction / normalizeProcedure\nNO schema stripping at normalization]
    H --> I[IR with original qualifiers preserved\ne.g., public.test in body]

    I --> J{Diff comparison\ndefinitionsEqualIgnoringSchema}
    J --> K[StripSchemaPrefixFromBody on both sides\nCompare stripped versions]
    K -->|Equal| L[No diff generated]
    K -->|Different| M[Generate CREATE OR REPLACE FUNCTION\nUsing IR body with qualifiers preserved]

    style E fill:#90EE90
    style I fill:#90EE90
    style K fill:#ADD8E6

Loading

Comments Outside Diff (1)

1. internal/postgres/desired_state.go, line 265-270 (link)

   Stale limitation comment

   This PR adds dollar-quote awareness to stripSchemaQualifications, so the "Like stripSchemaQualifications …" wording in the limitations is now inaccurate — only replaceSchemaInDefaultPrivileges and this function remain without that protection. Leaving the comment as-is could mislead future readers into thinking all three functions behave identically.

_{Last reviewed commit: 4708a17}

[Copilot]

Pull request overview

This PR addresses incorrect handling of schema-qualified references inside function/procedure bodies (notably when SET search_path = ''), by preserving qualifiers in the IR/DDL output while still allowing schema-insensitive comparisons during diffing.

Changes:

• Stop stripping same-schema qualifiers from function/procedure bodies during IR normalization; instead, ignore qualifier differences only at diff comparison time.
• Update function SQL generation to correctly render empty search_path ("" stored in catalog) as SET search_path = ''.
• Update desired-state SQL preprocessing to avoid stripping schema qualifiers inside dollar-quoted blocks, and add unit tests for the dollar-quote segmentation logic; update golden fixtures accordingly.

Reviewed changes

Copilot reviewed 17 out of 17 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
internal/postgres/desired_state.go	Skip schema-qualification stripping inside dollar-quoted blocks when preparing desired-state SQL for temp-schema application.
internal/postgres/desired_state_test.go	Add unit tests for splitting SQL into dollar-quoted vs non-quoted segments.
ir/normalize.go	Export StripSchemaPrefixFromBody; stop stripping qualifiers in function/procedure normalization while keeping view normalization behavior.
ir/normalize_test.go	Update tests to call exported StripSchemaPrefixFromBody.
internal/diff/function.go	Render empty search_path correctly; compare definitions via definitionsEqualIgnoringSchema instead of raw string equality.
internal/diff/procedure.go	Compare procedure definitions via definitionsEqualIgnoringSchema instead of raw string equality.
testdata/diff/create_function/issue_354_empty_search_path/plan.txt	Add golden output for issue #354 plan (human format).
testdata/diff/create_function/issue_354_empty_search_path/plan.sql	Add golden output for issue #354 plan (SQL format).
testdata/diff/create_function/issue_354_empty_search_path/plan.json	Add golden output for issue #354 plan (JSON format).
testdata/diff/create_function/issue_354_empty_search_path/old.sql	Add fixture for “old” state (table only).
testdata/diff/create_function/issue_354_empty_search_path/new.sql	Add fixture for “new” state (table + function with empty search_path).
testdata/diff/create_function/issue_354_empty_search_path/diff.sql	Add expected migration SQL for issue #354.
testdata/diff/dependency/table_to_function/plan.txt	Update fixture to preserve schema-qualified table reference in function body.
testdata/diff/dependency/table_to_function/plan.sql	Update fixture to preserve schema-qualified table reference in function body.
testdata/diff/dependency/table_to_function/plan.json	Update fixture to preserve schema-qualified table reference in function body.
testdata/diff/dependency/table_to_function/diff.sql	Update expected migration SQL to preserve schema-qualified table reference.
testdata/dump/issue_252_function_schema_qualifier/pgschema.sql	Update dump fixture to expect preserved schema-qualified references inside bodies.
testdata/dump/issue_320_plpgsql_reserved_keyword_type/pgschema.sql	Update dump fixture to expect preserved schema-qualified references inside bodies.
testdata/diff/create_trigger/add_trigger/plan.json	Update fingerprint hash due to fixture changes elsewhere.

---

💡 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.