Skip to content

Commit

Permalink
Online check constraint addition (#76)
Browse files Browse the repository at this point in the history
* Online check constraint addition
  • Loading branch information
bplunkett-stripe authored Oct 25, 2023
1 parent d1adc31 commit 6bad92c
Show file tree
Hide file tree
Showing 4 changed files with 65 additions and 32 deletions.
3 changes: 3 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -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 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:
* 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
Expand Down
27 changes: 0 additions & 27 deletions internal/migration_acceptance_tests/check_constraint_cases_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -52,9 +52,6 @@ var checkConstraintCases = []acceptanceTestCase{
);
`,
},
expectedHazardTypes: []diff.MigrationHazardType{
diff.MigrationHazardTypeAcquiresAccessExclusiveLock,
},
},
{
name: "Add check constraint with UDF dependency should error",
Expand Down Expand Up @@ -109,9 +106,6 @@ var checkConstraintCases = []acceptanceTestCase{
);
`,
},
expectedHazardTypes: []diff.MigrationHazardType{
diff.MigrationHazardTypeAcquiresAccessExclusiveLock,
},
},
{
name: "Add multiple check constraints",
Expand All @@ -134,9 +128,6 @@ var checkConstraintCases = []acceptanceTestCase{
);
`,
},
expectedHazardTypes: []diff.MigrationHazardType{
diff.MigrationHazardTypeAcquiresAccessExclusiveLock,
},
},
{
name: "Add check constraints to new column",
Expand All @@ -157,9 +148,6 @@ var checkConstraintCases = []acceptanceTestCase{
);
`,
},
expectedHazardTypes: []diff.MigrationHazardType{
diff.MigrationHazardTypeAcquiresAccessExclusiveLock,
},
},
{
name: "Add check constraint with quoted identifiers",
Expand All @@ -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",
Expand All @@ -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",
Expand Down Expand Up @@ -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",
Expand All @@ -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",
Expand All @@ -470,9 +446,6 @@ var checkConstraintCases = []acceptanceTestCase{
);
`,
},
expectedHazardTypes: []diff.MigrationHazardType{
diff.MigrationHazardTypeAcquiresAccessExclusiveLock,
},
},
{
name: "Alter check constraint with UDF dependency should error",
Expand Down
42 changes: 42 additions & 0 deletions pkg/diff/schema_migration_plan_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -838,6 +838,48 @@ var (
expectedStatements: nil,
expectedDiffErrIs: errDuplicateIdentifier,
},
{
name: "Online check constraint build",
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{
Expand Down
25 changes: 20 additions & 5 deletions pkg/diff/sql_generator.go
Original file line number Diff line number Diff line change
Expand Up @@ -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))
Expand All @@ -1461,20 +1476,20 @@ 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. " +
"Instead, consider adding the constraint as NOT VALID and validating it later.",
})
}

return []Statement{{
return Statement{
DDL: sb.String(),
Timeout: statementTimeoutDefault,
LockTimeout: lockTimeoutDefault,
Hazards: hazards,
}}, nil
}
}

func (csg *checkConstraintSQLGenerator) Delete(con schema.CheckConstraint) ([]Statement, error) {
Expand Down

0 comments on commit 6bad92c

Please sign in to comment.