migrations: Fix migration discrepancy on a system table.

[Xiang-Gu]

Previously, the migration code for system table
statement_diagnostics_requests results in discrepancies when compared
to the table definition after bootstrapping. The discrepancies are
summarized in #77980.

This PR fixes it to ensure this table has exactly the same definition
after cluster upgrading as that from bootstrapping.

Release note: None

Fixes: #77980

Sister Issue: #78302

Related PR that discovers this discrepancy: #77970

[cockroach-teamcity]

This change is 

[ajwerner]

Wonderful job catching this and great job fixing it!

Can you extend TestAlterSystemStmtDiagReqs to also check that the index no longer exists and the families look correct?
Can you associate the issue with the roachtest you're writing in some form?

mod the testing requests and commit message feedback.

I added the backport-22.1 label which will result in a backport being created automatically once this merges.

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

---

-- commits, line 13 at r1:
I don't think we need a release note because we're fixing a bug which was not shipped in a release.

Code quote:

  Release note (bug fix): Ensure the migration logic for
  `system.statement_diagnostics_requests` results in a table definition
  that is identical to that from bootstrapping.

---

-- commits, line 16 at r1:
The backport to 22.1 will fix this issue. I find it can be problematic to close the issue before the backport.

[yuzefovich]

Thanks for catching this!

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

---

-- commits, line 16 at r1:

Previously, ajwerner wrote…

The backport to 22.1 will fix this issue. I find it can be problematic to close the issue before the backport.

My preferred way to go about it is to not reference the issue in the commit message, but add the necessary details to the PR description only. This way you can add the Fixes #issue-number part to the relevant PR (the one against 22.1 branch in this case). An additional benefit to the cleanliness is that the issue itself won't be mentioned every time you amend the commit and force-push the branch.

[Xiang-Gu]

Thanks. I've extended the test to check that the intended-to-delete index is indeed deleted. The families check is already included in the primary family check.

Also, I'll associate this pr with the roachtest I'm working on.

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

---

-- commits, line 13 at r1:

Previously, ajwerner wrote…

I don't think we need a release note because we're fixing a bug which was not shipped in a release.

Done.

---

-- commits, line 16 at r1:

Previously, yuzefovich (Yahor Yuzefovich) wrote…

My preferred way to go about it is to not reference the issue in the commit message, but add the necessary details to the PR description only. This way you can add the Fixes #issue-number part to the relevant PR (the one against 22.1 branch in this case). An additional benefit to the cleanliness is that the issue itself won't be mentioned every time you amend the commit and force-push the branch.

Thanks for the suggestion. I'll remove the issue reference from commit message but add them back in the PR.

[ajwerner]

once you defeat the linter

Reviewed 2 of 3 files at r2, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @ajwerner, @Xiang-Gu, and @yuzefovich)

---

pkg/migration/migrations/schema_changes.go, line 260 at r2 (raw file):

	} else {
		return false, nil
	}

the linter is mad at this, how about writing this like:

idx, _ := storedTable.FindIndexWithName(indexName)
return idx == nil, nil

Code quote:

	if _, err := storedTable.FindIndexWithName(indexName); err != nil {
		return true, nil
	} else {
		return false, nil
	}

[Xiang-Gu]

Done. Thanks

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

---

pkg/migration/migrations/schema_changes.go, line 260 at r2 (raw file):

Previously, ajwerner wrote…

the linter is mad at this, how about writing this like:

idx, _ := storedTable.FindIndexWithName(indexName)
return idx == nil, nil

Done

[Xiang-Gu]

bors r+

[craig]

Build succeeded:

• GitHub CI (Cockroach)

[blathers-crl]

Encountered an error creating backports. Some common things that can go wrong:

1. The backport branch might have already existed.
2. There was a merge conflict.
3. The backport branch contained merge commits.

You might need to create your backport manually using the backport tool.

---

error creating merge commit from 63fa741 to blathers/backport-release-22.1-78679: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict []

you may need to manually resolve merge conflicts with the backport tool.

Backport to branch 22.1.x failed. See errors above.

---

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan.}