catalog: add SystemDatabaseSchemaVersion field to DatabaseDescriptor

[andyyang890]

catalog: add SystemDatabaseSchemaVersion field to DatabaseDescriptor

This patch adds a new SystemDatabaseSchemaVersion field to
descpb.DatabaseDescriptor that will be used to store the schema
version of the system database.

Release note: None

---

upgrades: add helper function to bump SystemDatabaseSchemaVersion

This patch adds a helper function that should be used by upgrades that
create or modify the schema of system tables to bump the schema version
field on the system database descriptor.

Release note: None

---

Fixes #109039

[cockroach-teamcity]

This change is 

[fqazi]

Just some minor things. Great work, Andy!

Reviewed 5 of 10 files at r1, 12 of 12 files at r3, 7 of 7 files at r4, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @andyyang890, @knz, and @rafiss)

---

pkg/sql/catalog/systemschema/system.go line 1059 at r3 (raw file):

// that should be used during bootstrap. It should be bumped up alongside any
// upgrade that creates or modifies the schema of a system table.
var SystemDatabaseSchemaBootstrapVersion = clusterversion.ByKey(clusterversion.V23_2_RegionaLivenessTable)

Should we add something to the bootstrap test so that we make sure people bump this up?

---

pkg/upgrade/upgrades/schema_changes.go line 428 at r4 (raw file):

		}
		if sv := systemDBDesc.GetSystemDatabaseSchemaVersion(); sv != nil {
			if cs.Version.Less(*sv) {

I think the validation layer will also catch it right before we write. So this might be redundant?

[andyyang890]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @fqazi, @knz, and @rafiss)

---

pkg/sql/catalog/systemschema/system.go line 1059 at r3 (raw file):

Previously, fqazi (Faizan Qazi) wrote…

Should we add something to the bootstrap test so that we make sure people bump this up?

Added a new test to check this!

---

pkg/upgrade/upgrades/schema_changes.go line 428 at r4 (raw file):

Previously, fqazi (Faizan Qazi) wrote…

I think the validation layer will also catch it right before we write. So this might be redundant?

We discussed this offline already but just noting it here for posterity, the check in ValidateSelf is checking that we're setting it to a valid version relative to BinaryMinSupportedVersion and BinaryVersion while this check is making sure we can't decrease the version.

We also discussed moving that check to ValidateSelf but since that operates only on an *dbdesc.immutable (not an *dbdesc.Mutable which has access to the previous descriptor), we decided that was too complex and unnecessary for now.

[fqazi]

Reviewed 9 of 9 files at r5, 7 of 7 files at r6, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @andyyang890, @knz, and @rafiss)

---

pkg/sql/catalog/systemschema/system.go line 1059 at r3 (raw file):

Previously, andyyang890 (Andy Yang) wrote…

Added a new test to check this!

Great work 😊

[andyyang890]

TFTRs!

bors r=dt,fqazi

[craig]

Build succeeded:

• Bazel Essential CI (Cockroach)