sql: fix FK validation join implementation

[thoszhang]

This PR updates the SQL query used for VALIDATE CONSTRAINT for foreign keys.
The new implementation is compatible with the recent changes to FK matching
semantics (both MATCH FULL and MATCH SIMPLE). It also uses a merge join, which
will improve performance significantly compared to the old hash join
implementation.

Release note (sql change): VALIDATE CONSTRAINT for foreign keys is now
compatible with the new MATCH FULL and MATCH SIMPLE semantics, and is more
performant.

Fixes #33452

[cockroach-teamcity]

This change is 

[knz]

Thank you for doing this! I will let David and Bram comment on the correctness, but a thought: do you think there's a way to prepare the logical plan for the validation query during the planning phase, so that it would show up in the output of EXPLAIN?

[thoszhang]

@knz probably. But we're planning to move the validation query out of the user transaction and have it happen asynchronously (by adding a mutation and moving it through the schema changer), at which point AFAIK it wouldn't be possible. We probably ultimately also want to implement a backfiller instead of using SQL to do this validation as well.

(The future changes I'm describing are implemented for check constraints in #34301)

[knz]

Thank you. If done asynchronously (in a job I assume) but we're careful to use standard internal executor machinery, the user can still go and inspect the plan using the statement stats page (this shows logical plans now). So that UX interest will be satisifed this way.

[vivekmenezes]

Nice work on this. You should probably in a followup PR add this constraint validation to a roachtest that already contains the FK relationship. Thanks!

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

---

pkg/sql/check.go, line 77 at r1 (raw file):

}

func matchFullUnacceptableKeyQuery(

Can you add a comment about what you're trying to do here?

---

pkg/sql/check.go, line 149 at r1 (raw file):

	}

	if srcIdx.ForeignKey.Match == sqlbase.ForeignKeyReference_FULL {

Can you add a comment here about why you need to run two queries in order to validate the constraint: the one below and the default one. Thanks!

---

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

ALTER TABLE b VALIDATE CONSTRAINT fk_ref

statement ok

Great tests!

[thoszhang]

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

---

pkg/sql/check.go, line 77 at r1 (raw file):

Previously, vivekmenezes wrote…

Can you add a comment about what you're trying to do here?

Done.

---

pkg/sql/check.go, line 149 at r1 (raw file):

Previously, vivekmenezes wrote…

Can you add a comment here about why you need to run two queries in order to validate the constraint: the one below and the default one. Thanks!

Done.

[vivekmenezes]

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

[BramGruneir]

This is great! I'm excited we're finally going to be able to do this.

I've left some comments.

Reviewed 1 of 2 files at r1, 1 of 1 files at r2.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @lucy-zhang)

---

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

	return fmt.Sprintf(
		`SELECT %s FROM (SELECT * FROM %s@%s WHERE %s) AS s LEFT OUTER JOIN %s@%s AS t ON %s WHERE %s IS NULL LIMIT 1`,

It would be really great to see a comment with an example or two of how this will look. With all the placeholders, it's hard to mentally ensure it looks correct. (and the same for the query from matchFullUnacceptableKeyQuery)

For example, what's the point of the AS s and AS t if the alias is not used?

---

pkg/sql/logictest/testdata/logic_test/fk, line 1382 at r2 (raw file):

# Add the constraint separately so that it's unvalidated, so we can test VALIDATE CONSTRAINT.
statement ok

Instead of just updating the one test, could you make this two separate tests?

Also, if you could use subtests, it would make this a lot more readable.

---

pkg/sql/logictest/testdata/logic_test/fk, line 1588 at r2 (raw file):

);

# Add the constraint separately so that it's unvalidated, so we can test VALIDATE CONSTRAINT.

Again, please just duplicate the test, once with validation and once without. We need to test both scenarios. Pretty easy to do with the subtests.

[thoszhang]

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

---

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

Previously, BramGruneir (Bram Gruneir) wrote…

It would be really great to see a comment with an example or two of how this will look. With all the placeholders, it's hard to mentally ensure it looks correct. (and the same for the query from matchFullUnacceptableKeyQuery)

For example, what's the point of the AS s and AS t if the alias is not used?

Done. (The aliases get used in the WHERE and ON clauses but it's not the easiest to see... I added a comment)

---

pkg/sql/logictest/testdata/logic_test/fk, line 1382 at r2 (raw file):

Previously, BramGruneir (Bram Gruneir) wrote…

Instead of just updating the one test, could you make this two separate tests?

Also, if you could use subtests, it would make this a lot more readable.

Done.

---

pkg/sql/logictest/testdata/logic_test/fk, line 1588 at r2 (raw file):

Previously, BramGruneir (Bram Gruneir) wrote…

Again, please just duplicate the test, once with validation and once without. We need to test both scenarios. Pretty easy to do with the subtests.

Done.

[BramGruneir]

Just a quick comment on the MATCH FULL check.

Reviewed 2 of 2 files at r3.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @lucy-zhang)

---

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

Previously, lucy-zhang (Lucy Zhang) wrote…

Done. (The aliases get used in the WHERE and ON clauses but it's not the easiest to see... I added a comment)

Ah, I can see it now. Thanks!

---

pkg/sql/check.go, line 87 at r3 (raw file):

// SELECT * FROM child@c_idx
// WHERE
//   NOT (a_id IS NULL AND b_id IS NULL OR a_id IS NOT NULL AND b_id IS NOT NULL)

This is a little convoluted.

For the null case, you can use COALESCE(col1, col2, col3) IS NULL

And could you add some parentheses to make this a little earlier to read? Relying on the order of operations is a little tough.

[thoszhang]

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

---

pkg/sql/check.go, line 87 at r3 (raw file):

Previously, BramGruneir (Bram Gruneir) wrote…

This is a little convoluted.

For the null case, you can use COALESCE(col1, col2, col3) IS NULL

And could you add some parentheses to make this a little earlier to read? Relying on the order of operations is a little tough.

Done.

[BramGruneir]

Reviewed 2 of 2 files at r4.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @lucy-zhang)

[thoszhang]

bors r+

[craig]

Build succeeded

• GitHub CI (Cockroach)