From 87dce7e973403447215602cda342f9db407775d0 Mon Sep 17 00:00:00 2001 From: Tianzhou Date: Fri, 16 Oct 2026 00:30:08 +0800 Subject: [PATCH] fix: batch per-row inspector lookups into the schema queries (#644, #654) The inspector issued one query per privilege row to resolve the grantee name, one per constraint column for its position in the key, one per owned sequence for the identity flag, and one per view for its column list. Over a high-latency connection that made dump and plan take minutes: the 200-table reproduction in #644 needed 4,028 queries per inspection, and plan inspects twice. Each value now comes back from the batched query it belongs to: - GetPrivilegesForSchema / GetColumnPrivilegesForSchema LEFT JOIN pg_roles on the aclexplode grantee and return grantee_name ('PUBLIC' for oid 0, empty when the role is gone, which keeps the skip behavior). - GetConstraintsForSchema returns array_position(conkey, attnum) as constraint_position for PK/UNIQUE/FK constraints. - GetSequencesForSchema returns owned_by_identity from the owning column's attidentity. - GetViewsForSchema returns column_names aggregated from pg_attribute. The reproduction drops from 4,028 to 30 queries; the employee fixture from 164 to 30. Dump output is unchanged on both. This also fixes a correctness bug: the old position lookup keyed on (schema, constraint name, column) without the table, so two tables with a same-named foreign key on the same columns could get each other's column order. New dump fixture issue_644_fk_column_order covers it. GetIndexesForSchema coalesces parent_index / parent_index_schema so that regenerating with sqlc v1.30.0 yields the nullable type the inspector already uses; the generated code had drifted from the generator. Co-Authored-By: Claude Fable 5.1 --- cmd/dump/dump_integration_test.go | 7 + ir/inspector.go | 142 +++--------------- ir/queries/queries.sql | 85 ++++++++--- ir/queries/queries.sql.go | 99 +++++++++--- .../issue_644_fk_column_order/manifest.json | 9 ++ .../dump/issue_644_fk_column_order/pgdump.sql | 91 +++++++++++ .../issue_644_fk_column_order/pgschema.sql | 39 +++++ .../dump/issue_644_fk_column_order/raw.sql | 23 +++ 8 files changed, 328 insertions(+), 167 deletions(-) create mode 100644 testdata/dump/issue_644_fk_column_order/manifest.json create mode 100644 testdata/dump/issue_644_fk_column_order/pgdump.sql create mode 100644 testdata/dump/issue_644_fk_column_order/pgschema.sql create mode 100644 testdata/dump/issue_644_fk_column_order/raw.sql diff --git a/cmd/dump/dump_integration_test.go b/cmd/dump/dump_integration_test.go index 505f14ed..81690f63 100644 --- a/cmd/dump/dump_integration_test.go +++ b/cmd/dump/dump_integration_test.go @@ -240,6 +240,13 @@ func TestDumpCommand_NameTypeNotDumpedAsCharArray(t *testing.T) { } } +func TestDumpCommand_Issue644FKColumnOrder(t *testing.T) { + if testing.Short() { + t.Skip("Skipping integration test in short mode") + } + runExactMatchTest(t, "issue_644_fk_column_order") +} + func TestDumpCommand_Issue318CrossSchemaComment(t *testing.T) { if testing.Short() { t.Skip("Skipping integration test in short mode") diff --git a/ir/inspector.go b/ir/inspector.go index c7e0a4bd..7ccd0f0d 100644 --- a/ir/inspector.go +++ b/ir/inspector.go @@ -548,8 +548,7 @@ func (i *Inspector) buildConstraints(ctx context.Context, schema *IR, targetSche table.AllConstraintNames[constraintName] = true } - // Skip ignored constraints early to avoid the per-column position - // queries below for constraints that would be discarded anyway. + // Skip ignored constraints early; they would be discarded anyway. if i.ignoreConfig != nil && i.ignoreConfig.ShouldIgnoreConstraint(constraintName) { continue } @@ -681,8 +680,8 @@ func (i *Inspector) buildConstraints(ctx context.Context, schema *IR, targetSche // Add column to constraint (skip for column-less constraints like CHECK (FALSE)) if columnName != "" && columnName != "" { - // Get column position in constraint - position := i.getConstraintColumnPosition(ctx, schemaName, constraintName, columnName) + // Column position within the constraint key, from the batched query + position := int(constraint.ConstraintPosition.Int32) // Check if column already exists in constraint to avoid duplicates columnExists := false @@ -1070,8 +1069,8 @@ func (i *Inspector) buildSequences(ctx context.Context, schema *IR, targetSchema // Skip sequences that are owned by identity columns // Identity sequences should be managed through the identity column, not as separate sequences if sequence.OwnedByTable != "" && sequence.OwnedByColumn != "" { - // Check if the owning column is an identity column - if i.isIdentityColumn(ctx, seq.SequenceSchema.String, sequence.OwnedByTable, sequence.OwnedByColumn) { + // The batched query reports whether the owning column is an identity column + if seq.OwnedByIdentity.Valid && seq.OwnedByIdentity.Bool { // Skip this sequence - it's managed by the identity column continue } @@ -1083,24 +1082,6 @@ func (i *Inspector) buildSequences(ctx context.Context, schema *IR, targetSchema return nil } -// isIdentityColumn checks if a column is an identity column -func (i *Inspector) isIdentityColumn(ctx context.Context, schemaName, tableName, columnName string) bool { - query := ` - SELECT is_identity - FROM information_schema.columns - WHERE table_schema = $1 - AND table_name = $2 - AND column_name = $3` - - var isIdentity string - err := i.db.QueryRowContext(ctx, query, schemaName, tableName, columnName).Scan(&isIdentity) - if err != nil { - return false - } - - return isIdentity == "YES" -} - func (i *Inspector) buildFunctions(ctx context.Context, schema *IR, targetSchema string) error { functions, err := i.queries.GetFunctionsForSchema(ctx, sql.NullString{String: targetSchema, Valid: true}) if err != nil { @@ -1841,11 +1822,8 @@ func (i *Inspector) buildViews(ctx context.Context, schema *IR, targetSchema str definition = strings.TrimSuffix(definition, ";") } - // Fetch view column names from pg_attribute (ordered by attnum) - columns, err := i.getViewColumns(ctx, schemaName, viewName) - if err != nil { - return fmt.Errorf("failed to get columns for view %s.%s: %w", schemaName, viewName, err) - } + // View column names in attnum order, from the batched query + columns := view.ColumnNames // Copy and sort reloptions for deterministic comparison and output options := append([]string(nil), view.Reloptions...) @@ -1867,37 +1845,6 @@ func (i *Inspector) buildViews(ctx context.Context, schema *IR, targetSchema str return nil } -// getViewColumns returns the ordered list of column names for a view or materialized view. -// Uses pg_attribute to get column names ordered by their position (attnum). -func (i *Inspector) getViewColumns(ctx context.Context, schemaName, viewName string) ([]string, error) { - query := ` - SELECT a.attname - FROM pg_attribute a - JOIN pg_class c ON a.attrelid = c.oid - JOIN pg_namespace n ON c.relnamespace = n.oid - WHERE n.nspname = $1 - AND c.relname = $2 - AND a.attnum > 0 - AND NOT a.attisdropped - ORDER BY a.attnum` - - rows, err := i.db.QueryContext(ctx, query, schemaName, viewName) - if err != nil { - return nil, err - } - defer rows.Close() - - var columns []string - for rows.Next() { - var colName string - if err := rows.Scan(&colName); err != nil { - return nil, err - } - columns = append(columns, colName) - } - return columns, rows.Err() -} - // extractWhenClauseFromTriggerDef extracts the WHEN clause from a trigger definition // returned by pg_get_triggerdef(). The format is: // "CREATE TRIGGER name ... WHEN (condition) EXECUTE FUNCTION ..." @@ -2456,23 +2403,6 @@ func (i *Inspector) buildTypes(ctx context.Context, schema *IR, targetSchema str // Helper methods -func (i *Inspector) getConstraintColumnPosition(ctx context.Context, schemaName, constraintName, columnName string) int { - query := ` - SELECT kcu.ordinal_position - FROM information_schema.key_column_usage kcu - WHERE kcu.table_schema = $1 - AND kcu.constraint_name = $2 - AND kcu.column_name = $3` - - var position int - err := i.db.QueryRowContext(ctx, query, schemaName, constraintName, columnName).Scan(&position) - if err != nil { - return 0 // Default position if query fails - } - - return position -} - // pgCatalogTriggerFunctions contains the hard-coded list of built-in PostgreSQL trigger functions // that exist in the pg_catalog schema. These functions should always be schema-qualified // to maintain consistency between parser and inspector outputs. @@ -2581,31 +2511,11 @@ func (i *Inspector) buildPrivileges(ctx context.Context, schema *IR, targetSchem owner := row.Owner.String isGrantable := row.IsGrantable.Valid && row.IsGrantable.Bool - // Determine grantee name from OID - grantee := "PUBLIC" - if row.GranteeOid != nil { - // Handle different possible types for grantee OID - var granteeOID int64 - switch v := row.GranteeOid.(type) { - case int64: - granteeOID = v - case int32: - granteeOID = int64(v) - case int: - granteeOID = int64(v) - } - if granteeOID != 0 { - // Look up role name - var roleName string - err := i.db.QueryRowContext(ctx, "SELECT rolname FROM pg_roles WHERE oid = $1", granteeOID).Scan(&roleName) - if err != nil { - if err == sql.ErrNoRows { - continue // Role no longer exists, skip this privilege - } - return fmt.Errorf("failed to lookup role name for OID %d: %w", granteeOID, err) - } - grantee = roleName - } + // The query resolves the grantee ("PUBLIC" for the public pseudo-role); + // an empty name means the role no longer exists, so skip the privilege. + grantee := row.GranteeName.String + if grantee == "" { + continue } // Skip owner grants (owners implicitly have all privileges) @@ -2947,29 +2857,11 @@ func (i *Inspector) buildColumnPrivileges(ctx context.Context, schema *IR, targe privilegeType := row.PrivilegeType.String isGrantable := row.IsGrantable.Valid && row.IsGrantable.Bool - // Determine grantee name from OID - grantee := "PUBLIC" - if row.GranteeOid != nil { - var granteeOID int64 - switch v := row.GranteeOid.(type) { - case int64: - granteeOID = v - case int32: - granteeOID = int64(v) - case int: - granteeOID = int64(v) - } - if granteeOID != 0 { - var roleName string - err := i.db.QueryRowContext(ctx, "SELECT rolname FROM pg_roles WHERE oid = $1", granteeOID).Scan(&roleName) - if err != nil { - if err == sql.ErrNoRows { - continue // Role no longer exists, skip this privilege - } - return fmt.Errorf("failed to lookup role name for OID %d: %w", granteeOID, err) - } - grantee = roleName - } + // The query resolves the grantee ("PUBLIC" for the public pseudo-role); + // an empty name means the role no longer exists, so skip the privilege. + grantee := row.GranteeName.String + if grantee == "" { + continue } // Skip column privileges for ignored grantees diff --git a/ir/queries/queries.sql b/ir/queries/queries.sql index 60ec9800..c6f8b19c 100644 --- a/ir/queries/queries.sql +++ b/ir/queries/queries.sql @@ -564,8 +564,8 @@ WITH index_base AS ( n.nspname as schemaname, t.relname as tablename, i.relname as indexname, - pi.relname AS parent_index, - pn.nspname AS parent_index_schema, + COALESCE(pi.relname, '') AS parent_index, + COALESCE(pn.nspname, '') AS parent_index_schema, idx.indisunique as is_unique, idx.indisprimary as is_primary, (idx.indpred IS NOT NULL) as is_partial, @@ -1380,6 +1380,14 @@ SELECT END AS constraint_type, COALESCE(a.attname, '') AS column_name, COALESCE(a.attnum, 0) AS ordinal_position, + -- 1-based position of the column within the constraint key, for the key + -- constraints whose column order matters. Computed here from conkey rather + -- than with one information_schema.key_column_usage lookup per column, + -- which also keyed on constraint name without the table and could return + -- another table's position for a same-named foreign key (issue #644). + (CASE WHEN c.contype IN ('p', 'u', 'f') + THEN COALESCE(array_position(c.conkey, a.attnum), 0) + ELSE 0 END)::int AS constraint_position, COALESCE(fn.nspname, '') AS foreign_table_schema, COALESCE(fcl.relname, '') AS foreign_table_name, COALESCE(fa.attname, '') AS foreign_column_name, @@ -1472,6 +1480,9 @@ SELECT s.cache_size, COALESCE(dep_table.relname, '') AS owned_by_table, COALESCE(dep_col.attname, '') AS owned_by_column, + -- Whether the owning column is an identity column, so identity-backed + -- sequences are skipped without a per-sequence lookup (issue #644). + COALESCE(dep_col.attidentity IN ('a', 'd'), false)::boolean AS owned_by_identity, COALESCE(obj_description(c.oid, 'pg_class'), '') AS sequence_comment FROM pg_sequences s LEFT JOIN pg_namespace n ON n.nspname = s.schemaname @@ -1747,13 +1758,23 @@ SELECT sp.view_def AS view_definition, vd.view_comment, vd.is_materialized, - vd.reloptions + vd.reloptions, + -- Column names in attnum order, folded in here rather than fetched with + -- one pg_attribute query per view (issue #644). + COALESCE(cols.column_names, '{}'::text[])::text[] AS column_names FROM view_definitions vd CROSS JOIN LATERAL ( SELECT set_config('search_path', vd.view_schema || ', pg_catalog', true) as dummy, pg_get_viewdef(vd.view_oid, true) as view_def ) sp +LEFT JOIN LATERAL ( + SELECT array_agg(a.attname::text ORDER BY a.attnum) AS column_names + FROM pg_attribute a + WHERE a.attrelid = vd.view_oid + AND a.attnum > 0 + AND NOT a.attisdropped +) cols ON true ORDER BY vd.table_schema, vd.table_name; -- GetTriggersForSchema retrieves all triggers for a specific schema @@ -2145,16 +2166,30 @@ WITH acl_data AS ( ) AND t.typacl IS NOT NULL ) +, exploded AS ( + SELECT + schema_name, + object_name, + object_type, + owner, + (aclexplode(acl)).grantee AS grantee, + (aclexplode(acl)).privilege_type AS privilege_type, + (aclexplode(acl)).is_grantable AS is_grantable + FROM acl_data +) SELECT - schema_name, - object_name, - object_type, - (aclexplode(acl)).grantee AS grantee_oid, - (aclexplode(acl)).privilege_type AS privilege_type, - (aclexplode(acl)).is_grantable AS is_grantable, - owner -FROM acl_data -ORDER BY object_type, object_name, grantee_oid, privilege_type; + e.schema_name, + e.object_name, + e.object_type, + -- Grantee resolved here rather than with one pg_roles lookup per row + -- (issue #644). Empty when the grantee role no longer exists. + COALESCE(CASE WHEN e.grantee = 0 THEN 'PUBLIC' ELSE r.rolname END, '')::text AS grantee_name, + e.privilege_type, + e.is_grantable, + e.owner +FROM exploded e +LEFT JOIN pg_roles r ON r.oid = e.grantee +ORDER BY e.object_type, e.object_name, e.grantee, e.privilege_type; -- GetRevokedDefaultPrivilegesForSchema finds objects where default PUBLIC grants have been explicitly revoked -- name: GetRevokedDefaultPrivilegesForSchema :many @@ -2272,14 +2307,26 @@ WITH column_acls AS ( AND NOT a.attisdropped AND a.attacl IS NOT NULL -- only columns with explicit ACL ) +, exploded AS ( + SELECT + table_name, + column_name, + (aclexplode(acl)).grantee AS grantee, + (aclexplode(acl)).privilege_type AS privilege_type, + (aclexplode(acl)).is_grantable AS is_grantable + FROM column_acls +) SELECT - table_name, - column_name, - (aclexplode(acl)).grantee AS grantee_oid, - (aclexplode(acl)).privilege_type AS privilege_type, - (aclexplode(acl)).is_grantable AS is_grantable -FROM column_acls -ORDER BY table_name, column_name, grantee_oid, privilege_type; + e.table_name, + e.column_name, + -- Grantee resolved here rather than with one pg_roles lookup per row + -- (issue #644). Empty when the grantee role no longer exists. + COALESCE(CASE WHEN e.grantee = 0 THEN 'PUBLIC' ELSE r.rolname END, '')::text AS grantee_name, + e.privilege_type, + e.is_grantable +FROM exploded e +LEFT JOIN pg_roles r ON r.oid = e.grantee +ORDER BY e.table_name, e.column_name, e.grantee, e.privilege_type; -- GetFunctionDependencies retrieves function-to-function dependencies for topological sorting -- name: GetFunctionDependencies :many diff --git a/ir/queries/queries.sql.go b/ir/queries/queries.sql.go index 2dfc73e3..f5d1cef0 100644 --- a/ir/queries/queries.sql.go +++ b/ir/queries/queries.sql.go @@ -342,20 +342,32 @@ WITH column_acls AS ( AND NOT a.attisdropped AND a.attacl IS NOT NULL -- only columns with explicit ACL ) +, exploded AS ( + SELECT + table_name, + column_name, + (aclexplode(acl)).grantee AS grantee, + (aclexplode(acl)).privilege_type AS privilege_type, + (aclexplode(acl)).is_grantable AS is_grantable + FROM column_acls +) SELECT - table_name, - column_name, - (aclexplode(acl)).grantee AS grantee_oid, - (aclexplode(acl)).privilege_type AS privilege_type, - (aclexplode(acl)).is_grantable AS is_grantable -FROM column_acls -ORDER BY table_name, column_name, grantee_oid, privilege_type + e.table_name, + e.column_name, + -- Grantee resolved here rather than with one pg_roles lookup per row + -- (issue #644). Empty when the grantee role no longer exists. + COALESCE(CASE WHEN e.grantee = 0 THEN 'PUBLIC' ELSE r.rolname END, '')::text AS grantee_name, + e.privilege_type, + e.is_grantable +FROM exploded e +LEFT JOIN pg_roles r ON r.oid = e.grantee +ORDER BY e.table_name, e.column_name, e.grantee, e.privilege_type ` type GetColumnPrivilegesForSchemaRow struct { TableName string `db:"table_name" json:"table_name"` ColumnName string `db:"column_name" json:"column_name"` - GranteeOid interface{} `db:"grantee_oid" json:"grantee_oid"` + GranteeName sql.NullString `db:"grantee_name" json:"grantee_name"` PrivilegeType sql.NullString `db:"privilege_type" json:"privilege_type"` IsGrantable sql.NullBool `db:"is_grantable" json:"is_grantable"` } @@ -374,7 +386,7 @@ func (q *Queries) GetColumnPrivilegesForSchema(ctx context.Context, dollar_1 sql if err := rows.Scan( &i.TableName, &i.ColumnName, - &i.GranteeOid, + &i.GranteeName, &i.PrivilegeType, &i.IsGrantable, ); err != nil { @@ -1183,6 +1195,14 @@ SELECT END AS constraint_type, COALESCE(a.attname, '') AS column_name, COALESCE(a.attnum, 0) AS ordinal_position, + -- 1-based position of the column within the constraint key, for the key + -- constraints whose column order matters. Computed here from conkey rather + -- than with one information_schema.key_column_usage lookup per column, + -- which also keyed on constraint name without the table and could return + -- another table's position for a same-named foreign key (issue #644). + (CASE WHEN c.contype IN ('p', 'u', 'f') + THEN COALESCE(array_position(c.conkey, a.attnum), 0) + ELSE 0 END)::int AS constraint_position, COALESCE(fn.nspname, '') AS foreign_table_schema, COALESCE(fcl.relname, '') AS foreign_table_name, COALESCE(fa.attname, '') AS foreign_column_name, @@ -1269,6 +1289,7 @@ type GetConstraintsForSchemaRow struct { ConstraintType sql.NullString `db:"constraint_type" json:"constraint_type"` ColumnName sql.NullString `db:"column_name" json:"column_name"` OrdinalPosition sql.NullInt32 `db:"ordinal_position" json:"ordinal_position"` + ConstraintPosition sql.NullInt32 `db:"constraint_position" json:"constraint_position"` ForeignTableSchema sql.NullString `db:"foreign_table_schema" json:"foreign_table_schema"` ForeignTableName sql.NullString `db:"foreign_table_name" json:"foreign_table_name"` ForeignColumnName sql.NullString `db:"foreign_column_name" json:"foreign_column_name"` @@ -1303,6 +1324,7 @@ func (q *Queries) GetConstraintsForSchema(ctx context.Context, dollar_1 sql.Null &i.ConstraintType, &i.ColumnName, &i.OrdinalPosition, + &i.ConstraintPosition, &i.ForeignTableSchema, &i.ForeignTableName, &i.ForeignColumnName, @@ -2196,8 +2218,8 @@ WITH index_base AS ( n.nspname as schemaname, t.relname as tablename, i.relname as indexname, - pi.relname AS parent_index, - pn.nspname AS parent_index_schema, + COALESCE(pi.relname, '') AS parent_index, + COALESCE(pn.nspname, '') AS parent_index_schema, idx.indisunique as is_unique, idx.indisprimary as is_primary, (idx.indpred IS NOT NULL) as is_partial, @@ -2720,23 +2742,37 @@ WITH acl_data AS ( ) AND t.typacl IS NOT NULL ) +, exploded AS ( + SELECT + schema_name, + object_name, + object_type, + owner, + (aclexplode(acl)).grantee AS grantee, + (aclexplode(acl)).privilege_type AS privilege_type, + (aclexplode(acl)).is_grantable AS is_grantable + FROM acl_data +) SELECT - schema_name, - object_name, - object_type, - (aclexplode(acl)).grantee AS grantee_oid, - (aclexplode(acl)).privilege_type AS privilege_type, - (aclexplode(acl)).is_grantable AS is_grantable, - owner -FROM acl_data -ORDER BY object_type, object_name, grantee_oid, privilege_type + e.schema_name, + e.object_name, + e.object_type, + -- Grantee resolved here rather than with one pg_roles lookup per row + -- (issue #644). Empty when the grantee role no longer exists. + COALESCE(CASE WHEN e.grantee = 0 THEN 'PUBLIC' ELSE r.rolname END, '')::text AS grantee_name, + e.privilege_type, + e.is_grantable, + e.owner +FROM exploded e +LEFT JOIN pg_roles r ON r.oid = e.grantee +ORDER BY e.object_type, e.object_name, e.grantee, e.privilege_type ` type GetPrivilegesForSchemaRow struct { SchemaName sql.NullString `db:"schema_name" json:"schema_name"` ObjectName sql.NullString `db:"object_name" json:"object_name"` ObjectType sql.NullString `db:"object_type" json:"object_type"` - GranteeOid interface{} `db:"grantee_oid" json:"grantee_oid"` + GranteeName sql.NullString `db:"grantee_name" json:"grantee_name"` PrivilegeType sql.NullString `db:"privilege_type" json:"privilege_type"` IsGrantable sql.NullBool `db:"is_grantable" json:"is_grantable"` Owner sql.NullString `db:"owner" json:"owner"` @@ -2756,7 +2792,7 @@ func (q *Queries) GetPrivilegesForSchema(ctx context.Context, dollar_1 sql.NullS &i.SchemaName, &i.ObjectName, &i.ObjectType, - &i.GranteeOid, + &i.GranteeName, &i.PrivilegeType, &i.IsGrantable, &i.Owner, @@ -3530,6 +3566,9 @@ SELECT s.cache_size, COALESCE(dep_table.relname, '') AS owned_by_table, COALESCE(dep_col.attname, '') AS owned_by_column, + -- Whether the owning column is an identity column, so identity-backed + -- sequences are skipped without a per-sequence lookup (issue #644). + COALESCE(dep_col.attidentity IN ('a', 'd'), false)::boolean AS owned_by_identity, COALESCE(obj_description(c.oid, 'pg_class'), '') AS sequence_comment FROM pg_sequences s LEFT JOIN pg_namespace n ON n.nspname = s.schemaname @@ -3579,6 +3618,7 @@ type GetSequencesForSchemaRow struct { CacheSize sql.NullInt64 `db:"cache_size" json:"cache_size"` OwnedByTable sql.NullString `db:"owned_by_table" json:"owned_by_table"` OwnedByColumn sql.NullString `db:"owned_by_column" json:"owned_by_column"` + OwnedByIdentity sql.NullBool `db:"owned_by_identity" json:"owned_by_identity"` SequenceComment sql.NullString `db:"sequence_comment" json:"sequence_comment"` } @@ -3607,6 +3647,7 @@ func (q *Queries) GetSequencesForSchema(ctx context.Context, dollar_1 sql.NullSt &i.CacheSize, &i.OwnedByTable, &i.OwnedByColumn, + &i.OwnedByIdentity, &i.SequenceComment, ); err != nil { return nil, err @@ -4246,13 +4287,23 @@ SELECT sp.view_def AS view_definition, vd.view_comment, vd.is_materialized, - vd.reloptions + vd.reloptions, + -- Column names in attnum order, folded in here rather than fetched with + -- one pg_attribute query per view (issue #644). + COALESCE(cols.column_names, '{}'::text[])::text[] AS column_names FROM view_definitions vd CROSS JOIN LATERAL ( SELECT set_config('search_path', vd.view_schema || ', pg_catalog', true) as dummy, pg_get_viewdef(vd.view_oid, true) as view_def ) sp +LEFT JOIN LATERAL ( + SELECT array_agg(a.attname::text ORDER BY a.attnum) AS column_names + FROM pg_attribute a + WHERE a.attrelid = vd.view_oid + AND a.attnum > 0 + AND NOT a.attisdropped +) cols ON true ORDER BY vd.table_schema, vd.table_name ` @@ -4263,6 +4314,7 @@ type GetViewsForSchemaRow struct { ViewComment sql.NullString `db:"view_comment" json:"view_comment"` IsMaterialized sql.NullBool `db:"is_materialized" json:"is_materialized"` Reloptions []string `db:"reloptions" json:"reloptions"` + ColumnNames []string `db:"column_names" json:"column_names"` } // GetViewsForSchema retrieves all views and materialized views for a specific schema @@ -4285,6 +4337,7 @@ func (q *Queries) GetViewsForSchema(ctx context.Context, dollar_1 sql.NullString &i.ViewComment, &i.IsMaterialized, pq.Array(&i.Reloptions), + pq.Array(&i.ColumnNames), ); err != nil { return nil, err } diff --git a/testdata/dump/issue_644_fk_column_order/manifest.json b/testdata/dump/issue_644_fk_column_order/manifest.json new file mode 100644 index 00000000..6c177eac --- /dev/null +++ b/testdata/dump/issue_644_fk_column_order/manifest.json @@ -0,0 +1,9 @@ +{ + "name": "issue_644_fk_column_order", + "description": "Foreign keys with the same name on different tables keep their own column order (GitHub issue #644)", + "source": "https://github.com/pgplex/pgschema/issues/644", + "notes": [ + "c1 and c2 both have a foreign key named fk_p to p, on (a, b) and (b, a) respectively", + "Reproduces the bug where constraint column positions were looked up by (schema, constraint name, column) without the table, so c2 was dumped with c1's column order and referenced p's primary key instead of its (b, a) unique constraint" + ] +} diff --git a/testdata/dump/issue_644_fk_column_order/pgdump.sql b/testdata/dump/issue_644_fk_column_order/pgdump.sql new file mode 100644 index 00000000..a22acb57 --- /dev/null +++ b/testdata/dump/issue_644_fk_column_order/pgdump.sql @@ -0,0 +1,91 @@ +-- +-- PostgreSQL database dump +-- + + +-- Dumped from database version 18.1 (Postgres.app) +-- Dumped by pg_dump version 18.1 (Postgres.app) + +SET statement_timeout = 0; +SET lock_timeout = 0; +SET idle_in_transaction_session_timeout = 0; +-- SET transaction_timeout = 0; +SET client_encoding = 'UTF8'; +SET standard_conforming_strings = on; +SELECT pg_catalog.set_config('search_path', '', false); +SET check_function_bodies = false; +SET xmloption = content; +SET client_min_messages = warning; +SET row_security = off; + +SET default_tablespace = ''; + +SET default_table_access_method = heap; + +-- +-- Name: c1; Type: TABLE; Schema: public; Owner: - +-- + +CREATE TABLE public.c1 ( + a integer, + b integer +); + + +-- +-- Name: c2; Type: TABLE; Schema: public; Owner: - +-- + +CREATE TABLE public.c2 ( + a integer, + b integer +); + + +-- +-- Name: p; Type: TABLE; Schema: public; Owner: - +-- + +CREATE TABLE public.p ( + a integer NOT NULL, + b integer NOT NULL +); + + +-- +-- Name: p p_b_a_key; Type: CONSTRAINT; Schema: public; Owner: - +-- + +ALTER TABLE ONLY public.p + ADD CONSTRAINT p_b_a_key UNIQUE (b, a); + + +-- +-- Name: p p_pkey; Type: CONSTRAINT; Schema: public; Owner: - +-- + +ALTER TABLE ONLY public.p + ADD CONSTRAINT p_pkey PRIMARY KEY (a, b); + + +-- +-- Name: c1 fk_p; Type: FK CONSTRAINT; Schema: public; Owner: - +-- + +ALTER TABLE ONLY public.c1 + ADD CONSTRAINT fk_p FOREIGN KEY (a, b) REFERENCES public.p(a, b); + + +-- +-- Name: c2 fk_p; Type: FK CONSTRAINT; Schema: public; Owner: - +-- + +ALTER TABLE ONLY public.c2 + ADD CONSTRAINT fk_p FOREIGN KEY (b, a) REFERENCES public.p(b, a); + + +-- +-- PostgreSQL database dump complete +-- + + diff --git a/testdata/dump/issue_644_fk_column_order/pgschema.sql b/testdata/dump/issue_644_fk_column_order/pgschema.sql new file mode 100644 index 00000000..c58b112c --- /dev/null +++ b/testdata/dump/issue_644_fk_column_order/pgschema.sql @@ -0,0 +1,39 @@ +-- +-- pgschema database dump +-- + +-- Dumped from database version PostgreSQL 18.1 +-- Dumped by pgschema version 1.13.2 + + +-- +-- Name: p; Type: TABLE; Schema: -; Owner: - +-- + +CREATE TABLE IF NOT EXISTS p ( + a integer, + b integer, + CONSTRAINT p_pkey PRIMARY KEY (a, b), + CONSTRAINT p_b_a_key UNIQUE (b, a) +); + +-- +-- Name: c1; Type: TABLE; Schema: -; Owner: - +-- + +CREATE TABLE IF NOT EXISTS c1 ( + a integer, + b integer, + CONSTRAINT fk_p FOREIGN KEY (a, b) REFERENCES p (a, b) +); + +-- +-- Name: c2; Type: TABLE; Schema: -; Owner: - +-- + +CREATE TABLE IF NOT EXISTS c2 ( + a integer, + b integer, + CONSTRAINT fk_p FOREIGN KEY (b, a) REFERENCES p (b, a) +); + diff --git a/testdata/dump/issue_644_fk_column_order/raw.sql b/testdata/dump/issue_644_fk_column_order/raw.sql new file mode 100644 index 00000000..cb1ac996 --- /dev/null +++ b/testdata/dump/issue_644_fk_column_order/raw.sql @@ -0,0 +1,23 @@ +-- Two tables with a same-named foreign key (fk_p) on the same columns but in +-- different order. Foreign key names only have to be unique per table, so a +-- constraint-column position lookup keyed on (schema, constraint name, column) +-- without the table can return the other table's position and dump c2 with +-- c1's column order (GitHub issue #644). +CREATE TABLE p ( + a int, + b int, + PRIMARY KEY (a, b), + UNIQUE (b, a) +); + +CREATE TABLE c1 ( + a int, + b int, + CONSTRAINT fk_p FOREIGN KEY (a, b) REFERENCES p (a, b) +); + +CREATE TABLE c2 ( + a int, + b int, + CONSTRAINT fk_p FOREIGN KEY (b, a) REFERENCES p (b, a) +);