diff --git a/pkg/clusterversion/cockroach_versions.go b/pkg/clusterversion/cockroach_versions.go index b6694444a665..4a81bf259cab 100644 --- a/pkg/clusterversion/cockroach_versions.go +++ b/pkg/clusterversion/cockroach_versions.go @@ -197,16 +197,6 @@ const ( // options lagging_ranges_threshold and lagging_ranges_polling_interval. TODODelete_V23_2_ChangefeedLaggingRangesOpts - // *************************************************************************** - // WHERE TO ADD VERSION GATES DURING 23.2 STABILITY? - // --------------------------------------------------------------------------- - // If version gate is for 23.2 (to be backported to release-23.2): - // Then add new gate above this comment (immediately above this comment). - // If version gate is for 24.1 (upcoming 24.1 development): - // Then add new gate at the end (immediately above the "Add new versions - // here" comment). - // *************************************************************************** - // V23_2 is CockroachDB v23.2. It's used for all v23.2.x patch releases. V23_2 @@ -214,11 +204,6 @@ const ( // the process of upgrading from 23.2 to 24.1. V24_1Start - // ************************************************* - // Step (1) Add new versions here. - // Do not add new versions to a patch release. - // ************************************************* - // V24_1_DropPayloadAndProgressFromSystemJobsTable drop the unused payload and // progress columns from system.jobs table. V24_1_DropPayloadAndProgressFromSystemJobsTable @@ -268,6 +253,11 @@ const ( // the system tenant to ensure it is a superset of secondary tenants. V24_1_AddSpanCounts + // ************************************************* + // Step (1) Add new versions above this comment. + // Do not add new versions to a patch release. + // ************************************************* + numKeys ) @@ -308,11 +298,6 @@ var versionTable = [numKeys]roachpb.Version{ // v24.1 versions. Internal versions must be even. V24_1Start: {Major: 23, Minor: 2, Internal: 2}, - // ************************************************* - // Step (2): Add new versions here. - // Do not add new versions to a patch release. - // ************************************************* - V24_1_DropPayloadAndProgressFromSystemJobsTable: {Major: 23, Minor: 2, Internal: 4}, V24_1_MigrateOldStylePTSRecords: {Major: 23, Minor: 2, Internal: 6}, @@ -327,6 +312,11 @@ var versionTable = [numKeys]roachpb.Version{ V24_1_EstimatedMVCCStatsInSplit: {Major: 23, Minor: 2, Internal: 22}, V24_1_ReplicatedLockPipelining: {Major: 23, Minor: 2, Internal: 24}, V24_1_AddSpanCounts: {Major: 23, Minor: 2, Internal: 26}, + + // ************************************************* + // Step (2): Add new versions above this comment. + // Do not add new versions to a patch release. + // ************************************************* } // Latest is always the highest version key. This is the maximum logical cluster diff --git a/pkg/server/BUILD.bazel b/pkg/server/BUILD.bazel index 43389bfd00e3..952eda66c670 100644 --- a/pkg/server/BUILD.bazel +++ b/pkg/server/BUILD.bazel @@ -510,6 +510,7 @@ go_test( "//pkg/spanconfig", "//pkg/sql", "//pkg/sql/appstatspb", + "//pkg/sql/catalog/descs", "//pkg/sql/execinfrapb", "//pkg/sql/isql", "//pkg/sql/roleoption", diff --git a/pkg/server/migration_test.go b/pkg/server/migration_test.go index fed9bc6cbb43..d0ba12424672 100644 --- a/pkg/server/migration_test.go +++ b/pkg/server/migration_test.go @@ -19,11 +19,13 @@ import ( "github.com/cockroachdb/cockroach/pkg/base" "github.com/cockroachdb/cockroach/pkg/clusterversion" + "github.com/cockroachdb/cockroach/pkg/keys" "github.com/cockroachdb/cockroach/pkg/kv/kvserver" "github.com/cockroachdb/cockroach/pkg/kv/kvserver/kvstorage" "github.com/cockroachdb/cockroach/pkg/roachpb" "github.com/cockroachdb/cockroach/pkg/server/serverpb" "github.com/cockroachdb/cockroach/pkg/settings/cluster" + "github.com/cockroachdb/cockroach/pkg/sql/catalog/descs" "github.com/cockroachdb/cockroach/pkg/storage" "github.com/cockroachdb/cockroach/pkg/storage/fs" "github.com/cockroachdb/cockroach/pkg/testutils" @@ -32,6 +34,7 @@ import ( "github.com/cockroachdb/cockroach/pkg/upgrade/upgradebase" "github.com/cockroachdb/cockroach/pkg/util/leaktest" "github.com/cockroachdb/cockroach/pkg/util/log" + "github.com/cockroachdb/errors" "github.com/stretchr/testify/require" ) @@ -318,7 +321,9 @@ func TestMigrationPurgeOutdatedReplicas(t *testing.T) { // TestUpgradeHappensAfterMigration is a regression test to ensure that // upgrades run prior to attempting to upgrade the cluster to the current -// version. +// version. It will also verify that any migrations that modify the system +// database schema properly update the SystemDatabaseSchemaVersion on the +// system database descriptor. func TestUpgradeHappensAfterMigrations(t *testing.T) { defer leaktest.AfterTest(t)() defer log.Scope(t).Close(t) @@ -349,6 +354,25 @@ func TestUpgradeHappensAfterMigrations(t *testing.T) { }, }, }) + + internalDB := s.ApplicationLayer().InternalDB().(descs.DB) + err := internalDB.DescsTxn(ctx, func(ctx context.Context, txn descs.Txn) error { + systemDBDesc, err := txn.Descriptors().ByID(txn.KV()).Get().Database(ctx, keys.SystemDatabaseID) + if err != nil { + return err + } + systemDBVersion := systemDBDesc.DatabaseDesc().GetSystemDatabaseSchemaVersion() + if clusterversion.MinSupported.Version().Less(*systemDBVersion) { + // NB: When MinSupported is changed to 24_2, this check can change to + // an equality check. This is because 24.2 is the first release where + // https://github.com/cockroachdb/cockroach/issues/121914 has been + // resolved. + return errors.Newf("before upgrade, expected system database version (%v) to be less than or equal to clusterversion.MinSupported (%v)", *systemDBVersion, clusterversion.MinSupported.Version()) + } + return nil + }) + require.NoError(t, err) + close(automaticUpgrade) sr := sqlutils.MakeSQLRunner(db) @@ -359,5 +383,19 @@ func TestUpgradeHappensAfterMigrations(t *testing.T) { SELECT version = crdb_internal.node_executable_version() FROM [SHOW CLUSTER SETTING version]`, [][]string{{"true"}}) + + err = internalDB.DescsTxn(ctx, func(ctx context.Context, txn descs.Txn) error { + systemDBDesc, err := txn.Descriptors().ByID(txn.KV()).Get().Database(ctx, keys.SystemDatabaseID) + if err != nil { + return err + } + systemDBVersion := systemDBDesc.DatabaseDesc().GetSystemDatabaseSchemaVersion() + if !systemDBVersion.Equal(clusterversion.Latest.Version()) { + return errors.Newf("after upgrade, expected system database version (%v) to be equal to clusterversion.Latest (%v)", *systemDBVersion, clusterversion.Latest.Version()) + } + return nil + }) + require.NoError(t, err) + s.Stopper().Stop(context.Background()) } diff --git a/pkg/upgrade/upgrademanager/BUILD.bazel b/pkg/upgrade/upgrademanager/BUILD.bazel index 4925ea233bc4..accdcb005d87 100644 --- a/pkg/upgrade/upgrademanager/BUILD.bazel +++ b/pkg/upgrade/upgrademanager/BUILD.bazel @@ -18,6 +18,7 @@ go_library( "//pkg/server/settingswatcher", "//pkg/settings/cluster", "//pkg/sql", + "//pkg/sql/catalog/descs", "//pkg/sql/catalog/lease", "//pkg/sql/isql", "//pkg/sql/protoreflect", diff --git a/pkg/upgrade/upgrademanager/manager.go b/pkg/upgrade/upgrademanager/manager.go index 79d9f8f1b5b3..25e84e9c8a11 100644 --- a/pkg/upgrade/upgrademanager/manager.go +++ b/pkg/upgrade/upgrademanager/manager.go @@ -29,6 +29,7 @@ import ( "github.com/cockroachdb/cockroach/pkg/server/settingswatcher" "github.com/cockroachdb/cockroach/pkg/settings/cluster" "github.com/cockroachdb/cockroach/pkg/sql" + "github.com/cockroachdb/cockroach/pkg/sql/catalog/descs" "github.com/cockroachdb/cockroach/pkg/sql/catalog/lease" "github.com/cockroachdb/cockroach/pkg/sql/isql" "github.com/cockroachdb/cockroach/pkg/sql/protoreflect" @@ -522,6 +523,14 @@ func (m *Manager) Migrate( } } + // Bump the version of the system database schema if there was a migration + // or if this is the final version for a release. + if exists || (clusterVersion.Equal(clusterversion.Latest.Version()) && clusterVersion.IsFinal()) { + if err := bumpSystemDatabaseSchemaVersion(ctx, cv, m.deps.DB); err != nil { + return err + } + } + if m.knobs.InterlockPausePoint == upgradebase.AfterMigration { m.postToPauseChannelAndWaitForResume(ctx) } @@ -549,6 +558,31 @@ func (m *Manager) Migrate( return nil } +// bumpSystemDatabaseSchemaVersion bumps the SystemDatabaseSchemaVersion +// field for the system database descriptor. It is called after every upgrade +// step. +func bumpSystemDatabaseSchemaVersion( + ctx context.Context, cs clusterversion.ClusterVersion, descDB descs.DB, +) error { + return descDB.DescsTxn(ctx, func(ctx context.Context, txn descs.Txn) error { + systemDBDesc, err := txn.Descriptors().MutableByID(txn.KV()).Database(ctx, keys.SystemDatabaseID) + if err != nil { + return err + } + if sv := systemDBDesc.GetSystemDatabaseSchemaVersion(); sv != nil { + if cs.Version.Less(*sv) { + return errors.AssertionFailedf( + "new system schema version (%#v) is lower than previous system schema version (%#v)", + cs.Version, + *sv, + ) + } + } + systemDBDesc.SystemDatabaseSchemaVersion = &cs.Version + return txn.Descriptors().WriteDesc(ctx, false /* kvTrace */, systemDBDesc, txn.KV()) + }) +} + // bumpClusterVersion will invoke the BumpClusterVersion rpc on every node // until the cluster is stable. func bumpClusterVersion( diff --git a/pkg/upgrade/upgrades/schema_changes.go b/pkg/upgrade/upgrades/schema_changes.go index 97fd286aad3b..1b6061fc3ecd 100644 --- a/pkg/upgrade/upgrades/schema_changes.go +++ b/pkg/upgrade/upgrades/schema_changes.go @@ -21,7 +21,6 @@ import ( "github.com/cockroachdb/cockroach/pkg/sql/catalog/catpb" "github.com/cockroachdb/cockroach/pkg/sql/catalog/descpb" "github.com/cockroachdb/cockroach/pkg/sql/catalog/descs" - "github.com/cockroachdb/cockroach/pkg/sql/sem/catconstants" "github.com/cockroachdb/cockroach/pkg/sql/sessiondata" "github.com/cockroachdb/cockroach/pkg/upgrade" "github.com/cockroachdb/cockroach/pkg/util/log" @@ -412,28 +411,3 @@ func onlyHasColumnFamily( } return true, nil } - -// bumpSystemDatabaseSchemaVersion bumps the SystemDatabaseSchemaVersion -// field for the system database descriptor. It should be called at the end -// of any upgrade that creates or modifies the schema of any system table. -func bumpSystemDatabaseSchemaVersion( - ctx context.Context, cs clusterversion.ClusterVersion, d upgrade.TenantDeps, -) error { - return d.DB.DescsTxn(ctx, func(ctx context.Context, txn descs.Txn) error { - systemDBDesc, err := txn.Descriptors().MutableByName(txn.KV()).Database(ctx, catconstants.SystemDatabaseName) - if err != nil { - return err - } - if sv := systemDBDesc.GetSystemDatabaseSchemaVersion(); sv != nil { - if cs.Version.Less(*sv) { - return errors.AssertionFailedf( - "new system schema version (%#v) is lower than previous system schema version (%#v)", - cs.Version, - *sv, - ) - } - } - systemDBDesc.SystemDatabaseSchemaVersion = &cs.Version - return txn.Descriptors().WriteDesc(ctx, false /* kvTrace */, systemDBDesc, txn.KV()) - }) -} diff --git a/pkg/upgrade/upgrades/v24_1_add_span_counts.go b/pkg/upgrade/upgrades/v24_1_add_span_counts.go index eee2e0e73cd8..1e6c797ec4ca 100644 --- a/pkg/upgrade/upgrades/v24_1_add_span_counts.go +++ b/pkg/upgrade/upgrades/v24_1_add_span_counts.go @@ -28,5 +28,5 @@ func addSpanCountTable( if err := createSystemTable(ctx, d.DB, d.Settings, d.Codec, systemschema.SpanCountTable, tree.LocalityLevelTable); err != nil { return err } - return bumpSystemDatabaseSchemaVersion(ctx, cv, d) + return nil } diff --git a/pkg/upgrade/upgrades/v24_1_drop_payload_and_progress_jobs.go b/pkg/upgrade/upgrades/v24_1_drop_payload_and_progress_jobs.go index 94d128c08480..986f9713006b 100644 --- a/pkg/upgrade/upgrades/v24_1_drop_payload_and_progress_jobs.go +++ b/pkg/upgrade/upgrades/v24_1_drop_payload_and_progress_jobs.go @@ -45,5 +45,5 @@ func hidePayloadProgressFromSystemJobs( return err } - return bumpSystemDatabaseSchemaVersion(ctx, cv, deps) + return nil } diff --git a/pkg/upgrade/upgrades/v24_1_session_based_lease.go b/pkg/upgrade/upgrades/v24_1_session_based_lease.go index 4f163bc5078f..8ed717599390 100644 --- a/pkg/upgrade/upgrades/v24_1_session_based_lease.go +++ b/pkg/upgrade/upgrades/v24_1_session_based_lease.go @@ -149,5 +149,5 @@ func upgradeSystemLeasesDescriptor( }); err != nil { return err } - return bumpSystemDatabaseSchemaVersion(ctx, version, deps) + return nil } diff --git a/pkg/upgrade/upgrades/v24_1_system_database.go b/pkg/upgrade/upgrades/v24_1_system_database.go index f48342c40e56..169e42e0aa04 100644 --- a/pkg/upgrade/upgrades/v24_1_system_database.go +++ b/pkg/upgrade/upgrades/v24_1_system_database.go @@ -129,5 +129,5 @@ ALTER DATABASE system SURVIVE REGION FAILURE; } } - return bumpSystemDatabaseSchemaVersion(ctx, cv, deps) + return nil }