diff --git a/README.md b/README.md index d872d00..7a9578f 100644 --- a/README.md +++ b/README.md @@ -75,7 +75,7 @@ $ pg-schema-diff plan --dsn "postgres://postgres:postgres@localhost:5432/postgre * The use of postgres native operations for zero-downtime migrations wherever possible: * Concurrent index builds * Online index replacement: If some index is changed, the new version will be built before the old version is dropped, preventing a window where no index is backing queries - * Online check constraint builds: Check constraints are added as `INVALID` before being validated, eliminating the need + * Online constraint builds: Constraints (check, foreign key) are added as `INVALID` before being validated, eliminating the need for a long access-exclusive lock on the table * Online `NOT NULL` constraint creation using check constraints to eliminate the need for an access-exclusive lock on the table * Prioritized index builds: Building new indexes is always prioritized over deleting old indexes diff --git a/internal/migration_acceptance_tests/backwards_compat_cases_test.go b/internal/migration_acceptance_tests/backwards_compat_cases_test.go index ed22c3b..81e76db 100644 --- a/internal/migration_acceptance_tests/backwards_compat_cases_test.go +++ b/internal/migration_acceptance_tests/backwards_compat_cases_test.go @@ -84,7 +84,6 @@ var backCompatAcceptanceTestCases = []acceptanceTestCase{ `, }, expectedHazardTypes: []diff.MigrationHazardType{ - diff.MigrationHazardTypeAcquiresShareRowExclusiveLock, diff.MigrationHazardTypeDeletesData, }, diff --git a/internal/migration_acceptance_tests/database_schema_source_cases_test.go b/internal/migration_acceptance_tests/database_schema_source_cases_test.go index 99120c2..cc1ddb9 100644 --- a/internal/migration_acceptance_tests/database_schema_source_cases_test.go +++ b/internal/migration_acceptance_tests/database_schema_source_cases_test.go @@ -117,7 +117,6 @@ var databaseSchemaSourceTestCases = []acceptanceTestCase{ `, }, expectedHazardTypes: []diff.MigrationHazardType{ - diff.MigrationHazardTypeAcquiresShareRowExclusiveLock, diff.MigrationHazardTypeDeletesData, }, diff --git a/internal/migration_acceptance_tests/foreign_key_constraint_cases_test.go b/internal/migration_acceptance_tests/foreign_key_constraint_cases_test.go index 16b2aef..887849d 100644 --- a/internal/migration_acceptance_tests/foreign_key_constraint_cases_test.go +++ b/internal/migration_acceptance_tests/foreign_key_constraint_cases_test.go @@ -20,7 +20,7 @@ var foreignKeyConstraintCases = []acceptanceTestCase{ ON UPDATE SET NULL NOT DEFERRABLE ); - `, + `, }, newSchemaDDL: []string{ ` @@ -37,13 +37,13 @@ var foreignKeyConstraintCases = []acceptanceTestCase{ ON UPDATE SET NULL NOT DEFERRABLE ); - `, + `, }, expectEmptyPlan: true, }, { - name: "Add FK with most options", + name: "Add FK", oldSchemaDDL: []string{ ` CREATE TABLE "foo bar"( @@ -54,7 +54,7 @@ var foreignKeyConstraintCases = []acceptanceTestCase{ CREATE TABLE "foobar fk"( fk_id INT ); - `, + `, }, newSchemaDDL: []string{ ` @@ -70,12 +70,137 @@ var foreignKeyConstraintCases = []acceptanceTestCase{ ON UPDATE SET NULL NOT DEFERRABLE ); - `, + `, + }, + }, + { + name: "Add FK on partitioned", + oldSchemaDDL: []string{ + ` + CREATE TABLE "foo bar"( + id INT, + PRIMARY KEY (id) + ); + + CREATE TABLE "foobar fk"( + fk_id INT + ) PARTITION BY LIST (fk_id); + `, + }, + newSchemaDDL: []string{ + ` + CREATE TABLE "foo bar"( + id INT, + PRIMARY KEY (id) + ); + + CREATE TABLE "foobar fk"( + fk_id INT, + FOREIGN KEY (fk_id) REFERENCES "foo bar"(id) + ON DELETE SET NULL + ON UPDATE SET NULL + NOT DEFERRABLE + ) PARTITION BY LIST (fk_id); + `, }, expectedHazardTypes: []diff.MigrationHazardType{ diff.MigrationHazardTypeAcquiresShareRowExclusiveLock, }, }, + { + name: "Add FK on partition", + oldSchemaDDL: []string{ + ` + CREATE TABLE "foo bar"( + id INT, + PRIMARY KEY (id) + ); + + CREATE TABLE foobar_fk( + fk_id INT + ) PARTITION BY LIST (fk_id); + CREATE TABLE foobar_fk_1 PARTITION OF foobar_fk FOR VALUES IN (1); + `, + }, + newSchemaDDL: []string{ + ` + CREATE TABLE "foo bar"( + id INT, + PRIMARY KEY (id) + ); + + CREATE TABLE foobar_fk( + fk_id INT + ) PARTITION BY LIST (fk_id); + CREATE TABLE foobar_fk_1 PARTITION OF foobar_fk FOR VALUES IN (1); + ALTER TABLE foobar_fk_1 ADD CONSTRAINT some_fk FOREIGN KEY (fk_id) REFERENCES "foo bar"(id); + `, + }, + }, + { + name: "Add FK referencing partitioned", + oldSchemaDDL: []string{ + ` + CREATE TABLE foobar( + id INT, + PRIMARY KEY (id) + ) PARTITION BY LIST (id); + + CREATE TABLE "foobar fk"( + fk_id INT + ); + `, + }, + newSchemaDDL: []string{ + ` + CREATE TABLE foobar( + id INT, + PRIMARY KEY (id) + ) PARTITION BY LIST (id); + + CREATE TABLE "foobar fk"( + fk_id INT, + FOREIGN KEY (fk_id) REFERENCES foobar(id) + ON DELETE SET NULL + ON UPDATE SET NULL + NOT DEFERRABLE + ); + `, + }, + }, + { + name: "Add FK referencing partition", + oldSchemaDDL: []string{ + ` + CREATE TABLE foobar( + id INT, + PRIMARY KEY (id) + ) PARTITION BY LIST (id); + CREATE TABLE foobar_1 PARTITION OF foobar FOR VALUES IN (1); + + CREATE TABLE "foobar fk"( + fk_id INT + ); + `, + }, + newSchemaDDL: []string{ + ` + CREATE TABLE foobar( + id INT, + PRIMARY KEY (id) + ) PARTITION BY LIST (id); + CREATE TABLE foobar_1 PARTITION OF foobar FOR VALUES IN (1); + + CREATE TABLE "foobar fk"( + fk_id INT, + FOREIGN KEY (fk_id) REFERENCES foobar_1(id) + ON DELETE SET NULL + ON UPDATE SET NULL + NOT DEFERRABLE + ); + `, + }, + }, { name: "Add FK (only referenced table is new)", oldSchemaDDL: []string{ @@ -84,7 +209,7 @@ var foreignKeyConstraintCases = []acceptanceTestCase{ id INT, PRIMARY KEY (id) ); - `, + `, }, newSchemaDDL: []string{ ` @@ -97,10 +222,7 @@ var foreignKeyConstraintCases = []acceptanceTestCase{ fk_id INT, FOREIGN KEY (fk_id) REFERENCES foobar(id) ); - `, - }, - expectedHazardTypes: []diff.MigrationHazardType{ - diff.MigrationHazardTypeAcquiresShareRowExclusiveLock, + `, }, }, { @@ -110,7 +232,7 @@ var foreignKeyConstraintCases = []acceptanceTestCase{ CREATE TABLE "foobar fk"( fk_id INT ); - `, + `, }, newSchemaDDL: []string{ ` @@ -126,10 +248,7 @@ var foreignKeyConstraintCases = []acceptanceTestCase{ ON UPDATE SET NULL NOT DEFERRABLE ); - `, - }, - expectedHazardTypes: []diff.MigrationHazardType{ - diff.MigrationHazardTypeAcquiresShareRowExclusiveLock, + `, }, }, { @@ -148,7 +267,7 @@ var foreignKeyConstraintCases = []acceptanceTestCase{ ON UPDATE SET NULL NOT DEFERRABLE ); - `, + `, }, }, { @@ -163,7 +282,7 @@ var foreignKeyConstraintCases = []acceptanceTestCase{ CREATE TABLE "foobar fk"( fk_id INT ); - `, + `, }, newSchemaDDL: []string{ ` @@ -178,9 +297,8 @@ var foreignKeyConstraintCases = []acceptanceTestCase{ ALTER TABLE "foobar fk" ADD CONSTRAINT some_foobar_fk FOREIGN KEY (fk_id) REFERENCES foobar(id) NOT VALID; - `, + `, }, - expectedHazardTypes: []diff.MigrationHazardType{}, }, { name: "Add FK (partitioned table)", @@ -197,7 +315,7 @@ var foreignKeyConstraintCases = []acceptanceTestCase{ fk_foo VARCHAR(255), fk_id INT ); - `, + `, }, newSchemaDDL: []string{ ` @@ -213,10 +331,7 @@ var foreignKeyConstraintCases = []acceptanceTestCase{ fk_id INT, FOREIGN KEY (fk_foo, fk_id) REFERENCES foobar(foo, id) ); - `, - }, - expectedHazardTypes: []diff.MigrationHazardType{ - diff.MigrationHazardTypeAcquiresShareRowExclusiveLock, + `, }, }, { @@ -235,7 +350,7 @@ var foreignKeyConstraintCases = []acceptanceTestCase{ ON UPDATE SET NULL NOT DEFERRABLE ); - `, + `, }, newSchemaDDL: []string{ ` @@ -247,7 +362,7 @@ var foreignKeyConstraintCases = []acceptanceTestCase{ CREATE TABLE "foobar fk"( fk_id INT ); - `, + `, }, }, { @@ -266,7 +381,7 @@ var foreignKeyConstraintCases = []acceptanceTestCase{ ON UPDATE SET NULL NOT DEFERRABLE ); - `, + `, }, newSchemaDDL: []string{ ` @@ -283,10 +398,10 @@ var foreignKeyConstraintCases = []acceptanceTestCase{ ON UPDATE SET NULL NOT DEFERRABLE ); - `, + `, }, expectedHazardTypes: []diff.MigrationHazardType{ - diff.MigrationHazardTypeAcquiresShareRowExclusiveLock, + diff.MigrationHazardTypeDeletesData, }, }, @@ -307,7 +422,7 @@ var foreignKeyConstraintCases = []acceptanceTestCase{ fk_id INT, FOREIGN KEY (fk_foo, fk_id) REFERENCES schema_1.foobar(foo, id) ); - `, + `, }, newSchemaDDL: []string{ ` @@ -323,7 +438,37 @@ var foreignKeyConstraintCases = []acceptanceTestCase{ fk_foo VARCHAR(255), fk_id INT ); - `, + `, + }, + }, + { + name: "Drop FK with table (references partitioned table)", + oldSchemaDDL: []string{ + ` + CREATE TABLE foobar( + foo varchar(255), + id INT, + PRIMARY KEY (id) + ); + + CREATE TABLE "foobar partitioned"( + foo varchar(255), + id INT, + PRIMARY KEY (foo, id) + ) PARTITION BY LIST (foo); + CREATE TABLE foobar_1 PARTITION OF "foobar partitioned" FOR VALUES IN ('1'); + CREATE TABLE foobar_2 PARTITION OF "foobar partitioned" FOR VALUES IN ('2'); + + CREATE TABLE "foobar fk"( + fk_foo VARCHAR(255), + fk_id INT + ); + ALTER TABLE "foobar fk" ADD CONSTRAINT some_fk + FOREIGN KEY (fk_foo, fk_id) REFERENCES "foobar partitioned"(foo, id); + `, + }, + expectedHazardTypes: []diff.MigrationHazardType{ + diff.MigrationHazardTypeDeletesData, }, }, { @@ -341,7 +486,7 @@ var foreignKeyConstraintCases = []acceptanceTestCase{ ALTER TABLE "foobar fk" ADD CONSTRAINT some_fk FOREIGN KEY (fk_id) REFERENCES foobar(id) NOT VALID; - `, + `, }, newSchemaDDL: []string{ ` @@ -355,7 +500,7 @@ var foreignKeyConstraintCases = []acceptanceTestCase{ ); ALTER TABLE "foobar fk" ADD CONSTRAINT some_fk FOREIGN KEY (fk_id) REFERENCES foobar(id); - `, + `, }, expectedPlanDDL: []string{ "ALTER TABLE \"public\".\"foobar fk\" VALIDATE CONSTRAINT \"some_fk\"", @@ -375,7 +520,7 @@ var foreignKeyConstraintCases = []acceptanceTestCase{ ); ALTER TABLE "foobar fk" ADD CONSTRAINT some_fk FOREIGN KEY (fk_id) REFERENCES foobar(id); - `, + `, }, newSchemaDDL: []string{ ` @@ -390,7 +535,7 @@ var foreignKeyConstraintCases = []acceptanceTestCase{ ALTER TABLE "foobar fk" ADD CONSTRAINT some_fk FOREIGN KEY (fk_id) REFERENCES foobar(id) NOT VALID; - `, + `, }, }, { @@ -411,7 +556,7 @@ var foreignKeyConstraintCases = []acceptanceTestCase{ ALTER TABLE "foobar fk" ADD CONSTRAINT some_fk FOREIGN KEY (fk_id) REFERENCES foobar(id) NOT VALID; - `, + `, }, newSchemaDDL: []string{ ` @@ -428,10 +573,7 @@ var foreignKeyConstraintCases = []acceptanceTestCase{ ); ALTER TABLE "foobar fk" ADD CONSTRAINT some_fk FOREIGN KEY (fk_foo) REFERENCES foobar(foo); - `, - }, - expectedHazardTypes: []diff.MigrationHazardType{ - diff.MigrationHazardTypeAcquiresShareRowExclusiveLock, + `, }, }, { @@ -447,7 +589,7 @@ var foreignKeyConstraintCases = []acceptanceTestCase{ fk_id INT, FOREIGN KEY (fk_id) REFERENCES foobar(id) ); - `, + `, }, newSchemaDDL: []string{ ` @@ -461,10 +603,7 @@ var foreignKeyConstraintCases = []acceptanceTestCase{ FOREIGN KEY (fk_id) REFERENCES foobar(id) ON UPDATE CASCADE ); - `, - }, - expectedHazardTypes: []diff.MigrationHazardType{ - diff.MigrationHazardTypeAcquiresShareRowExclusiveLock, + `, }, }, { @@ -480,7 +619,7 @@ var foreignKeyConstraintCases = []acceptanceTestCase{ fk_id INT, FOREIGN KEY (fk_id) REFERENCES foobar(id) ); - `, + `, }, newSchemaDDL: []string{ ` @@ -494,10 +633,7 @@ var foreignKeyConstraintCases = []acceptanceTestCase{ FOREIGN KEY (fk_id) REFERENCES foobar(id) ON UPDATE CASCADE ); - `, - }, - expectedHazardTypes: []diff.MigrationHazardType{ - diff.MigrationHazardTypeAcquiresShareRowExclusiveLock, + `, }, }, { @@ -513,7 +649,7 @@ var foreignKeyConstraintCases = []acceptanceTestCase{ fk_id INT, FOREIGN KEY (fk_id) REFERENCES foobar(id) ); - `, + `, }, newSchemaDDL: []string{ ` @@ -526,7 +662,7 @@ var foreignKeyConstraintCases = []acceptanceTestCase{ fk_id INT, FOREIGN KEY (fk_id) REFERENCES foobar(id) ); - `, + `, }, expectedHazardTypes: []diff.MigrationHazardType{ diff.MigrationHazardTypeAcquiresAccessExclusiveLock, @@ -546,7 +682,7 @@ var foreignKeyConstraintCases = []acceptanceTestCase{ fk_id BIGINT, FOREIGN KEY (fk_id) REFERENCES foobar(id) ); - `, + `, }, newSchemaDDL: []string{ ` @@ -559,7 +695,7 @@ var foreignKeyConstraintCases = []acceptanceTestCase{ fk_id TIMESTAMP, FOREIGN KEY (fk_id) REFERENCES foobar(id) ); - `, + `, }, expectedPlanErrorContains: errValidatingPlan.Error(), @@ -585,7 +721,7 @@ var foreignKeyConstraintCases = []acceptanceTestCase{ ) PARTITION BY LIST (foo); CREATE TABLE foobar_1 PARTITION OF "foobar fk partitioned" FOR VALUES IN ('1'); CREATE TABLE foobar_2 PARTITION OF "foobar fk partitioned" FOR VALUES IN ('2'); - `, + `, }, newSchemaDDL: []string{ ` @@ -606,12 +742,54 @@ var foreignKeyConstraintCases = []acceptanceTestCase{ CREATE TABLE foobar_2 PARTITION OF "foobar fk partitioned" FOR VALUES IN ('2'); ALTER TABLE "foobar fk partitioned" ADD CONSTRAINT some_fk FOREIGN KEY (fk_id) REFERENCES foobar(id); - `, + `, }, expectedHazardTypes: []diff.MigrationHazardType{ diff.MigrationHazardTypeAcquiresShareRowExclusiveLock, }, }, + { + name: "Switch FK owning table (from partitioned table)", + oldSchemaDDL: []string{ + ` + CREATE TABLE foobar( + id INT, + PRIMARY KEY (id) + ); + + CREATE TABLE "foobar fk"( + fk_id INT + ); + CREATE TABLE "foobar fk partitioned"( + foo varchar(255), + fk_id INT + ) PARTITION BY LIST (foo); + ALTER TABLE "foobar fk partitioned" ADD CONSTRAINT some_fk + FOREIGN KEY (fk_id) REFERENCES foobar(id); + `, + }, + newSchemaDDL: []string{ + ` + CREATE TABLE foobar( + id INT, + PRIMARY KEY (id) + ); + + CREATE TABLE "foobar fk"( + fk_id INT + ); + + CREATE TABLE "foobar fk partitioned"( + foo varchar(255), + fk_id INT + ) PARTITION BY LIST (foo); + CREATE TABLE foobar_1 PARTITION OF "foobar fk partitioned" FOR VALUES IN ('1'); + CREATE TABLE foobar_2 PARTITION OF "foobar fk partitioned" FOR VALUES IN ('2'); + ALTER TABLE "foobar fk" ADD CONSTRAINT some_fk + FOREIGN KEY (fk_id) REFERENCES foobar(id); + `, + }, + }, { name: "Switch FK owning table (analog tables in different schemas stay same)", oldSchemaDDL: []string{ @@ -644,7 +822,7 @@ var foreignKeyConstraintCases = []acceptanceTestCase{ ); ALTER TABLE schema_1."foobar fk" ADD CONSTRAINT some_fk FOREIGN KEY (fk_id) REFERENCES schema_1.foobar(id); - `, + `, }, newSchemaDDL: []string{ ` @@ -681,8 +859,9 @@ var foreignKeyConstraintCases = []acceptanceTestCase{ FOREIGN KEY (fk_id, fk_val) REFERENCES schema_1.foobar(id, val); `}, expectedHazardTypes: []diff.MigrationHazardType{ - diff.MigrationHazardTypeAcquiresShareRowExclusiveLock, + diff.MigrationHazardTypeAcquiresAccessExclusiveLock, + diff.MigrationHazardTypeAcquiresShareRowExclusiveLock, diff.MigrationHazardTypeIndexBuild, diff.MigrationHazardTypeIndexDropped, }, @@ -711,7 +890,7 @@ var foreignKeyConstraintCases = []acceptanceTestCase{ ); ALTER TABLE "foobar fk" ADD CONSTRAINT some_fk FOREIGN KEY (fk_foo, fk_id) REFERENCES foobar(foo, id) NOT VALID ; - `, + `, }, newSchemaDDL: []string{ ` @@ -735,12 +914,11 @@ var foreignKeyConstraintCases = []acceptanceTestCase{ ); ALTER TABLE "foobar fk" ADD CONSTRAINT some_fk FOREIGN KEY (fk_foo, fk_id) REFERENCES "foobar partitioned"(foo, id); - `, + `, }, expectedHazardTypes: []diff.MigrationHazardType{ diff.MigrationHazardTypeIndexDropped, diff.MigrationHazardTypeIndexBuild, - diff.MigrationHazardTypeAcquiresShareRowExclusiveLock, }, }, } diff --git a/internal/migration_acceptance_tests/schema_cases_test.go b/internal/migration_acceptance_tests/schema_cases_test.go index ed59b53..9a85cf3 100644 --- a/internal/migration_acceptance_tests/schema_cases_test.go +++ b/internal/migration_acceptance_tests/schema_cases_test.go @@ -337,7 +337,6 @@ var schemaAcceptanceTests = []acceptanceTestCase{ `, }, expectedHazardTypes: []diff.MigrationHazardType{ - diff.MigrationHazardTypeAcquiresShareRowExclusiveLock, diff.MigrationHazardTypeAuthzUpdate, diff.MigrationHazardTypeDeletesData, diff.MigrationHazardTypeHasUntrackableDependencies, @@ -422,7 +421,6 @@ var schemaAcceptanceTests = []acceptanceTestCase{ `, }, expectedHazardTypes: []diff.MigrationHazardType{ - diff.MigrationHazardTypeAcquiresShareRowExclusiveLock, diff.MigrationHazardTypeDeletesData, }, }, diff --git a/internal/migration_acceptance_tests/table_cases_test.go b/internal/migration_acceptance_tests/table_cases_test.go index b97b064..8dc89bf 100644 --- a/internal/migration_acceptance_tests/table_cases_test.go +++ b/internal/migration_acceptance_tests/table_cases_test.go @@ -462,7 +462,6 @@ var tableAcceptanceTestCases = []acceptanceTestCase{ }, expectedHazardTypes: []diff.MigrationHazardType{ diff.MigrationHazardTypeAcquiresAccessExclusiveLock, - diff.MigrationHazardTypeAcquiresShareRowExclusiveLock, diff.MigrationHazardTypeAuthzUpdate, diff.MigrationHazardTypeImpactsDatabasePerformance, diff.MigrationHazardTypeDeletesData, @@ -531,7 +530,6 @@ var tableAcceptanceTestCases = []acceptanceTestCase{ }, expectedHazardTypes: []diff.MigrationHazardType{ diff.MigrationHazardTypeAcquiresAccessExclusiveLock, - diff.MigrationHazardTypeAcquiresShareRowExclusiveLock, diff.MigrationHazardTypeAuthzUpdate, diff.MigrationHazardTypeDeletesData, diff.MigrationHazardTypeIndexDropped, diff --git a/pkg/diff/sql_generator.go b/pkg/diff/sql_generator.go index eeda9ad..8fa0967 100644 --- a/pkg/diff/sql_generator.go +++ b/pkg/diff/sql_generator.go @@ -1,6 +1,7 @@ package diff import ( + "errors" "fmt" "regexp" "sort" @@ -245,37 +246,10 @@ func buildSchemaDiff(old, new schema.Schema) (schemaDiff, bool, error) { if err != nil { return schemaDiff{}, false, fmt.Errorf("diffing indexes: %w", err) } - foreignKeyConstraintDiffs, err := diffLists(old.ForeignKeyConstraints, new.ForeignKeyConstraints, func(old, new schema.ForeignKeyConstraint, _, _ int) (foreignKeyConstraintDiff, bool, error) { - if _, isOnNewTable := addedTablesByName[new.OwningTable.GetName()]; isOnNewTable { - - // If the owning table is new, then it must be re-created (this occurs if the base table has been - // re-created). In other words, a foreign key constraint must be re-created if the owning table or referenced - // table is re-created - return foreignKeyConstraintDiff{}, true, nil - } else if _, isReferencingNewTable := addedTablesByName[new.ForeignTable.GetName()]; isReferencingNewTable { - // Same as above, but for the referenced table - return foreignKeyConstraintDiff{}, true, nil - } - - // Set the new clone to be equal to the old for all fields that can actually be altered - newClone := new - if !old.IsValid && new.IsValid { - // We only support alter from NOT VALID to VALID and no other alterations. - // Instead of checking that each individual property is equal (that's a lot of parsing), we will just - // assert that the constraint definitions are equal if we append "NOT VALID" to the new constraint def - newClone.IsValid = old.IsValid - newClone.ConstraintDef = fmt.Sprintf("%s NOT VALID", newClone.ConstraintDef) - } - if !cmp.Equal(old, newClone) { - return foreignKeyConstraintDiff{}, true, nil - } - return foreignKeyConstraintDiff{ - oldAndNew[schema.ForeignKeyConstraint]{ - old: old, - new: new, - }, - }, false, nil + fsg := newForeignKeyConstraintSQLVertexGenerator(oldAndNew[schema.Schema]{old: old, new: new}, tableDiffs) + foreignKeyConstraintDiffs, err := diffLists(old.ForeignKeyConstraints, new.ForeignKeyConstraints, func(old, new schema.ForeignKeyConstraint, _, _ int) (foreignKeyConstraintDiff, bool, error) { + return buildForeignKeyConstraintDiff(fsg, addedTablesByName, old, new) }) if err != nil { return schemaDiff{}, false, fmt.Errorf("diffing foreign key constraints: %w", err) @@ -526,15 +500,6 @@ func (schemaSQLGenerator) Alter(diff schemaDiff) ([]Statement, error) { deletedTablesByName := buildSchemaObjByNameMap(diff.tableDiffs.deletes) addedTablesByName := buildSchemaObjByNameMap(diff.tableDiffs.adds) - indexesOldSchemaByTableName := make(map[string][]schema.Index) - for _, idx := range diff.old.Indexes { - indexesOldSchemaByTableName[idx.OwningTable.GetName()] = append(indexesOldSchemaByTableName[idx.OwningTable.GetName()], idx) - } - indexesInNewSchemaByTableName := make(map[string][]schema.Index) - for _, idx := range diff.new.Indexes { - indexesInNewSchemaByTableName[idx.OwningTable.GetName()] = append(indexesInNewSchemaByTableName[idx.OwningTable.GetName()], idx) - } - namedSchemaStatements, err := diff.namedSchemaDiffs.resolveToSQLGroupedByEffect(&namedSchemaSQLGenerator{}) if err != nil { return nil, fmt.Errorf("resolving named schema sql statements: %w", err) @@ -559,7 +524,7 @@ func (schemaSQLGenerator) Alter(diff schemaDiff) ([]Statement, error) { return nil, fmt.Errorf("resolving enum sql graphs: %w", err) } - attachPartitionSQLVertexGenerator := newAttachPartitionSQLVertexGenerator(indexesInNewSchemaByTableName, addedTablesByName) + attachPartitionSQLVertexGenerator := newAttachPartitionSQLVertexGenerator(diff.new.Indexes, diff.tableDiffs.adds) attachPartitionGraphs, err := diff.tableDiffs.resolveToSQLGraph(attachPartitionSQLVertexGenerator) if err != nil { return nil, fmt.Errorf("resolving attach partition sql graphs: %w", err) @@ -584,12 +549,7 @@ func (schemaSQLGenerator) Alter(diff schemaDiff) ([]Statement, error) { return nil, fmt.Errorf("resolving index sql graphs: %w", err) } - fkConsGraphs, err := diff.foreignKeyConstraintDiffs.resolveToSQLGraph(&foreignKeyConstraintSQLVertexGenerator{ - deletedTablesByName: deletedTablesByName, - addedTablesByName: addedTablesByName, - indexInOldSchemaByTableName: indexesInNewSchemaByTableName, - indexesInNewSchemaByTableName: indexesInNewSchemaByTableName, - }) + fkConsGraphs, err := diff.foreignKeyConstraintDiffs.resolveToSQLGraph(newForeignKeyConstraintSQLVertexGenerator(diff.oldAndNew, diff.tableDiffs)) if err != nil { return nil, fmt.Errorf("resolving foreign key constraint sql graphs: %w", err) } @@ -667,6 +627,62 @@ func (schemaSQLGenerator) Alter(diff schemaDiff) ([]Statement, error) { return statements, nil } +func buildIndexesByTableNameMap(indexes []schema.Index) map[string][]schema.Index { + var output = make(map[string][]schema.Index) + for _, idx := range indexes { + output[idx.OwningTable.GetName()] = append(output[idx.OwningTable.GetName()], idx) + } + return output +} + +// buildChildrenByPartitionedIndexNameMap builds a map of indexes by their parent index name. This map will include +// all descendents, not just direct descendents. For example, if foobar idx has 5 children and each of those children +// has 5 children, the slice for foobar index wil contain 30 indexes (the 5 direct children and the 25 "grandchildren") +func buildChildrenByPartitionedIndexNameMap(indexes []schema.Index) map[string][]schema.Index { + // Build a map of children by their direct parent index + var childrenByDirectParentIdxName = make(map[string][]schema.Index) + for _, idx := range indexes { + parentIdxName := "" + if idx.ParentIdx != nil { + parentIdxName = idx.ParentIdx.GetName() + } + childrenByDirectParentIdxName[parentIdxName] = append(childrenByDirectParentIdxName[parentIdxName], idx) + } + + // Use this map to build a map of root index by child index name + var rootsByChildIdxName = make(map[string]schema.Index) + // We need to calculate this level by level. We will start with the roots. + var currentNodes = childrenByDirectParentIdxName[""] + for len(currentNodes) > 0 { + var idx = currentNodes[0] + currentNodes = currentNodes[1:] + + rootIdx := idx + if idx.ParentIdx != nil { + rootIdx = rootsByChildIdxName[idx.ParentIdx.GetName()] + } + rootsByChildIdxName[idx.GetName()] = rootIdx + + // We can now calculate the next level of children for this node + for _, child := range childrenByDirectParentIdxName[idx.GetName()] { + currentNodes = append(currentNodes, child) + } + } + + // Build a map of all children by the root index name using the result of the previous step + var allChildrenByRootIndexName = make(map[string][]schema.Index) + for _, idx := range indexes { + if idx.ParentIdx == nil { + // Skip nodes without parents + continue + } + rootIdx := rootsByChildIdxName[idx.GetName()] + allChildrenByRootIndexName[rootIdx.GetName()] = append(allChildrenByRootIndexName[rootIdx.GetName()], idx) + } + + return allChildrenByRootIndexName +} + func buildSchemaObjByNameMap[S schema.Object](s []S) map[string]S { return buildMap(s, func(s S) string { return s.GetName() @@ -1901,10 +1917,10 @@ type attachPartitionSQLVertexGenerator struct { isPartitionAttachedAfterIdxBuildsByTableName map[string]bool } -func newAttachPartitionSQLVertexGenerator(indexesInNewSchemaByTableName map[string][]schema.Index, addedTablesByName map[string]schema.Table) *attachPartitionSQLVertexGenerator { +func newAttachPartitionSQLVertexGenerator(newSchemaIndexes []schema.Index, addedTables []schema.Table) *attachPartitionSQLVertexGenerator { return &attachPartitionSQLVertexGenerator{ - indexesInNewSchemaByTableName: indexesInNewSchemaByTableName, - addedTablesByName: addedTablesByName, + indexesInNewSchemaByTableName: buildIndexesByTableNameMap(newSchemaIndexes), + addedTablesByName: buildSchemaObjByNameMap(addedTables), isPartitionAttachedAfterIdxBuildsByTableName: make(map[string]bool), } @@ -1970,23 +1986,77 @@ func (a *attachPartitionSQLVertexGenerator) GetDeleteDependencies(_ schema.Table return nil, nil } +func buildForeignKeyConstraintDiff(fsg *foreignKeyConstraintSQLVertexGenerator, addedTablesByName map[string]schema.Table, old, new schema.ForeignKeyConstraint) (foreignKeyConstraintDiff, bool, error) { + if _, isOnNewTable := addedTablesByName[new.OwningTable.GetName()]; isOnNewTable { + // If the owning table is new, then it must be re-created (this occurs if the base table has been + // re-created). In other words, a foreign key constraint must be re-created if the owning table or referenced + // table is re-created + return foreignKeyConstraintDiff{}, true, nil + } + if _, isReferencingNewTable := addedTablesByName[new.ForeignTable.GetName()]; isReferencingNewTable { + // Same as above, but for the referenced table + return foreignKeyConstraintDiff{}, true, nil + } + fkDiff := foreignKeyConstraintDiff{ + oldAndNew[schema.ForeignKeyConstraint]{ + old: old, + new: new, + }, + } + if _, err := fsg.Alter(fkDiff); err != nil { + if errors.Is(err, ErrNotImplemented) { + // The foreign key must be re-created if the diff cannot be reconciled via alter statements + return foreignKeyConstraintDiff{}, true, nil + } + return foreignKeyConstraintDiff{}, false, fmt.Errorf("generating foreign key alter statements: %w", err) + } + + return fkDiff, false, nil +} + type foreignKeyConstraintSQLVertexGenerator struct { - // deletedTablesByName is a map of table name to tables (and partitions) that are deleted - // This is used to identify if the owning table is being dropped (meaning - // the foreign key can be implicitly dropped) - deletedTablesByName map[string]schema.Table + // newSchemaTablesByName is a map of table name to tables in the new schema. + newSchemaTablesByName map[string]schema.Table // addedTablesByName is a map of table name to the added tables // This is used to identify if hazards are necessary addedTablesByName map[string]schema.Table // indexInOldSchemaByTableName is a map of index name to the index in the old schema // This is used to force the foreign key constraint to be dropped before the index it depends on is dropped indexInOldSchemaByTableName map[string][]schema.Index + // childrenInOldSchemaByPartitionedIndexName gives all child indexes (across all levels) for a given + // partitioned index in the old schema. + childrenInOldSchemaByPartitionedIndexName map[string][]schema.Index // indexesInNewSchemaByTableName is a map of index name to the index // Same as above but for adds and after indexesInNewSchemaByTableName map[string][]schema.Index + // childrenInNewSchemaByPartitionedIndexName gives all child indexes (across all levels) for a given + // partitioned index in the new schema. + childrenInNewSchemaByPartitionedIndexName map[string][]schema.Index +} + +func newForeignKeyConstraintSQLVertexGenerator(oldAndNewSchema oldAndNew[schema.Schema], tableDiffs listDiff[schema.Table, tableDiff]) *foreignKeyConstraintSQLVertexGenerator { + return &foreignKeyConstraintSQLVertexGenerator{ + newSchemaTablesByName: buildSchemaObjByNameMap(oldAndNewSchema.new.Tables), + addedTablesByName: buildSchemaObjByNameMap(tableDiffs.adds), + indexInOldSchemaByTableName: buildIndexesByTableNameMap(oldAndNewSchema.old.Indexes), + childrenInOldSchemaByPartitionedIndexName: buildChildrenByPartitionedIndexNameMap(oldAndNewSchema.old.Indexes), + indexesInNewSchemaByTableName: buildIndexesByTableNameMap(oldAndNewSchema.new.Indexes), + childrenInNewSchemaByPartitionedIndexName: buildChildrenByPartitionedIndexNameMap(oldAndNewSchema.new.Indexes), + } } func (f *foreignKeyConstraintSQLVertexGenerator) Add(con schema.ForeignKeyConstraint) ([]Statement, error) { + if con.IsValid { + table, ok := f.newSchemaTablesByName[con.OwningTable.GetName()] + if !ok { + return nil, fmt.Errorf("could not find table with name %s", con.OwningTable.GetName()) + } + if !table.IsPartitioned() { + // Postgres does not support adding invalid foreign keys to partitioned tables. + return f.addAsInvalidThenValidateStatements(con), nil + } + } + var hazards []MigrationHazard _, isOnNewTable := f.addedTablesByName[con.OwningTable.GetName()] _, isReferencedTableNew := f.addedTablesByName[con.ForeignTable.GetName()] @@ -2005,6 +2075,19 @@ func (f *foreignKeyConstraintSQLVertexGenerator) Add(con schema.ForeignKeyConstr }}, nil } +func (f *foreignKeyConstraintSQLVertexGenerator) addAsInvalidThenValidateStatements(con schema.ForeignKeyConstraint) []Statement { + // If adding a valid constraint, we will first add the constraint as not valid then validate it in order to + // circumvent requiring a SHARE_ROW_EXCLUSIVE lock on the tables. + return []Statement{ + { + DDL: fmt.Sprintf("%s %s NOT VALID", addConstraintPrefix(con.OwningTable, con.EscapedName), con.ConstraintDef), + Timeout: statementTimeoutDefault, + LockTimeout: lockTimeoutDefault, + }, + validateConstraintStatement(con.OwningTable, con.EscapedName), + } +} + func (f *foreignKeyConstraintSQLVertexGenerator) Delete(con schema.ForeignKeyConstraint) ([]Statement, error) { // Always generate a drop statement even if the owning table is being deleted. This simplifies the logic a bit because // if the owning table has a circular FK dependency with another table being dropped, we will need to explicitly drop @@ -2052,6 +2135,10 @@ func (f *foreignKeyConstraintSQLVertexGenerator) GetAddAlterDependencies(con, _ // because of partitioned indexes, which are only valid when all child indexes have been built for _, i := range f.indexesInNewSchemaByTableName[con.ForeignTable.GetName()] { deps = append(deps, mustRun(f.GetSQLVertexId(con), diffTypeAddAlter).after(buildIndexVertexId(i.GetSchemaQualifiedName()), diffTypeAddAlter)) + // Build a dependency on any child index if the index is partitioned + for _, c := range f.childrenInNewSchemaByPartitionedIndexName[i.GetName()] { + deps = append(deps, mustRun(f.GetSQLVertexId(con), diffTypeAddAlter).after(buildIndexVertexId(c.GetSchemaQualifiedName()), diffTypeAddAlter)) + } } return deps, nil @@ -2067,6 +2154,10 @@ func (f *foreignKeyConstraintSQLVertexGenerator) GetDeleteDependencies(con schem // because of partitioned indexes, which are only valid when all child indexes have been built for _, i := range f.indexInOldSchemaByTableName[con.ForeignTable.GetName()] { deps = append(deps, mustRun(f.GetSQLVertexId(con), diffTypeDelete).before(buildIndexVertexId(i.GetSchemaQualifiedName()), diffTypeDelete)) + // Build a dependency on any child index if the index is partitioned + for _, c := range f.childrenInOldSchemaByPartitionedIndexName[i.GetName()] { + deps = append(deps, mustRun(f.GetSQLVertexId(con), diffTypeDelete).before(buildIndexVertexId(c.GetSchemaQualifiedName()), diffTypeDelete)) + } } return deps, nil } @@ -2220,7 +2311,7 @@ func (s sequenceOwnershipSQLVertexGenerator) Add(seq schema.Sequence) ([]Stateme // If a new sequence has no owner, we don't need to alter it. The default is no owner return nil, nil } - return []Statement{s.buildAlterOwnershipStmt(seq, nil)}, nil + return []Statement{s.buildAlterOwnershipStmt(seq)}, nil } func (s sequenceOwnershipSQLVertexGenerator) Delete(_ schema.Sequence) ([]Statement, error) { @@ -2231,10 +2322,10 @@ func (s sequenceOwnershipSQLVertexGenerator) Alter(diff sequenceDiff) ([]Stateme if cmp.Equal(diff.new.Owner, diff.old.Owner) { return nil, nil } - return []Statement{s.buildAlterOwnershipStmt(diff.new, &diff.old)}, nil + return []Statement{s.buildAlterOwnershipStmt(diff.new)}, nil } -func (s sequenceOwnershipSQLVertexGenerator) buildAlterOwnershipStmt(new schema.Sequence, old *schema.Sequence) Statement { +func (s sequenceOwnershipSQLVertexGenerator) buildAlterOwnershipStmt(new schema.Sequence) Statement { newOwner := "NONE" if new.Owner != nil { newOwner = schema.FQEscapedColumnName(new.Owner.TableName, new.Owner.ColumnName)