Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Online check constraint addition #76

Merged
merged 2 commits into from
Oct 25, 2023
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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 addition: Check constraints are added as `INVALID` before being validated, eliminating the need
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe use builds here? Addition is a bit of an awkward term in my head

Suggested change
* 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:
* 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 addition",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thought: Do we already have a case for the validation of a previously invalid constraint?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a different test case:

  • one is testing altering a check constraint from invalid to valid
  • one is testing that add a constraint is online

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 {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If !con.IsValid shouldn't we have a validateConstraintStatement?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope! Because the user is adding an invalid constraint! (They want it to be invalid)

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 {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We strip the migration hazards for new tables within the TableSQLGenerator, so we don't actually need this conditional

} 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