From 55f80c2c1618ba9ec45fad394fea5c07a853e6a3 Mon Sep 17 00:00:00 2001 From: Shivam Maurya Date: Fri, 16 Oct 2026 04:58:22 +0530 Subject: [PATCH] fix: emit partition attach/detach when bounds or attachment change in plan diffTables never compared PartitionOf/PartitionOfSchema/PartitionBound, so a re-bounded or detached partition produced an empty plan. Mirror the existing table-level structural-change idiom (PersistenceChanged): flag the change, force a non-nil table diff, and emit DETACH + ATTACH PARTITION so the desired bound is applied. A table that is no longer a partition is detached and left standalone. Fixes #639 --- internal/diff/diff.go | 9 +++ internal/diff/partition_bound_test.go | 91 +++++++++++++++++++++++++++ internal/diff/table.go | 55 +++++++++++++++- 3 files changed, 154 insertions(+), 1 deletion(-) create mode 100644 internal/diff/partition_bound_test.go diff --git a/internal/diff/diff.go b/internal/diff/diff.go index 4b53e1b2..43bba912 100644 --- a/internal/diff/diff.go +++ b/internal/diff/diff.go @@ -454,6 +454,15 @@ type tableDiff struct { OldComment string NewComment string PersistenceChanged bool + // PartitionOfChanged records a changed parent (attach/detach) or a changed + // partition bound. Postgres cannot change either in place, so the change is + // emitted as DETACH + ATTACH PARTITION. + PartitionOfChanged bool + // OldPartitionOf/OldPartitionOfSchema name the parent the table was attached + // to before this change, needed to emit DETACH for a re-bounded or + // un-partitioned table. Empty when the table was not a partition. (#639) + OldPartitionOf string + OldPartitionOfSchema string } // ColumnDiff represents changes to a column diff --git a/internal/diff/partition_bound_test.go b/internal/diff/partition_bound_test.go new file mode 100644 index 00000000..e27c3295 --- /dev/null +++ b/internal/diff/partition_bound_test.go @@ -0,0 +1,91 @@ +package diff + +import ( + "strings" + "testing" + + "github.com/pgplex/pgschema/ir" +) + +// partitionBoundIR builds an IR with a partitioned parent and a single child +// partition whose attachment/bound is given. +func partitionBoundIR(child *ir.Table) *ir.IR { + parent := &ir.Table{ + Schema: "public", Name: "events", IsPartitioned: true, + PartitionKey: "RANGE (id)", + Columns: []*ir.Column{{Name: "id", DataType: "integer"}}, + } + return &ir.IR{Schemas: map[string]*ir.Schema{ + "public": {Name: "public", Tables: map[string]*ir.Table{ + "events": parent, + child.Name: child, + }}, + }} +} + +func boundSQL(t *testing.T, oldIR, newIR *ir.IR) string { + t.Helper() + var stmts []string + for _, change := range GenerateMigration(oldIR, newIR, "public") { + for _, s := range change.Statements { + stmts = append(stmts, s.SQL) + } + } + return strings.Join(stmts, "\n") +} + +func childTable(bound string) *ir.Table { + return &ir.Table{ + Schema: "public", Name: "events_early", + PartitionOf: "events", PartitionOfSchema: "public", PartitionBound: bound, + Columns: []*ir.Column{{Name: "id", DataType: "integer"}}, + } +} + +// A changed bound must produce a plan that re-attaches the partition, not an +// empty plan. (#639) +func TestPartitionBoundChangeReattaches(t *testing.T) { + oldIR := partitionBoundIR(childTable("FOR VALUES FROM (0) TO (10)")) + newIR := partitionBoundIR(childTable("FOR VALUES FROM (0) TO (20)")) + + sql := boundSQL(t, oldIR, newIR) + if strings.TrimSpace(sql) == "" { + t.Fatal("changed partition bound produced an empty plan") + } + if !strings.Contains(sql, "DETACH PARTITION") { + t.Fatalf("expected DETACH PARTITION, got:\n%s", sql) + } + if !strings.Contains(sql, "ATTACH PARTITION") || !strings.Contains(sql, "FOR VALUES FROM (0) TO (20)") { + t.Fatalf("expected re-ATTACH with the new bound, got:\n%s", sql) + } +} + +// A partition that is removed from the desired schema must be detached. (#639) +func TestPartitionDetachEmitsStatement(t *testing.T) { + oldIR := partitionBoundIR(childTable("FOR VALUES FROM (0) TO (10)")) + standalone := childTable("") + standalone.PartitionOf = "" + standalone.PartitionOfSchema = "" + newIR := partitionBoundIR(standalone) + + sql := boundSQL(t, oldIR, newIR) + if strings.TrimSpace(sql) == "" { + t.Fatal("detached partition produced an empty plan") + } + if !strings.Contains(sql, "DETACH PARTITION") { + t.Fatalf("expected DETACH PARTITION, got:\n%s", sql) + } + if strings.Contains(sql, "ATTACH PARTITION") { + t.Fatalf("detached partition should not be re-attached, got:\n%s", sql) + } +} + +// An unchanged partition must produce no plan. (#639 regression guard) +func TestPartitionBoundUnchangedNoPlan(t *testing.T) { + oldIR := partitionBoundIR(childTable("FOR VALUES FROM (0) TO (10)")) + newIR := partitionBoundIR(childTable("FOR VALUES FROM (0) TO (10)")) + + if sql := strings.TrimSpace(boundSQL(t, oldIR, newIR)); sql != "" { + t.Fatalf("unchanged partition produced a plan:\n%s", sql) + } +} diff --git a/internal/diff/table.go b/internal/diff/table.go index c9fc2e6d..eae7fab5 100644 --- a/internal/diff/table.go +++ b/internal/diff/table.go @@ -418,6 +418,17 @@ func diffTables(oldTable, newTable *ir.Table, targetSchema string, targetMajorVe diff.PersistenceChanged = true } + // Check for partition attachment/bound changes. Postgres cannot alter a + // partition bound in place and attaching/detaching changes the table's + // identity, so any difference must force a statement (#639). + if oldTable.PartitionOf != newTable.PartitionOf || + oldTable.PartitionOfSchema != newTable.PartitionOfSchema || + oldTable.PartitionBound != newTable.PartitionBound { + diff.PartitionOfChanged = true + diff.OldPartitionOf = oldTable.PartitionOf + diff.OldPartitionOfSchema = oldTable.PartitionOfSchema + } + // Return nil if no changes if len(diff.AddedColumns) == 0 && len(diff.DroppedColumns) == 0 && len(diff.ModifiedColumns) == 0 && len(diff.AddedConstraints) == 0 && @@ -427,7 +438,7 @@ func diffTables(oldTable, newTable *ir.Table, targetSchema string, targetMajorVe len(diff.DroppedTriggers) == 0 && len(diff.ModifiedTriggers) == 0 && len(diff.AddedPolicies) == 0 && len(diff.DroppedPolicies) == 0 && len(diff.ModifiedPolicies) == 0 && len(diff.RLSChanges) == 0 && - !diff.CommentChanged && !diff.PersistenceChanged { + !diff.CommentChanged && !diff.PersistenceChanged && !diff.PartitionOfChanged { return nil } @@ -1253,6 +1264,48 @@ func (td *tableDiff) generateAlterTableStatements(targetSchema string, collector collector.collect(context, sql) } + // Partition attachment/bound change: Postgres cannot ALTER a bound in place, + // so re-attach with DETACH + ATTACH PARTITION. A table that is no longer a + // partition is simply detached and left standalone. (#639) + if td.PartitionOfChanged { + tableName := getTableNameWithSchema(td.Table.Schema, td.Table.Name, targetSchema) + + // Detach from the old parent. The old schema may be empty for tables + // inspected before the parent schema was tracked; fall back to the + // table's own schema so the statement is still fully qualified. + if td.OldPartitionOf != "" { + oldSchema := td.OldPartitionOfSchema + if oldSchema == "" { + oldSchema = td.Table.Schema + } + oldParent := ir.QualifyEntityNameWithQuotesMode(oldSchema, td.OldPartitionOf, targetSchema, collector.qualifySchema) + collector.collect(&diffContext{ + Type: DiffTypeTable, + Operation: DiffOperationAlter, + Path: fmt.Sprintf("%s.%s", td.Table.Schema, td.Table.Name), + Source: td.Table, + CanRunInTransaction: true, + }, fmt.Sprintf("ALTER TABLE %s DETACH PARTITION %s;", oldParent, tableName)) + } + + // Re-attach to the new parent with the new bound, unless the table is + // no longer a partition (detach-only leaves it standalone). + if td.Table.PartitionOf != "" && td.Table.PartitionBound != "" { + parentSchema := td.Table.PartitionOfSchema + if parentSchema == "" { + parentSchema = td.Table.Schema + } + parentName := ir.QualifyEntityNameWithQuotesMode(parentSchema, td.Table.PartitionOf, targetSchema, collector.qualifySchema) + collector.collect(&diffContext{ + Type: DiffTypeTable, + Operation: DiffOperationAlter, + Path: fmt.Sprintf("%s.%s", td.Table.Schema, td.Table.Name), + Source: td.Table, + CanRunInTransaction: true, + }, fmt.Sprintf("ALTER TABLE %s ATTACH PARTITION %s %s;", parentName, tableName, td.Table.PartitionBound)) + } + } + // Policies that reference a column being re-created must go before the // DROP COLUMN they would otherwise block; the remaining policy drops keep // their usual place after the RLS changes below. (#591)