sql: add support for NOT VALID Foreign Key constraints

[tyler314]

Implemented the support of not validating an ALTER TABLE foreign key constraint to a table. Within alter_table.go we set the validity of the foreign key reference to be unvalidated, and within shema_changer.go we then check the case where the foreign key being added is unvalidated and do not check its validity.

This allows users to create a reference between two non-empty tables who's entries don't comply with the constraint when it's first added.

[cockroach-teamcity]

This change is 

[thoszhang]

Your commit doesn't seem to have a message beyond the first line (?). You can use what you have in the PR description and amend your PR (and add a release note). The last line should be something like This allows users to create a FK reference between two tables whose rows don't comply with the constraint when it's first added (no need for "non-empty".)

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

---

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

// The passed Txn is used to lookup databases to qualify names in error messages
// but if nil, will result in unqualified names in those errors.
func ResolveFK(

Can you add to the doc comment explaining the validationState parameter? I'd also rename it to validationBehavior.

---

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

	now := timeutil.Now().UnixNano()

	// Get the other tables whose foreign key backreferences need to be removed.

Can you add a brief comment to explain that we need to call PublishMultiple() to potentially add foreign key backreferences?

---

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

					return err
				}
				// TODO (tyler): Consider case where a retry is done and this method is called again

This isn't necessary

---

pkg/sql/logictest/testdata/logic_test/fk, line 1393 at r1 (raw file):

# currently don't support adding a validated FK in the same transaction as
# CREATE TABLE
# TODO (lucy): Once this is no longer true (and we support adding NOT VALID

This test should be updated now, per the TODO. I think it should be sufficient to remove the BEGIN and COMMIT, and add a NOT VALID to the ALTER TABLE statement.

There's also another test that needs to be updated in the same way, in pkg/sql/opt/exec/execbuilder/testdata/fk.

---

pkg/sql/logictest/testdata/logic_test/fk, line 1438 at r1 (raw file):

DROP TABLE a, b

subtest Composite_Simple

Ideally we'd also want to bring back all the logic tests that test the behavior of VALIDATE CONSTRAINT (I originally took them all out in #37433, which started validating all added FKs), now that we can create unvalidated FK constraints. It doesn't have to be in this PR, so you can either put them back in now (you can just copy them from that PR diff and add NOT VALID everywhere) or add a TODO to do it later.

---

pkg/sql/logictest/testdata/logic_test/fk, line 2105 at r1 (raw file):

DROP TABLE t2, t2 CASCADE

statement ok

can you make this a subtest (and drop all the tables at the end)?

---

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

// MakeMutationComplete updates the descriptor upon completion of a mutation.
func (desc *MutableTableDescriptor) MakeMutationComplete(m DescriptorMutation) error {

Can you add to the doc comment to indicate

1. the difference in behavior between validating vs. unvalidated constraints (i.e., validating constraints are assumed to have been added earlier and just need to be marked as validated, whereas unvalidated constraints have not been added earlier and need to be added); and
2. the fact that this method can't add backreferences for foreign keys, and that those need to be added separately by the caller as needed?

I'd also add a TODO to somehow refactor this so that the FK reference and backreference are added in the same place (maybe by passing in a closure to this method?), since this situation isn't ideal.

---

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

			}

		case *DescriptorMutation_Constraint:

It'd be good to also add some inline comments to explain why we need all these cases.

[tyler314]

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

---

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

Previously, lucy-zhang (Lucy Zhang) wrote…

Can you add to the doc comment explaining the validationState parameter? I'd also rename it to validationBehavior.

Done.

---

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

Previously, lucy-zhang (Lucy Zhang) wrote…

Can you add a brief comment to explain that we need to call PublishMultiple() to potentially add foreign key backreferences?

Done.

---

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

Previously, lucy-zhang (Lucy Zhang) wrote…

This isn't necessary

Done.

---

pkg/sql/logictest/testdata/logic_test/fk, line 1393 at r1 (raw file):

Previously, lucy-zhang (Lucy Zhang) wrote…

This test should be updated now, per the TODO. I think it should be sufficient to remove the BEGIN and COMMIT, and add a NOT VALID to the ALTER TABLE statement.

There's also another test that needs to be updated in the same way, in pkg/sql/opt/exec/execbuilder/testdata/fk.

Done.

---

pkg/sql/logictest/testdata/logic_test/fk, line 1438 at r1 (raw file):

Previously, lucy-zhang (Lucy Zhang) wrote…

Ideally we'd also want to bring back all the logic tests that test the behavior of VALIDATE CONSTRAINT (I originally took them all out in #37433, which started validating all added FKs), now that we can create unvalidated FK constraints. It doesn't have to be in this PR, so you can either put them back in now (you can just copy them from that PR diff and add NOT VALID everywhere) or add a TODO to do it later.

Done.

---

pkg/sql/logictest/testdata/logic_test/fk, line 2105 at r1 (raw file):

Previously, lucy-zhang (Lucy Zhang) wrote…

can you make this a subtest (and drop all the tables at the end)?

Done.

---

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

Previously, lucy-zhang (Lucy Zhang) wrote…

Can you add to the doc comment to indicate

1. the difference in behavior between validating vs. unvalidated constraints (i.e., validating constraints are assumed to have been added earlier and just need to be marked as validated, whereas unvalidated constraints have not been added earlier and need to be added); and
2. the fact that this method can't add backreferences for foreign keys, and that those need to be added separately by the caller as needed?

I'd also add a TODO to somehow refactor this so that the FK reference and backreference are added in the same place (maybe by passing in a closure to this method?), since this situation isn't ideal.

Done.

---

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

Previously, lucy-zhang (Lucy Zhang) wrote…

It'd be good to also add some inline comments to explain why we need all these cases.

Done.

[thoszhang]

There are some test failures - when I ran the tests on my laptop, something was hung and hit the test timeout (which is presumably why a lot of the tests in CI failed with "test ended in panic." We need to figure out what the problem is.

Also, could you add a release note, since this is a new user-facing feature/change in behavior? (And copy the new commit message into the PR message.)

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

---

pkg/sql/create_table.go, line 443 at r2 (raw file):

//
// The passed validationBehavior is used to determine whether or not preexisting
// entries in the backref table need to be validated against the foreign key

Remove backref. Also, this should probably mention that this argument only applies for existing tables, not new tables.

---

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

Previously, Tyler314 (Tyler Roberts) wrote…

Done.

nit: I'd prefer to put the added comment here, instead of when PublishMultiple is called, since we start constructing the arguments to PublishMultiple here.

[tyler314]

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

---

pkg/sql/create_table.go, line 443 at r2 (raw file):

Previously, lucy-zhang (Lucy Zhang) wrote…

Remove backref. Also, this should probably mention that this argument only applies for existing tables, not new tables.

Done.

[tyler314]

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

---

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

Previously, lucy-zhang (Lucy Zhang) wrote…

nit: I'd prefer to put the added comment here, instead of when PublishMultiple is called, since we start constructing the arguments to PublishMultiple here.

Done.

[tyler314]

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

[thoszhang]

lgtm except for minor comments

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

---

pkg/sql/schema_changer.go, line 1217 at r5 (raw file):

	// Get the other tables whose foreign key backreferences need to be removed.
	// We make a call to PublishMultiple to handle the situation to add Foreign Key backreferences.
	// We begin to construct the arguments of the method here, which includes:

I don't think these 3 lines are really necessary

---

pkg/sql/logictest/testdata/logic_test/schema_change_in_txn, line 943 at r5 (raw file):

statement ok
ALTER TABLE check_table ADD CONSTRAINT d_0 CHECK (d > 0) NOT VALID

I'm fine with adding NOT VALID to these, but can you just add it for one constraint and not the other (so we can test both cases)? I'd also remove the VALIDATE CONSTRAINT statements below so we can check using SHOW CONSTRAINTS that it's actually unvalidated.

---

pkg/sql/opt/exec/execbuilder/testdata/fk, line 6 at r5 (raw file):

# currently don't support adding a validated FK in the same transaction as
# CREATE TABLE
statement ok

As mentioned before, can you update this test in the same way as the other logic test? i.e., remove the BEGIN/COMMIT, add a NOT VALID to the constraint

---

pkg/sql/sqlbase/structured.go, line 2377 at r5 (raw file):

// MakeMutationComplete updates the descriptor upon completion of a mutation.
// There are three Validity types for the mutations:

I find these comments confusing. I think the important thing to explain (both here and inline) is the difference between the behavior for Validating and Unvalidated constraints: Validating constraints have already been added and need to be marked as Validated, whereas Unvalidated constraints needed to be added (for the first time). (And it's not even possible for Validated constraints to enter this function, hence the assertion error.)

---

pkg/sql/sqlbase/structured.go, line 2398 at r5 (raw file):

				switch t.Constraint.Check.Validity {
				case ConstraintValidity_Unvalidated:
					// add the constraint to a slice of constraints to be checked

This should be something like "add the constraint to the list of check constraints on the table descriptor"

---

pkg/sql/sqlbase/structured.go, line 2422 at r5 (raw file):

				case ConstraintValidity_Validating:
					idx.ForeignKey.Validity = ConstraintValidity_Validated
				case ConstraintValidity_Unvalidated:

it's a little confusing that the ordering for the validity cases don't match between the FK case and the check case - can you reorder them?

[tyler314]

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

---

pkg/sql/schema_changer.go, line 1217 at r5 (raw file):

Previously, lucy-zhang (Lucy Zhang) wrote…

I don't think these 3 lines are really necessary

Done.

---

pkg/sql/logictest/testdata/logic_test/schema_change_in_txn, line 943 at r5 (raw file):

Previously, lucy-zhang (Lucy Zhang) wrote…

I'm fine with adding NOT VALID to these, but can you just add it for one constraint and not the other (so we can test both cases)? I'd also remove the VALIDATE CONSTRAINT statements below so we can check using SHOW CONSTRAINTS that it's actually unvalidated.

Yes, this makes sense.

---

pkg/sql/opt/exec/execbuilder/testdata/fk, line 6 at r5 (raw file):

Previously, lucy-zhang (Lucy Zhang) wrote…

As mentioned before, can you update this test in the same way as the other logic test? i.e., remove the BEGIN/COMMIT, add a NOT VALID to the constraint

yes, I think I just misunderstand when we discussed changing this test the other day.

---

pkg/sql/sqlbase/structured.go, line 2377 at r5 (raw file):

Previously, lucy-zhang (Lucy Zhang) wrote…

I find these comments confusing. I think the important thing to explain (both here and inline) is the difference between the behavior for Validating and Unvalidated constraints: Validating constraints have already been added and need to be marked as Validated, whereas Unvalidated constraints needed to be added (for the first time). (And it's not even possible for Validated constraints to enter this function, hence the assertion error.)

Updated the function comments, and added a few inline ones as well.

---

pkg/sql/sqlbase/structured.go, line 2398 at r5 (raw file):

Previously, lucy-zhang (Lucy Zhang) wrote…

This should be something like "add the constraint to the list of check constraints on the table descriptor"

Done.

---

pkg/sql/sqlbase/structured.go, line 2422 at r5 (raw file):

Previously, lucy-zhang (Lucy Zhang) wrote…

it's a little confusing that the ordering for the validity cases don't match between the FK case and the check case - can you reorder them?

Yes!

[tyler314]

bors r+

[craig]

Build succeeded

• GitHub CI (Cockroach)