sql: allow for pre-19.2 foreign keys in table validation

[thoszhang]

sql: allow for pre-19.2 foreign keys in table validation

We introduced a bug in v20.2 where we failed to upgrade referenced
descriptors' FK representations from pre-19.2 descriptors when
validating cross-references for tables, leading to validation failures
that would make the table unusable. This PR gets rid of the validation
errors by having Validate() account for pre-19.2-style foreign keys on
referenced table descriptors.

Fixes #57032.

Release note (bug fix): Fixes a bug where tables and metadata were
unavailable due to spurious missing fk back reference validation
errors.

---

catalogkv: add testing of descriptor unwrapping and validation

This commit adds a framework to test that descriptors validate properly.
It also adds testdata and a bash script to generate that test data.

This test is to exercise a problematic path whereby tables constructed
in v19.1 would fail to validate in v20.2.

Release note: None

[cockroach-teamcity]

This change is 

[ajwerner]

I might extract some helpers and do some more line wrapping but that' extremely nitpicky.

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

---

pkg/sql/catalog/catalogkv/unwrap_validation_test.go, line 16 at r2 (raw file):

	"context"
	"encoding/hex"
	"errors"

Is the linter going to complain about this?

---

pkg/sql/catalog/tabledesc/structured.go, line 1648 at r1 (raw file):

					"missing index %d on %q from pre-19.2 foreign key forward reference %q on %q",
					fk.Index, desc.Name, backref.Name, originTable.GetName(),

Would we be happier if these errors had the entire descriptor? The names are going to disappear in redaction. Same for all the other errors around here.

[jordanlewis]

Looks good. If it's not too much trouble, I would like to see a unit test in the first commit that validates a hand-constructed descriptor pair of this type as well, so it's as easy as possible to see the behavior of this commit, and so we don't accidentally "lose" this test if the datadriven approach changes over time.

If this is silly, feel free to push back.

Overall, I'm confident in the small footprint of this change.

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

---

pkg/sql/catalog/tabledesc/structured.go, line 1544 at r1 (raw file):

		// somewhat parallels the logic in maybeUpgradeForeignKeyRepOnIndex.
		unupgradedFKsPresent := false
		if err := referencedTable.ForeachIndex(catalog.IndexOpts{}, func(referencedIdx *descpb.IndexDescriptor, isPrimary bool) error {

Should we add a if found == true breakout from this section to, to parallel the one in the second section?

[thoszhang]

I added some testing for (1) auto-created referencing-side indexes in 19.2, (2) extra indexes on the referencing side that overlap with the one that's designated for the FK, and (3) the case where we switch out the referencing-side index from the original pre-19.2 one (just sort of a special case in the upgrade path which I don't expect to cause problems).

[thoszhang]

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

---

pkg/sql/catalog/catalogkv/unwrap_validation_test.go, line 16 at r2 (raw file):

Previously, ajwerner wrote…

Is the linter going to complain about this?

Good point, fixed.

---

pkg/sql/catalog/tabledesc/structured.go, line 1544 at r1 (raw file):

Previously, jordanlewis (Jordan Lewis) wrote…

Should we add a if found == true breakout from this section to, to parallel the one in the second section?

Oh yeah, done.

---

pkg/sql/catalog/tabledesc/structured.go, line 1648 at r1 (raw file):

Previously, ajwerner wrote…

					"missing index %d on %q from pre-19.2 foreign key forward reference %q on %q",
					fk.Index, desc.Name, backref.Name, originTable.GetName(),

Would we be happier if these errors had the entire descriptor? The names are going to disappear in redaction. Same for all the other errors around here.

Yeah, I think so. How do you think we should format this log line (given that the table descriptor is some large thing with newlines)?

[thoszhang]

If it's not too much trouble, I would like to see a unit test in the first commit that validates a hand-constructed descriptor pair of this type as well, so it's as easy as possible to see the behavior of this commit, and so we don't accidentally "lose" this test if the datadriven approach changes over time.

I think that's reasonable. We can add it to TestValidateCrossTableReferences.

[ajwerner]

Reviewed 3 of 3 files at r3.
Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @ajwerner, @jordanlewis, and @lucy-zhang)

---

pkg/sql/catalog/tabledesc/structured.go, line 1648 at r1 (raw file):

Previously, lucy-zhang (Lucy Zhang) wrote…

Yeah, I think so. How do you think we should format this log line (given that the table descriptor is some large thing with newlines)?

for better or for worse, it doesn't have newlines.

[thoszhang]

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

---

pkg/sql/catalog/tabledesc/structured.go, line 1648 at r1 (raw file):

Previously, ajwerner wrote…

for better or for worse, it doesn't have newlines.

Feel free to just do something reasonable here and push it.

[ajwerner]

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

---

pkg/sql/catalog/tabledesc/structured.go, line 1648 at r1 (raw file):

Previously, lucy-zhang (Lucy Zhang) wrote…

Feel free to just do something reasonable here and push it.

I'm skipping for now because it breaks a bunch of tests because we don't actually have valid descriptors and the formatting assumes we do (maybe that's a mistake). Will deal with it later in a more generic way.

[thoszhang]

@jordanlewis do you want to take another look at the tests?

[jordanlewis]

Do we have a test that checks what happens if neither descriptor had been upgraded? I see 2, which I think are both the 1-side case: one of the descriptors was upgraded, but not the other.

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

[thoszhang]

We're assuming (both in code and in the tests) that once the descriptors make their way into validateCrossReferences, the descriptor whose descriptor is being validated is always upgraded. The problem is just with the referenced descriptors.

[ajwerner]

Do we have a test that checks what happens if neither descriptor had been upgraded? I see 2, which I think are both the 1-side case: one of the descriptors was upgraded, but not the other.

We always upgrade the current descriptor before attempting to read the other side. This test just tests validating the other side. We do exercise the specific scenario you're bringing up in the datadriven test. I can try to finagle one that runs the whole validation logic on both but it's going to be annoying.

[jordanlewis]

No problem, I'm satisfied if you are.

[thoszhang]

bors r+

[craig]

Build succeeded:

• GitHub CI (Cockroach)