Skip to content

fix: batch per-row inspector lookups into the schema queries (#644, #654) #656

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
Oct 15, 2026
+328 −167
Merged

fix: batch per-row inspector lookups into the schema queries (#644, #654) #656

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
7 changes: 7 additions & 0 deletions cmd/dump/dump_integration_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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")
Expand Down
142 changes: 17 additions & 125 deletions ir/inspector.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand Down Expand Up @@ -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 != "<nil>" {
// 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
Expand Down Expand Up @@ -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
}
Expand All @@ -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 {
Expand Down Expand Up @@ -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...)
Expand All @@ -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 ..."
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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
Expand Down
85 changes: 66 additions & 19 deletions ir/queries/queries.sql
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down
Loading
Loading