schemachanger: implement ALTER TABLE .. VALIDATE CONSTRAINT ..

[Xiang-Gu]

The main idea is to drop the unvalidated element and add a vanilla counterpart in the builder state.

Commit 2 involves some changes to allow both legacy and declarative schema changer handle
this stmt correctly: ALTER TABLE .. ADD/DROP CONSTRAINT .. [NOT VALID]; VALIDATE CONSTRAINT ..;
so this PR warrants a release note.

Epic: None
Release note (bug fix): Allow ALTER TABLE .. ADD/DROP CONSTRAINT .. [NOT VALID]; VALIDATE CONSTRAINT ..; to behave consistently with Postgres. Previously, the VALIDATE CONSTRAINT part will fail and cause the whole statement to fail.

[cockroach-teamcity]

This change is 

[Xiang-Gu]

This is ready for a look!

[ajwerner]

Reviewed 15 of 15 files at r1, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @rytaft and @Xiang-Gu)

---

pkg/sql/schemachanger/scbuild/internal/scbuildstmt/alter_table_validate_constraint.go line 44 at r1 (raw file):

	}
	if retrieveUniqueWithoutIndexConstraintElem(b, tbl.TableID, constraintID) != nil {
		return

Consider factoring out a helper here called getConstraintElementByID. Also, is there an issue if the constraint had been dropped? Like imagine you dropped the constraint and then validated it? Would you find it?

Code quote:

	if retrieveCheckConstraintElem(b, tbl.TableID, constraintID) != nil {
		return
	}
	if retrieveUniqueWithoutIndexConstraintElem(b, tbl.TableID, constraintID) != nil {
		return
	}
	if retrieveForeignKeyConstraintElem(b, tbl.TableID, constraintID) != nil {
		return
	}

---

pkg/sql/schemachanger/scbuild/internal/scbuildstmt/alter_table_validate_constraint.go line 57 at r1 (raw file):

		uwiNotValidElem:    retrieveUniqueWithoutIndexConstraintUnvalidatedElem(b, tbl.TableID, constraintID),
		fkNotValidElem:     retrieveForeignKeyConstraintUnvalidatedElem(b, tbl.TableID, constraintID),
	}

nit: any reason to not just construct the spec inline? I feel like it's slightly more pleasant to not introduce the intermediate variable.

Code quote:

	spec := validateConstraintSpec{
		constraintNameElem: constraintNameElem,
		ckNotValidElem:     retrieveCheckConstraintUnvalidatedElem(b, tbl.TableID, constraintID),
		uwiNotValidElem:    retrieveUniqueWithoutIndexConstraintUnvalidatedElem(b, tbl.TableID, constraintID),
		fkNotValidElem:     retrieveForeignKeyConstraintUnvalidatedElem(b, tbl.TableID, constraintID),
	}

---

pkg/sql/schemachanger/scbuild/internal/scbuildstmt/alter_table_validate_constraint.go line 61 at r1 (raw file):

}

func retrieveCheckConstraintElem(

These smell, but I don't have anything better for you right now. Just a passing thought.

[Xiang-Gu]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner and @rytaft)

---

pkg/sql/schemachanger/scbuild/internal/scbuildstmt/alter_table_validate_constraint.go line 44 at r1 (raw file):

Previously, ajwerner wrote…

Consider factoring out a helper here called getConstraintElementByID. Also, is there an issue if the constraint had been dropped? Like imagine you dropped the constraint and then validated it? Would you find it?

1. Done. I instead created a helper where we determine whether or not we should validate the constraint.

• if the constraint had just been dropped: the constraint name will be set to a placeholder and we will return an error from resolveConstraint, consistent with what the legacy schema changer does.
• if the constraint had just been added: the logic will determine that this is something we should validate and thus it can proceed.

Code quote:

if retrieveUniqueWithoutIndexConstraintElem(b, tbl.TableID, constraintID) != nil {

---

pkg/sql/schemachanger/scbuild/internal/scbuildstmt/alter_table_validate_constraint.go line 57 at r1 (raw file):

Previously, ajwerner wrote…

nit: any reason to not just construct the spec inline? I feel like it's slightly more pleasant to not introduce the intermediate variable.

nothing particularly special: I just feel like the resulting function signature doesn't look pretty
validateConstraint(b, tableID, constraintID, constraintNameElem)

[ajwerner]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @rytaft and @Xiang-Gu)

---

pkg/sql/schemachanger/scbuild/internal/scbuildstmt/alter_table_validate_constraint.go line 44 at r1 (raw file):

Previously, Xiang-Gu (Xiang Gu) wrote…

1. Done. I instead created a helper where we determine whether or not we should validate the constraint.

• if the constraint had just been dropped: the constraint name will be set to a placeholder and we will return an error from resolveConstraint, consistent with what the legacy schema changer does.
• if the constraint had just been added: the logic will determine that this is something we should validate and thus it can proceed.

When you say the constraint name will be set to a placeholder, where does that happen?

---

pkg/sql/schemachanger/scbuild/internal/scbuildstmt/alter_table_validate_constraint.go line 57 at r1 (raw file):

Previously, Xiang-Gu (Xiang Gu) wrote…

nothing particularly special: I just feel like the resulting function signature doesn't look pretty
validateConstraint(b, tableID, constraintID, constraintNameElem)

I was just suggesting:

validateConstraint(b, tbl.TableID, validateConstraintSpec{
	constraintNameElem: constraintNameElem,
	ckNotValidElem:     retrieveCheckConstraintUnvalidatedElem(b, tbl.TableID, constraintID),
	uwiNotValidElem:    retrieveUniqueWithoutIndexConstraintUnvalidatedElem(b, tbl.TableID, constraintID),
	fkNotValidElem:     retrieveForeignKeyConstraintUnvalidatedElem(b, tbl.TableID, constraintID),
})

[Xiang-Gu]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner and @rytaft)

---

pkg/sql/schemachanger/scbuild/internal/scbuildstmt/alter_table_validate_constraint.go line 44 at r1 (raw file):

Previously, ajwerner wrote…

When you say the constraint name will be set to a placeholder, where does that happen?

In the statement phase of that DROP CONSTRAINT, that not-valid constraint will be dropped as well as its name.

---

pkg/sql/schemachanger/scbuild/internal/scbuildstmt/alter_table_validate_constraint.go line 57 at r1 (raw file):

Previously, ajwerner wrote…

I was just suggesting:

validateConstraint(b, tbl.TableID, validateConstraintSpec{
	constraintNameElem: constraintNameElem,
	ckNotValidElem:     retrieveCheckConstraintUnvalidatedElem(b, tbl.TableID, constraintID),
	uwiNotValidElem:    retrieveUniqueWithoutIndexConstraintUnvalidatedElem(b, tbl.TableID, constraintID),
	fkNotValidElem:     retrieveForeignKeyConstraintUnvalidatedElem(b, tbl.TableID, constraintID),
})

ah I see . done

[ajwerner]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @rytaft and @Xiang-Gu)

---

pkg/sql/schemachanger/scbuild/internal/scbuildstmt/alter_table_validate_constraint.go line 44 at r1 (raw file):

Previously, Xiang-Gu (Xiang Gu) wrote…

In the statement phase of that DROP CONSTRAINT, that not-valid constraint will be dropped as well as its name.

So what happens if you do:

CREATE TABLE t (i INT PRIMARY KEY);
ALTER TABLE t ADD CONSTRAINT c CHECK (i > 0) NOT VALID;

and

ALTER TABLE t DROP CONSTRAINT c, VALIDATE CONSTRAINT c;

Or:

ALTER TABLE t DROP CONSTRAINT c, ADD CONSTRAINT c CHECK (i > 1) NOT VALID, VALIDATE CONSTRAINT c;

Does the first case error and the second case validate the newly added check constraint?

[Xiang-Gu]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner and @rytaft)

---

pkg/sql/schemachanger/scbuild/internal/scbuildstmt/alter_table_validate_constraint.go line 44 at r1 (raw file):

Previously, ajwerner wrote…

So what happens if you do:

CREATE TABLE t (i INT PRIMARY KEY);
ALTER TABLE t ADD CONSTRAINT c CHECK (i > 0) NOT VALID;

and

ALTER TABLE t DROP CONSTRAINT c, VALIDATE CONSTRAINT c;

Or:

ALTER TABLE t DROP CONSTRAINT c, ADD CONSTRAINT c CHECK (i > 1) NOT VALID, VALIDATE CONSTRAINT c;

Does the first case error and the second case validate the newly added check constraint?

Yes, I have ensured that new schema changer handles both cases as expected.

I also tried to make legacy schema changer handles the first case correctly, but it's not easy to get the second case work properly (see #96831)

[chengxiong-ruan]

Nice work. just one request on test and a question on constraint ID increments.

[chengxiong-ruan]

could you add some test that the validation fails after inserting some bad data with i <= 0?

[Xiang-Gu]

TFTR!

bors r+

[craig]

Build succeeded:

• Bazel Essential CI (Cockroach)