sql: support ALTER COLUMN SET NOT NULL

[thoszhang]

This PR adds support for ALTER TABLE ... ALTER COLUMN ... SET NOT NULL, to
add a non-null constraint to an existing column. This is done by creating a
dummy CHECK constraint in the Validating state which moves through the states
of the schema changer, and setting Nullable to true on the column descriptor
only at the very end. Adding a constraint to an existing column requires the
constraint to be in a "write-only" state while validation occurs, and the
advantage of the approach in this PR is that we can reuse the existing CHECK
constraint states, where "write-only" corresponds to the state where the
constraint is on the table descriptor in the Validating state.

(As with FK and check constraints, we still need to process dropped constraints
in the schema changer. That's been left for a separate PR.)

Closes #28751.

Release note (sql change): Add support for ALTER TABLE ... ALTER COLUMN ... SET NOT NULL, which adds a non-null constraint to an existing column.

[cockroach-teamcity]

This change is 

[dt]

Reviewed 13 of 15 files at r1.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @dt and @lucy-zhang)

---

pkg/sql/sqlbase/structured.go, line 2459 at r1 (raw file):

func (desc *MutableTableDescriptor) AddNotNullValidationMutation(name string, c ColumnID) {
	// TODO (lucy): generate this constraint properly, as in MakeCheckConstraint
	ck := TableDescriptor_CheckConstraint{

I think we should add a real, structured field to CheckConstraint that indicates that this is really a NOT NULL instead of just the name match, even if we don't use it for now.

We could then later decide to e.g. hide them from check constraints, or find them by column id (assuming that the field was, say, the col id for which it was added), etc.

---

pkg/sql/sqlbase/structured.go, line 2530 at r1 (raw file):

// with a schema mutation that is still not yet public: Data validation,
// error reporting.
func (desc *ImmutableTableDescriptor) MakeFirstMutationPublic(includeConstraints bool) (*MutableTableDescriptor, error) {

Style nits w.r.t. bool args:

The style guide, for better or for worse, says all the callsites should include a comment with the arg name, e.g. doFoo(true /* includeConstraints */) OR pass a named constants instead of literal. I usually do the later, exporting the named const along side the function that takes it, e.g.const IgnoreConstraints, IncludeConstraints = false, true and then use those expressive names at the callsites instead of the literal + inline comment clutter.

In either case, I'd include in this function comment what this arg actually means / does, e.g. A reader might wonder what the behavior is if the first mutation is a constraint but the flag is false? or if it is true, but the first is an index?

Alternatively, the style guide also suggests avoiding bool args if it makes sense to instead just offer separate named functions that do behavior A vs B (that maybe then just hide an internal, bool-arg'ed helper).

[thoszhang]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @dt and @lucy-zhang)

---

pkg/sql/sqlbase/structured.go, line 2459 at r1 (raw file):

Previously, dt (David Taylor) wrote…

I think we should add a real, structured field to CheckConstraint that indicates that this is really a NOT NULL instead of just the name match, even if we don't use it for now.

We could then later decide to e.g. hide them from check constraints, or find them by column id (assuming that the field was, say, the col id for which it was added), etc.

Relatedly: Currently, this PR removes the dummy check constraint from the table descriptor when the schema change is complete, so that only the non-nullable flag remains. One problem with keeping a special marked check constraint on the descriptor even after the schema change is that there won't be a corresponding check constraint for non-null constraints that existed prior to this change being rolled out. We either have to do a migration for existing not null constraints somehow (which seems undesirable), or deal with the special marked check constraints being possibly not present, which adds confusion. (There's also the fact that the presence of the check constraint means an extra check during writes, which isn't ideal.)

I do think it's a good idea to add a field to CheckConstraint anyway to indicate this, but I think we are going to have to delete the dummy check constraint after adding the non-null constraint, and re-synthesize a new one for the schema changer if/when the not null constraint is dropped.

[thoszhang]

This is ready for another look.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @dt)

---

pkg/sql/sqlbase/structured.go, line 2530 at r1 (raw file):

Previously, dt (David Taylor) wrote…

Style nits w.r.t. bool args:

The style guide, for better or for worse, says all the callsites should include a comment with the arg name, e.g. doFoo(true /* includeConstraints */) OR pass a named constants instead of literal. I usually do the later, exporting the named const along side the function that takes it, e.g.const IgnoreConstraints, IncludeConstraints = false, true and then use those expressive names at the callsites instead of the literal + inline comment clutter.

In either case, I'd include in this function comment what this arg actually means / does, e.g. A reader might wonder what the behavior is if the first mutation is a constraint but the flag is false? or if it is true, but the first is an index?

Alternatively, the style guide also suggests avoiding bool args if it makes sense to instead just offer separate named functions that do behavior A vs B (that maybe then just hide an internal, bool-arg'ed helper).

Done. I ended up defining new constants.

[thoszhang]

TFTR

bors r+

[craig]

Build succeeded

• GitHub CI (Cockroach)