Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes issue #218 where schema qualifiers in column default type casts were being incorrectly stripped when dumping non-public schemas. The fix ensures that PostgreSQL's pg_get_expr() function properly preserves schema qualifiers by setting search_path to pg_catalog during expression extraction, forcing full qualification of user-defined types and functions.
Key changes:
- Modified SQL queries to use LATERAL joins with
search_path='pg_catalog'for proper schema qualification - Added normalization logic to strip same-schema type qualifiers during comparison for semantic equivalence
- Updated test infrastructure to handle cross-schema comparisons
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| ir/queries/queries.sql | Refactored GetColumns and GetColumnsForSchema queries to use CTE with LATERAL join for controlled search_path during pg_get_expr execution |
| ir/queries/queries.sql.go | Generated Go code reflecting the SQL query changes |
| ir/normalize.go | Added regex pattern to strip same-schema type qualifiers (e.g., ::tenant1.status → ::status) for semantic comparison |
| cmd/dump/dump_integration_test.go | Enhanced test normalization to handle schema qualifier differences when comparing dumps from different schemas |
| testdata/dump/tenant/util.sql | Added get_default_status() function to test cross-schema function references with type casts |
| testdata/dump/tenant/tenant.sql | Added test case with get_default_status_text() function and updated users table with account_status and secondary_status columns |
| testdata/dump/tenant/pgschema.sql | Updated expected output to reflect new column defaults and function ordering |
| testdata/dump/tenant/tenant1.sql | New expected output file for tenant1 schema dump |
| testdata/dump/tenant/tenant2.sql | New expected output file for tenant2 schema dump |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| cb.identity_cycle, | ||
| cb.attgenerated, | ||
| -- Use LATERAL join to guarantee execution order: | ||
| -- 1. set_config sets search_path to only the table's schema |
There was a problem hiding this comment.
The comment states "set_config sets search_path to only the table's schema" but the actual code on line 158 sets the search_path to 'pg_catalog', not the table's schema. This comment should be updated to match the implementation, similar to the comment on lines 259-262.
| -- 1. set_config sets search_path to only the table's schema | |
| -- 1. set_config sets search_path to 'pg_catalog' to force schema qualification |
| -- 1. set_config sets search_path to only pg_catalog | ||
| -- 2. pg_get_expr then uses that search_path and includes schema qualifiers for user types | ||
| -- This ensures type references in column defaults and generated columns are properly | ||
| -- qualified (Issue #218). The normalization code strips same-schema function qualifiers. |
There was a problem hiding this comment.
The comment states "1. set_config sets search_path to only the table's schema" but the actual code on line 489 sets the search_path to 'pg_catalog', not the table's schema. This comment is outdated and should match the implementation, which correctly sets search_path to 'pg_catalog' to force schema qualification.
| -- When dumping from tenant1 schema, PostgreSQL stores this with qualified type cast | ||
| -- Expected: (get_default_status_text())::tenant1.status (type qualifier preserved) | ||
| -- Bug in v1.5.1: (get_default_status_text())::status (type qualifier stripped) |
There was a problem hiding this comment.
This comment is misleading. It states "Expected: (get_default_status_text())::tenant1.status (type qualifier preserved)" but the actual expected output (as seen in tenant1.sql line 172) shows "::status" without the schema qualifier. This is because the normalization code in normalize.go (lines 144-151) strips same-schema type qualifiers for consistent comparison. The comment should clarify that the schema qualifier is preserved during extraction but normalized away for output/comparison purposes.
| -- When dumping from tenant1 schema, PostgreSQL stores this with qualified type cast | |
| -- Expected: (get_default_status_text())::tenant1.status (type qualifier preserved) | |
| -- Bug in v1.5.1: (get_default_status_text())::status (type qualifier stripped) | |
| -- When dumping from tenant1 schema, PostgreSQL stores this with a schema-qualified type cast | |
| -- Extraction preserves the schema qualifier (e.g., (get_default_status_text())::tenant1.status), | |
| -- but normalization strips same-schema qualifiers so the expected output is (get_default_status_text())::status. |
| -- NOTE: The 'dummy' column in the LATERAL subquery forces set_config to execute | ||
| -- before pg_get_expr. PostgreSQL evaluates SELECT columns left-to-right within |
There was a problem hiding this comment.
The comment refers to "'true' in the WHERE clause" but the actual SQL uses "ON true" in the LATERAL join condition (line 291), not a WHERE clause. The comment should be corrected to say "ON condition" instead of "WHERE clause".
| -- NOTE: The 'dummy' column in the LATERAL subquery forces set_config to execute | ||
| -- before pg_get_expr. PostgreSQL evaluates SELECT columns left-to-right within |
There was a problem hiding this comment.
The comment refers to "'true' in the WHERE clause" but the actual SQL uses "ON true" in the LATERAL join condition, not a WHERE clause. The comment should be corrected to say "ON condition" instead of "WHERE clause".
The pg_get_expr() call for column_default was positioned outside the LATERAL join that sets search_path, so it didn't benefit from schema qualification. This caused type casts like ::my_schema.status to be stripped to just ::status when dumping non-public schemas. Changes: - Move column_default calculation into LATERAL join with search_path set to 'pg_catalog' to force schema qualification - Add normalization to strip same-schema type qualifiers during comparison so ::public.status matches ::status semantically - Update test expected files and add cross-schema type reference tests 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
…219) The pg_get_expr() call for column_default was positioned outside the LATERAL join that sets search_path, so it didn't benefit from schema qualification. This caused type casts like ::my_schema.status to be stripped to just ::status when dumping non-public schemas. Changes: - Move column_default calculation into LATERAL join with search_path set to 'pg_catalog' to force schema qualification - Add normalization to strip same-schema type qualifiers during comparison so ::public.status matches ::status semantically - Update test expected files and add cross-schema type reference tests 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 4.5 <noreply@anthropic.com>
) (pgplex#219) The pg_get_expr() call for column_default was positioned outside the LATERAL join that sets search_path, so it didn't benefit from schema qualification. This caused type casts like ::my_schema.status to be stripped to just ::status when dumping non-public schemas. Changes: - Move column_default calculation into LATERAL join with search_path set to 'pg_catalog' to force schema qualification - Add normalization to strip same-schema type qualifiers during comparison so ::public.status matches ::status semantically - Update test expected files and add cross-schema type reference tests 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 4.5 <noreply@anthropic.com>
1 participant
Fix #218
The pg_get_expr() call for column_default was positioned outside the LATERAL join that sets search_path, so it didn't benefit from schema qualification. This caused type casts like ::my_schema.status to be stripped to just ::status when dumping non-public schemas.
Changes:
🤖 Generated with Claude Code