sql: add doctor check for parent schema in parent db

[postamar]

With this patch, doctor now also checks that the parent id of an object
matches the parent id of the parent schema of the object.

Fixes #55716.

Release note: None

[cockroach-teamcity]

This change is 

[postamar]

For good measure I checked out the commit prior to the merge commit #55723, cherry-picked this commit, successfully reproduced the problem in the issue and checked that doctor ran the new check correctly:

Examining 36 descriptors and 40 namespace entries...
   Table  55: ParentID  52, ParentSchemaID 54, Name 'bar': invalid parent schema parent id 50
Examining 0 running jobs...
ERROR: validation failed
Failed running "debug doctor cluster"

[postamar]

CI failed on what appears to be a flaky test, which already has an issue open: #58624 . I don't believe the failure is related to my changes.

[ajwerner]

Thanks for picking this up

Reviewed 2 of 2 files at r1.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @postamar)

---

pkg/sql/doctor/doctor.go, line 177 at r1 (raw file):

			} else if !invalidParentID && parentSchema.GetParentID() != desc.GetParentID() {
				problemsFound = true
				fmt.Fprint(stdout, reportMsg(desc, "invalid parent schema parent id %d", parentSchema.GetParentID()))

I think this message can be clearer and can have a test. Namely, it'd be interesting to know both parent id values.

[postamar]

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

---

pkg/sql/doctor/doctor.go, line 177 at r1 (raw file):

Previously, ajwerner wrote…

I think this message can be clearer and can have a test. Namely, it'd be interesting to know both parent id values.

Perhaps I'm misunderstanding but it is tested and the other parent id values already get reported in the prefix of the error message. See the test case I added for an example of this.

[postamar]

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

---

pkg/sql/doctor/doctor.go, line 177 at r1 (raw file):

Previously, postamar (Marius Posta) wrote…

Perhaps I'm misunderstanding but it is tested and the other parent id values already get reported in the prefix of the error message. See the test case I added for an example of this.

I changed the message to something that should be less confusing.

[ajwerner]

Reviewed 1 of 2 files at r2.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner and @postamar)

---

pkg/sql/doctor/doctor.go, line 174 at r2 (raw file):

			if !parentSchemaExists {
				problemsFound = true
				fmt.Fprint(stdout, reportMsg(desc, "invalid parent schema id %d", desc.GetParentSchemaID()))

err, then this case is not tested then, right?

[postamar]

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

---

pkg/sql/doctor/doctor.go, line 174 at r2 (raw file):

Previously, ajwerner wrote…

err, then this case is not tested then, right?

This case was already there and only shows up in the diff because I broke down the if clause into an if-else statement, so I didn't check if it was covered. I'll add a test for good measure.

[ajwerner]

Reviewed 1 of 2 files at r2.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @postamar)

---

pkg/sql/doctor/doctor.go, line 174 at r2 (raw file):

Previously, postamar (Marius Posta) wrote…

This case was already there and only shows up in the diff because I broke down the if clause into an if-else statement, so I didn't check if it was covered. I'll add a test for good measure.

gotcha, thanks

[postamar]

bors r+

[craig]

Build failed:

• GitHub CI (Cockroach)

[postamar]

bors r+

Flaky test fix has been merged, trying again.

[craig]

Build succeeded:

• GitHub CI (Cockroach)