fix: strip extension-owned type schema qualifiers to prevent false-positive diffs - #517
Conversation
…sitive diffs pgschema was emitting spurious ALTER COLUMN TYPE / CREATE TABLE diffs for columns typed with extension-owned types (e.g. pgvector's `vector`) whenever the extension resolved to a different schema between the live database and the plan-time comparison environment. The type itself hadn't changed - only the schema it happened to be installed in - but the schema-unaware qualifier comparison treated `public.vector` and `domain.vector` as different types. - ir: added ExtensionName (Column, TypeColumn, Type) and HasOwnedSequence (Column) / IsOwned (Sequence), sourced from pg_depend/pg_extension joins in the schema-scoped column/domain/composite-type/sequence queries. - internal/diff: columnsEqual, typesEqual, and the CREATE-path type/column SQL generators now strip schema qualifiers unconditionally when both sides are owned by the same extension, while still catching genuine changes (e.g. a vector dimension bump). Ordinary cross-schema types (domains, enums, contrib types installed at a fixed location) are unaffected. - internal/diff: isSerialColumn and generateSequenceSQL's OWNED BY emission now require genuine sequence ownership (HasOwnedSequence/IsOwned) rather than pattern-matching on column type + nextval() default text, fixing a related false-positive where a column merely referencing another table's sequence was treated as SERIAL. - internal/diff/topological: added a dependency edge for columns whose nextval() default targets a sequence owned by a different table, so that owner is always emitted first. - testdata/dump/sakila: restored missing ALTER SEQUENCE ... OWNED BY statements that a real pg_dump would emit; their absence was masking the ownership-detection gap above. Fixes the false-positive/false-negative pair end-to-end: a plan-time schema difference for an extension-owned type produces no diff, while a genuine change (dimension, real cross-schema move of a non-extension type) still does.
|
@ayusssmaan please create an issue first and reference it from the PR. Thx |
Greptile SummaryThis PR enriches schema introspection with extension and sequence ownership metadata, normalizes extension-owned type comparisons and DDL, and adds shared-sequence dependency ordering.
Confidence Score: 2/5The PR should not be merged until extension type identity, modified standalone sequence handling, and schema-aware shared-sequence ordering are corrected. Generated DDL can fail or bind the wrong type outside the configured search_path, structural changes to referenced-but-unowned sequences can be silently omitted, and same-named sequences across schemas can produce an incorrect table creation order. internal/diff/table.go, internal/diff/type.go, internal/diff/diff.go, internal/diff/topological.go
|
| Filename | Overview |
|---|---|
| internal/diff/table.go | Extension-aware column generation fixes false diffs but emits bare extension types that may not resolve to the intended target type. |
| internal/diff/diff.go | Added and dropped sequence handling uses real ownership, while modified sequences retain the fallback owner-name heuristic and can lose structural changes. |
| internal/diff/topological.go | Adds shared-sequence ordering, but schema-less sequence keys collide across schemas. |
| internal/diff/type.go | Composite and domain comparisons account for extension ownership, while composite DDL shares the unqualified-type resolution issue. |
| ir/queries/queries.sql | Adds coordinated catalog queries for extension and sequence dependency metadata. |
| ir/inspector.go | Maps the new catalog-derived ownership fields into the IR consistently. |
| ir/ir.go | Extends columns, sequences, and types with ownership metadata consumed by diff generation. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart LR
Catalog[PostgreSQL catalogs] --> Inspector[IR Inspector]
Inspector --> Metadata[Extension and sequence ownership metadata]
Metadata --> Compare[Schema comparison]
Compare --> DDL[DDL generation]
Metadata --> Ordering[Sequence dependency ordering]
Ordering --> DDL
DDL --> Apply[Apply migration with target search_path]
Comments Outside Diff (1)
-
internal/diff/diff.go, line 1109-1110 (link)Referenced sequences remain misclassified
When a standalone or shared sequence is referenced by a column default without a real ownership dependency, the fallback owner names make this branch treat it as owned and skip structural comparison, causing requested increment, bounds, cycle, or cache changes to be silently omitted.
Context Used: CLAUDE.md (source)
Knowledge Base Used:
Reviews (1): Last reviewed commit: "fix: strip extension-owned type schema q..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if column.ExtensionName != "" { | ||
| columnType = stripAnySchemaPrefix(columnType) | ||
| } else { | ||
| columnType = stripSchemaPrefix(columnType, targetSchema) |
There was a problem hiding this comment.
When an extension-owned type is installed outside the target schema and public, stripping its schema makes PostgreSQL resolve the bare name through search_path, causing the migration to fail with a missing type or bind the column to an unintended same-named type.
Context Used: CLAUDE.md (source)
Knowledge Base Used:
| sequenceOwnerTable := make(map[string]string) | ||
| for key, table := range tableMap { | ||
| for _, col := range table.Columns { | ||
| if col.HasOwnedSequence { | ||
| if seqName := nextvalTargetSequenceName(col); seqName != "" { | ||
| sequenceOwnerTable[seqName] = key | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
When two managed schemas contain same-named owned sequences, this bare-name map overwrites one owner and a qualified nextval reference can receive an edge to the wrong table, causing the referencing table to be created before the table whose SERIAL definition creates the actual sequence.
Knowledge Base Used: Diff Engine
There was a problem hiding this comment.
Pull request overview
Fixes false-positive diffs (and occasional apply failures) caused by schema-qualified comparisons of extension-owned types (e.g. public.vector vs domain.vector), and corrects SERIAL/sequence ownership detection so shared/referenced sequences aren’t misclassified as column-owned.
Changes:
- Track extension ownership for column/types and true sequence ownership via
pg_depend/pg_extension, and use this metadata to ignore schema-only qualifier mismatches for extension-owned types. - Require genuine ownership (
HasOwnedSequence/IsOwned) for SERIAL inference andOWNED BYemission; add a topological dependency edge for tables that reference another table’s owned sequence vianextval(). - Add/adjust tests and fixtures (including restoring missing
ALTER SEQUENCE ... OWNED BYin sakila pgdump fixture).
Reviewed changes
Copilot reviewed 22 out of 25 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| testdata/dump/sakila/pgdump.sql | Restores expected ALTER SEQUENCE ... OWNED BY statements to match real pg_dump behavior and validate ownership handling. |
| testdata/diff/create_table/add_column_cross_schema_custom_type/new.sql | Clarifies fixture intent for extension-owned types vs ordinary cross-schema types. |
| testdata/diff/create_table/add_column_cross_schema_custom_type/diff.sql | Updates expected diff to drop schema qualifier for extension-owned type (hstore). |
| testdata/diff/alter_column/extension_owned_type_no_diff/old.sql | New fixture reproducing extension-owned type schema mismatch on the “old/live” side. |
| testdata/diff/alter_column/extension_owned_type_no_diff/new.sql | New fixture reproducing extension-owned type schema mismatch on the “new/plan” side. |
| testdata/diff/alter_column/extension_owned_type_no_diff/plan.txt | New expected output asserting no changes are detected. |
| testdata/diff/alter_column/extension_owned_type_no_diff/plan.json | New expected plan JSON for the no-diff case. |
| ir/queries/queries.sql | Adds extension_name, has_owned_sequence, and is_owned signals to inspector queries. |
| ir/queries/queries.sql.go | Regenerates sqlc output for new query columns/fields. |
| ir/queries/queries_test.go | Adds coverage for extension-owned type detection across schemas. |
| ir/ir.go | Extends IR structs with extension ownership and true ownership flags. |
| ir/inspector.go | Populates new IR fields from query results (extension name, owned-sequence flags). |
| internal/diff/column.go | Strips arbitrary schema qualifiers when both sides share the same owning extension; keeps semantic diffs (e.g. dimension changes). |
| internal/diff/type.go | Extends type equality/generation to strip qualifiers for extension-owned composite/domain type references. |
| internal/diff/table.go | Applies extension-owned stripping in CREATE/ALTER generation paths; updates SERIAL detection to require genuine ownership. |
| internal/diff/sequence.go | Gates OWNED BY emission on Sequence.IsOwned. |
| internal/diff/diff.go | Uses the new ownership signals to avoid implicit-sequence handling when the column doesn’t truly own the sequence; updates typesEqual call signature. |
| internal/diff/topological.go | Adds dependency edges for tables that reference another table’s owned sequence via nextval() defaults. |
| internal/diff/topological_test.go | Adds test ensuring shared-sequence references are ordered after the owning table. |
| internal/diff/serial_ownership_test.go | Adds focused unit tests for isSerialColumn, generateSequenceSQL ownership gating, and columnHasOwnedSequence. |
| internal/diff/qualify_schema_test.go | Updates tests to set IsOwned where OWNED BY is expected. |
| internal/diff/identifier_quote_test.go | Updates tests to set IsOwned where OWNED BY is expected. |
| internal/diff/extension_type_test.go | Adds tests for extension-owned schema-qualifier stripping across compare and DDL generation paths. |
Files not reviewed (1)
- ir/queries/queries.sql.go: Generated file
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if column.ExtensionName != "" { | ||
| dataType = stripAnySchemaPrefix(dataType) | ||
| } else { | ||
| dataType = stripSchemaPrefixMode(dataType, targetSchema, qualifySchema) | ||
| } |
| // nextvalSequenceNamePattern extracts the bare sequence name from a nextval() | ||
| // default expression, e.g. nextval('"questionReference_questionRefId_seq"'::regclass) | ||
| // or nextval('domain.some_seq'::regclass) -> "some_seq" (schema prefix and quoting | ||
| // stripped, matching how sequence names are keyed elsewhere in this package). | ||
| var nextvalSequenceNamePattern = regexp.MustCompile(`nextval\('([^']+)'`) | ||
|
|
||
| // nextvalTargetSequenceName returns the bare sequence name a column's nextval() | ||
| // default targets, or "" if the column has no such default. | ||
| func nextvalTargetSequenceName(column *ir.Column) string { | ||
| if column.DefaultValue == nil { | ||
| return "" | ||
| } | ||
| m := nextvalSequenceNamePattern.FindStringSubmatch(*column.DefaultValue) | ||
| if m == nil { | ||
| return "" | ||
| } | ||
| name := m[1] | ||
| // Strip an optional schema prefix (quoted or bare) before the sequence name. | ||
| if idx := strings.LastIndex(name, "."); idx != -1 { | ||
| name = name[idx+1:] | ||
| } | ||
| return strings.Trim(name, `"`) | ||
| } |
Fixes #518
pgschema was emitting spurious ALTER COLUMN TYPE / CREATE TABLE diffs for columns typed with extension-owned types (e.g. pgvector's
vector) whenever the extension resolved to a different schema between the live database and the plan-time comparison environment. The type itself hadn't changed - only the schema it happened to be installed in - but the schema-unaware qualifier comparison treatedpublic.vectoranddomain.vectoras different types.Fixes the false-positive/false-negative pair end-to-end: a plan-time schema difference for an extension-owned type produces no diff, while a genuine change (dimension, real cross-schema move of a non-extension type) still does.