From fff855d82c0c5b73b5699abcbec47bf7e734c791 Mon Sep 17 00:00:00 2001 From: tianzhou Date: Thu, 6 Nov 2025 23:42:07 -0800 Subject: [PATCH] Revert "fix: extension qualifier (#146)" This reverts commit e99d0e59c84ab320ec54f3f07b43b3246bf415a0. --- internal/diff/diff.go | 14 +------ internal/diff/table.go | 33 ++------------- ir/queries/queries.sql | 42 ++++++++++--------- ir/queries/queries.sql.go | 40 +++++++----------- .../add_column_custom_type/diff.sql | 6 +-- .../add_column_custom_type/new.sql | 19 ++++----- .../add_column_custom_type/plan.json | 12 ++---- .../add_column_custom_type/plan.sql | 6 +-- .../add_column_custom_type/plan.txt | 7 +--- .../add_column_custom_type/setup.sql | 20 +++++---- 10 files changed, 71 insertions(+), 128 deletions(-) diff --git a/internal/diff/diff.go b/internal/diff/diff.go index b81f9fe0..c17d7e75 100644 --- a/internal/diff/diff.go +++ b/internal/diff/diff.go @@ -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) diff --git a/internal/diff/table.go b/internal/diff/table.go index a34a4641..1be87df7 100644 --- a/internal/diff/table.go +++ b/internal/diff/table.go @@ -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) } } @@ -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) @@ -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 diff --git a/ir/queries/queries.sql b/ir/queries/queries.sql index 4fd64a51..aec891ff 100644 --- a/ir/queries/queries.sql +++ b/ir/queries/queries.sql @@ -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 ELSE c.udt_name @@ -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_%' @@ -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 @@ -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; diff --git a/ir/queries/queries.sql.go b/ir/queries/queries.sql.go index a214af37..838890c2 100644 --- a/ir/queries/queries.sql.go +++ b/ir/queries/queries.sql.go @@ -207,18 +207,14 @@ 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 - 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 - ELSE dn.nspname || '.' || dt.typname + 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 ELSE c.udt_name END AS resolved_type, @@ -242,7 +238,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_%' @@ -339,18 +334,14 @@ 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 - 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 - ELSE dn.nspname || '.' || dt.typname + 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 ELSE c.udt_name END AS resolved_type, @@ -374,8 +365,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 ` diff --git a/testdata/diff/create_table/add_column_custom_type/diff.sql b/testdata/diff/create_table/add_column_custom_type/diff.sql index cffb3c31..16c3cfff 100644 --- a/testdata/diff/create_table/add_column_custom_type/diff.sql +++ b/testdata/diff/create_table/add_column_custom_type/diff.sql @@ -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; diff --git a/testdata/diff/create_table/add_column_custom_type/new.sql b/testdata/diff/create_table/add_column_custom_type/new.sql index 87127881..22d5c9c1 100644 --- a/testdata/diff/create_table/add_column_custom_type/new.sql +++ b/testdata/diff/create_table/add_column_custom_type/new.sql @@ -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 +-- 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' ); diff --git a/testdata/diff/create_table/add_column_custom_type/plan.json b/testdata/diff/create_table/add_column_custom_type/plan.json index 4041d3d3..c46910be 100644 --- a/testdata/diff/create_table/add_column_custom_type/plan.json +++ b/testdata/diff/create_table/add_column_custom_type/plan.json @@ -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" diff --git a/testdata/diff/create_table/add_column_custom_type/plan.sql b/testdata/diff/create_table/add_column_custom_type/plan.sql index fd804350..16c3cfff 100644 --- a/testdata/diff/create_table/add_column_custom_type/plan.sql +++ b/testdata/diff/create_table/add_column_custom_type/plan.sql @@ -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; diff --git a/testdata/diff/create_table/add_column_custom_type/plan.txt b/testdata/diff/create_table/add_column_custom_type/plan.txt index f22481e7..0569c66a 100644 --- a/testdata/diff/create_table/add_column_custom_type/plan.txt +++ b/testdata/diff/create_table/add_column_custom_type/plan.txt @@ -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; diff --git a/testdata/diff/create_table/add_column_custom_type/setup.sql b/testdata/diff/create_table/add_column_custom_type/setup.sql index 33a28cf1..6fc1a2d6 100644 --- a/testdata/diff/create_table/add_column_custom_type/setup.sql +++ b/testdata/diff/create_table/add_column_custom_type/setup.sql @@ -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 -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' +);