sql: run check constraint mutations and validation by schema changer

[eriktrinh]

This change moves check constraint adds and drops through the schema
changer when the transaction commits, moving the constraint through the
same intermediate states as when index/columns are added or dropped. The
only small differences are:

• Constraint adds can start in write-only and immediately start being
  enforced if all used columns are in write-only or public.
• Constraint drops can move immediately from public to absent if they
  have not yet been validated.

Therefore, the check constraint is no longer immediately public when it
is added, but allows data validation of the constraint to be performed
when the constraint is added (and is now the default behaviour).
Constraints can now also be added on columns in the process of being
added.

This change also ensures that there are no data anomalies in either
versions of the schema when dropping a validated check constraint, as
previously the transition moved the constraint from public -> absent.
Writes on the new version were not being checked even though nodes on an
older version expect all rows to conform to the constraint.

Release note (sql change): Check constraint adds by default will
validate table data with the added constraint asynchronously after the
transaction commits.

[cockroach-teamcity]

This change is 

[dt]

Great Stuff! A couple really minor nits. The schema_changer.go stuff is in code with which I have a little less familiarity but overall approach looks great (though as commented below, I still get confused about how txn semantics work with schema changes that "fail").

Reviewed 19 of 21 files at r1.
Reviewable status: complete! 0 of 0 LGTMs obtained

---

pkg/sql/backfill.go, line 47 at r1 (raw file):

	// checkConstraintBackfillChunkSize is the maximum number of rows
	// processed per chunk during check constraint validation.
	checkConstraintBackfillChunkSize = 800

How did we pick this number?

---

pkg/sql/backfill/backfill.go, line 111 at r1 (raw file):

	); err != nil {
		return err
	} else {

this really should be allowed -- minimizing the scope in which typed has to be bound is nice and clean -- but sadly the linter doesn't like it since first block ends in return. dumb linter.

---

pkg/sql/backfill/backfill.go, line 183 at r1 (raw file):

			}
			if val == tree.DBoolFalse {
				return roachpb.Key{}, errors.Errorf("validation of CHECK %q failed on row: %s",

I think there's a proper pgerror code we should be returning -- though I don't know if returning it here will make it back to the client.

---

pkg/sql/logictest/testdata/logic_test/schema_change_in_txn, line 611 at r1 (raw file):

statement ok
ALTER TABLE forlater ADD CONSTRAINT e_0 CHECK (e > 0)

Is it sort of weird that you get a success reply here, but if you then ran SELECT ... WHERE e < 0 in the txn, you could observe a violation?

---

pkg/sql/logictest/testdata/logic_test/schema_change_in_txn, line 613 at r1 (raw file):

ALTER TABLE forlater ADD CONSTRAINT e_0 CHECK (e > 0)

statement error pq: validation of CHECK "e > 0" failed on row: k='c', v='d', d=NULL, e=0

So you get an error from commit but the txn does commit, right? just the CHECK doesn't get added? I realize some of our other schema changes have similar behavior, but it really seems like a strange violation of the all-or-nothing aspect of committing a txn. Maybe I just need to get over that expectation though.

[eriktrinh]

Reviewable status: complete! 0 of 0 LGTMs obtained

---

pkg/sql/backfill.go, line 47 at r1 (raw file):

Previously, dt (David Taylor) wrote…

How did we pick this number?

I chose a number larger than any of the other backfill chunk sizes because check backfills don't actually write any data. Testing and benchmarking could be done to tune this.

---

pkg/sql/backfill/backfill.go, line 183 at r1 (raw file):

Previously, dt (David Taylor) wrote…

I think there's a proper pgerror code we should be returning -- though I don't know if returning it here will make it back to the client.

You're right, there exists 23514 aka check_validation data exception. However error codes from the schema changer get replaced with a statement_completion_unknown code.

---

pkg/sql/logictest/testdata/logic_test/schema_change_in_txn, line 611 at r1 (raw file):

Previously, dt (David Taylor) wrote…

Is it sort of weird that you get a success reply here, but if you then ran SELECT ... WHERE e < 0 in the txn, you could observe a violation?

Yea this is kinda weird, a similar result is observed with unique constraints as well.

---

pkg/sql/logictest/testdata/logic_test/schema_change_in_txn, line 613 at r1 (raw file):

Previously, dt (David Taylor) wrote…

So you get an error from commit but the txn does commit, right? just the CHECK doesn't get added? I realize some of our other schema changes have similar behavior, but it really seems like a strange violation of the all-or-nothing aspect of committing a txn. Maybe I just need to get over that expectation though.

Right, the txn which added the mutation does commit. The violation is strange but it's communicated to the user through the error code and name.

[eriktrinh]

Reviewable status: complete! 0 of 0 LGTMs obtained

---

pkg/sql/sqlbase/structured.proto, line 480 at r2 (raw file):

}

message CheckConstraint {

Not sure if pulling out this message type is safe or not.

[eriktrinh]

Gave this PR another pass: updating the commit message, adding tests, adding a validating state, and also dropping check constraints through the schema changer if the constraint has been validated. PTAL.

[thoszhang]

Reviewable status: complete! 0 of 0 LGTMs obtained

---

pkg/sql/sqlbase/structured.go, line 2261 at r4 (raw file):

Previously, lucy-zhang (Lucy Zhang) wrote…

Updated AddCheckMutation to match the implementation for indexes and columns.

Also removed a test in schema_changer_test.go that was testing this special case.

[thoszhang]

bors r+

[craig]

Build succeeded

• GitHub CI (Cockroach)