Skip to content

Revert "fix: extension qualifier (#146)" #147

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
tianzhou merged 1 commit into from
Nov 7, 2025
Merged

Revert "fix: extension qualifier (#146)" #147

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 1 addition & 13 deletions internal/diff/diff.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -946,20 +946,8 @@ func (d *ddlDiff) generateModifySQL(targetSchema string, collector *diffCollecto
// Modify sequences
generateModifySequencesSQL(d.modifiedSequences, targetSchema, collector)

// Create a map of added type names for reference during table modifications.
// This is used to determine whether type references in ALTER TABLE ADD COLUMN
// should be schema-qualified or not:
// - Types being created in this migration (in addedTypes) → unqualified
// Example: CREATE TYPE employee_status; ALTER TABLE ADD COLUMN status employee_status
// - Pre-existing types (not in addedTypes) → qualified with schema
// Example: Types from extensions or setup.sql → ALTER TABLE ADD COLUMN email public.custom_text
addedTypeNames := make(map[string]bool)
for _, typeObj := range d.addedTypes {
addedTypeNames[typeObj.Name] = true
}

// Modify tables
generateModifyTablesSQL(d.modifiedTables, targetSchema, addedTypeNames, collector)
generateModifyTablesSQL(d.modifiedTables, targetSchema, collector)

// Modify views
generateModifyViewsSQL(d.modifiedViews, targetSchema, collector)
Expand Down
33 changes: 3 additions & 30 deletions internal/diff/table.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -413,11 +413,11 @@ func generateCreateTablesSQL(tables []*ir.Table, targetSchema string, collector
}

// generateModifyTablesSQL generates ALTER TABLE statements
func generateModifyTablesSQL(diffs []*tableDiff, targetSchema string, addedTypeNames map[string]bool, collector *diffCollector) {
func generateModifyTablesSQL(diffs []*tableDiff, targetSchema string, collector *diffCollector) {
// Diffs are already sorted by the Diff operation
for _, diff := range diffs {
// Pass collector to generateAlterTableStatements to collect with proper context
diff.generateAlterTableStatements(targetSchema, addedTypeNames, collector)
diff.generateAlterTableStatements(targetSchema, collector)
}
}

Expand Down Expand Up @@ -491,7 +491,7 @@ func generateTableSQL(table *ir.Table, targetSchema string) string {
}

// generateAlterTableStatements generates SQL statements for table modifications
func (td *tableDiff) generateAlterTableStatements(targetSchema string, addedTypeNames map[string]bool, collector *diffCollector) {
func (td *tableDiff) generateAlterTableStatements(targetSchema string, collector *diffCollector) {
// Drop constraints first (before dropping columns) - already sorted by the Diff operation
for _, constraint := range td.DroppedConstraints {
tableName := getTableNameWithSchema(td.Table.Schema, td.Table.Name, targetSchema)
Expand Down Expand Up @@ -545,33 +545,6 @@ func (td *tableDiff) generateAlterTableStatements(targetSchema string, addedType

// Build column type
columnType := formatColumnDataType(column)

// Conditionally strip schema prefix based on whether the type is being created.
//
// Context: Types come from the inspector with schema qualification due to the
// pattern matching fix for temp schemas (WHEN dn.nspname LIKE 'pgschema_tmp_%').
// However, we want different qualification behavior depending on the type's origin:
//
// Case 1: Type is being created in this migration (in addedTypeNames)
// - The type and table are defined together in the same schema file
// - References should be unqualified for readability
// - Example: CREATE TYPE employee_status; ALTER TABLE ADD COLUMN status employee_status
//
// Case 2: Type already exists (NOT in addedTypeNames)
// - The type is pre-existing infrastructure (extension, domain from another migration)
// - References should be qualified for explicitness
// - Example: Extension citext or setup.sql types → ALTER TABLE ADD COLUMN email public.custom_text
if addedTypeNames != nil {
// Extract the base type name (without schema prefix) from the column's data type
baseTypeName := column.DataType
if idx := strings.LastIndex(baseTypeName, "."); idx >= 0 {
baseTypeName = baseTypeName[idx+1:]
}
// If this type is being created in the current diff, strip the schema prefix
if addedTypeNames[baseTypeName] {
columnType = stripSchemaPrefix(columnType, targetSchema)
}
}
tableName := getTableNameWithSchema(td.Table.Schema, td.Table.Name, targetSchema)

// Build and append all column clauses
Expand Down
42 changes: 23 additions & 19 deletions ir/queries/queries.sql
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,17 +72,20 @@ SELECT
c.numeric_scale,
c.udt_name,
COALESCE(d.description, '') AS column_comment,
-- Always qualify non-pg_catalog types regardless of table schema
-- This is necessary because pgschema uses restricted search_path during plan
-- that may not include the type's schema (even if it matches the table's schema)
-- Exception: Extension types are left unqualified to allow search_path resolution
-- Temp schema types (pgschema_tmp_*) are re-qualified to public schema
CASE
WHEN dt.typtype IN ('d', 'e', 'c', 'b') THEN
WHEN dt.typtype = 'd' THEN
CASE WHEN dn.nspname = c.table_schema THEN dt.typname
ELSE dn.nspname || '.' || dt.typname
END
WHEN dt.typtype = 'e' OR dt.typtype = 'c' THEN
CASE WHEN dn.nspname = c.table_schema THEN dt.typname
ELSE dn.nspname || '.' || dt.typname
END
WHEN dt.typtype = 'b' THEN
-- Base types: qualify if not in pg_catalog or table's schema
CASE
WHEN dn.nspname = 'pg_catalog' THEN c.udt_name
WHEN dep.objid IS NOT NULL THEN dt.typname -- Extension type: unqualified
WHEN dn.nspname LIKE 'pgschema_tmp_%' THEN 'public.' || dt.typname -- Temp schema: re-qualify to public
WHEN dn.nspname = c.table_schema THEN dt.typname
ELSE dn.nspname || '.' || dt.typname
END
Comment on lines +84 to 90
Copy link

Copilot AI Nov 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The base type qualification logic (typtype = 'b') is missing from the generated code in queries.sql.go. The generated file still uses the old single CASE statement for domains, enums, and composite types but doesn't include the base type handling. This inconsistency could lead to incorrect type qualification for base types.

Copilot uses AI. Check for mistakes.
ELSE c.udt_name
Expand All @@ -107,7 +110,6 @@ LEFT JOIN pg_attribute a ON a.attrelid = cl.oid AND a.attname = c.column_name
LEFT JOIN pg_attrdef ad ON ad.adrelid = a.attrelid AND ad.adnum = a.attnum
LEFT JOIN pg_type dt ON dt.oid = a.atttypid
LEFT JOIN pg_namespace dn ON dt.typnamespace = dn.oid
LEFT JOIN pg_depend dep ON dep.objid = dt.oid AND dep.deptype = 'e'
WHERE
c.table_schema NOT IN ('information_schema', 'pg_catalog', 'pg_toast')
AND c.table_schema NOT LIKE 'pg_temp_%'
Expand All @@ -132,17 +134,20 @@ SELECT
c.numeric_scale,
c.udt_name,
COALESCE(d.description, '') AS column_comment,
-- Always qualify non-pg_catalog types regardless of table schema
-- This is necessary because pgschema uses restricted search_path during plan
-- that may not include the type's schema (even if it matches the table's schema)
-- Exception: Extension types are left unqualified to allow search_path resolution
-- Temp schema types (pgschema_tmp_*) are re-qualified to public schema
CASE
WHEN dt.typtype IN ('d', 'e', 'c', 'b') THEN
WHEN dt.typtype = 'd' THEN
CASE WHEN dn.nspname = c.table_schema THEN dt.typname
ELSE dn.nspname || '.' || dt.typname
END
WHEN dt.typtype = 'e' OR dt.typtype = 'c' THEN
CASE WHEN dn.nspname = c.table_schema THEN dt.typname
ELSE dn.nspname || '.' || dt.typname
END
WHEN dt.typtype = 'b' THEN
-- Base types: qualify if not in pg_catalog or table's schema
CASE
WHEN dn.nspname = 'pg_catalog' THEN c.udt_name
WHEN dep.objid IS NOT NULL THEN dt.typname -- Extension type: unqualified
WHEN dn.nspname LIKE 'pgschema_tmp_%' THEN 'public.' || dt.typname -- Temp schema: re-qualify to public
WHEN dn.nspname = c.table_schema THEN dt.typname
ELSE dn.nspname || '.' || dt.typname
END
ELSE c.udt_name
Expand All @@ -167,8 +172,7 @@ LEFT JOIN pg_attribute a ON a.attrelid = cl.oid AND a.attname = c.column_name
LEFT JOIN pg_attrdef ad ON ad.adrelid = a.attrelid AND ad.adnum = a.attnum
LEFT JOIN pg_type dt ON dt.oid = a.atttypid
LEFT JOIN pg_namespace dn ON dt.typnamespace = dn.oid
LEFT JOIN pg_depend dep ON dep.objid = dt.oid AND dep.deptype = 'e'
WHERE
WHERE
c.table_schema = $1
ORDER BY c.table_name, c.ordinal_position;

Expand Down
40 changes: 15 additions & 25 deletions ir/queries/queries.sql.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

6 changes: 3 additions & 3 deletions testdata/diff/create_table/add_column_custom_type/diff.sql
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,3 @@
ALTER TABLE users ADD COLUMN email citext NOT NULL;
ALTER TABLE users ADD COLUMN description public.custom_text;
ALTER TABLE users ADD COLUMN status public.status_enum DEFAULT 'active';
ALTER TABLE users ADD COLUMN email email_address NOT NULL;

ALTER TABLE users ADD COLUMN status user_status DEFAULT 'active' NOT NULL;
19 changes: 7 additions & 12 deletions testdata/diff/create_table/add_column_custom_type/new.sql
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,18 +1,13 @@
-- New state: Add columns using extension type, custom domain, and enum
-- Types are created via setup.sql
-- This tests GitHub #144 fix and PR #145 functionality:
-- - Extension types (citext) should be unqualified
-- - Custom domains (custom_text) should be schema-qualified
-- - Enums (status_enum) should be schema-qualified
-- New state: Add columns using custom types from setup.sql
-- Types (email_address, user_status) are created in setup.sql
-- Use unqualified names - types will be resolved via setup.sql context
Copy link

Copilot AI Nov 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment on lines 1-3 is outdated and contradicts the actual behavior. The comment states 'Use unqualified names - types will be resolved via setup.sql context', but with the new logic, types are qualified based on whether their schema matches the table's schema. Since both types are in the public schema (same as the table), they remain unqualified, but this is due to schema matching, not setup.sql context resolution.

Suggested change
-- Use unqualified names - types will be resolved via setup.sql context
-- Unqualified names are used because the types' schema matches the table's schema (public)

Copilot uses AI. Check for mistakes.

-- Modified table with new columns using custom types
CREATE TABLE public.users (
id bigint PRIMARY KEY,
username text NOT NULL,
created_at timestamp DEFAULT CURRENT_TIMESTAMP,
-- Extension type: should be unqualified
email citext NOT NULL,
-- Custom domain: should be qualified as public.custom_text
description custom_text,
-- Enum type: should be qualified as public.status_enum
status status_enum DEFAULT 'active'
-- New columns using types from setup.sql
email email_address NOT NULL,
status user_status NOT NULL DEFAULT 'active'
);
12 changes: 3 additions & 9 deletions testdata/diff/create_table/add_column_custom_type/plan.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,25 +3,19 @@
"pgschema_version": "1.4.1",
"created_at": "1970-01-01T00:00:00Z",
"source_fingerprint": {
"hash": "b7273391888b1ccf9500e9687c8ef201e6e5534c9325db5026a9eab3b01fdc35"
"hash": "4c7808528324af927b7ef815b8b70beac95edd62ed24c6da247a89cdf74ebb0f"
},
"groups": [
{
"steps": [
{
"sql": "ALTER TABLE users ADD COLUMN email citext NOT NULL;",
"sql": "ALTER TABLE users ADD COLUMN email email_address NOT NULL;",
"type": "table.column",
"operation": "create",
"path": "public.users.email"
},
{
"sql": "ALTER TABLE users ADD COLUMN description public.custom_text;",
"type": "table.column",
"operation": "create",
"path": "public.users.description"
},
{
"sql": "ALTER TABLE users ADD COLUMN status public.status_enum DEFAULT 'active';",
"sql": "ALTER TABLE users ADD COLUMN status user_status DEFAULT 'active' NOT NULL;",
"type": "table.column",
"operation": "create",
"path": "public.users.status"
Expand Down
6 changes: 2 additions & 4 deletions testdata/diff/create_table/add_column_custom_type/plan.sql
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,3 @@
ALTER TABLE users ADD COLUMN email citext NOT NULL;
ALTER TABLE users ADD COLUMN email email_address NOT NULL;

ALTER TABLE users ADD COLUMN description public.custom_text;

ALTER TABLE users ADD COLUMN status public.status_enum DEFAULT 'active';
ALTER TABLE users ADD COLUMN status user_status DEFAULT 'active' NOT NULL;
7 changes: 2 additions & 5 deletions testdata/diff/create_table/add_column_custom_type/plan.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,15 +5,12 @@ Summary by type:

Tables:
~ users
+ description (column)
+ email (column)
+ status (column)

DDL to be executed:
--------------------------------------------------

ALTER TABLE users ADD COLUMN email citext NOT NULL;
ALTER TABLE users ADD COLUMN email email_address NOT NULL;

ALTER TABLE users ADD COLUMN description public.custom_text;

ALTER TABLE users ADD COLUMN status public.status_enum DEFAULT 'active';
ALTER TABLE users ADD COLUMN status user_status DEFAULT 'active' NOT NULL;
20 changes: 12 additions & 8 deletions testdata/diff/create_table/add_column_custom_type/setup.sql
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,14 @@
-- Setup: Create extension type, custom domain, and enum to test type qualification
-- This reproduces GitHub #144 and validates PR #145 fixes
-- Extension types (citext) should be unqualified (search_path resolution)
-- Custom domains and enums should be schema-qualified (public.*)
-- Setup: Create types in public schema
-- This simulates extension types like citext installed in public schema
Copy link

Copilot AI Nov 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment 'This simulates extension types like citext installed in public schema' is misleading. The types being created (composite type and enum) are not extension types and don't behave like extension types. This comment should be updated to reflect that these are regular user-defined types used to test schema-based qualification logic.

Suggested change
-- This simulates extension types like citext installed in public schema
-- These are regular user-defined types (composite and enum) created in the public schema to test schema-based qualification logic.

Copilot uses AI. Check for mistakes.

CREATE EXTENSION IF NOT EXISTS citext;
CREATE TYPE public.email_address AS (
local_part text,
domain text
);

CREATE DOMAIN public.custom_text AS text;

CREATE TYPE public.status_enum AS ENUM ('active', 'inactive', 'pending');
CREATE TYPE public.user_status AS ENUM (
'active',
'inactive',
'suspended',
'pending'
);
Loading