Online not null constraints: `SET NOT NULL` #83

bplunkett-stripe · 2024-01-09T01:25:22Z

Description

Use check constraints to make SET NOT NULL constraints online

One exception: This does not yet work for partitioned tables, since we don't yet support local check constraints

Motivation

#82

Testing

Tested via unit tests

bplunkett-stripe · 2024-01-09T08:17:22Z

README.md

@@ -45,6 +45,7 @@ $ pg-schema-diff plan --dsn "postgres://postgres:postgres@localhost:5432/postgre
  * 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
+  * Online `ALTER ... SET NOT NULL` using check constraints to eliminate the need for an access-exclusive lock on the table


Once this gets merged in, I'll cut a follow-up PR that highlights this behavior via an example, since I think showing rather than telling will work better.

alexaub-stripe · 2024-01-10T15:41:02Z

internal/migration_acceptance_tests/column_cases_test.go

+			"ALTER TABLE \"public\".\"foobar\" ADD CONSTRAINT \"not_null_10111213-1415-4617-9819-1a1b1c1d1e1f\" CHECK(\"foobar\" IS NOT NULL) NOT VALID",
+			"ALTER TABLE \"public\".\"foobar\" VALIDATE CONSTRAINT \"not_null_10111213-1415-4617-9819-1a1b1c1d1e1f\"",
+			"ALTER TABLE \"public\".\"foobar\" ALTER COLUMN \"foobar\" SET NOT NULL",
+			"ALTER TABLE \"public\".\"foobar\" DROP CONSTRAINT \"not_null_10111213-1415-4617-9819-1a1b1c1d1e1f\"",


Yeah, this format is much better than the manually "schema" defined tests. We should migrate all of those over except for the schemas that are impossible to recreate via DDL, e.g., invalid indexes.

alexaub-stripe

Looks great! Some thoughts but all non-blocking.

alexaub-stripe · 2024-01-10T15:43:13Z

internal/migration_acceptance_tests/column_cases_test.go

+			"ALTER TABLE \"public\".\"foobar\" ADD CONSTRAINT \"foobar\" CHECK((foobar IS NOT NULL)) NOT VALID",
+			"ALTER TABLE \"public\".\"foobar\" VALIDATE CONSTRAINT \"foobar\"",
+			"ALTER TABLE \"public\".\"foobar\" ALTER COLUMN \"foobar\" SET NOT NULL",


Amazing that this works without adding a system check constraint

alexaub-stripe · 2024-01-10T15:57:20Z

pkg/diff/sql_generator.go

+		return schema.CheckConstraint{}, fmt.Errorf("generating uuid: %w", err)
+	}
+	return schema.CheckConstraint{
+		Name:               fmt.Sprintf("not_null_%s", uuid.String()),


thought: maybe we can fit in pg_schema_diff to the identifier somehow? That'd help with debugging/confusion if one of these are left around, but it's not critical

Great point!

I want to update the temporary indexes to use this pattern, but I'm going to save that for a subsequent PR, since I also am going to switch to the UUID's to base64 encoded to make them smaller.

alexaub-stripe · 2024-01-10T16:02:03Z

pkg/diff/sql_generator.go

+
+	if isOnPreExistingColumn && isValidNotNullCC(con) {
+		// If the NOT NULL check constraint is on a pre-existing column, then we should ensure it is added before
+		// the column.


Is this correction right?

Suggested change

// the column.

// the column alter.

pkg/diff/sql_generator.go

alexaub-stripe · 2024-01-10T16:10:32Z

pkg/diff/sql_generator.go

+	// If it's a not-null check constraint, we can drop the check constraint whenever convenient, e.g., after
+	// the column has been altered because `NOT NULL` does not depend on the type of the column.
+	// For all other check constraints, they can rely on the type of the column. Thus, we should drop these
+	// check constraint before any columns are altered because the new type might not be compatible with the old
+	// check constraint.


Is there value to allowing the NN constraints to be dropped whenever? Maybe it'd be best to add the dependency regardless just for consistency among the constraint types?

It's critical we drop this last, which the topographical sort does if an obj has no deps because of the scenario where we're deleting a valid not null constraint AND changing a column to NOT NULL.

I imagine the scenario above is pretty common because it would occur for a half-complete migration, where the valid constraint was added but the SET NOT NULL never ran

bplunkett-stripe · 2024-01-10T17:56:56Z

internal/graph/graph.go

-			return nil, fmt.Errorf("cycle detected: %+v, %+v", graph, incomingEdgeCountByVertex)
+			dotSB := strings.Builder{}
+			if err := EncodeDOT(g, &dotSB, true); err != nil {
+				dotSB.Reset()


The dot encoding has been super useful for debugging! @kevinmingtarja

Initial MVP

0c85158

bplunkett-stripe force-pushed the bplunkett/online-not-null-constraints branch from 081f29e to 41e3d6f Compare January 9, 2024 01:27

Switch to SQLGraphs

f7bb468

bplunkett-stripe force-pushed the bplunkett/online-not-null-constraints branch 7 times, most recently from bdb3706 to 61969a5 Compare January 9, 2024 07:40

bplunkett-stripe requested a review from alexaub-stripe January 9, 2024 07:41

bplunkett-stripe assigned alexaub-stripe Jan 9, 2024

bplunkett-stripe added the enhancement New feature or request label Jan 9, 2024

bplunkett-stripe force-pushed the bplunkett/online-not-null-constraints branch from 61969a5 to 25af9f2 Compare January 9, 2024 07:44

Remove attr num. More fixes

9c0af84

bplunkett-stripe force-pushed the bplunkett/online-not-null-constraints branch from 25af9f2 to 9c0af84 Compare January 9, 2024 07:58

bplunkett-stripe commented Jan 9, 2024

View reviewed changes

bplunkett-stripe changed the title ~~Online not null constraints~~ Online not null constraints: SET NOT NULL Jan 10, 2024

alexaub-stripe reviewed Jan 10, 2024

View reviewed changes

alexaub-stripe previously approved these changes Jan 10, 2024

View reviewed changes

bplunkett-stripe dismissed alexaub-stripe’s stale review via 12f773c January 10, 2024 17:55

bplunkett-stripe commented Jan 10, 2024

View reviewed changes

bplunkett-stripe force-pushed the bplunkett/online-not-null-constraints branch 2 times, most recently from 0e15973 to 4953a1b Compare January 10, 2024 18:02

Suggested changes

27a1517

bplunkett-stripe force-pushed the bplunkett/online-not-null-constraints branch from 4953a1b to 27a1517 Compare January 10, 2024 18:07

Update name generation

5fddc92

bplunkett-stripe force-pushed the bplunkett/online-not-null-constraints branch from 372c33d to 7e891de Compare January 10, 2024 18:34

Revert previous changes. We'll save that for a follow up PR

ab005bd

bplunkett-stripe force-pushed the bplunkett/online-not-null-constraints branch from 7e891de to ab005bd Compare January 10, 2024 18:40

bplunkett-stripe requested a review from alexaub-stripe January 10, 2024 18:41

alexaub-stripe approved these changes Jan 10, 2024

View reviewed changes

bplunkett-stripe merged commit d091969 into main Jan 10, 2024
6 checks passed

bplunkett-stripe deleted the bplunkett/online-not-null-constraints branch January 10, 2024 19:24

bplunkett-stripe restored the bplunkett/online-not-null-constraints branch January 10, 2024 19:24

bplunkett-stripe deleted the bplunkett/online-not-null-constraints branch January 10, 2024 19:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Online not null constraints: `SET NOT NULL` #83

Online not null constraints: `SET NOT NULL` #83

bplunkett-stripe commented Jan 9, 2024 •

edited

Loading

bplunkett-stripe Jan 9, 2024

alexaub-stripe Jan 10, 2024

bplunkett-stripe Jan 10, 2024 •

edited

Loading

alexaub-stripe left a comment

alexaub-stripe Jan 10, 2024

alexaub-stripe Jan 10, 2024

bplunkett-stripe Jan 10, 2024

bplunkett-stripe Jan 10, 2024

alexaub-stripe Jan 10, 2024

alexaub-stripe Jan 10, 2024

bplunkett-stripe Jan 10, 2024

bplunkett-stripe Jan 10, 2024

Online not null constraints: SET NOT NULL #83

Online not null constraints: SET NOT NULL #83

Conversation

bplunkett-stripe commented Jan 9, 2024 • edited Loading

Description

Motivation

Testing

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bplunkett-stripe Jan 10, 2024 • edited Loading

Choose a reason for hiding this comment

alexaub-stripe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Online not null constraints: `SET NOT NULL` #83

Online not null constraints: `SET NOT NULL` #83

bplunkett-stripe commented Jan 9, 2024 •

edited

Loading

bplunkett-stripe Jan 10, 2024 •

edited

Loading