Add foreign key support (#53)

* Add foreign key support * Add locking hazard for new check constraints * Small changes * SQL generation formatting * Apply suggestions from code review Co-authored-by: alexaub-stripe <[email protected]> --------- Co-authored-by: alexaub-stripe <[email protected]>
stripe · Aug 10, 2023 · 702737f · 702737f
1 parent 7741e09
commit 702737f
Show file tree

Hide file tree

Showing 17 changed files with 1,959 additions and 546 deletions.
diff --git a/README.md b/README.md
@@ -130,7 +130,6 @@ Postgres v13 and below are not supported. Use at your own risk.
 Note, the library only currently supports diffing the *public* schema. Support for diffing other schemas is on the roadmap
 
 *Unsupported*:
-- (On roadmap) Foreign key constraints
 - (On roadmap) Diffing schemas other than "public"
 - (On roadmap) Unique constraints (unique indexes are supported but not unique constraints)
 - (On roadmap) Adding and remove partitions from an existing partitioned table

diff --git a/internal/migration_acceptance_tests/acceptance_test.go b/internal/migration_acceptance_tests/acceptance_test.go
@@ -17,6 +17,10 @@ import (
 	"github.com/stripe/pg-schema-diff/pkg/tempdb"
 )
 
+var (
+	errValidatingPlan = fmt.Errorf("validating migration plan")
+)
+
 type (
 	expectations struct {
 		planErrorIs       error

diff --git a/internal/migration_acceptance_tests/check_constraint_cases_test.go b/internal/migration_acceptance_tests/check_constraint_cases_test.go
@@ -52,6 +52,9 @@ var checkConstraintCases = []acceptanceTestCase{
 			);
 			`,
 		},
+		expectedHazardTypes: []diff.MigrationHazardType{
+			diff.MigrationHazardTypeAcquiresAccessExclusiveLock,
+		},
 	},
 	{
 		name: "Add check constraint with UDF dependency should error",
@@ -106,6 +109,9 @@ var checkConstraintCases = []acceptanceTestCase{
 			);
 			`,
 		},
+		expectedHazardTypes: []diff.MigrationHazardType{
+			diff.MigrationHazardTypeAcquiresAccessExclusiveLock,
+		},
 	},
 	{
 		name: "Add multiple check constraints",
@@ -128,6 +134,9 @@ var checkConstraintCases = []acceptanceTestCase{
 			);
 			`,
 		},
+		expectedHazardTypes: []diff.MigrationHazardType{
+			diff.MigrationHazardTypeAcquiresAccessExclusiveLock,
+		},
 	},
 	{
 		name: "Add check constraints to new column",
@@ -148,6 +157,9 @@ var checkConstraintCases = []acceptanceTestCase{
 			);
 			`,
 		},
+		expectedHazardTypes: []diff.MigrationHazardType{
+			diff.MigrationHazardTypeAcquiresAccessExclusiveLock,
+		},
 	},
 	{
 		name: "Add check constraint with quoted identifiers",
@@ -170,6 +182,9 @@ var checkConstraintCases = []acceptanceTestCase{
 			ALTER TABLE foobar ADD CONSTRAINT "BAR_CHECK" CHECK ( "Bar" < "ID" );
 			`,
 		},
+		expectedHazardTypes: []diff.MigrationHazardType{
+			diff.MigrationHazardTypeAcquiresAccessExclusiveLock,
+		},
 	},
 	{
 		name: "Add no inherit check constraint",
@@ -192,6 +207,9 @@ var checkConstraintCases = []acceptanceTestCase{
 			ALTER TABLE foobar ADD CONSTRAINT bar_check CHECK ( bar > id ) NO INHERIT;
 			`,
 		},
+		expectedHazardTypes: []diff.MigrationHazardType{
+			diff.MigrationHazardTypeAcquiresAccessExclusiveLock,
+		},
 	},
 	{
 		name: "Add No-Inherit, Not-Valid check constraint",
@@ -381,7 +399,7 @@ var checkConstraintCases = []acceptanceTestCase{
 		},
 	},
 	{
-		name: "Alter a No-Inherit check constraint to be Inheritable",
+		name: "Alter a no-Inherit check constraint to be Inheritable",
 		oldSchemaDDL: []string{
 			`
 			CREATE TABLE foobar(
@@ -402,9 +420,12 @@ var checkConstraintCases = []acceptanceTestCase{
 			ALTER TABLE foobar ADD CONSTRAINT bar_check CHECK ( bar > id );
 			`,
 		},
+		expectedHazardTypes: []diff.MigrationHazardType{
+			diff.MigrationHazardTypeAcquiresAccessExclusiveLock,
+		},
 	},
 	{
-		name: "Alter an Inheritable check constraint to be No-Inherit",
+		name: "Alter an Inheritable check constraint to be no-inherit",
 		oldSchemaDDL: []string{
 			`
 			CREATE TABLE foobar(
@@ -425,6 +446,9 @@ var checkConstraintCases = []acceptanceTestCase{
 			ALTER TABLE foobar ADD CONSTRAINT bar_check CHECK ( bar > id ) NO INHERIT;
 			`,
 		},
+		expectedHazardTypes: []diff.MigrationHazardType{
+			diff.MigrationHazardTypeAcquiresAccessExclusiveLock,
+		},
 	},
 	{
 		name: "Alter a check constraint expression",
@@ -446,6 +470,9 @@ var checkConstraintCases = []acceptanceTestCase{
 			);
 			`,
 		},
+		expectedHazardTypes: []diff.MigrationHazardType{
+			diff.MigrationHazardTypeAcquiresAccessExclusiveLock,
+		},
 	},
 	{
 		name: "Alter check constraint with UDF dependency should error",

diff --git a/internal/migration_acceptance_tests/column_cases_test.go b/internal/migration_acceptance_tests/column_cases_test.go
@@ -543,8 +543,12 @@ var columnAcceptanceTestCases = []acceptanceTestCase{
 			diff.MigrationHazardTypeAcquiresAccessExclusiveLock,
 			diff.MigrationHazardTypeImpactsDatabasePerformance,
 		},
-		vanillaExpectations:     expectations{planErrorContains: "validating migration plan"},
-		dataPackingExpectations: expectations{planErrorContains: "validating migration plan"},
+		vanillaExpectations: expectations{
+			planErrorContains: errValidatingPlan.Error(),
+		},
+		dataPackingExpectations: expectations{
+			planErrorContains: errValidatingPlan.Error(),
+		},
 	},
 	{
 		name: "Change to not null",