From e165745e7659023b1020a5cdd0d9643baa2da0f1 Mon Sep 17 00:00:00 2001 From: bplunkett-stripe Date: Wed, 25 Oct 2023 01:52:34 -0700 Subject: [PATCH 1/2] Online check constraint addition --- README.md | 3 ++ .../check_constraint_cases_test.go | 27 ------------ pkg/diff/schema_migration_plan_test.go | 42 +++++++++++++++++++ pkg/diff/sql_generator.go | 25 ++++++++--- 4 files changed, 65 insertions(+), 32 deletions(-) diff --git a/README.md b/README.md index 42a9145..71c7838 100644 --- a/README.md +++ b/README.md @@ -43,10 +43,13 @@ $ 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 addition: Check constraints are added as `INVALID` before being validated, eliminating the need + for a long access-exclusive lock on the table * Prioritized index builds: Building new indexes is always prioritized over deleting old indexes * A comprehensive set of features to ensure the safety of planned migrations: * Operators warned of dangerous operations. * Migration plans are validated first against a temporary database exactly as they would be performed against the real database. +* Strong support of partitions # Install ## CLI ```bash diff --git a/internal/migration_acceptance_tests/check_constraint_cases_test.go b/internal/migration_acceptance_tests/check_constraint_cases_test.go index 6c6b396..0f758d5 100644 --- a/internal/migration_acceptance_tests/check_constraint_cases_test.go +++ b/internal/migration_acceptance_tests/check_constraint_cases_test.go @@ -52,9 +52,6 @@ var checkConstraintCases = []acceptanceTestCase{ ); `, }, - expectedHazardTypes: []diff.MigrationHazardType{ - diff.MigrationHazardTypeAcquiresAccessExclusiveLock, - }, }, { name: "Add check constraint with UDF dependency should error", @@ -109,9 +106,6 @@ var checkConstraintCases = []acceptanceTestCase{ ); `, }, - expectedHazardTypes: []diff.MigrationHazardType{ - diff.MigrationHazardTypeAcquiresAccessExclusiveLock, - }, }, { name: "Add multiple check constraints", @@ -134,9 +128,6 @@ var checkConstraintCases = []acceptanceTestCase{ ); `, }, - expectedHazardTypes: []diff.MigrationHazardType{ - diff.MigrationHazardTypeAcquiresAccessExclusiveLock, - }, }, { name: "Add check constraints to new column", @@ -157,9 +148,6 @@ var checkConstraintCases = []acceptanceTestCase{ ); `, }, - expectedHazardTypes: []diff.MigrationHazardType{ - diff.MigrationHazardTypeAcquiresAccessExclusiveLock, - }, }, { name: "Add check constraint with quoted identifiers", @@ -182,9 +170,6 @@ var checkConstraintCases = []acceptanceTestCase{ ALTER TABLE foobar ADD CONSTRAINT "BAR_CHECK" CHECK ( "Bar" < "ID" ); `, }, - expectedHazardTypes: []diff.MigrationHazardType{ - diff.MigrationHazardTypeAcquiresAccessExclusiveLock, - }, }, { name: "Add no inherit check constraint", @@ -207,9 +192,6 @@ 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", @@ -420,9 +402,6 @@ 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", @@ -446,9 +425,6 @@ 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", @@ -470,9 +446,6 @@ var checkConstraintCases = []acceptanceTestCase{ ); `, }, - expectedHazardTypes: []diff.MigrationHazardType{ - diff.MigrationHazardTypeAcquiresAccessExclusiveLock, - }, }, { name: "Alter check constraint with UDF dependency should error", diff --git a/pkg/diff/schema_migration_plan_test.go b/pkg/diff/schema_migration_plan_test.go index 67c00b4..a48626e 100644 --- a/pkg/diff/schema_migration_plan_test.go +++ b/pkg/diff/schema_migration_plan_test.go @@ -838,6 +838,48 @@ var ( expectedStatements: nil, expectedDiffErrIs: errDuplicateIdentifier, }, + { + name: "Online check constraint addition", + oldSchema: schema.Schema{ + Tables: []schema.Table{ + { + Name: "foobar", + Columns: []schema.Column{ + {Name: "id", Type: "integer"}, + }, + ReplicaIdentity: schema.ReplicaIdentityDefault, + }, + }, + }, + newSchema: schema.Schema{ + Tables: []schema.Table{ + { + Name: "foobar", + Columns: []schema.Column{ + {Name: "id", Type: "integer"}, + }, + CheckConstraints: []schema.CheckConstraint{ + {Name: "id_check", Expression: "(id > 0)", IsInheritable: true, IsValid: true}, + }, + ReplicaIdentity: schema.ReplicaIdentityDefault, + }, + }, + }, + expectedStatements: []Statement{ + { + DDL: "ALTER TABLE \"public\".\"foobar\" ADD CONSTRAINT \"id_check\" CHECK((id > 0)) NOT VALID", + Timeout: statementTimeoutDefault, + LockTimeout: lockTimeoutDefault, + Hazards: nil, + }, + { + DDL: "ALTER TABLE \"public\".\"foobar\" VALIDATE CONSTRAINT \"id_check\"", + Timeout: statementTimeoutDefault, + LockTimeout: lockTimeoutDefault, + Hazards: nil, + }, + }, + }, { name: "Invalid check constraint made valid", oldSchema: schema.Schema{ diff --git a/pkg/diff/sql_generator.go b/pkg/diff/sql_generator.go index 3420b7e..cbdbe81 100644 --- a/pkg/diff/sql_generator.go +++ b/pkg/diff/sql_generator.go @@ -1444,14 +1444,29 @@ type checkConstraintSQLGenerator struct { } func (csg *checkConstraintSQLGenerator) Add(con schema.CheckConstraint) ([]Statement, error) { - var hazards []MigrationHazard - // UDF's in check constraints are a bad idea. Check constraints are not re-validated // if the UDF changes, so it's not really a safe practice. We won't support it for now if len(con.DependsOnFunctions) > 0 { return nil, fmt.Errorf("check constraints that depend on UDFs: %w", ErrNotImplemented) } + var stmts []Statement + if !con.IsValid || csg.isNewTable { + stmts = append(stmts, csg.createCheckConstraintStatement(con)) + } else { + // If the check constraint is not on a new table and is marked as valid, we should: + // 1. Build the constraint as invalid + // 2. Validate the constraint + con.IsValid = false + stmts = append(stmts, csg.createCheckConstraintStatement(con)) + stmts = append(stmts, validateConstraintStatement(csg.tableName, schema.EscapeIdentifier(con.Name))) + } + + return stmts, nil +} + +func (csg *checkConstraintSQLGenerator) createCheckConstraintStatement(con schema.CheckConstraint) Statement { + var hazards []MigrationHazard sb := strings.Builder{} sb.WriteString(fmt.Sprintf("%s CHECK(%s)", addConstraintPrefix(csg.tableName, schema.EscapeIdentifier(con.Name)), con.Expression)) @@ -1461,7 +1476,7 @@ func (csg *checkConstraintSQLGenerator) Add(con schema.CheckConstraint) ([]State if !con.IsValid { sb.WriteString(" NOT VALID") - } else if !csg.isNewTable { + } else { hazards = append(hazards, MigrationHazard{ Type: MigrationHazardTypeAcquiresAccessExclusiveLock, Message: "This will lock reads and writes to the owning table while the constraint is being added. " + @@ -1469,12 +1484,12 @@ func (csg *checkConstraintSQLGenerator) Add(con schema.CheckConstraint) ([]State }) } - return []Statement{{ + return Statement{ DDL: sb.String(), Timeout: statementTimeoutDefault, LockTimeout: lockTimeoutDefault, Hazards: hazards, - }}, nil + } } func (csg *checkConstraintSQLGenerator) Delete(con schema.CheckConstraint) ([]Statement, error) { From 0c413fa609311b8875d804b928aa7eecb0dc2891 Mon Sep 17 00:00:00 2001 From: bplunkett-stripe Date: Wed, 25 Oct 2023 14:31:33 -0400 Subject: [PATCH 2/2] Suggested change --- README.md | 2 +- pkg/diff/schema_migration_plan_test.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/README.md b/README.md index 71c7838..5ad8924 100644 --- a/README.md +++ b/README.md @@ -43,7 +43,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 addition: Check constraints are added as `INVALID` before being validated, eliminating the need + * Online check constraint builds: Check constraints are added as `INVALID` before being validated, eliminating the need for a long access-exclusive lock on the table * Prioritized index builds: Building new indexes is always prioritized over deleting old indexes * A comprehensive set of features to ensure the safety of planned migrations: diff --git a/pkg/diff/schema_migration_plan_test.go b/pkg/diff/schema_migration_plan_test.go index a48626e..7be93bc 100644 --- a/pkg/diff/schema_migration_plan_test.go +++ b/pkg/diff/schema_migration_plan_test.go @@ -839,7 +839,7 @@ var ( expectedDiffErrIs: errDuplicateIdentifier, }, { - name: "Online check constraint addition", + name: "Online check constraint build", oldSchema: schema.Schema{ Tables: []schema.Table{ {