From 804dabced055a806b869f918244706d8a757f1ab Mon Sep 17 00:00:00 2001 From: Vincent Huang Date: Wed, 8 Apr 2026 23:05:54 -0700 Subject: [PATCH 1/5] feat(pg/catalog): add DropsFromDiff helper for fast drop-only plans MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit DropsFromDiff returns a MigrationPlan containing only the destructive operations that GenerateMigration would produce for the same inputs, without building SQL text or running the dependency-driven topological sort. Callers that only need to know *what* will be dropped (e.g., destructive-change advisories) can skip GenerateMigration's cost — on large schemas this can save tens of seconds and over a gigabyte of transient heap. All metadata fields (Type, SchemaName, ObjectName, ParentObject, Phase, ObjType, ObjOID, Priority) are populated identically to what GenerateMigration sets; only SQL is left empty (documented contract). Operations are sorted lexicographically by (SchemaName, ObjectName, ParentObject, Type) for determinism instead of topologically. This commit adds the helper skeleton plus 8 of ~17 producer arms, each mirroring the DiffDrop (and where applicable DiffModify) arm of its corresponding generate*DDL function field-for-field: - dropsForSchemas (migration_schema.go) - dropsForEnums (migration_enum.go) - dropsForDomains (migration_domain.go) - dropsForRanges (migration_range.go) - dropsForComposites (migration_composite.go) [ObjType='r', not 't'] - dropsForSequences (migration_sequence.go) [isIdentitySequence filter] - dropsForFunctions (migration_function.go) [entry.Identity, signatureChanged] - dropsForTables (migration_table.go) [RelKind 'r'/'p' only] Remaining producers (table recreate, columns + cascaded checks, constraints, triggers, views/matviews, dependent-view cascade) plus the differential test against GenerateMigration will land in follow-up commits. Tests: per-category regression tests using a shared assertSingleDropOp helper and a table-driven pattern for the trivial categories. Function drops have dedicated subtests for DiffDrop, overload disambiguation, DiffModify via return-type change (with precondition assertions to prevent vacuous passes), and body-only changes. No existing tests touched. Co-Authored-By: Claude Opus 4.6 (1M context) --- pg/catalog/drops.go | 314 ++++++++++++++++++++++++++ pg/catalog/drops_test.go | 470 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 784 insertions(+) create mode 100644 pg/catalog/drops.go create mode 100644 pg/catalog/drops_test.go diff --git a/pg/catalog/drops.go b/pg/catalog/drops.go new file mode 100644 index 00000000..ef442ee6 --- /dev/null +++ b/pg/catalog/drops.go @@ -0,0 +1,314 @@ +package catalog + +import "sort" + +// DropsFromDiff returns a MigrationPlan containing only the destructive +// (DROP*) operations that GenerateMigration would produce for the same +// inputs, in the same form, with two differences: +// +// - SQL is always empty. Callers needing DDL text must call GenerateMigration. +// - Operations are sorted by a deterministic lexicographic key +// (SchemaName, ObjectName, ParentObject, Type) — see sortDropOps — not +// topologically. +// +// This is a fast path for callers (e.g., destructive-change advisories) that +// need to know what would be dropped without paying the cost of building DDL +// text or running the dependency-driven topological sort. +// +// All metadata fields (Type, SchemaName, ObjectName, ParentObject, Phase, +// ObjType, ObjOID, Priority) are populated identically to what +// GenerateMigration sets. ParentObject follows omni's existing convention: +// for OpDropConstraint and OpDropTrigger it holds the unqualified parent +// table name; for OpDropColumn the table name is in ObjectName (not +// ParentObject) per migration_column.go. +func DropsFromDiff(from, to *Catalog, diff *SchemaDiff) *MigrationPlan { + if diff == nil { + return &MigrationPlan{} + } + var ops []MigrationOp + // Producers are appended in the same order as GenerateMigration. + // Each dropsForX helper documents which migration_*.go function it mirrors. + // All helpers take the same (from, to, diff) signature for call-site + // uniformity, even when a particular helper does not need from or to. + ops = append(ops, dropsForSchemas(from, to, diff)...) + ops = append(ops, dropsForEnums(from, to, diff)...) + ops = append(ops, dropsForDomains(from, to, diff)...) + ops = append(ops, dropsForRanges(from, to, diff)...) + ops = append(ops, dropsForComposites(from, to, diff)...) + ops = append(ops, dropsForSequences(from, to, diff)...) + ops = append(ops, dropsForFunctions(from, to, diff)...) + ops = append(ops, dropsForTables(from, to, diff)...) + + sortDropOps(ops) + return &MigrationPlan{Ops: ops} +} + +// dropsForSchemas mirrors the DiffDrop arm of generateSchemaDDL in +// migration_schema.go. Schemas are top-level objects: SchemaName is empty, +// ObjectName holds the schema name. +// +// from and to are unused (the OID lives on entry.From) but kept for +// signature uniformity across all dropsForX helpers — see DropsFromDiff. +func dropsForSchemas(_, _ *Catalog, diff *SchemaDiff) []MigrationOp { + var ops []MigrationOp + for _, entry := range diff.Schemas { + if entry.Action != DiffDrop { + continue + } + var schemaOID uint32 + if entry.From != nil { + schemaOID = entry.From.OID + } + ops = append(ops, MigrationOp{ + Type: OpDropSchema, + ObjectName: entry.Name, + Transactional: true, + Phase: PhasePre, + ObjType: 'n', + ObjOID: schemaOID, + Priority: PrioritySchema, + }) + } + return ops +} + +// dropsForEnums mirrors the DiffDrop arm of generateEnumDDL in +// migration_enum.go. The OID is resolved against the from catalog by name +// (resolveTypeOIDByName), so this helper genuinely uses from; to is unused +// but kept for signature uniformity — see DropsFromDiff. +func dropsForEnums(from, _ *Catalog, diff *SchemaDiff) []MigrationOp { + var ops []MigrationOp + for _, entry := range diff.Enums { + if entry.Action != DiffDrop { + continue + } + ops = append(ops, MigrationOp{ + Type: OpDropType, + SchemaName: entry.SchemaName, + ObjectName: entry.Name, + Transactional: true, + Phase: PhasePre, + ObjType: 't', + ObjOID: resolveTypeOIDByName(from, entry.SchemaName, entry.Name), + Priority: PriorityType, + }) + } + return ops +} + +// dropsForDomains mirrors the DiffDrop arm of generateDomainDDL in +// migration_domain.go. The OID lives on entry.From.TypeOID (NOT entry.From.OID), +// guarded by an entry.From nil check. from and to are unused but kept for +// signature uniformity — see DropsFromDiff. +func dropsForDomains(_, _ *Catalog, diff *SchemaDiff) []MigrationOp { + var ops []MigrationOp + for _, entry := range diff.Domains { + if entry.Action != DiffDrop { + continue + } + var typeOID uint32 + if entry.From != nil { + typeOID = entry.From.TypeOID + } + ops = append(ops, MigrationOp{ + Type: OpDropType, + SchemaName: entry.SchemaName, + ObjectName: entry.Name, + Transactional: true, + Phase: PhasePre, + ObjType: 't', + ObjOID: typeOID, + Priority: PriorityType, + }) + } + return ops +} + +// dropsForRanges mirrors the DiffDrop arm of generateRangeDDL in +// migration_range.go. Skips entries with nil From. from and to are unused +// but kept for signature uniformity — see DropsFromDiff. +func dropsForRanges(_, _ *Catalog, diff *SchemaDiff) []MigrationOp { + var ops []MigrationOp + for _, entry := range diff.Ranges { + if entry.Action != DiffDrop { + continue + } + if entry.From == nil { + continue + } + ops = append(ops, MigrationOp{ + Type: OpDropType, + SchemaName: entry.SchemaName, + ObjectName: entry.Name, + Transactional: true, + Phase: PhasePre, + ObjType: 't', + ObjOID: entry.From.OID, + Priority: PriorityType, + }) + } + return ops +} + +// dropsForComposites mirrors the DiffDrop arm of generateCompositeDDL in +// migration_composite.go. Note: ObjType is 'r' (relation), not 't' — composite +// types are stored as relations in PostgreSQL. Skips entries with nil From. +// from and to are unused but kept for signature uniformity — see DropsFromDiff. +func dropsForComposites(_, _ *Catalog, diff *SchemaDiff) []MigrationOp { + var ops []MigrationOp + for _, entry := range diff.CompositeTypes { + if entry.Action != DiffDrop { + continue + } + if entry.From == nil { + continue + } + ops = append(ops, MigrationOp{ + Type: OpDropType, + SchemaName: entry.SchemaName, + ObjectName: entry.Name, + Transactional: true, + Phase: PhasePre, + ObjType: 'r', + ObjOID: entry.From.OID, + Priority: PriorityType, + }) + } + return ops +} + +// dropsForSequences mirrors the DiffDrop arm of generateSequenceDDL in +// migration_sequence.go. Skips identity-backed sequences (DROP IDENTITY on +// the owning column drops them automatically) — this filter requires the +// from catalog. to is unused but kept for signature uniformity — see +// DropsFromDiff. +func dropsForSequences(from, _ *Catalog, diff *SchemaDiff) []MigrationOp { + var ops []MigrationOp + for _, entry := range diff.Sequences { + if entry.Action != DiffDrop { + continue + } + if entry.From == nil { + continue + } + if isIdentitySequence(from, entry.SchemaName, entry.Name) { + continue + } + ops = append(ops, MigrationOp{ + Type: OpDropSequence, + SchemaName: entry.SchemaName, + ObjectName: entry.Name, + Transactional: true, + Phase: PhasePre, + ObjType: 'S', + ObjOID: entry.From.OID, + Priority: PrioritySequence, + }) + } + return ops +} + +// dropsForFunctions mirrors the DiffDrop arm of generateFunctionDDL in +// migration_function.go, plus the DiffModify arm where signatureChanged +// returns true (signature changes force DROP + CREATE; body-only changes +// use CREATE OR REPLACE and emit no drop op). +// +// ObjectName is set to entry.Identity (e.g. "add_one(integer)"), not +// entry.Name, so overloaded functions remain distinguishable. Both from +// and to catalogs are required because the DiffModify path must call +// signatureChanged(from, entry.From, to, entry.To). +func dropsForFunctions(from, to *Catalog, diff *SchemaDiff) []MigrationOp { + var ops []MigrationOp + for _, entry := range diff.Functions { + switch entry.Action { + case DiffDrop: + if entry.From == nil { + continue + } + ops = append(ops, dropFunctionOp(entry)) + case DiffModify: + if entry.From == nil || entry.To == nil { + continue + } + if signatureChanged(from, entry.From, to, entry.To) { + ops = append(ops, dropFunctionOp(entry)) + } + } + } + return ops +} + +// dropFunctionOp builds the OpDropFunction MigrationOp for a function diff +// entry, mirroring buildDropFunctionOp in migration_function.go minus SQL. +func dropFunctionOp(entry FunctionDiffEntry) MigrationOp { + return MigrationOp{ + Type: OpDropFunction, + SchemaName: entry.SchemaName, + ObjectName: entry.Identity, + Transactional: true, + Phase: PhasePre, + ObjType: 'f', + ObjOID: entry.From.OID, + Priority: PriorityFunction, + } +} + +// dropsForTables mirrors the DiffDrop arm of generateTableDDL in +// migration_table.go. Only relations with RelKind 'r' (regular table) +// or 'p' (partitioned table) qualify; views, matviews, and composite +// types are handled by their own helpers (dropsForViews, +// dropsForComposites). The DiffModify recreate cases (RelKind flip, +// inheritance change) are handled by dropsForTableRecreates. +// +// from and to are unused (the OID lives on entry.From) but kept for +// signature uniformity — see DropsFromDiff. +func dropsForTables(_, _ *Catalog, diff *SchemaDiff) []MigrationOp { + var ops []MigrationOp + for _, entry := range diff.Relations { + if entry.Action != DiffDrop { + continue + } + if entry.From == nil { + continue + } + rk := entry.From.RelKind + if rk != 'r' && rk != 'p' { + continue + } + ops = append(ops, MigrationOp{ + Type: OpDropTable, + SchemaName: entry.SchemaName, + ObjectName: entry.Name, + Transactional: true, + Phase: PhasePre, + ObjType: 'r', + ObjOID: entry.From.OID, + Priority: PriorityTable, + }) + } + return ops +} + +// sortDropOps gives DropsFromDiff output deterministic ordering. We do NOT +// run sortMigrationOps because that performs an expensive topological sort +// over the dep graph that drop advice does not need. +// +// The sort key includes ParentObject because OpDropConstraint and +// OpDropTrigger ops carry the constraint/trigger name in ObjectName and the +// parent table name in ParentObject — without ParentObject in the key, two +// drops of the same-named constraint on different tables would be ordered +// nondeterministically. +func sortDropOps(ops []MigrationOp) { + sort.Slice(ops, func(i, j int) bool { + if ops[i].SchemaName != ops[j].SchemaName { + return ops[i].SchemaName < ops[j].SchemaName + } + if ops[i].ObjectName != ops[j].ObjectName { + return ops[i].ObjectName < ops[j].ObjectName + } + if ops[i].ParentObject != ops[j].ParentObject { + return ops[i].ParentObject < ops[j].ParentObject + } + return ops[i].Type < ops[j].Type + }) +} diff --git a/pg/catalog/drops_test.go b/pg/catalog/drops_test.go new file mode 100644 index 00000000..ebbb9abd --- /dev/null +++ b/pg/catalog/drops_test.go @@ -0,0 +1,470 @@ +package catalog + +import ( + "strings" + "testing" +) + +func TestDropsFromDiff_NoChanges(t *testing.T) { + from, err := LoadSQL("CREATE TABLE t (id integer);") + if err != nil { + t.Fatal(err) + } + to, err := LoadSQL("CREATE TABLE t (id integer);") + if err != nil { + t.Fatal(err) + } + diff := Diff(from, to) + plan := DropsFromDiff(from, to, diff) + if plan == nil { + t.Fatal("expected non-nil plan") + } + if len(plan.Ops) != 0 { + t.Errorf("expected 0 ops for unchanged schemas, got %d: %+v", len(plan.Ops), plan.Ops) + } + if plan.SQL() != "" { + t.Errorf("expected empty SQL for empty plan, got %q", plan.SQL()) + } +} + +func TestDropsFromDiff_NilDiff(t *testing.T) { + plan := DropsFromDiff(nil, nil, nil) + if plan == nil { + t.Fatal("expected non-nil plan even for nil diff") + } + if len(plan.Ops) != 0 { + t.Errorf("expected 0 ops for nil diff, got %d", len(plan.Ops)) + } +} + +func TestDropsFromDiff_DropSchema(t *testing.T) { + from, err := LoadSQL("CREATE SCHEMA reporting; CREATE TABLE reporting.r (id integer);") + if err != nil { + t.Fatal(err) + } + to, err := LoadSQL("") + if err != nil { + t.Fatal(err) + } + diff := Diff(from, to) + plan := DropsFromDiff(from, to, diff) + + var schemaDrops []MigrationOp + for _, op := range plan.Ops { + if op.Type == OpDropSchema { + schemaDrops = append(schemaDrops, op) + } + } + if len(schemaDrops) != 1 { + t.Fatalf("expected 1 OpDropSchema, got %d; ops: %+v", len(schemaDrops), plan.Ops) + } + op := schemaDrops[0] + if op.ObjectName != "reporting" { + t.Errorf("expected ObjectName=reporting, got %q", op.ObjectName) + } + if op.SchemaName != "" { + t.Errorf("expected empty SchemaName for top-level schema op, got %q", op.SchemaName) + } + if op.SQL != "" { + t.Errorf("expected empty SQL, got %q", op.SQL) + } + if op.Phase != PhasePre { + t.Errorf("expected PhasePre, got %v", op.Phase) + } + if op.Priority != PrioritySchema { + t.Errorf("expected PrioritySchema, got %d", op.Priority) + } + if op.ObjType != 'n' { + t.Errorf("expected ObjType='n', got %c", op.ObjType) + } + if !op.Transactional { + t.Errorf("expected Transactional=true") + } + if op.ObjOID == 0 { + t.Errorf("expected non-zero ObjOID") + } +} + +// dropOpExpect is the metadata that every "simple drop" test asserts on the +// resulting MigrationOp. It deliberately omits the SQL and ObjOID fields +// (SQL is always "" by DropsFromDiff contract; ObjOID is verified to be +// nonzero rather than equal to a fixed value, since it depends on catalog +// allocation order). +type dropOpExpect struct { + opType MigrationOpType + schemaName string + objectName string + objType byte + priority int +} + +// assertSingleDropOp finds the single op in plan matching want.opType and +// want.objectName, then verifies every metadata field that DropsFromDiff +// must populate identically to GenerateMigration: empty SQL, PhasePre, +// Transactional, nonzero ObjOID, plus the per-category fields in want. +func assertSingleDropOp(t *testing.T, plan *MigrationPlan, want dropOpExpect) { + t.Helper() + matches := plan.Filter(func(op MigrationOp) bool { + return op.Type == want.opType && op.ObjectName == want.objectName + }).Ops + if len(matches) != 1 { + t.Fatalf("expected 1 %s for %s, got %d; all ops: %+v", want.opType, want.objectName, len(matches), plan.Ops) + } + op := matches[0] + if op.SchemaName != want.schemaName { + t.Errorf("SchemaName: got %q, want %q", op.SchemaName, want.schemaName) + } + if op.SQL != "" { + t.Errorf("SQL: got %q, want empty (DropsFromDiff contract)", op.SQL) + } + if op.Phase != PhasePre { + t.Errorf("Phase: got %v, want PhasePre", op.Phase) + } + if op.Priority != want.priority { + t.Errorf("Priority: got %d, want %d", op.Priority, want.priority) + } + if op.ObjType != want.objType { + t.Errorf("ObjType: got %c, want %c", op.ObjType, want.objType) + } + if !op.Transactional { + t.Errorf("Transactional: got false, want true") + } + if op.ObjOID == 0 { + t.Errorf("ObjOID: got 0, want nonzero") + } +} + +// TestDropsFromDiff_SimpleTypeAndSequenceDrops covers the trivial-mirror +// drop categories (enum, domain, range, composite, sequence) where the +// helper just copies fields from a single DiffDrop entry. Each subtest +// loads SDL into from, empty into to, runs DropsFromDiff, and verifies +// every metadata field. +// +// Categories with non-trivial logic (schemas — empty SchemaName; sequence +// identity-skip filter; constraints/triggers with ParentObject; dependent +// view cascade) have their own dedicated tests below. +func TestDropsFromDiff_SimpleTypeAndSequenceDrops(t *testing.T) { + tests := []struct { + name string + fromSQL string + want dropOpExpect + }{ + { + name: "enum", + fromSQL: "CREATE TYPE color AS ENUM ('red', 'green');", + want: dropOpExpect{ + opType: OpDropType, schemaName: "public", objectName: "color", + objType: 't', priority: PriorityType, + }, + }, + { + name: "domain", + fromSQL: "CREATE DOMAIN positive_int AS integer CHECK (VALUE > 0);", + want: dropOpExpect{ + opType: OpDropType, schemaName: "public", objectName: "positive_int", + objType: 't', priority: PriorityType, + }, + }, + { + name: "range", + fromSQL: "CREATE TYPE intrange AS RANGE (subtype = integer);", + want: dropOpExpect{ + opType: OpDropType, schemaName: "public", objectName: "intrange", + objType: 't', priority: PriorityType, + }, + }, + { + // Composites are stored as relations in PG, so ObjType is 'r' + // not 't' — easy to get wrong, so it's pinned here. + name: "composite", + fromSQL: "CREATE TYPE point2d AS (x integer, y integer);", + want: dropOpExpect{ + opType: OpDropType, schemaName: "public", objectName: "point2d", + objType: 'r', priority: PriorityType, + }, + }, + { + name: "sequence", + fromSQL: "CREATE SEQUENCE my_seq;", + want: dropOpExpect{ + opType: OpDropSequence, schemaName: "public", objectName: "my_seq", + objType: 'S', priority: PrioritySequence, + }, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + from, err := LoadSQL(tc.fromSQL) + if err != nil { + t.Fatal(err) + } + to, err := LoadSQL("") + if err != nil { + t.Fatal(err) + } + diff := Diff(from, to) + plan := DropsFromDiff(from, to, diff) + assertSingleDropOp(t, plan, tc.want) + }) + } +} + +func TestDropsFromDiff_DropFunction(t *testing.T) { + t.Run("simple drop emits with signature in ObjectName", func(t *testing.T) { + from, err := LoadSQL(`CREATE FUNCTION add_one(x integer) RETURNS integer AS $$ SELECT x+1 $$ LANGUAGE sql;`) + if err != nil { + t.Fatal(err) + } + to, err := LoadSQL("") + if err != nil { + t.Fatal(err) + } + diff := Diff(from, to) + plan := DropsFromDiff(from, to, diff) + + // Use plan.Filter to find the single drop. + drops := plan.Filter(func(op MigrationOp) bool { + return op.Type == OpDropFunction + }).Ops + if len(drops) != 1 { + t.Fatalf("expected 1 OpDropFunction, got %d; ops: %+v", len(drops), plan.Ops) + } + op := drops[0] + // ObjectName must contain both the function name AND the argument type. + if !strings.Contains(op.ObjectName, "add_one") { + t.Errorf("expected ObjectName to contain 'add_one', got %q", op.ObjectName) + } + if !strings.Contains(op.ObjectName, "integer") { + t.Errorf("expected ObjectName to contain 'integer' (signature), got %q", op.ObjectName) + } + if op.SchemaName != "public" { + t.Errorf("expected SchemaName=public, got %q", op.SchemaName) + } + if op.ObjType != 'f' { + t.Errorf("expected ObjType='f', got %c", op.ObjType) + } + if op.Priority != PriorityFunction { + t.Errorf("expected PriorityFunction, got %d", op.Priority) + } + if op.Phase != PhasePre { + t.Errorf("expected PhasePre") + } + if op.SQL != "" { + t.Errorf("expected empty SQL, got %q", op.SQL) + } + if !op.Transactional { + t.Errorf("expected Transactional=true") + } + if op.ObjOID == 0 { + t.Errorf("expected non-zero ObjOID") + } + }) + + t.Run("overloads are distinguishable by signature in ObjectName", func(t *testing.T) { + from, err := LoadSQL(` + CREATE FUNCTION add_one(x integer) RETURNS integer AS $$ SELECT x+1 $$ LANGUAGE sql; + CREATE FUNCTION add_one(x bigint) RETURNS bigint AS $$ SELECT x+1 $$ LANGUAGE sql; + `) + if err != nil { + t.Fatal(err) + } + to, err := LoadSQL("") + if err != nil { + t.Fatal(err) + } + diff := Diff(from, to) + plan := DropsFromDiff(from, to, diff) + + drops := plan.Filter(func(op MigrationOp) bool { + return op.Type == OpDropFunction + }).Ops + if len(drops) != 2 { + t.Fatalf("expected 2 OpDropFunction (one per overload), got %d", len(drops)) + } + // The two ObjectNames must differ — they encode different signatures. + if drops[0].ObjectName == drops[1].ObjectName { + t.Errorf("expected distinct ObjectNames for overloads, both got %q", drops[0].ObjectName) + } + }) + + t.Run("return type change exercises DiffModify+signatureChanged path", func(t *testing.T) { + // CRITICAL: arg types must stay identical, otherwise funcIdentity + // (which includes arg types) changes and the diff produces + // DiffDrop+DiffAdd instead of DiffModify — bypassing the + // signatureChanged code path entirely. Return type is NOT in + // funcIdentity but IS in signatureChanged, so changing only the + // return type is the simplest way to actually exercise the + // DiffModify branch in dropsForFunctions. + from, err := LoadSQL(`CREATE FUNCTION foo(x integer) RETURNS integer AS $$ SELECT x $$ LANGUAGE sql;`) + if err != nil { + t.Fatal(err) + } + to, err := LoadSQL(`CREATE FUNCTION foo(x integer) RETURNS bigint AS $$ SELECT x::bigint $$ LANGUAGE sql;`) + if err != nil { + t.Fatal(err) + } + diff := Diff(from, to) + + // Precondition: confirm we're actually exercising DiffModify, not + // DiffDrop+DiffAdd. If this fails, the test passes vacuously — see + // commit log for the original bug. + modifies := 0 + for _, e := range diff.Functions { + if e.Action == DiffModify { + modifies++ + } + } + if modifies != 1 { + t.Fatalf("precondition: expected exactly 1 DiffModify entry for return-type change, got %d; test would pass vacuously. diff.Functions: %+v", modifies, diff.Functions) + } + + plan := DropsFromDiff(from, to, diff) + drops := plan.Filter(func(op MigrationOp) bool { + return op.Type == OpDropFunction + }).Ops + if len(drops) != 1 { + t.Errorf("expected 1 OpDropFunction for return-type change via DiffModify+signatureChanged, got %d: %+v", len(drops), drops) + } + }) + + t.Run("body-only change emits no drop", func(t *testing.T) { + from, err := LoadSQL(`CREATE FUNCTION foo(x integer) RETURNS integer AS $$ SELECT x $$ LANGUAGE sql;`) + if err != nil { + t.Fatal(err) + } + to, err := LoadSQL(`CREATE FUNCTION foo(x integer) RETURNS integer AS $$ SELECT x + 0 $$ LANGUAGE sql;`) + if err != nil { + t.Fatal(err) + } + diff := Diff(from, to) + + // Precondition: confirm we're actually exercising DiffModify, not + // "no diff at all". Without this, the test passes vacuously if a + // future refactor stops detecting the body change. + modifies := 0 + for _, e := range diff.Functions { + if e.Action == DiffModify { + modifies++ + } + } + if modifies != 1 { + t.Fatalf("precondition: expected exactly 1 DiffModify entry for body-only change, got %d; test would pass vacuously. diff.Functions: %+v", modifies, diff.Functions) + } + + plan := DropsFromDiff(from, to, diff) + drops := plan.Filter(func(op MigrationOp) bool { + return op.Type == OpDropFunction + }).Ops + if len(drops) != 0 { + t.Errorf("expected 0 OpDropFunction for body-only change (CREATE OR REPLACE handles it), got %d: %+v", len(drops), drops) + } + }) +} + +func TestDropsFromDiff_DropTable(t *testing.T) { + t.Run("regular table emits OpDropTable", func(t *testing.T) { + from, err := LoadSQL("CREATE TABLE t (id integer);") + if err != nil { + t.Fatal(err) + } + to, err := LoadSQL("") + if err != nil { + t.Fatal(err) + } + diff := Diff(from, to) + plan := DropsFromDiff(from, to, diff) + assertSingleDropOp(t, plan, dropOpExpect{ + opType: OpDropTable, + schemaName: "public", + objectName: "t", + objType: 'r', + priority: PriorityTable, + }) + }) + + t.Run("partitioned table emits OpDropTable", func(t *testing.T) { + from, err := LoadSQL(` + CREATE TABLE measurements (id integer, ts timestamp) PARTITION BY RANGE (ts); + `) + if err != nil { + t.Fatal(err) + } + // Precondition: confirm the parser tagged this as RelKind 'p'. + // Without this check the subtest would pass vacuously via the 'r' + // branch if a future parser refactor changed the behavior. + if rel := from.GetRelation("public", "measurements"); rel == nil || rel.RelKind != 'p' { + relKind := byte(0) + if rel != nil { + relKind = rel.RelKind + } + t.Fatalf("precondition: expected RelKind 'p' for partitioned parent, got %q", relKind) + } + to, err := LoadSQL("") + if err != nil { + t.Fatal(err) + } + diff := Diff(from, to) + plan := DropsFromDiff(from, to, diff) + assertSingleDropOp(t, plan, dropOpExpect{ + opType: OpDropTable, + schemaName: "public", + objectName: "measurements", + objType: 'r', + priority: PriorityTable, + }) + }) + + t.Run("view does not emit OpDropTable", func(t *testing.T) { + from, err := LoadSQL(` + CREATE TABLE t (id integer); + CREATE VIEW v AS SELECT id FROM t; + `) + if err != nil { + t.Fatal(err) + } + to, err := LoadSQL(`CREATE TABLE t (id integer);`) + if err != nil { + t.Fatal(err) + } + diff := Diff(from, to) + plan := DropsFromDiff(from, to, diff) + // Filter for OpDropTable; expect zero (the view drop will be emitted + // by dropsForViews in a later task, not here). + drops := plan.Filter(func(op MigrationOp) bool { + return op.Type == OpDropTable + }).Ops + if len(drops) != 0 { + t.Errorf("expected 0 OpDropTable for a dropped view, got %d: %+v", len(drops), drops) + } + }) +} + +func TestDropsFromDiff_DropSequence_SkipsIdentity(t *testing.T) { + from, err := LoadSQL(` + CREATE TABLE t (id integer GENERATED ALWAYS AS IDENTITY); + CREATE SEQUENCE my_seq; + `) + if err != nil { + t.Fatal(err) + } + to, err := LoadSQL(`CREATE TABLE t (id integer GENERATED ALWAYS AS IDENTITY);`) + if err != nil { + t.Fatal(err) + } + diff := Diff(from, to) + plan := DropsFromDiff(from, to, diff) + + var seqDrops []MigrationOp + for _, op := range plan.Ops { + if op.Type == OpDropSequence { + seqDrops = append(seqDrops, op) + } + } + if len(seqDrops) != 1 { + t.Fatalf("expected 1 OpDropSequence (my_seq, NOT the identity-backed sequence), got %d: %+v", len(seqDrops), seqDrops) + } + if seqDrops[0].ObjectName != "my_seq" { + t.Errorf("expected ObjectName=my_seq, got %q", seqDrops[0].ObjectName) + } +} From bab65404841241e90cc0d851d881d272e9aca889 Mon Sep 17 00:00:00 2001 From: Vincent Huang Date: Thu, 9 Apr 2026 16:18:33 -0700 Subject: [PATCH 2/5] feat(pg/catalog): complete DropsFromDiff with all 15 producers + differential test MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add the remaining drop producers to DropsFromDiff, completing coverage of all 21 drop-emission sites across omni's generate*DDL functions: - dropsForTableRecreates (migration_partition.go — RelKind/inheritance change) - dropsForColumns (migration_column.go — DiffDrop) - dropsForCheckCascades (migration_column.go — CHECK cascade from type change) - dropsForConstraints (migration_constraint.go — DiffDrop + DiffModify) - dropsForViews (migration_view.go — 5 emission sites including matview modify) - dropsForTriggers (migration_trigger.go — DiffDrop + DiffModify) - dropsForDependentViews (migration.go — BFS over from.deps for transitive view cascade) The dependent-view cascade is the most complex producer: it walks the unexported catalog dep graph to find views that transitively depend on tables undergoing column type changes, column drops, RelKind changes, or inheritance changes, then deduplicates against ops already emitted by dropsForViews. Adds a differential test that builds a non-trivial fixture (schema drop, column type change with CHECK cascade, matview modification, function drop, enum drop, sequence drop, trigger drop, dependent-view cascade), runs both GenerateMigration and DropsFromDiff on the same input, and asserts the drop subsets match field-by-field (12 ops across 8 categories, zero mismatches). This test is the primary correctness gate preventing the duplicated decide logic from drifting. Full omni test suite passes (go test ./... -count=1 -short), race detector clean on pg/catalog. Co-Authored-By: Claude Opus 4.6 (1M context) --- pg/catalog/drops.go | 479 +++++++++++++++++++++++ pg/catalog/drops_test.go | 824 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 1303 insertions(+) diff --git a/pg/catalog/drops.go b/pg/catalog/drops.go index ef442ee6..a2e6da7a 100644 --- a/pg/catalog/drops.go +++ b/pg/catalog/drops.go @@ -38,6 +38,13 @@ func DropsFromDiff(from, to *Catalog, diff *SchemaDiff) *MigrationPlan { ops = append(ops, dropsForSequences(from, to, diff)...) ops = append(ops, dropsForFunctions(from, to, diff)...) ops = append(ops, dropsForTables(from, to, diff)...) + ops = append(ops, dropsForTableRecreates(from, to, diff)...) + ops = append(ops, dropsForColumns(from, to, diff)...) + ops = append(ops, dropsForCheckCascades(from, to, diff)...) + ops = append(ops, dropsForConstraints(from, to, diff)...) + ops = append(ops, dropsForViews(from, to, diff)...) + ops = append(ops, dropsForTriggers(from, to, diff)...) + ops = append(ops, dropsForDependentViews(from, to, diff, ops)...) sortDropOps(ops) return &MigrationPlan{Ops: ops} @@ -289,6 +296,478 @@ func dropsForTables(_, _ *Catalog, diff *SchemaDiff) []MigrationOp { return ops } +// dropsForTableRecreates mirrors the DiffModify arm of generatePartitionDDL +// in migration_partition.go where a table must be recreated via DROP+CREATE +// because PostgreSQL does not support in-place RelKind changes (e.g., +// regular → partitioned) or inheritance changes. +// +// Only the OpDropTable half of buildTableRecreateOps is emitted; the +// OpCreateTable and OpCreateIndex halves are not drops. +// +// Both from and to catalogs are required for inhParentsEqual. +func dropsForTableRecreates(from, to *Catalog, diff *SchemaDiff) []MigrationOp { + var ops []MigrationOp + for _, entry := range diff.Relations { + if entry.Action != DiffModify { + continue + } + if entry.From == nil || entry.To == nil { + continue + } + fromIsTable := entry.From.RelKind == 'r' || entry.From.RelKind == 'p' + toIsTable := entry.To.RelKind == 'r' || entry.To.RelKind == 'p' + if !fromIsTable || !toIsTable { + continue + } + + needsRecreate := false + if entry.From.RelKind != entry.To.RelKind { + needsRecreate = true + } + if !needsRecreate && !inhParentsEqual(from, to, entry.From.InhParents, entry.To.InhParents) { + needsRecreate = true + } + if !needsRecreate { + continue + } + + ops = append(ops, MigrationOp{ + Type: OpDropTable, + SchemaName: entry.SchemaName, + ObjectName: entry.Name, + Transactional: true, + Phase: PhasePre, + ObjType: 'r', + ObjOID: entry.From.OID, + Priority: PriorityTable, + }) + } + return ops +} + +// dropsForColumns mirrors the DiffDrop arm of generateColumnDDL in +// migration_column.go. Column drops live inside DiffModify relation entries +// (not as top-level diff entries). Views and matviews are skipped because +// column changes there are handled by view DDL. +// +// ObjectName is the TABLE name (not the column name) — this is omni's +// established convention for OpDropColumn. +func dropsForColumns(_, _ *Catalog, diff *SchemaDiff) []MigrationOp { + var ops []MigrationOp + for _, rel := range diff.Relations { + if rel.Action != DiffModify { + continue + } + if len(rel.Columns) == 0 { + continue + } + if rel.To != nil && (rel.To.RelKind == 'v' || rel.To.RelKind == 'm') { + continue + } + + var relOID uint32 + if rel.To != nil { + relOID = rel.To.OID + } + + for _, col := range rel.Columns { + if col.Action != DiffDrop { + continue + } + ops = append(ops, MigrationOp{ + Type: OpDropColumn, + SchemaName: rel.SchemaName, + ObjectName: rel.Name, + Transactional: true, + Phase: PhasePre, + ObjType: 'r', + ObjOID: relOID, + Priority: PriorityColumn, + }) + } + } + return ops +} + +// dropsForCheckCascades mirrors the CHECK-constraint cascade logic inside +// columnModifyOps in migration_column.go (lines 147-174). When a column's +// type changes, CHECK constraints referencing that column must be dropped +// first (PG requirement). The outer loop in generateColumnDDL overrides +// Phase to PhaseMain and Priority to PriorityColumn. +func dropsForCheckCascades(from, to *Catalog, diff *SchemaDiff) []MigrationOp { + var ops []MigrationOp + for _, rel := range diff.Relations { + if rel.Action != DiffModify { + continue + } + if len(rel.Columns) == 0 { + continue + } + if rel.To != nil && (rel.To.RelKind == 'v' || rel.To.RelKind == 'm') { + continue + } + + var relOID uint32 + if rel.To != nil { + relOID = rel.To.OID + } + + for _, col := range rel.Columns { + if col.Action != DiffModify { + continue + } + if col.From == nil || col.To == nil { + continue + } + typeChanged := from.FormatType(col.From.TypeOID, col.From.TypeMod) != + to.FormatType(col.To.TypeOID, col.To.TypeMod) + if !typeChanged { + continue + } + fromRel := from.GetRelation(rel.SchemaName, rel.Name) + if fromRel == nil { + continue + } + for _, con := range from.ConstraintsOf(fromRel.OID) { + if con.Type != ConstraintCheck { + continue + } + if !containsColumnRef(con.CheckExpr, col.From.Name) { + continue + } + ops = append(ops, MigrationOp{ + Type: OpDropConstraint, + SchemaName: rel.SchemaName, + ObjectName: rel.Name, + Transactional: true, + Phase: PhaseMain, + ObjType: 'r', + ObjOID: relOID, + Priority: PriorityColumn, + }) + } + } + } + return ops +} + +// dropsForConstraints mirrors the DiffDrop and DiffModify arms of +// constraintOpsForRelation in migration_constraint.go. Both DiffDrop and +// DiffModify emit drops (modify = DROP old + ADD new). Constraint-trigger +// types are skipped (handled by trigger DDL). +// +// ObjectName is the CONSTRAINT name; ParentObject is the TABLE name. +// ObjOID is NOT set (zero), matching buildDropConstraintOp. +func dropsForConstraints(_, _ *Catalog, diff *SchemaDiff) []MigrationOp { + var ops []MigrationOp + for _, rel := range diff.Relations { + if rel.Action != DiffModify { + continue + } + for _, ce := range rel.Constraints { + if (ce.To != nil && ce.To.Type == ConstraintTrigger) || + (ce.From != nil && ce.From.Type == ConstraintTrigger) { + continue + } + switch ce.Action { + case DiffDrop: + if ce.From == nil { + continue + } + ops = append(ops, MigrationOp{ + Type: OpDropConstraint, + SchemaName: rel.SchemaName, + ObjectName: ce.From.Name, + ParentObject: rel.Name, + Phase: PhasePre, + ObjType: 'c', + Priority: PriorityConstraint, + }) + case DiffModify: + if ce.From == nil { + continue + } + ops = append(ops, MigrationOp{ + Type: OpDropConstraint, + SchemaName: rel.SchemaName, + ObjectName: ce.From.Name, + ParentObject: rel.Name, + Phase: PhasePre, + ObjType: 'c', + Priority: PriorityConstraint, + }) + } + } + } + return ops +} + +// dropsForViews mirrors the drop-emission sites in generateViewDDL in +// migration_view.go. There are five sites: +// +// 1. DiffDrop view (RelKind 'v') +// 2. DiffDrop matview (RelKind 'm') +// 3. DiffModify RelKind flip (view↔matview) +// 4. DiffModify regular view with viewColumnsChanged +// 5. DiffModify matview (all modifications — matviews don't support ALTER) +func dropsForViews(_, _ *Catalog, diff *SchemaDiff) []MigrationOp { + var ops []MigrationOp + for _, entry := range diff.Relations { + switch entry.Action { + case DiffDrop: + if entry.From == nil { + continue + } + if entry.From.RelKind != 'v' && entry.From.RelKind != 'm' { + continue + } + ops = append(ops, MigrationOp{ + Type: OpDropView, + SchemaName: entry.SchemaName, + ObjectName: entry.Name, + Transactional: true, + Phase: PhasePre, + ObjType: 'r', + ObjOID: entry.From.OID, + Priority: PriorityView, + }) + + case DiffModify: + if entry.From == nil || entry.To == nil { + continue + } + fromIsView := entry.From.RelKind == 'v' || entry.From.RelKind == 'm' + toIsView := entry.To.RelKind == 'v' || entry.To.RelKind == 'm' + + // Site 3: RelKind flip (view↔matview). + if (fromIsView || toIsView) && entry.From.RelKind != entry.To.RelKind { + ops = append(ops, MigrationOp{ + Type: OpDropView, + SchemaName: entry.SchemaName, + ObjectName: entry.Name, + Transactional: true, + Phase: PhasePre, + ObjType: 'r', + ObjOID: entry.From.OID, + Priority: PriorityView, + }) + continue + } + + switch entry.To.RelKind { + case 'v': + // Site 4: view with incompatible column changes. + if viewColumnsChanged(entry) { + ops = append(ops, MigrationOp{ + Type: OpDropView, + SchemaName: entry.SchemaName, + ObjectName: entry.Name, + Transactional: true, + Phase: PhasePre, + ObjType: 'r', + ObjOID: entry.From.OID, + Priority: PriorityView, + }) + } + case 'm': + // Site 5: ALL matview modifications emit drop. + ops = append(ops, MigrationOp{ + Type: OpDropView, + SchemaName: entry.SchemaName, + ObjectName: entry.Name, + Transactional: true, + Phase: PhasePre, + ObjType: 'r', + ObjOID: entry.From.OID, + Priority: PriorityView, + }) + } + } + } + return ops +} + +// dropsForTriggers mirrors the DiffDrop and DiffModify arms of +// triggerOpsForRelation in migration_trigger.go. Both DiffDrop and DiffModify +// emit drops (modify = DROP old + CREATE new). +// +// ObjectName is the TRIGGER name; ParentObject is the TABLE name. +// ObjType is 'T' (capital T). +func dropsForTriggers(_, _ *Catalog, diff *SchemaDiff) []MigrationOp { + var ops []MigrationOp + for _, rel := range diff.Relations { + if rel.Action != DiffModify { + continue + } + for _, te := range rel.Triggers { + switch te.Action { + case DiffDrop: + if te.From == nil { + continue + } + ops = append(ops, MigrationOp{ + Type: OpDropTrigger, + SchemaName: rel.SchemaName, + ObjectName: te.From.Name, + ParentObject: rel.Name, + Transactional: true, + Phase: PhasePre, + ObjType: 'T', + ObjOID: te.From.OID, + Priority: PriorityTrigger, + }) + case DiffModify: + if te.From == nil { + continue + } + ops = append(ops, MigrationOp{ + Type: OpDropTrigger, + SchemaName: rel.SchemaName, + ObjectName: te.From.Name, + ParentObject: rel.Name, + Transactional: true, + Phase: PhasePre, + ObjType: 'T', + ObjOID: te.From.OID, + Priority: PriorityTrigger, + }) + } + } + } + return ops +} + +// dropsForDependentViews mirrors wrapColumnTypeChangesWithViewOps in +// migration.go:241-401. When a table's columns are dropped, retyped, or +// the table undergoes a RelKind/inheritance change, PostgreSQL requires +// dependent views to be dropped first. This helper walks from.deps via +// BFS to find all transitively dependent views and emits OpDropView ops +// for each, deduplicating against existingOps so views already handled +// by dropsForViews are not double-counted. +// +// Only regular views (RelKind 'v') are considered — matviews have their +// own handling in buildModifyMatViewOps. +func dropsForDependentViews(from, to *Catalog, diff *SchemaDiff, existingOps []MigrationOp) []MigrationOp { + // Step 1: Find OIDs of tables that need dependent-view wrapping. + tableOIDs := make(map[uint32]bool) + for _, rel := range diff.Relations { + if rel.Action != DiffModify { + continue + } + needsViewWrap := false + + if rel.From != nil && rel.To != nil { + if rel.From.RelKind != rel.To.RelKind { + needsViewWrap = true + } + if !inhParentsEqual(from, to, rel.From.InhParents, rel.To.InhParents) { + needsViewWrap = true + } + } + + if !needsViewWrap { + for _, col := range rel.Columns { + if col.Action == DiffDrop { + needsViewWrap = true + break + } + if col.Action != DiffModify || col.From == nil || col.To == nil { + continue + } + if from.FormatType(col.From.TypeOID, col.From.TypeMod) != to.FormatType(col.To.TypeOID, col.To.TypeMod) { + needsViewWrap = true + break + } + } + } + if needsViewWrap { + r := from.GetRelation(rel.SchemaName, rel.Name) + if r != nil { + tableOIDs[r.OID] = true + } + } + } + if len(tableOIDs) == 0 { + return nil + } + + // Step 2: BFS over from.deps to find dependent views transitively. + type viewInfo struct { + schema string + name string + oid uint32 + } + seen := make(map[uint32]bool) + var viewsToDrop []viewInfo + + queue := make([]uint32, 0, len(tableOIDs)) + for oid := range tableOIDs { + queue = append(queue, oid) + } + for len(queue) > 0 { + refOID := queue[0] + queue = queue[1:] + for _, d := range from.deps { + if d.RefType != 'r' || d.RefOID != refOID || d.ObjType != 'r' { + continue + } + if seen[d.ObjOID] { + continue + } + rel := from.GetRelationByOID(d.ObjOID) + if rel == nil || rel.RelKind != 'v' { + continue + } + seen[d.ObjOID] = true + if rel.Schema == nil { + continue + } + viewsToDrop = append(viewsToDrop, viewInfo{schema: rel.Schema.Name, name: rel.Name, oid: rel.OID}) + queue = append(queue, d.ObjOID) + } + } + + if len(viewsToDrop) == 0 { + return nil + } + + // Step 3: Sort for determinism. + sort.Slice(viewsToDrop, func(i, j int) bool { + if viewsToDrop[i].schema != viewsToDrop[j].schema { + return viewsToDrop[i].schema < viewsToDrop[j].schema + } + return viewsToDrop[i].name < viewsToDrop[j].name + }) + + // Step 4: Build dedup set from existing ops. + existing := make(map[string]bool) + for _, op := range existingOps { + if op.Type == OpDropView || op.Type == OpCreateView || op.Type == OpAlterView { + existing[op.SchemaName+"."+op.ObjectName] = true + } + } + + // Step 5: Emit OpDropView for each dependent view not already covered. + var ops []MigrationOp + for _, v := range viewsToDrop { + key := v.schema + "." + v.name + if existing[key] { + continue + } + ops = append(ops, MigrationOp{ + Type: OpDropView, + SchemaName: v.schema, + ObjectName: v.name, + Transactional: true, + Phase: PhasePre, + ObjType: 'r', + ObjOID: v.oid, + Priority: PriorityView, + }) + } + return ops +} + // sortDropOps gives DropsFromDiff output deterministic ordering. We do NOT // run sortMigrationOps because that performs an expensive topological sort // over the dep graph that drop advice does not need. diff --git a/pg/catalog/drops_test.go b/pg/catalog/drops_test.go index ebbb9abd..2d56f6bd 100644 --- a/pg/catalog/drops_test.go +++ b/pg/catalog/drops_test.go @@ -440,6 +440,729 @@ func TestDropsFromDiff_DropTable(t *testing.T) { }) } +func TestDropsFromDiff_TableRecreate(t *testing.T) { + t.Run("RelKind change regular to partitioned emits OpDropTable", func(t *testing.T) { + from, err := LoadSQL(`CREATE TABLE t (id integer, ts timestamp);`) + if err != nil { + t.Fatal(err) + } + to, err := LoadSQL(`CREATE TABLE t (id integer, ts timestamp) PARTITION BY RANGE (ts);`) + if err != nil { + t.Fatal(err) + } + diff := Diff(from, to) + // Precondition: confirm this produces a DiffModify, not DiffDrop+DiffAdd. + modifies := 0 + for _, e := range diff.Relations { + if e.Action == DiffModify && e.Name == "t" { + modifies++ + } + } + if modifies != 1 { + t.Fatalf("precondition: expected 1 DiffModify for RelKind change, got %d", modifies) + } + + plan := DropsFromDiff(from, to, diff) + assertSingleDropOp(t, plan, dropOpExpect{ + opType: OpDropTable, + schemaName: "public", + objectName: "t", + objType: 'r', + priority: PriorityTable, + }) + }) + + t.Run("no recreate for column-only modification", func(t *testing.T) { + from, err := LoadSQL(`CREATE TABLE t (id integer, name text);`) + if err != nil { + t.Fatal(err) + } + to, err := LoadSQL(`CREATE TABLE t (id integer, name text, age integer);`) + if err != nil { + t.Fatal(err) + } + diff := Diff(from, to) + plan := DropsFromDiff(from, to, diff) + drops := plan.Filter(func(op MigrationOp) bool { + return op.Type == OpDropTable + }).Ops + if len(drops) != 0 { + t.Errorf("expected 0 OpDropTable for column-only change, got %d: %+v", len(drops), drops) + } + }) +} + +// --------------------------------------------------------------------------- +// Task 11: dropsForColumns +// --------------------------------------------------------------------------- + +func TestDropsFromDiff_DropColumn(t *testing.T) { + t.Run("drop column emits OpDropColumn with table name in ObjectName", func(t *testing.T) { + from, err := LoadSQL("CREATE TABLE t (id integer, name text);") + if err != nil { + t.Fatal(err) + } + to, err := LoadSQL("CREATE TABLE t (id integer);") + if err != nil { + t.Fatal(err) + } + diff := Diff(from, to) + + // Precondition: this should be a DiffModify on relation "t" with a DiffDrop column. + var foundColDrop bool + for _, rel := range diff.Relations { + if rel.Action == DiffModify && rel.Name == "t" { + for _, col := range rel.Columns { + if col.Action == DiffDrop && col.Name == "name" { + foundColDrop = true + } + } + } + } + if !foundColDrop { + t.Fatal("precondition: expected DiffModify on t with DiffDrop for column 'name'") + } + + plan := DropsFromDiff(from, to, diff) + drops := plan.Filter(func(op MigrationOp) bool { + return op.Type == OpDropColumn + }).Ops + if len(drops) != 1 { + t.Fatalf("expected 1 OpDropColumn, got %d: %+v", len(drops), drops) + } + op := drops[0] + // CRITICAL: ObjectName is the TABLE name, not the column name. + if op.ObjectName != "t" { + t.Errorf("ObjectName: got %q, want %q (table name, not column name)", op.ObjectName, "t") + } + if op.SchemaName != "public" { + t.Errorf("SchemaName: got %q, want %q", op.SchemaName, "public") + } + if op.SQL != "" { + t.Errorf("SQL: got %q, want empty", op.SQL) + } + if op.Phase != PhasePre { + t.Errorf("Phase: got %v, want PhasePre", op.Phase) + } + if op.ObjType != 'r' { + t.Errorf("ObjType: got %c, want 'r'", op.ObjType) + } + if op.Priority != PriorityColumn { + t.Errorf("Priority: got %d, want PriorityColumn(%d)", op.Priority, PriorityColumn) + } + if !op.Transactional { + t.Errorf("Transactional: got false, want true") + } + if op.ObjOID == 0 { + t.Errorf("ObjOID: got 0, want nonzero") + } + }) + + t.Run("view column drop does not emit OpDropColumn", func(t *testing.T) { + from, err := LoadSQL(` + CREATE TABLE t (id integer, name text); + CREATE VIEW v AS SELECT id, name FROM t; + `) + if err != nil { + t.Fatal(err) + } + to, err := LoadSQL(` + CREATE TABLE t (id integer, name text); + CREATE VIEW v AS SELECT id FROM t; + `) + if err != nil { + t.Fatal(err) + } + diff := Diff(from, to) + plan := DropsFromDiff(from, to, diff) + drops := plan.Filter(func(op MigrationOp) bool { + return op.Type == OpDropColumn + }).Ops + if len(drops) != 0 { + t.Errorf("expected 0 OpDropColumn for view column changes, got %d: %+v", len(drops), drops) + } + }) +} + +// --------------------------------------------------------------------------- +// Task 12: dropsForCheckCascades +// --------------------------------------------------------------------------- + +func TestDropsFromDiff_CheckCascade(t *testing.T) { + t.Run("type change drops referencing CHECK constraint", func(t *testing.T) { + from, err := LoadSQL(` + CREATE TABLE t (val integer CHECK (val > 0)); + `) + if err != nil { + t.Fatal(err) + } + to, err := LoadSQL(` + CREATE TABLE t (val text); + `) + if err != nil { + t.Fatal(err) + } + diff := Diff(from, to) + + // Precondition: DiffModify on relation with a column type change. + var foundColModify bool + for _, rel := range diff.Relations { + if rel.Action == DiffModify && rel.Name == "t" { + for _, col := range rel.Columns { + if col.Action == DiffModify && col.Name == "val" { + foundColModify = true + } + } + } + } + if !foundColModify { + t.Fatal("precondition: expected DiffModify on t with DiffModify for column 'val'") + } + + plan := DropsFromDiff(from, to, diff) + drops := plan.Filter(func(op MigrationOp) bool { + return op.Type == OpDropConstraint + }).Ops + if len(drops) < 1 { + t.Fatalf("expected at least 1 OpDropConstraint for CHECK cascade, got %d: all ops: %+v", len(drops), plan.Ops) + } + // The cascade CHECK drop should have PhaseMain (from outer loop override), + // NOT PhasePre (which is for direct constraint drops). + op := drops[0] + if op.Phase != PhaseMain { + t.Errorf("Phase: got %v, want PhaseMain (outer loop override)", op.Phase) + } + if op.ObjType != 'r' { + t.Errorf("ObjType: got %c, want 'r' (from outer loop override)", op.ObjType) + } + if op.Priority != PriorityColumn { + t.Errorf("Priority: got %d, want PriorityColumn(%d) (from outer loop override)", op.Priority, PriorityColumn) + } + if op.ObjectName != "t" { + t.Errorf("ObjectName: got %q, want %q (table name)", op.ObjectName, "t") + } + if !op.Transactional { + t.Errorf("Transactional: got false, want true") + } + }) + + t.Run("no type change emits no check cascade drop", func(t *testing.T) { + from, err := LoadSQL(`CREATE TABLE t (val integer CHECK (val > 0));`) + if err != nil { + t.Fatal(err) + } + // Change default only, no type change. + to, err := LoadSQL(`CREATE TABLE t (val integer DEFAULT 1 CHECK (val > 0));`) + if err != nil { + t.Fatal(err) + } + diff := Diff(from, to) + plan := DropsFromDiff(from, to, diff) + drops := plan.Filter(func(op MigrationOp) bool { + return op.Type == OpDropConstraint + }).Ops + if len(drops) != 0 { + t.Errorf("expected 0 OpDropConstraint when no type change, got %d: %+v", len(drops), drops) + } + }) +} + +// --------------------------------------------------------------------------- +// Task 13: dropsForConstraints +// --------------------------------------------------------------------------- + +func TestDropsFromDiff_DropConstraint(t *testing.T) { + t.Run("drop CHECK constraint emits OpDropConstraint", func(t *testing.T) { + from, err := LoadSQL(` + CREATE TABLE t (id integer, val integer, CONSTRAINT val_positive CHECK (val > 0)); + `) + if err != nil { + t.Fatal(err) + } + to, err := LoadSQL(` + CREATE TABLE t (id integer, val integer); + `) + if err != nil { + t.Fatal(err) + } + diff := Diff(from, to) + plan := DropsFromDiff(from, to, diff) + + drops := plan.Filter(func(op MigrationOp) bool { + return op.Type == OpDropConstraint + }).Ops + if len(drops) != 1 { + t.Fatalf("expected 1 OpDropConstraint, got %d: %+v", len(drops), plan.Ops) + } + op := drops[0] + if op.ObjectName != "val_positive" { + t.Errorf("ObjectName: got %q, want %q (constraint name)", op.ObjectName, "val_positive") + } + if op.ParentObject != "t" { + t.Errorf("ParentObject: got %q, want %q (table name)", op.ParentObject, "t") + } + if op.SchemaName != "public" { + t.Errorf("SchemaName: got %q, want %q", op.SchemaName, "public") + } + if op.Phase != PhasePre { + t.Errorf("Phase: got %v, want PhasePre", op.Phase) + } + if op.ObjType != 'c' { + t.Errorf("ObjType: got %c, want 'c'", op.ObjType) + } + if op.Priority != PriorityConstraint { + t.Errorf("Priority: got %d, want PriorityConstraint(%d)", op.Priority, PriorityConstraint) + } + if op.SQL != "" { + t.Errorf("SQL: got %q, want empty", op.SQL) + } + // ObjOID should NOT be set (zero) for constraint drops per source. + if op.ObjOID != 0 { + t.Errorf("ObjOID: got %d, want 0 (not set by buildDropConstraintOp)", op.ObjOID) + } + }) + + t.Run("modify CHECK constraint emits OpDropConstraint for old", func(t *testing.T) { + from, err := LoadSQL(` + CREATE TABLE t (val integer, CONSTRAINT val_check CHECK (val > 0)); + `) + if err != nil { + t.Fatal(err) + } + to, err := LoadSQL(` + CREATE TABLE t (val integer, CONSTRAINT val_check CHECK (val > 10)); + `) + if err != nil { + t.Fatal(err) + } + diff := Diff(from, to) + + // Precondition: confirm DiffModify on the constraint. + var foundConModify bool + for _, rel := range diff.Relations { + if rel.Action == DiffModify { + for _, ce := range rel.Constraints { + if ce.Action == DiffModify && ce.Name == "val_check" { + foundConModify = true + } + } + } + } + if !foundConModify { + t.Fatal("precondition: expected DiffModify on constraint 'val_check'") + } + + plan := DropsFromDiff(from, to, diff) + drops := plan.Filter(func(op MigrationOp) bool { + return op.Type == OpDropConstraint + }).Ops + if len(drops) != 1 { + t.Fatalf("expected 1 OpDropConstraint for modified constraint, got %d: %+v", len(drops), plan.Ops) + } + if drops[0].ObjectName != "val_check" { + t.Errorf("ObjectName: got %q, want %q", drops[0].ObjectName, "val_check") + } + }) +} + +// --------------------------------------------------------------------------- +// Tasks 14-15: dropsForViews +// --------------------------------------------------------------------------- + +func TestDropsFromDiff_DropView(t *testing.T) { + t.Run("drop view emits OpDropView", func(t *testing.T) { + from, err := LoadSQL(` + CREATE TABLE t (id integer); + CREATE VIEW v AS SELECT id FROM t; + `) + if err != nil { + t.Fatal(err) + } + to, err := LoadSQL(`CREATE TABLE t (id integer);`) + if err != nil { + t.Fatal(err) + } + diff := Diff(from, to) + plan := DropsFromDiff(from, to, diff) + + drops := plan.Filter(func(op MigrationOp) bool { + return op.Type == OpDropView && op.ObjectName == "v" + }).Ops + if len(drops) != 1 { + t.Fatalf("expected 1 OpDropView for view, got %d: %+v", len(drops), plan.Ops) + } + op := drops[0] + if op.SchemaName != "public" { + t.Errorf("SchemaName: got %q, want %q", op.SchemaName, "public") + } + if op.Phase != PhasePre { + t.Errorf("Phase: got %v, want PhasePre", op.Phase) + } + if op.ObjType != 'r' { + t.Errorf("ObjType: got %c, want 'r'", op.ObjType) + } + if op.Priority != PriorityView { + t.Errorf("Priority: got %d, want PriorityView(%d)", op.Priority, PriorityView) + } + if !op.Transactional { + t.Errorf("Transactional: got false, want true") + } + if op.ObjOID == 0 { + t.Errorf("ObjOID: got 0, want nonzero") + } + }) + + t.Run("drop matview emits OpDropView", func(t *testing.T) { + from, err := LoadSQL(` + CREATE TABLE t (id integer); + CREATE MATERIALIZED VIEW mv AS SELECT id FROM t; + `) + if err != nil { + t.Fatal(err) + } + to, err := LoadSQL(`CREATE TABLE t (id integer);`) + if err != nil { + t.Fatal(err) + } + diff := Diff(from, to) + plan := DropsFromDiff(from, to, diff) + + drops := plan.Filter(func(op MigrationOp) bool { + return op.Type == OpDropView && op.ObjectName == "mv" + }).Ops + if len(drops) != 1 { + t.Fatalf("expected 1 OpDropView for matview, got %d: %+v", len(drops), plan.Ops) + } + }) + + t.Run("view to matview flip emits OpDropView", func(t *testing.T) { + from, err := LoadSQL(` + CREATE TABLE t (id integer); + CREATE VIEW v AS SELECT id FROM t; + `) + if err != nil { + t.Fatal(err) + } + to, err := LoadSQL(` + CREATE TABLE t (id integer); + CREATE MATERIALIZED VIEW v AS SELECT id FROM t; + `) + if err != nil { + t.Fatal(err) + } + diff := Diff(from, to) + + // Precondition: confirm DiffModify with relkind flip. + var foundFlip bool + for _, rel := range diff.Relations { + if rel.Action == DiffModify && rel.Name == "v" { + if rel.From != nil && rel.To != nil && rel.From.RelKind != rel.To.RelKind { + foundFlip = true + } + } + } + if !foundFlip { + t.Fatal("precondition: expected DiffModify with RelKind flip on 'v'") + } + + plan := DropsFromDiff(from, to, diff) + drops := plan.Filter(func(op MigrationOp) bool { + return op.Type == OpDropView && op.ObjectName == "v" + }).Ops + if len(drops) != 1 { + t.Fatalf("expected 1 OpDropView for view→matview flip, got %d: %+v", len(drops), plan.Ops) + } + }) + + t.Run("matview query modification emits OpDropView", func(t *testing.T) { + from, err := LoadSQL(` + CREATE TABLE t (id integer, name text); + CREATE MATERIALIZED VIEW mv AS SELECT id FROM t; + `) + if err != nil { + t.Fatal(err) + } + to, err := LoadSQL(` + CREATE TABLE t (id integer, name text); + CREATE MATERIALIZED VIEW mv AS SELECT id, name FROM t; + `) + if err != nil { + t.Fatal(err) + } + diff := Diff(from, to) + + // Precondition: DiffModify on the matview. + var foundMatviewModify bool + for _, rel := range diff.Relations { + if rel.Action == DiffModify && rel.Name == "mv" { + foundMatviewModify = true + } + } + if !foundMatviewModify { + t.Fatal("precondition: expected DiffModify on matview 'mv'") + } + + plan := DropsFromDiff(from, to, diff) + drops := plan.Filter(func(op MigrationOp) bool { + return op.Type == OpDropView && op.ObjectName == "mv" + }).Ops + if len(drops) != 1 { + t.Fatalf("expected 1 OpDropView for matview query modification, got %d: %+v", len(drops), plan.Ops) + } + }) + + t.Run("add-only diff emits no OpDropView", func(t *testing.T) { + from, err := LoadSQL(`CREATE TABLE t (id integer);`) + if err != nil { + t.Fatal(err) + } + to, err := LoadSQL(` + CREATE TABLE t (id integer); + CREATE VIEW v AS SELECT id FROM t; + `) + if err != nil { + t.Fatal(err) + } + diff := Diff(from, to) + plan := DropsFromDiff(from, to, diff) + drops := plan.Filter(func(op MigrationOp) bool { + return op.Type == OpDropView + }).Ops + if len(drops) != 0 { + t.Errorf("expected 0 OpDropView for add-only diff, got %d: %+v", len(drops), drops) + } + }) +} + +// --------------------------------------------------------------------------- +// Task 16: dropsForTriggers +// --------------------------------------------------------------------------- + +func TestDropsFromDiff_DropTrigger(t *testing.T) { + t.Run("drop trigger emits OpDropTrigger", func(t *testing.T) { + from, err := LoadSQL(` + CREATE TABLE t (id integer); + CREATE FUNCTION trg_fn() RETURNS trigger AS $$ BEGIN RETURN NEW; END $$ LANGUAGE plpgsql; + CREATE TRIGGER my_trg BEFORE INSERT ON t FOR EACH ROW EXECUTE FUNCTION trg_fn(); + `) + if err != nil { + t.Fatal(err) + } + to, err := LoadSQL(` + CREATE TABLE t (id integer); + CREATE FUNCTION trg_fn() RETURNS trigger AS $$ BEGIN RETURN NEW; END $$ LANGUAGE plpgsql; + `) + if err != nil { + t.Fatal(err) + } + diff := Diff(from, to) + plan := DropsFromDiff(from, to, diff) + + drops := plan.Filter(func(op MigrationOp) bool { + return op.Type == OpDropTrigger + }).Ops + if len(drops) != 1 { + t.Fatalf("expected 1 OpDropTrigger, got %d: %+v", len(drops), plan.Ops) + } + op := drops[0] + if op.ObjectName != "my_trg" { + t.Errorf("ObjectName: got %q, want %q (trigger name)", op.ObjectName, "my_trg") + } + if op.ParentObject != "t" { + t.Errorf("ParentObject: got %q, want %q (table name)", op.ParentObject, "t") + } + if op.SchemaName != "public" { + t.Errorf("SchemaName: got %q, want %q", op.SchemaName, "public") + } + if op.Phase != PhasePre { + t.Errorf("Phase: got %v, want PhasePre", op.Phase) + } + if op.ObjType != 'T' { + t.Errorf("ObjType: got %c, want 'T'", op.ObjType) + } + if op.Priority != PriorityTrigger { + t.Errorf("Priority: got %d, want PriorityTrigger(%d)", op.Priority, PriorityTrigger) + } + if !op.Transactional { + t.Errorf("Transactional: got false, want true") + } + if op.ObjOID == 0 { + t.Errorf("ObjOID: got 0, want nonzero") + } + }) + + t.Run("modify trigger BEFORE to AFTER emits OpDropTrigger", func(t *testing.T) { + from, err := LoadSQL(` + CREATE TABLE t (id integer); + CREATE FUNCTION trg_fn() RETURNS trigger AS $$ BEGIN RETURN NEW; END $$ LANGUAGE plpgsql; + CREATE TRIGGER my_trg BEFORE INSERT ON t FOR EACH ROW EXECUTE FUNCTION trg_fn(); + `) + if err != nil { + t.Fatal(err) + } + to, err := LoadSQL(` + CREATE TABLE t (id integer); + CREATE FUNCTION trg_fn() RETURNS trigger AS $$ BEGIN RETURN NEW; END $$ LANGUAGE plpgsql; + CREATE TRIGGER my_trg AFTER INSERT ON t FOR EACH ROW EXECUTE FUNCTION trg_fn(); + `) + if err != nil { + t.Fatal(err) + } + diff := Diff(from, to) + + // Precondition: DiffModify on trigger. + var foundTrigModify bool + for _, rel := range diff.Relations { + if rel.Action == DiffModify { + for _, te := range rel.Triggers { + if te.Action == DiffModify && te.Name == "my_trg" { + foundTrigModify = true + } + } + } + } + if !foundTrigModify { + t.Fatal("precondition: expected DiffModify on trigger 'my_trg'") + } + + plan := DropsFromDiff(from, to, diff) + drops := plan.Filter(func(op MigrationOp) bool { + return op.Type == OpDropTrigger + }).Ops + if len(drops) != 1 { + t.Fatalf("expected 1 OpDropTrigger for modified trigger (BEFORE→AFTER), got %d: %+v", len(drops), plan.Ops) + } + if drops[0].ObjectName != "my_trg" { + t.Errorf("ObjectName: got %q, want %q", drops[0].ObjectName, "my_trg") + } + }) +} + +// --------------------------------------------------------------------------- +// Task 17: dropsForDependentViews +// --------------------------------------------------------------------------- + +func TestDropsFromDiff_DependentViewCascade(t *testing.T) { + t.Run("column type change cascades to dependent view", func(t *testing.T) { + from, err := LoadSQL(` + CREATE TABLE t (id integer, val integer); + CREATE VIEW v AS SELECT id, val FROM t; + `) + if err != nil { + t.Fatal(err) + } + to, err := LoadSQL(` + CREATE TABLE t (id integer, val text); + CREATE VIEW v AS SELECT id, val FROM t; + `) + if err != nil { + t.Fatal(err) + } + diff := Diff(from, to) + plan := DropsFromDiff(from, to, diff) + + hasViewDrop := false + for _, op := range plan.Ops { + if op.Type == OpDropView && op.ObjectName == "v" { + hasViewDrop = true + } + } + if !hasViewDrop { + t.Errorf("expected dependent OpDropView for v cascaded by column type change, got none. ops: %+v", plan.Ops) + } + }) + + t.Run("transitive view cascade (view depends on view depends on table)", func(t *testing.T) { + from, err := LoadSQL(` + CREATE TABLE t (id integer, val integer); + CREATE VIEW v1 AS SELECT id, val FROM t; + CREATE VIEW v2 AS SELECT id FROM v1; + `) + if err != nil { + t.Fatal(err) + } + to, err := LoadSQL(` + CREATE TABLE t (id integer, val text); + CREATE VIEW v1 AS SELECT id, val FROM t; + CREATE VIEW v2 AS SELECT id FROM v1; + `) + if err != nil { + t.Fatal(err) + } + diff := Diff(from, to) + plan := DropsFromDiff(from, to, diff) + + dropped := map[string]bool{} + for _, op := range plan.Ops { + if op.Type == OpDropView { + dropped[op.ObjectName] = true + } + } + if !dropped["v1"] { + t.Errorf("expected OpDropView for v1, got none. ops: %+v", plan.Ops) + } + if !dropped["v2"] { + t.Errorf("expected transitive OpDropView for v2 (depends on v1), got none. ops: %+v", plan.Ops) + } + }) + + t.Run("column drop cascades to dependent view", func(t *testing.T) { + from, err := LoadSQL(` + CREATE TABLE t (id integer, name text); + CREATE VIEW v AS SELECT id, name FROM t; + `) + if err != nil { + t.Fatal(err) + } + to, err := LoadSQL(` + CREATE TABLE t (id integer); + CREATE VIEW v AS SELECT id FROM t; + `) + if err != nil { + t.Fatal(err) + } + diff := Diff(from, to) + plan := DropsFromDiff(from, to, diff) + + hasViewDrop := false + for _, op := range plan.Ops { + if op.Type == OpDropView && op.ObjectName == "v" { + hasViewDrop = true + } + } + if !hasViewDrop { + t.Errorf("expected dependent OpDropView for v cascaded by column drop, got none. ops: %+v", plan.Ops) + } + }) + + t.Run("no cascade when columns unchanged", func(t *testing.T) { + from, err := LoadSQL(` + CREATE TABLE t (id integer, val integer); + CREATE VIEW v AS SELECT id, val FROM t; + `) + if err != nil { + t.Fatal(err) + } + to, err := LoadSQL(` + CREATE TABLE t (id integer, val integer, extra text); + CREATE VIEW v AS SELECT id, val FROM t; + `) + if err != nil { + t.Fatal(err) + } + diff := Diff(from, to) + plan := DropsFromDiff(from, to, diff) + + // Adding a column should NOT cascade a view drop. + viewDrops := plan.Filter(func(op MigrationOp) bool { + return op.Type == OpDropView && op.ObjectName == "v" + }).Ops + if len(viewDrops) != 0 { + t.Errorf("expected no dependent view drop for column-add-only change, got %d: %+v", len(viewDrops), viewDrops) + } + }) +} + func TestDropsFromDiff_DropSequence_SkipsIdentity(t *testing.T) { from, err := LoadSQL(` CREATE TABLE t (id integer GENERATED ALWAYS AS IDENTITY); @@ -468,3 +1191,104 @@ func TestDropsFromDiff_DropSequence_SkipsIdentity(t *testing.T) { t.Errorf("expected ObjectName=my_seq, got %q", seqDrops[0].ObjectName) } } + +// --------------------------------------------------------------------------- +// Task 18: Differential test — DropsFromDiff vs GenerateMigration +// --------------------------------------------------------------------------- + +// TestDropsFromDiff_DifferentialAgainstGenerateMigration is the correctness +// gate ensuring all dropsForX helpers produce the same drop ops as the +// canonical GenerateMigration path. It builds a non-trivial fixture +// exercising 8+ drop categories, runs both code paths on the same input, +// normalizes the outputs (zeroing SQL/Warning, re-sorting with sortDropOps), +// and compares field-by-field. +func TestDropsFromDiff_DifferentialAgainstGenerateMigration(t *testing.T) { + // The "from" catalog has objects across many categories. The "to" catalog + // drops the entire reporting schema (covering OpDropSchema, OpDropTable, + // OpDropView, OpDropFunction, OpDropType, OpDropSequence), changes + // public.t1.val from integer to text (covering OpDropConstraint via CHECK + // cascade, and OpDropView via dependent-view cascade), modifies the matview + // query (covering matview OpDropView), and drops the trigger (covering + // OpDropTrigger). + from, err := LoadSQL(` + CREATE SCHEMA reporting; + CREATE TABLE reporting.users (id integer, name text, val integer); + ALTER TABLE reporting.users ADD CONSTRAINT users_val_check CHECK (val > 0); + CREATE TABLE reporting.orders (id integer); + CREATE VIEW reporting.user_view AS SELECT id, val FROM reporting.users; + CREATE FUNCTION reporting.add_one(x integer) RETURNS integer AS $$ SELECT x+1 $$ LANGUAGE sql; + CREATE TYPE reporting.color AS ENUM ('red', 'green'); + CREATE SEQUENCE reporting.seq; + CREATE TABLE public.t1 (id integer, val integer); + ALTER TABLE public.t1 ADD CONSTRAINT t1_val_positive CHECK (val > 0); + CREATE VIEW public.v1 AS SELECT id, val FROM public.t1; + CREATE MATERIALIZED VIEW public.mv1 AS SELECT id FROM public.t1; + CREATE FUNCTION public.trig_fn() RETURNS trigger AS $$ BEGIN RETURN NEW; END $$ LANGUAGE plpgsql; + CREATE TRIGGER trg BEFORE INSERT ON public.t1 FOR EACH ROW EXECUTE FUNCTION public.trig_fn(); + `) + if err != nil { + t.Fatal(err) + } + + to, err := LoadSQL(` + CREATE TABLE public.t1 (id integer, val text); + CREATE VIEW public.v1 AS SELECT id, val FROM public.t1; + CREATE MATERIALIZED VIEW public.mv1 AS SELECT id, val FROM public.t1; + CREATE FUNCTION public.trig_fn() RETURNS trigger AS $$ BEGIN RETURN NEW; END $$ LANGUAGE plpgsql; + `) + if err != nil { + t.Fatal(err) + } + + diff := Diff(from, to) + + // --- GenerateMigration path (canonical) --- + gmPlan := GenerateMigration(from, to, diff) + isDrop := func(op MigrationOp) bool { + return strings.HasPrefix(string(op.Type), "Drop") + } + gmDrops := gmPlan.Filter(isDrop).Ops + // Normalize: zero out SQL and Warning (DropsFromDiff doesn't populate these). + for i := range gmDrops { + gmDrops[i].SQL = "" + gmDrops[i].Warning = "" + } + sortDropOps(gmDrops) + + // --- DropsFromDiff path (under test) --- + dfdPlan := DropsFromDiff(from, to, diff) + dfdDrops := append([]MigrationOp(nil), dfdPlan.Ops...) + sortDropOps(dfdDrops) + + // --- Compare --- + if len(gmDrops) != len(dfdDrops) { + t.Logf("GenerateMigration drops (%d):", len(gmDrops)) + for i, op := range gmDrops { + t.Logf(" [%d] %+v", i, op) + } + t.Logf("DropsFromDiff drops (%d):", len(dfdDrops)) + for i, op := range dfdDrops { + t.Logf(" [%d] %+v", i, op) + } + t.Fatalf("count mismatch: GenerateMigration produced %d drops, DropsFromDiff produced %d", + len(gmDrops), len(dfdDrops)) + } + + for i := range gmDrops { + gm := gmDrops[i] + dfd := dfdDrops[i] + if gm != dfd { + t.Errorf("mismatch at index %d:\n GM: %+v\n DFD: %+v", i, gm, dfd) + } + } + + // Sanity: we should have at least 5 different drop categories. + categories := make(map[MigrationOpType]bool) + for _, op := range gmDrops { + categories[op.Type] = true + } + if len(categories) < 5 { + t.Errorf("expected at least 5 drop categories exercised, got %d: %v", len(categories), categories) + } + t.Logf("differential test passed: %d drop ops compared across %d categories", len(gmDrops), len(categories)) +} From 6c35deef0fd1b5995a31b93b2b6b5fcc33e5fd6c Mon Sep 17 00:00:00 2001 From: Vincent Huang Date: Thu, 9 Apr 2026 16:47:46 -0700 Subject: [PATCH 3/5] fix(pg/catalog): add missing extension/index/policy drop producers + fix doc comment MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses review findings: 1. Add three missing dropsForX helpers that GenerateMigration covers but DropsFromDiff previously omitted: - dropsForExtensions (migration_extension.go — DiffDrop) - dropsForIndexes (migration_index.go — DiffDrop + DiffModify, skipping constraint-backed indexes) - dropsForPolicies (migration_policy.go — DiffDrop + DiffModify when CmdType or Permissive changed) 2. Fix DropsFromDiff doc comment: document Warning as a third difference (was "two differences", now correctly says "three differences"). 3. Broaden the differential test fixture to include extensions, explicit indexes, and RLS policies. The test now compares 15 drop ops across 11 categories with zero mismatches. Per-category regression tests added for all three new producers. Co-Authored-By: Claude Opus 4.6 (1M context) --- pg/catalog/drops.go | 143 ++++++++++++++++++++++++++++++++++++++- pg/catalog/drops_test.go | 133 ++++++++++++++++++++++++++++++++++-- 2 files changed, 270 insertions(+), 6 deletions(-) diff --git a/pg/catalog/drops.go b/pg/catalog/drops.go index a2e6da7a..29cf2f92 100644 --- a/pg/catalog/drops.go +++ b/pg/catalog/drops.go @@ -4,9 +4,11 @@ import "sort" // DropsFromDiff returns a MigrationPlan containing only the destructive // (DROP*) operations that GenerateMigration would produce for the same -// inputs, in the same form, with two differences: +// inputs, in the same form, with three differences: // // - SQL is always empty. Callers needing DDL text must call GenerateMigration. +// - Warning is always empty. GenerateMigration populates warnings on some +// destructive ops (e.g., table recreates); DropsFromDiff omits them. // - Operations are sorted by a deterministic lexicographic key // (SchemaName, ObjectName, ParentObject, Type) — see sortDropOps — not // topologically. @@ -31,6 +33,7 @@ func DropsFromDiff(from, to *Catalog, diff *SchemaDiff) *MigrationPlan { // All helpers take the same (from, to, diff) signature for call-site // uniformity, even when a particular helper does not need from or to. ops = append(ops, dropsForSchemas(from, to, diff)...) + ops = append(ops, dropsForExtensions(from, to, diff)...) ops = append(ops, dropsForEnums(from, to, diff)...) ops = append(ops, dropsForDomains(from, to, diff)...) ops = append(ops, dropsForRanges(from, to, diff)...) @@ -43,7 +46,9 @@ func DropsFromDiff(from, to *Catalog, diff *SchemaDiff) *MigrationPlan { ops = append(ops, dropsForCheckCascades(from, to, diff)...) ops = append(ops, dropsForConstraints(from, to, diff)...) ops = append(ops, dropsForViews(from, to, diff)...) + ops = append(ops, dropsForIndexes(from, to, diff)...) ops = append(ops, dropsForTriggers(from, to, diff)...) + ops = append(ops, dropsForPolicies(from, to, diff)...) ops = append(ops, dropsForDependentViews(from, to, diff, ops)...) sortDropOps(ops) @@ -79,6 +84,35 @@ func dropsForSchemas(_, _ *Catalog, diff *SchemaDiff) []MigrationOp { return ops } +// dropsForExtensions mirrors the DiffDrop arm of generateExtensionDDL in +// migration_extension.go. Extensions are top-level objects: SchemaName is +// empty, ObjectName holds the extension name. +// +// from and to are unused (the OID lives on entry.From) but kept for +// signature uniformity across all dropsForX helpers — see DropsFromDiff. +func dropsForExtensions(_, _ *Catalog, diff *SchemaDiff) []MigrationOp { + var ops []MigrationOp + for _, entry := range diff.Extensions { + if entry.Action != DiffDrop { + continue + } + var extOID uint32 + if entry.From != nil { + extOID = entry.From.OID + } + ops = append(ops, MigrationOp{ + Type: OpDropExtension, + ObjectName: entry.Name, + Transactional: true, + Phase: PhasePre, + ObjType: 'e', + ObjOID: extOID, + Priority: PriorityExtension, + }) + } + return ops +} + // dropsForEnums mirrors the DiffDrop arm of generateEnumDDL in // migration_enum.go. The OID is resolved against the from catalog by name // (resolveTypeOIDByName), so this helper genuinely uses from; to is unused @@ -587,6 +621,59 @@ func dropsForViews(_, _ *Catalog, diff *SchemaDiff) []MigrationOp { return ops } +// dropsForIndexes mirrors the DiffDrop and DiffModify arms of +// generateIndexDDL in migration_index.go. Walks diff.Relations for +// DiffModify entries, then walks relEntry.Indexes. Both DiffDrop and +// DiffModify on indexes emit drops (modified indexes = DROP old + CREATE +// new). Indexes where ConstraintOID != 0 are skipped (managed by +// constraint DDL). +// +// from and to are unused but kept for signature uniformity — see DropsFromDiff. +func dropsForIndexes(_, _ *Catalog, diff *SchemaDiff) []MigrationOp { + var ops []MigrationOp + for _, relEntry := range diff.Relations { + if relEntry.Action != DiffModify { + continue + } + for _, idxEntry := range relEntry.Indexes { + switch idxEntry.Action { + case DiffDrop: + idx := idxEntry.From + if idx.ConstraintOID != 0 { + continue + } + ops = append(ops, MigrationOp{ + Type: OpDropIndex, + SchemaName: relEntry.SchemaName, + ObjectName: idx.Name, + Transactional: true, + Phase: PhasePre, + ObjType: 'i', + ObjOID: idx.OID, + Priority: PriorityIndex, + }) + case DiffModify: + idxFrom := idxEntry.From + idxTo := idxEntry.To + if idxFrom.ConstraintOID != 0 || idxTo.ConstraintOID != 0 { + continue + } + ops = append(ops, MigrationOp{ + Type: OpDropIndex, + SchemaName: relEntry.SchemaName, + ObjectName: idxFrom.Name, + Transactional: true, + Phase: PhasePre, + ObjType: 'i', + ObjOID: idxFrom.OID, + Priority: PriorityIndex, + }) + } + } + } + return ops +} + // dropsForTriggers mirrors the DiffDrop and DiffModify arms of // triggerOpsForRelation in migration_trigger.go. Both DiffDrop and DiffModify // emit drops (modify = DROP old + CREATE new). @@ -637,6 +724,60 @@ func dropsForTriggers(_, _ *Catalog, diff *SchemaDiff) []MigrationOp { return ops } +// dropsForPolicies mirrors the DiffDrop and DiffModify arms of +// policyOpsForRelation in migration_policy.go. Walks diff.Relations for +// DiffModify entries, then walks rel.Policies. Emits drops for DiffDrop +// (always) and DiffModify (only when CmdType or Permissive changed, +// which forces DROP + CREATE). +// +// ObjectName is the POLICY name; ParentObject is the TABLE name (unqualified). +// from and to are unused but kept for signature uniformity — see DropsFromDiff. +func dropsForPolicies(_, _ *Catalog, diff *SchemaDiff) []MigrationOp { + var ops []MigrationOp + for _, rel := range diff.Relations { + if rel.Action != DiffModify { + continue + } + for _, pe := range rel.Policies { + switch pe.Action { + case DiffDrop: + if pe.From == nil { + continue + } + ops = append(ops, MigrationOp{ + Type: OpDropPolicy, + SchemaName: rel.SchemaName, + ObjectName: pe.From.Name, + ParentObject: rel.Name, + Transactional: true, + Phase: PhasePre, + ObjType: 'p', + ObjOID: pe.From.OID, + Priority: PriorityPolicy, + }) + case DiffModify: + if pe.From == nil || pe.To == nil { + continue + } + if pe.From.CmdType != pe.To.CmdType || pe.From.Permissive != pe.To.Permissive { + ops = append(ops, MigrationOp{ + Type: OpDropPolicy, + SchemaName: rel.SchemaName, + ObjectName: pe.From.Name, + ParentObject: rel.Name, + Transactional: true, + Phase: PhasePre, + ObjType: 'p', + ObjOID: pe.From.OID, + Priority: PriorityPolicy, + }) + } + } + } + } + return ops +} + // dropsForDependentViews mirrors wrapColumnTypeChangesWithViewOps in // migration.go:241-401. When a table's columns are dropped, retyped, or // the table undergoes a RelKind/inheritance change, PostgreSQL requires diff --git a/pg/catalog/drops_test.go b/pg/catalog/drops_test.go index 2d56f6bd..7d70ff6d 100644 --- a/pg/catalog/drops_test.go +++ b/pg/catalog/drops_test.go @@ -1192,6 +1192,122 @@ func TestDropsFromDiff_DropSequence_SkipsIdentity(t *testing.T) { } } +// --------------------------------------------------------------------------- +// Task 19: dropsForExtensions +// --------------------------------------------------------------------------- + +func TestDropsFromDiff_DropExtension(t *testing.T) { + RegisterExtensionSQL("pgcrypto", "") + from, err := LoadSQL("CREATE EXTENSION pgcrypto;") + if err != nil { + t.Fatal(err) + } + to, err := LoadSQL("") + if err != nil { + t.Fatal(err) + } + diff := Diff(from, to) + plan := DropsFromDiff(from, to, diff) + + drops := plan.Filter(func(op MigrationOp) bool { + return op.Type == OpDropExtension + }).Ops + if len(drops) != 1 { + t.Fatalf("expected 1 OpDropExtension, got %d", len(drops)) + } + op := drops[0] + if op.ObjectName != "pgcrypto" { + t.Errorf("expected ObjectName=pgcrypto, got %q", op.ObjectName) + } + if op.SQL != "" { + t.Errorf("expected empty SQL") + } + if op.Phase != PhasePre { + t.Errorf("expected PhasePre") + } + if op.ObjType != 'e' { + t.Errorf("expected ObjType='e', got %c", op.ObjType) + } + if op.Priority != PriorityExtension { + t.Errorf("expected PriorityExtension") + } +} + +// --------------------------------------------------------------------------- +// Task 20: dropsForIndexes +// --------------------------------------------------------------------------- + +func TestDropsFromDiff_DropIndex(t *testing.T) { + from, err := LoadSQL(` + CREATE TABLE t (id integer, val integer); + CREATE INDEX idx_t_val ON t (val); + `) + if err != nil { + t.Fatal(err) + } + to, err := LoadSQL(`CREATE TABLE t (id integer, val integer);`) + if err != nil { + t.Fatal(err) + } + diff := Diff(from, to) + plan := DropsFromDiff(from, to, diff) + + drops := plan.Filter(func(op MigrationOp) bool { + return op.Type == OpDropIndex + }).Ops + if len(drops) != 1 { + t.Fatalf("expected 1 OpDropIndex, got %d", len(drops)) + } + op := drops[0] + if op.ObjectName != "idx_t_val" { + t.Errorf("expected ObjectName=idx_t_val, got %q", op.ObjectName) + } + if op.ObjType != 'i' { + t.Errorf("expected ObjType='i', got %c", op.ObjType) + } +} + +// --------------------------------------------------------------------------- +// Task 21: dropsForPolicies +// --------------------------------------------------------------------------- + +func TestDropsFromDiff_DropPolicy(t *testing.T) { + from, err := LoadSQL(` + CREATE TABLE t (id integer); + ALTER TABLE t ENABLE ROW LEVEL SECURITY; + CREATE POLICY read_all ON t FOR SELECT USING (true); + `) + if err != nil { + t.Fatal(err) + } + to, err := LoadSQL(` + CREATE TABLE t (id integer); + ALTER TABLE t ENABLE ROW LEVEL SECURITY; + `) + if err != nil { + t.Fatal(err) + } + diff := Diff(from, to) + plan := DropsFromDiff(from, to, diff) + + drops := plan.Filter(func(op MigrationOp) bool { + return op.Type == OpDropPolicy + }).Ops + if len(drops) != 1 { + t.Fatalf("expected 1 OpDropPolicy, got %d", len(drops)) + } + op := drops[0] + if op.ObjectName != "read_all" { + t.Errorf("expected ObjectName=read_all, got %q", op.ObjectName) + } + if op.ParentObject != "t" { + t.Errorf("expected ParentObject=t, got %q", op.ParentObject) + } + if op.ObjType != 'p' { + t.Errorf("expected ObjType='p', got %c", op.ObjType) + } +} + // --------------------------------------------------------------------------- // Task 18: Differential test — DropsFromDiff vs GenerateMigration // --------------------------------------------------------------------------- @@ -1207,11 +1323,15 @@ func TestDropsFromDiff_DifferentialAgainstGenerateMigration(t *testing.T) { // drops the entire reporting schema (covering OpDropSchema, OpDropTable, // OpDropView, OpDropFunction, OpDropType, OpDropSequence), changes // public.t1.val from integer to text (covering OpDropConstraint via CHECK - // cascade, and OpDropView via dependent-view cascade), modifies the matview - // query (covering matview OpDropView), and drops the trigger (covering - // OpDropTrigger). + // cascade, OpDropIndex for the index on val, and OpDropView via + // dependent-view cascade), modifies the matview query (covering matview + // OpDropView), drops the trigger (covering OpDropTrigger), drops the + // extension (covering OpDropExtension), and drops the policy (covering + // OpDropPolicy). + RegisterExtensionSQL("pgcrypto", "") from, err := LoadSQL(` CREATE SCHEMA reporting; + CREATE EXTENSION pgcrypto; CREATE TABLE reporting.users (id integer, name text, val integer); ALTER TABLE reporting.users ADD CONSTRAINT users_val_check CHECK (val > 0); CREATE TABLE reporting.orders (id integer); @@ -1221,10 +1341,13 @@ func TestDropsFromDiff_DifferentialAgainstGenerateMigration(t *testing.T) { CREATE SEQUENCE reporting.seq; CREATE TABLE public.t1 (id integer, val integer); ALTER TABLE public.t1 ADD CONSTRAINT t1_val_positive CHECK (val > 0); + CREATE INDEX idx_t1_val ON public.t1 (val); CREATE VIEW public.v1 AS SELECT id, val FROM public.t1; CREATE MATERIALIZED VIEW public.mv1 AS SELECT id FROM public.t1; CREATE FUNCTION public.trig_fn() RETURNS trigger AS $$ BEGIN RETURN NEW; END $$ LANGUAGE plpgsql; CREATE TRIGGER trg BEFORE INSERT ON public.t1 FOR EACH ROW EXECUTE FUNCTION public.trig_fn(); + ALTER TABLE public.t1 ENABLE ROW LEVEL SECURITY; + CREATE POLICY read_all ON public.t1 FOR SELECT USING (true); `) if err != nil { t.Fatal(err) @@ -1287,8 +1410,8 @@ func TestDropsFromDiff_DifferentialAgainstGenerateMigration(t *testing.T) { for _, op := range gmDrops { categories[op.Type] = true } - if len(categories) < 5 { - t.Errorf("expected at least 5 drop categories exercised, got %d: %v", len(categories), categories) + if len(categories) < 8 { + t.Errorf("expected at least 8 drop categories exercised, got %d: %v", len(categories), categories) } t.Logf("differential test passed: %d drop ops compared across %d categories", len(gmDrops), len(categories)) } From c2ce63a38fd1e992c5b2a6b9cc1417aac62b95e6 Mon Sep 17 00:00:00 2001 From: Vincent Huang Date: Thu, 9 Apr 2026 21:28:37 -0700 Subject: [PATCH 4/5] refactor(pg/catalog): clean up DropsFromDiff per review feedback MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address code quality findings: 1. Collapse identical DiffDrop/DiffModify case bodies in dropsForConstraints and dropsForTriggers — both arms produce the same drop op (modify = DROP old + ADD new; we only emit the drop). 2. Extract dropViewOp helper (mirrors dropFunctionOp pattern) to replace 5 identical OpDropView struct literals across dropsForViews and dropsForDependentViews. 3. Add explanatory comment on dropsForConstraints documenting why OpDropConstraint has two different field conventions in the same file (dropsForCheckCascades puts table name in ObjectName with PhaseMain; dropsForConstraints puts constraint name in ObjectName with ParentObject=tableName and PhasePre). Both mirror their respective omni sources faithfully. Net -44 lines of production code. Co-Authored-By: Claude Opus 4.6 (1M context) --- pg/catalog/drops.go | 172 +++++++++++++++++--------------------------- 1 file changed, 64 insertions(+), 108 deletions(-) diff --git a/pg/catalog/drops.go b/pg/catalog/drops.go index 29cf2f92..ede5824c 100644 --- a/pg/catalog/drops.go +++ b/pg/catalog/drops.go @@ -492,6 +492,15 @@ func dropsForCheckCascades(from, to *Catalog, diff *SchemaDiff) []MigrationOp { // // ObjectName is the CONSTRAINT name; ParentObject is the TABLE name. // ObjOID is NOT set (zero), matching buildDropConstraintOp. +// +// NOTE: this uses a DIFFERENT field convention from dropsForCheckCascades, +// even though both produce OpDropConstraint. The difference mirrors omni's +// own inconsistency: buildDropConstraintOp (migration_constraint.go) puts +// the constraint name in ObjectName and the table in ParentObject; +// columnModifyOps (migration_column.go) puts the table name in ObjectName +// with no ParentObject, and the outer loop overrides Phase/Priority. Both +// conventions are correct per their sources, and the differential test +// confirms field-level parity with GenerateMigration. func dropsForConstraints(_, _ *Catalog, diff *SchemaDiff) []MigrationOp { var ops []MigrationOp for _, rel := range diff.Relations { @@ -503,34 +512,23 @@ func dropsForConstraints(_, _ *Catalog, diff *SchemaDiff) []MigrationOp { (ce.From != nil && ce.From.Type == ConstraintTrigger) { continue } - switch ce.Action { - case DiffDrop: - if ce.From == nil { - continue - } - ops = append(ops, MigrationOp{ - Type: OpDropConstraint, - SchemaName: rel.SchemaName, - ObjectName: ce.From.Name, - ParentObject: rel.Name, - Phase: PhasePre, - ObjType: 'c', - Priority: PriorityConstraint, - }) - case DiffModify: - if ce.From == nil { - continue - } - ops = append(ops, MigrationOp{ - Type: OpDropConstraint, - SchemaName: rel.SchemaName, - ObjectName: ce.From.Name, - ParentObject: rel.Name, - Phase: PhasePre, - ObjType: 'c', - Priority: PriorityConstraint, - }) + // DiffDrop and DiffModify both produce the same drop op (modify = + // DROP old + ADD new in omni; we only emit the drop half). + if ce.Action != DiffDrop && ce.Action != DiffModify { + continue + } + if ce.From == nil { + continue } + ops = append(ops, MigrationOp{ + Type: OpDropConstraint, + SchemaName: rel.SchemaName, + ObjectName: ce.From.Name, + ParentObject: rel.Name, + Phase: PhasePre, + ObjType: 'c', + Priority: PriorityConstraint, + }) } } return ops @@ -555,16 +553,7 @@ func dropsForViews(_, _ *Catalog, diff *SchemaDiff) []MigrationOp { if entry.From.RelKind != 'v' && entry.From.RelKind != 'm' { continue } - ops = append(ops, MigrationOp{ - Type: OpDropView, - SchemaName: entry.SchemaName, - ObjectName: entry.Name, - Transactional: true, - Phase: PhasePre, - ObjType: 'r', - ObjOID: entry.From.OID, - Priority: PriorityView, - }) + ops = append(ops, dropViewOp(entry.SchemaName, entry.Name, entry.From.OID)) case DiffModify: if entry.From == nil || entry.To == nil { @@ -575,16 +564,7 @@ func dropsForViews(_, _ *Catalog, diff *SchemaDiff) []MigrationOp { // Site 3: RelKind flip (view↔matview). if (fromIsView || toIsView) && entry.From.RelKind != entry.To.RelKind { - ops = append(ops, MigrationOp{ - Type: OpDropView, - SchemaName: entry.SchemaName, - ObjectName: entry.Name, - Transactional: true, - Phase: PhasePre, - ObjType: 'r', - ObjOID: entry.From.OID, - Priority: PriorityView, - }) + ops = append(ops, dropViewOp(entry.SchemaName, entry.Name, entry.From.OID)) continue } @@ -592,35 +572,33 @@ func dropsForViews(_, _ *Catalog, diff *SchemaDiff) []MigrationOp { case 'v': // Site 4: view with incompatible column changes. if viewColumnsChanged(entry) { - ops = append(ops, MigrationOp{ - Type: OpDropView, - SchemaName: entry.SchemaName, - ObjectName: entry.Name, - Transactional: true, - Phase: PhasePre, - ObjType: 'r', - ObjOID: entry.From.OID, - Priority: PriorityView, - }) + ops = append(ops, dropViewOp(entry.SchemaName, entry.Name, entry.From.OID)) } case 'm': // Site 5: ALL matview modifications emit drop. - ops = append(ops, MigrationOp{ - Type: OpDropView, - SchemaName: entry.SchemaName, - ObjectName: entry.Name, - Transactional: true, - Phase: PhasePre, - ObjType: 'r', - ObjOID: entry.From.OID, - Priority: PriorityView, - }) + ops = append(ops, dropViewOp(entry.SchemaName, entry.Name, entry.From.OID)) } } } return ops } +// dropViewOp builds the OpDropView MigrationOp for a view or matview, +// mirroring the drop struct literals in generateViewDDL and +// buildModifyMatViewOps in migration_view.go minus SQL. +func dropViewOp(schemaName, name string, oid uint32) MigrationOp { + return MigrationOp{ + Type: OpDropView, + SchemaName: schemaName, + ObjectName: name, + Transactional: true, + Phase: PhasePre, + ObjType: 'r', + ObjOID: oid, + Priority: PriorityView, + } +} + // dropsForIndexes mirrors the DiffDrop and DiffModify arms of // generateIndexDDL in migration_index.go. Walks diff.Relations for // DiffModify entries, then walks relEntry.Indexes. Both DiffDrop and @@ -687,38 +665,25 @@ func dropsForTriggers(_, _ *Catalog, diff *SchemaDiff) []MigrationOp { continue } for _, te := range rel.Triggers { - switch te.Action { - case DiffDrop: - if te.From == nil { - continue - } - ops = append(ops, MigrationOp{ - Type: OpDropTrigger, - SchemaName: rel.SchemaName, - ObjectName: te.From.Name, - ParentObject: rel.Name, - Transactional: true, - Phase: PhasePre, - ObjType: 'T', - ObjOID: te.From.OID, - Priority: PriorityTrigger, - }) - case DiffModify: - if te.From == nil { - continue - } - ops = append(ops, MigrationOp{ - Type: OpDropTrigger, - SchemaName: rel.SchemaName, - ObjectName: te.From.Name, - ParentObject: rel.Name, - Transactional: true, - Phase: PhasePre, - ObjType: 'T', - ObjOID: te.From.OID, - Priority: PriorityTrigger, - }) + // DiffDrop and DiffModify both produce the same drop op (modify = + // DROP old + CREATE new in omni; we only emit the drop half). + if te.Action != DiffDrop && te.Action != DiffModify { + continue + } + if te.From == nil { + continue } + ops = append(ops, MigrationOp{ + Type: OpDropTrigger, + SchemaName: rel.SchemaName, + ObjectName: te.From.Name, + ParentObject: rel.Name, + Transactional: true, + Phase: PhasePre, + ObjType: 'T', + ObjOID: te.From.OID, + Priority: PriorityTrigger, + }) } } return ops @@ -895,16 +860,7 @@ func dropsForDependentViews(from, to *Catalog, diff *SchemaDiff, existingOps []M if existing[key] { continue } - ops = append(ops, MigrationOp{ - Type: OpDropView, - SchemaName: v.schema, - ObjectName: v.name, - Transactional: true, - Phase: PhasePre, - ObjType: 'r', - ObjOID: v.oid, - Priority: PriorityView, - }) + ops = append(ops, dropViewOp(v.schema, v.name, v.oid)) } return ops } From 06a9f7579e435b2d22287841b81899f758cf5bff Mon Sep 17 00:00:00 2001 From: Vincent Huang Date: Thu, 9 Apr 2026 21:39:25 -0700 Subject: [PATCH 5/5] fix(pg/catalog): add nil guard on idxEntry.From in dropsForIndexes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds a defensive nil check on the DiffDrop branch of dropsForIndexes, matching the pattern used by every other dropsForX helper. The omni source (migration_index.go) also lacks this check, so this is not a correctness fix — purely a consistency improvement. Co-Authored-By: Claude Opus 4.6 (1M context) --- pg/catalog/drops.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/pg/catalog/drops.go b/pg/catalog/drops.go index ede5824c..49611738 100644 --- a/pg/catalog/drops.go +++ b/pg/catalog/drops.go @@ -617,6 +617,9 @@ func dropsForIndexes(_, _ *Catalog, diff *SchemaDiff) []MigrationOp { switch idxEntry.Action { case DiffDrop: idx := idxEntry.From + if idx == nil { + continue + } if idx.ConstraintOID != 0 { continue }