sql: validate check constraints with the schema changer

[thoszhang]

Currently, constraints are added in the Unvalidated state, and are not
validated for existing rows until ALTER TABLE ... VALIDATE CONSTRAINT is run.
With this change, check constraints will be validated asynchronously after they
are added by default (and similar changes to FKs are to follow). This addresses
the problematic long-running transactions caused by the current implementation
of VALIDATE CONSTRAINT. This PR is a rework of #32504 and has the same tests.

With this change, check constraints will be added to the table descriptor in
the new Validating state, visible to CRUD operations, and a mutation is
queued indicating that the constraint is to be validated. During the backfill
step, the constraint is validated for existing rows. If validation succeeds,
then the constraint moves to the Validated state; otherwise, it is dropped.

The behavior when dropping constraints (either via DROP CONSTRAINT or
indirectly when a column is dropped) is unchanged: no mutation is enqueued.

As part of this change, check constraints can be added to non-public columns in
the process of being added, including columns that were created earlier in the
same transaction.

The main difference between this PR and #32504 is that #32504 does not add the
constraint to the table descriptor until it has been validated.

See #34238 for more context.

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 

[vivekmenezes]

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

---

pkg/sql/alter_table.go, line 217 at r1 (raw file):

				// A previous command could have added a column which the new constraint uses,
				// allocate IDs now.
				if err != n.tableDesc.AllocateIDs() {

This seems out of place. Does something break without it?

---

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

					return err
				}
				doneCheckValidation = true

What if there are two columns with two check constraints?

---

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

			case *sqlbase.DescriptorMutation_Constraint:
				return errors.Errorf("constraint validation mutation cannot be in the DROP state within the same transaction: %+v", m)

How about BEGIN; ALTER ADD COLUMN a INT CHECK a < 1; ALTER DROP COLUMN a; COMMIT ?

---

pkg/sql/create_table.go, line 1488 at r1 (raw file):

		col, dropped, err := desc.FindColumnByName(c.ColumnName)
		if err != nil || dropped {

Why do you need this change?

---

pkg/sql/schema_changer.go, line 1160 at r1 (raw file):

					}
				}
			}

Do you think you can add this logic to sc.deleteIndexMutationsWithReversedColumns() and rename that to sc.DeleteDependentMutationsWithReversedColumns() ?

---

pkg/sql/schema_changer.go, line 1838 at r1 (raw file):

			Location: dummyLocation,
		},
		User: security.NodeUser,

I don't believe you need this because you are using the backfiller and not the internal executor

---

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

	cb.evalCtx = evalCtx
	numCols := len(desc.Columns)
	cb.cols = desc.Columns

Does desc.WritableColumns() work here?

---

pkg/sql/distsqlrun/checkbackfiller.go, line 28 at r1 (raw file):

)

// indexBackfiller is a processor that backfills new indexes.

checkBackfiller

---

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

	}

	checks := make([]TableDescriptor_CheckConstraint, len(tbl.Checks))

desc.allChecks =

---

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

		}
	}
	return nil, fmt.Errorf("index %q does not exist", name)

check %q does not exist.

---

pkg/sql/sqlbase/structured.proto, line 194 at r1 (raw file):

enum ConstraintValidity {
  Validated = 0;

Can you add a comment for Validated as well:

// The constraint is valid across all rows.

---

pkg/sql/sqlbase/structured.proto, line 196 at r1 (raw file):

  Validated = 0;
  Unvalidated = 1;
  // The state for a constraint that was just added, but whose validation

Perhaps add: // The constraint has yet to be checked across all rows.

---

pkg/sql/sqlbase/structured.proto, line 501 at r1 (raw file):

// containing each column/index descriptor being added or dropped.
message DescriptorMutation {
  message ConstraintToValidate {

let's place this message outside of DescriptorMutation like the other messages.

[thoszhang]

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

---

pkg/sql/alter_table.go, line 217 at r1 (raw file):

Previously, vivekmenezes wrote…

This seems out of place. Does something break without it?

Yeah, if you add a check constraint on a column that's being added, the check will contain a column ID of 0 in its ColumnIDs field which will never get updated later. The test that fails is TestSchemaChangeAfterCreateInTxn, at the SCRUB check.

To make sure it wasn't just a SCRUB false positive, I added an UPDATE statement to it, which returns an error when it can't find column 0. (For some reason, INSERT still works even with the invalid column id.)

---

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

Previously, vivekmenezes wrote…

What if there are two columns with two check constraints?

The backfiller validates every check constraint at once. I updated the comment to make this more clear.

---

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

Previously, vivekmenezes wrote…

How about BEGIN; ALTER ADD COLUMN a INT CHECK a < 1; ALTER DROP COLUMN a; COMMIT ?

If you do that, you'll get the constraint "a" in the middle of being added error.

This method only gets called when there's a schema change for a new table added earlier in the same transaction. We never add a DROP mutation when dropping a check, we just remove it from the list of checks. So the only time any check validation mutation can be in the DROP state is if it's getting rolled back because of an error, which is a different situation from what this method covers.

---

pkg/sql/create_table.go, line 1488 at r1 (raw file):

Previously, vivekmenezes wrote…

Why do you need this change?

The method gets called by MakeCheckConstraint. This change is to deal with non-public columns currently being added.

---

pkg/sql/schema_changer.go, line 1160 at r1 (raw file):

Previously, vivekmenezes wrote…

Do you think you can add this logic to sc.deleteIndexMutationsWithReversedColumns() and rename that to sc.DeleteDependentMutationsWithReversedColumns() ?

I'm not sure about adding it to that function or any existing function, since this is a new kind of behavior (we're removing something from the table descriptor itself, not just the mutations). I split this out into a new function.

---

pkg/sql/schema_changer.go, line 1838 at r1 (raw file):

Previously, vivekmenezes wrote…

I don't believe you need this because you are using the backfiller and not the internal executor

You're right (I forgot to remove this when I added the backfiller)

---

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

Previously, vivekmenezes wrote…

Does desc.WritableColumns() work here?

I don't think so, since we also want to exclude columns being dropped

---

pkg/sql/distsqlrun/checkbackfiller.go, line 28 at r1 (raw file):

Previously, vivekmenezes wrote…

checkBackfiller

Done.

---

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

Previously, vivekmenezes wrote…

desc.allChecks =

Done.

---

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

Previously, vivekmenezes wrote…

check %q does not exist.

Done.

---

pkg/sql/sqlbase/structured.proto, line 194 at r1 (raw file):

Previously, vivekmenezes wrote…

Can you add a comment for Validated as well:

// The constraint is valid across all rows.

Done.

---

pkg/sql/sqlbase/structured.proto, line 196 at r1 (raw file):

Previously, vivekmenezes wrote…

Perhaps add: // The constraint has yet to be checked across all rows.

Done.

---

pkg/sql/sqlbase/structured.proto, line 501 at r1 (raw file):

Previously, vivekmenezes wrote…

let's place this message outside of DescriptorMutation like the other messages.

Done.

[thoszhang]

Ready for another look.

I also added a few tests to the schema_change_in_txn logic test for the errors that are returned when trying to drop or validate a check constraint that's being added.

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

[vivekmenezes]

Thanks for updating the PR. I think this PR should wait on #34263 to merge so you can remove the backfill functionality in this PR. Thanks

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

[thoszhang]

This is ready for another look. I replaced the backfiller with the InternalExecutor SQL query, and added some more tests, mostly for when a table is created and schema changes are made in the same transaction, since that case currently has very little coverage.

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

[vivekmenezes]

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

---

pkg/sql/backfill.go, line 284 at r2 (raw file):

				}

				return validateCheckExpr(ctx, check.Expr, desc.TableDesc(), ie, txn)

All the code above is a copy of the code in ValidateCheckInTxn() . Make the recommended change to ValidateCheckInTxn and call it from here too.

---

pkg/sql/backfill.go, line 898 at r2 (raw file):

	tableDesc *MutableTableDescriptor,
	txn *client.Txn,
	checks []sqlbase.ConstraintToValidate,

You can iterate on the checks outside this function and then you will be able to use ValidateCheckInTxn() both here and in the normal case above. See my other comment.

---

pkg/sql/check.go, line 54 at r2 (raw file):

	queryStr := tree.AsStringWithFlags(stmt, tree.FmtParsable)
	log.Infof(ctx, "Validating check constraint %q with query %q", expr.String(), queryStr)

take out

---

pkg/sql/sqlbase/structured.go, line 2291 at r2 (raw file):

		i++
	}
	table.Mutations = table.Mutations[i:]

good catch

[vivekmenezes]

Nice work!

[thoszhang]

TFTR, just 1 minor question

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

---

pkg/sql/backfill.go, line 284 at r2 (raw file):

Previously, vivekmenezes wrote…

All the code above is a copy of the code in ValidateCheckInTxn() . Make the recommended change to ValidateCheckInTxn and call it from here too.

Done.

---

pkg/sql/backfill.go, line 898 at r2 (raw file):

Previously, vivekmenezes wrote…

You can iterate on the checks outside this function and then you will be able to use ValidateCheckInTxn() both here and in the normal case above. See my other comment.

Done.

---

pkg/sql/check.go, line 54 at r2 (raw file):

Previously, vivekmenezes wrote…

take out

I added this to match the logging for the FK validation queries (elsewhere in this file). Should the logging be removed?

[vivekmenezes]

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

[thoszhang]

bors r+

[craig]

Build failed (retrying...)

• GitHub CI (Cockroach)

[craig]

Build failed

• GitHub CI (Cockroach)

[thoszhang]

I'm getting No space left on device on the build agent. I'm going to recreate the agent and try again.

[thoszhang]

bors r+

[craig]

Build succeeded

• GitHub CI (Cockroach)