From 614ce3eb3903f5a1e6465b91531c33e2b7afdb21 Mon Sep 17 00:00:00 2001 From: Rafi Shamim Date: Thu, 25 Apr 2024 15:50:22 -0400 Subject: [PATCH] upgrade: automatically bump the system DB version Rather than making authors of an upgrade remember that they should bump the version of the system DB schema, now it will be bumped automatically after any upgrade step that has a migration or if upgrading to the final cluster version for a release. Two tests are added: - TestSystemDatabaseSchemaBootstrapVersionBumped makes sure the SystemDatabaseSchemaBootstrapVersion is incremented whenever clusterversion.Latest changes AND if that version has an upgrade migration or is the final version in a release. - TestUpgradeHappensAfterMigrations is augmented to check that after all upgrades run, the actual system database schema version is equal to the SystemDatabaseSchemaBootstrapVersion. Taken together, these tests ensure that the system database bootstrap version is manually incremented in the code when it should be, and that the automatic upgrade process correctly increments the version. This change results in more work, since it means that the version will be bumped when it is not strictly necessary to do so. But since it makes it harder to forget a manual step, it seems worth doing. Release note: None --- pkg/clusterversion/cockroach_versions.go | 30 ++++-------- pkg/server/BUILD.bazel | 2 + pkg/server/migration_test.go | 48 ++++++++++++++++++- pkg/sql/catalog/bootstrap/BUILD.bazel | 1 - pkg/sql/catalog/bootstrap/bootstrap_test.go | 28 ----------- pkg/sql/catalog/bootstrap/testdata/testdata | 8 ++-- pkg/sql/catalog/systemschema/system.go | 3 +- .../testdata/bootstrap_system | 2 +- .../testdata/bootstrap_tenant | 2 +- .../testdata/logic_test/crdb_internal_catalog | 2 +- pkg/upgrade/BUILD.bazel | 1 + pkg/upgrade/helpers.go | 35 ++++++++++++++ pkg/upgrade/upgradejob/upgrade_job.go | 8 ++++ pkg/upgrade/upgrademanager/manager.go | 25 ++++++++-- pkg/upgrade/upgrades/main_test.go | 2 + .../upgrades/permanent_sql_stats_ttl_test.go | 2 +- ...rmanent_system_activity_update_job_test.go | 2 +- pkg/upgrade/upgrades/schema_changes.go | 26 ---------- .../upgrades/schema_changes_external_test.go | 2 + pkg/upgrade/upgrades/upgrades_test.go | 21 ++++++++ pkg/upgrade/upgrades/v24_1_add_span_counts.go | 2 +- .../upgrades/v24_1_add_span_counts_test.go | 3 +- .../v24_1_drop_payload_and_progress_jobs.go | 2 +- .../upgrades/v24_1_session_based_lease.go | 2 +- pkg/upgrade/upgrades/v24_1_system_database.go | 2 +- 25 files changed, 166 insertions(+), 95 deletions(-) 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..c873e1c8e6d7 100644 --- a/pkg/server/BUILD.bazel +++ b/pkg/server/BUILD.bazel @@ -510,6 +510,8 @@ go_test( "//pkg/spanconfig", "//pkg/sql", "//pkg/sql/appstatspb", + "//pkg/sql/catalog/descs", + "//pkg/sql/catalog/systemschema", "//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..5a48e9157d6b 100644 --- a/pkg/server/migration_test.go +++ b/pkg/server/migration_test.go @@ -19,11 +19,14 @@ 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/sql/catalog/systemschema" "github.com/cockroachdb/cockroach/pkg/storage" "github.com/cockroachdb/cockroach/pkg/storage/fs" "github.com/cockroachdb/cockroach/pkg/testutils" @@ -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,27 @@ 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() + // 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. + require.False( + t, clusterversion.MinSupported.Version().Less(*systemDBVersion), + "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 +385,25 @@ func TestUpgradeHappensAfterMigrations(t *testing.T) { SELECT version = crdb_internal.node_executable_version() FROM [SHOW CLUSTER SETTING version]`, [][]string{{"true"}}) + + // After the upgrade, make sure the new system database version is equal to + // the SystemDatabaseSchemaBootstrapVersion. This serves two purposes: + // - reminder to bump SystemDatabaseSchemaBootstrapVersion. + // - ensure that upgrades have run and updated the system database version. + 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() + require.Equalf( + t, systemschema.SystemDatabaseSchemaBootstrapVersion, *systemDBVersion, + "after upgrade, expected system database version (%v) to be equal to systemschema.SystemDatabaseSchemaBootstrapVersion (%v)", + *systemDBVersion, systemschema.SystemDatabaseSchemaBootstrapVersion, + ) + return nil + }) + require.NoError(t, err) + s.Stopper().Stop(context.Background()) } diff --git a/pkg/sql/catalog/bootstrap/BUILD.bazel b/pkg/sql/catalog/bootstrap/BUILD.bazel index d014c7aca0f0..c28c3c2d9751 100644 --- a/pkg/sql/catalog/bootstrap/BUILD.bazel +++ b/pkg/sql/catalog/bootstrap/BUILD.bazel @@ -61,7 +61,6 @@ go_test( "//pkg/security/securityassets", "//pkg/security/securitytest", "//pkg/server", - "//pkg/sql/catalog/systemschema", "//pkg/testutils", "//pkg/testutils/datapathutils", "//pkg/testutils/serverutils", diff --git a/pkg/sql/catalog/bootstrap/bootstrap_test.go b/pkg/sql/catalog/bootstrap/bootstrap_test.go index 1baf3ac3b166..cf1d80f40c18 100644 --- a/pkg/sql/catalog/bootstrap/bootstrap_test.go +++ b/pkg/sql/catalog/bootstrap/bootstrap_test.go @@ -17,7 +17,6 @@ import ( "github.com/cockroachdb/cockroach/pkg/config/zonepb" "github.com/cockroachdb/cockroach/pkg/keys" "github.com/cockroachdb/cockroach/pkg/roachpb" - "github.com/cockroachdb/cockroach/pkg/sql/catalog/systemschema" "github.com/cockroachdb/cockroach/pkg/testutils/datapathutils" "github.com/cockroachdb/cockroach/pkg/util/leaktest" "github.com/cockroachdb/cockroach/pkg/util/log" @@ -140,30 +139,3 @@ func makeMetadataSchema(tenantID uint64) MetadataSchema { } return MakeMetadataSchema(codec, zonepb.DefaultZoneConfigRef(), zonepb.DefaultSystemZoneConfigRef()) } - -// TestSystemDatabaseSchemaBootstrapVersionBumped serves as a reminder to bump -// systemschema.SystemDatabaseSchemaBootstrapVersion whenever a new upgrade -// creates or modifies the schema of system tables. We unfortunately cannot -// programmatically determine if an upgrade should bump the version so by -// adding a test failure when the initial values change, the programmer and -// code reviewers are reminded to manually check whether the version should -// be bumped. -func TestSystemDatabaseSchemaBootstrapVersionBumped(t *testing.T) { - defer leaktest.AfterTest(t)() - defer log.Scope(t).Close(t) - - // If you need to update this value (i.e. failed this test), check whether - // you need to bump systemschema.SystemDatabaseSchemaBootstrapVersion too. - const prevSystemHash = "cc22bc73bd41333a8706272d157bb1760d34c9dbbda112856a44f950637aacdb" - _, curSystemHash := GetAndHashInitialValuesToString(0 /* tenantID */) - - if prevSystemHash != curSystemHash { - t.Fatalf( - `Check whether you need to bump systemschema.SystemDatabaseSchemaBootstrapVersion -and then update prevSystemHash to %q. -The current value of SystemDatabaseSchemaBootstrapVersion is %s.`, - curSystemHash, - systemschema.SystemDatabaseSchemaBootstrapVersion, - ) - } -} diff --git a/pkg/sql/catalog/bootstrap/testdata/testdata b/pkg/sql/catalog/bootstrap/testdata/testdata index 976e6eef0b92..33d7217c3cad 100644 --- a/pkg/sql/catalog/bootstrap/testdata/testdata +++ b/pkg/sql/catalog/bootstrap/testdata/testdata @@ -1,7 +1,7 @@ -system hash=cc22bc73bd41333a8706272d157bb1760d34c9dbbda112856a44f950637aacdb +system hash=17ebb12f8ac77327e6ab3af4dba91106b240ac9b2c889ac9f23f69b85539ca3c ---- [{"key":"8b"} -,{"key":"8b89898a89","value":"0312450a0673797374656d10011a250a0d0a0561646d696e1080101880100a0c0a04726f6f7410801018801012046e6f646518032200280140004a006a0a08d7843d10021800200e"} +,{"key":"8b89898a89","value":"0312450a0673797374656d10011a250a0d0a0561646d696e1080101880100a0c0a04726f6f7410801018801012046e6f646518032200280140004a006a0a08d7843d10021800201a"} ,{"key":"8b898b8a89","value":"030a8e030a0a64657363726970746f721803200128013a0042270a02696410011a0c08011040180030005014600020003000680070007800800100880100980100422f0a0a64657363726970746f7210021a0c08081000180030005011600020013000680070007800800100880100980100480352710a077072696d61727910011801220269642a0a64657363726970746f72300140004a10080010001a00200028003000380040005a0070027a0408002000800100880100900104980101a20106080012001800a80100b20100ba0100c00100c80100d00101e00100e901000000000000000060026a210a0b0a0561646d696e102018200a0a0a04726f6f741020182012046e6f64651803800101880103980100b201130a077072696d61727910001a02696420012800b201240a1066616d5f325f64657363726970746f7210021a0a64657363726970746f7220022802b80103c20100e80100f2010408001200f801008002009202009a0200b20200b80200c0021dc80200e00200800300880302a80300b00300d00300d80300e00300"} ,{"key":"8b898c8a89","value":"030ac7050a0575736572731804200128013a00422d0a08757365726e616d6510011a0c0807100018003000501960002000300068007000780080010088010098010042330a0e68617368656450617373776f726410021a0c0808100018003000501160002001300068007000780080010088010098010042320a066973526f6c6510031a0c08001000180030005010600020002a0566616c73653000680070007800800100880100980100422c0a07757365725f696410041a0c080c100018003000501a60002000300068007000780080010088010098010048055290010a077072696d617279100118012208757365726e616d652a0e68617368656450617373776f72642a066973526f6c652a07757365725f6964300140004a10080010001a00200028003000380040005a007002700370047a0408002000800100880100900104980101a20106080012001800a80100b20100ba0100c00100c80100d00102e00100e90100000000000000005a740a1175736572735f757365725f69645f696478100218012207757365725f69643004380140004a10080010001a00200028003000380040005a007a0408002000800100880100900103980100a20106080012001800a80100b20100ba0100c00100c80100d00101e00100e901000000000000000060036a250a0d0a0561646d696e10e00318e0030a0c0a04726f6f7410e00318e00312046e6f64651803800101880103980100b201240a077072696d61727910001a08757365726e616d651a07757365725f6964200120042804b2012c0a1466616d5f325f68617368656450617373776f726410021a0e68617368656450617373776f726420022802b2011c0a0c66616d5f335f6973526f6c6510031a066973526f6c6520032803b80104c20100e80100f2010408001200f801008002009202009a0200b20200b80200c0021dc80200e00200800300880303a80300b00300d00300d80300e00300"} ,{"key":"8b898d8a89","value":"030afd020a057a6f6e65731805200128013a0042270a02696410011a0c08011040180030005014600020003000680070007800800100880100980100422b0a06636f6e66696710021a0c080810001800300050116000200130006800700078008001008801009801004803526d0a077072696d61727910011801220269642a06636f6e666967300140004a10080010001a00200028003000380040005a0070027a0408002000800100880100900104980101a20106080012001800a80100b20100ba0100c00100c80100d00101e00100e901000000000000000060026a250a0d0a0561646d696e10e00318e0030a0c0a04726f6f7410e00318e00312046e6f64651803800101880103980100b201130a077072696d61727910001a02696420012800b2011c0a0c66616d5f325f636f6e66696710021a06636f6e66696720022802b80103c20100e80100f2010408001200f801008002009202009a0200b20200b80200c0021dc80200e00200800300880302a80300b00300d00300d80300e00300"} @@ -194,10 +194,10 @@ system hash=cc22bc73bd41333a8706272d157bb1760d34c9dbbda112856a44f950637aacdb ,{"key":"ca"} ] -tenant hash=f20ed7fb713c784083c3fdf946f6e794f6a74a05a91e5804518775017314c358 +tenant hash=064f1382f676795b02b278e4fd0d7d3b64954095ba874eca71470be88fad4715 ---- [{"key":""} -,{"key":"8b89898a89","value":"0312450a0673797374656d10011a250a0d0a0561646d696e1080101880100a0c0a04726f6f7410801018801012046e6f646518032200280140004a006a0a08d7843d10021800200e"} +,{"key":"8b89898a89","value":"0312450a0673797374656d10011a250a0d0a0561646d696e1080101880100a0c0a04726f6f7410801018801012046e6f646518032200280140004a006a0a08d7843d10021800201a"} ,{"key":"8b898b8a89","value":"030a8e030a0a64657363726970746f721803200128013a0042270a02696410011a0c08011040180030005014600020003000680070007800800100880100980100422f0a0a64657363726970746f7210021a0c08081000180030005011600020013000680070007800800100880100980100480352710a077072696d61727910011801220269642a0a64657363726970746f72300140004a10080010001a00200028003000380040005a0070027a0408002000800100880100900104980101a20106080012001800a80100b20100ba0100c00100c80100d00101e00100e901000000000000000060026a210a0b0a0561646d696e102018200a0a0a04726f6f741020182012046e6f64651803800101880103980100b201130a077072696d61727910001a02696420012800b201240a1066616d5f325f64657363726970746f7210021a0a64657363726970746f7220022802b80103c20100e80100f2010408001200f801008002009202009a0200b20200b80200c0021dc80200e00200800300880302a80300b00300d00300d80300e00300"} ,{"key":"8b898c8a89","value":"030ac7050a0575736572731804200128013a00422d0a08757365726e616d6510011a0c0807100018003000501960002000300068007000780080010088010098010042330a0e68617368656450617373776f726410021a0c0808100018003000501160002001300068007000780080010088010098010042320a066973526f6c6510031a0c08001000180030005010600020002a0566616c73653000680070007800800100880100980100422c0a07757365725f696410041a0c080c100018003000501a60002000300068007000780080010088010098010048055290010a077072696d617279100118012208757365726e616d652a0e68617368656450617373776f72642a066973526f6c652a07757365725f6964300140004a10080010001a00200028003000380040005a007002700370047a0408002000800100880100900104980101a20106080012001800a80100b20100ba0100c00100c80100d00102e00100e90100000000000000005a740a1175736572735f757365725f69645f696478100218012207757365725f69643004380140004a10080010001a00200028003000380040005a007a0408002000800100880100900103980100a20106080012001800a80100b20100ba0100c00100c80100d00101e00100e901000000000000000060036a250a0d0a0561646d696e10e00318e0030a0c0a04726f6f7410e00318e00312046e6f64651803800101880103980100b201240a077072696d61727910001a08757365726e616d651a07757365725f6964200120042804b2012c0a1466616d5f325f68617368656450617373776f726410021a0e68617368656450617373776f726420022802b2011c0a0c66616d5f335f6973526f6c6510031a066973526f6c6520032803b80104c20100e80100f2010408001200f801008002009202009a0200b20200b80200c0021dc80200e00200800300880303a80300b00300d00300d80300e00300"} ,{"key":"8b898d8a89","value":"030afd020a057a6f6e65731805200128013a0042270a02696410011a0c08011040180030005014600020003000680070007800800100880100980100422b0a06636f6e66696710021a0c080810001800300050116000200130006800700078008001008801009801004803526d0a077072696d61727910011801220269642a06636f6e666967300140004a10080010001a00200028003000380040005a0070027a0408002000800100880100900104980101a20106080012001800a80100b20100ba0100c00100c80100d00101e00100e901000000000000000060026a250a0d0a0561646d696e10e00318e0030a0c0a04726f6f7410e00318e00312046e6f64651803800101880103980100b201130a077072696d61727910001a02696420012800b2011c0a0c66616d5f325f636f6e66696710021a06636f6e66696720022802b80103c20100e80100f2010408001200f801008002009202009a0200b20200b80200c0021dc80200e00200800300880302a80300b00300d00300d80300e00300"} diff --git a/pkg/sql/catalog/systemschema/system.go b/pkg/sql/catalog/systemschema/system.go index c6e7efabda15..88477be8ab95 100644 --- a/pkg/sql/catalog/systemschema/system.go +++ b/pkg/sql/catalog/systemschema/system.go @@ -1208,7 +1208,8 @@ const SystemDatabaseName = catconstants.SystemDatabaseName // SystemDatabaseSchemaBootstrapVersion is the system database schema version // 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.V24_1_SessionBasedLeasingUpgradeDescriptor.Version() +// NB: Don't set this to clusterversion.Latest; use a specific version instead. +var SystemDatabaseSchemaBootstrapVersion = clusterversion.V24_1_AddSpanCounts.Version() // MakeSystemDatabaseDesc constructs a copy of the system database // descriptor. diff --git a/pkg/sql/catalog/systemschema_test/testdata/bootstrap_system b/pkg/sql/catalog/systemschema_test/testdata/bootstrap_system index 157c34b10815..8b2c52ee4356 100644 --- a/pkg/sql/catalog/systemschema_test/testdata/bootstrap_system +++ b/pkg/sql/catalog/systemschema_test/testdata/bootstrap_system @@ -639,7 +639,7 @@ schema_telemetry ---- {"database":{"name":"defaultdb","id":100,"modificationTime":{"wallTime":"0"},"version":"1","privileges":{"users":[{"userProto":"admin","privileges":"2","withGrantOption":"2"},{"userProto":"public","privileges":"2048"},{"userProto":"root","privileges":"2","withGrantOption":"2"}],"ownerProto":"root","version":3},"schemas":{"public":{"id":101}},"defaultPrivileges":{}}} {"database":{"name":"postgres","id":102,"modificationTime":{"wallTime":"0"},"version":"1","privileges":{"users":[{"userProto":"admin","privileges":"2","withGrantOption":"2"},{"userProto":"public","privileges":"2048"},{"userProto":"root","privileges":"2","withGrantOption":"2"}],"ownerProto":"root","version":3},"schemas":{"public":{"id":103}},"defaultPrivileges":{}}} -{"database":{"name":"system","id":1,"modificationTime":{"wallTime":"0"},"version":"1","privileges":{"users":[{"userProto":"admin","privileges":"2048","withGrantOption":"2048"},{"userProto":"root","privileges":"2048","withGrantOption":"2048"}],"ownerProto":"node","version":3},"systemDatabaseSchemaVersion":{"majorVal":1000023,"minorVal":2,"internal":14}}} +{"database":{"name":"system","id":1,"modificationTime":{"wallTime":"0"},"version":"1","privileges":{"users":[{"userProto":"admin","privileges":"2048","withGrantOption":"2048"},{"userProto":"root","privileges":"2048","withGrantOption":"2048"}],"ownerProto":"node","version":3},"systemDatabaseSchemaVersion":{"majorVal":1000023,"minorVal":2,"internal":26}}} {"table":{"name":"comments","id":24,"version":"1","modificationTime":{"wallTime":"0"},"parentId":1,"unexposedParentSchemaId":29,"columns":[{"name":"type","id":1,"type":{"family":"IntFamily","width":64,"oid":20}},{"name":"object_id","id":2,"type":{"family":"IntFamily","width":64,"oid":20}},{"name":"sub_id","id":3,"type":{"family":"IntFamily","width":64,"oid":20}},{"name":"comment","id":4,"type":{"family":"StringFamily","oid":25}}],"nextColumnId":5,"families":[{"name":"primary","columnNames":["type","object_id","sub_id"],"columnIds":[1,2,3]},{"name":"fam_4_comment","id":4,"columnNames":["comment"],"columnIds":[4],"defaultColumnId":4}],"nextFamilyId":5,"primaryIndex":{"name":"primary","id":1,"unique":true,"version":4,"keyColumnNames":["type","object_id","sub_id"],"keyColumnDirections":["ASC","ASC","ASC"],"storeColumnNames":["comment"],"keyColumnIds":[1,2,3],"storeColumnIds":[4],"foreignKey":{},"interleave":{},"partitioning":{},"encodingType":1,"sharded":{},"geoConfig":{},"constraintId":1},"nextIndexId":2,"privileges":{"users":[{"userProto":"admin","privileges":"480","withGrantOption":"480"},{"userProto":"public","privileges":"32"},{"userProto":"root","privileges":"480","withGrantOption":"480"}],"ownerProto":"node","version":3},"nextMutationId":1,"formatVersion":3,"replacementOf":{"time":{}},"createAsOfTime":{},"nextConstraintId":2}} {"table":{"name":"database_role_settings","id":44,"version":"1","modificationTime":{"wallTime":"0"},"parentId":1,"unexposedParentSchemaId":29,"columns":[{"name":"database_id","id":1,"type":{"family":"OidFamily","oid":26}},{"name":"role_name","id":2,"type":{"family":"StringFamily","oid":25}},{"name":"settings","id":3,"type":{"family":"ArrayFamily","arrayElemType":"StringFamily","oid":1009,"arrayContents":{"family":"StringFamily","oid":25}}},{"name":"role_id","id":4,"type":{"family":"OidFamily","oid":26}}],"nextColumnId":5,"families":[{"name":"primary","columnNames":["database_id","role_name","settings","role_id"],"columnIds":[1,2,3,4]}],"nextFamilyId":1,"primaryIndex":{"name":"primary","id":1,"unique":true,"version":4,"keyColumnNames":["database_id","role_name"],"keyColumnDirections":["ASC","ASC"],"storeColumnNames":["settings","role_id"],"keyColumnIds":[1,2],"storeColumnIds":[3,4],"foreignKey":{},"interleave":{},"partitioning":{},"encodingType":1,"sharded":{},"geoConfig":{},"constraintId":2},"indexes":[{"name":"database_role_settings_database_id_role_id_key","id":2,"unique":true,"version":3,"keyColumnNames":["database_id","role_id"],"keyColumnDirections":["ASC","ASC"],"storeColumnNames":["settings"],"keyColumnIds":[1,4],"keySuffixColumnIds":[2],"storeColumnIds":[3],"foreignKey":{},"interleave":{},"partitioning":{},"sharded":{},"geoConfig":{},"constraintId":1}],"nextIndexId":3,"privileges":{"users":[{"userProto":"admin","privileges":"480","withGrantOption":"480"},{"userProto":"root","privileges":"480","withGrantOption":"480"}],"ownerProto":"node","version":3},"nextMutationId":1,"formatVersion":3,"replacementOf":{"time":{}},"createAsOfTime":{},"nextConstraintId":3}} {"table":{"name":"descriptor","id":3,"version":"1","modificationTime":{"wallTime":"0"},"parentId":1,"unexposedParentSchemaId":29,"columns":[{"name":"id","id":1,"type":{"family":"IntFamily","width":64,"oid":20}},{"name":"descriptor","id":2,"type":{"family":"BytesFamily","oid":17},"nullable":true}],"nextColumnId":3,"families":[{"name":"primary","columnNames":["id"],"columnIds":[1]},{"name":"fam_2_descriptor","id":2,"columnNames":["descriptor"],"columnIds":[2],"defaultColumnId":2}],"nextFamilyId":3,"primaryIndex":{"name":"primary","id":1,"unique":true,"version":4,"keyColumnNames":["id"],"keyColumnDirections":["ASC"],"storeColumnNames":["descriptor"],"keyColumnIds":[1],"storeColumnIds":[2],"foreignKey":{},"interleave":{},"partitioning":{},"encodingType":1,"sharded":{},"geoConfig":{},"constraintId":1},"nextIndexId":2,"privileges":{"users":[{"userProto":"admin","privileges":"32","withGrantOption":"32"},{"userProto":"root","privileges":"32","withGrantOption":"32"}],"ownerProto":"node","version":3},"nextMutationId":1,"formatVersion":3,"replacementOf":{"time":{}},"createAsOfTime":{},"nextConstraintId":2}} diff --git a/pkg/sql/catalog/systemschema_test/testdata/bootstrap_tenant b/pkg/sql/catalog/systemschema_test/testdata/bootstrap_tenant index 157c34b10815..8b2c52ee4356 100644 --- a/pkg/sql/catalog/systemschema_test/testdata/bootstrap_tenant +++ b/pkg/sql/catalog/systemschema_test/testdata/bootstrap_tenant @@ -639,7 +639,7 @@ schema_telemetry ---- {"database":{"name":"defaultdb","id":100,"modificationTime":{"wallTime":"0"},"version":"1","privileges":{"users":[{"userProto":"admin","privileges":"2","withGrantOption":"2"},{"userProto":"public","privileges":"2048"},{"userProto":"root","privileges":"2","withGrantOption":"2"}],"ownerProto":"root","version":3},"schemas":{"public":{"id":101}},"defaultPrivileges":{}}} {"database":{"name":"postgres","id":102,"modificationTime":{"wallTime":"0"},"version":"1","privileges":{"users":[{"userProto":"admin","privileges":"2","withGrantOption":"2"},{"userProto":"public","privileges":"2048"},{"userProto":"root","privileges":"2","withGrantOption":"2"}],"ownerProto":"root","version":3},"schemas":{"public":{"id":103}},"defaultPrivileges":{}}} -{"database":{"name":"system","id":1,"modificationTime":{"wallTime":"0"},"version":"1","privileges":{"users":[{"userProto":"admin","privileges":"2048","withGrantOption":"2048"},{"userProto":"root","privileges":"2048","withGrantOption":"2048"}],"ownerProto":"node","version":3},"systemDatabaseSchemaVersion":{"majorVal":1000023,"minorVal":2,"internal":14}}} +{"database":{"name":"system","id":1,"modificationTime":{"wallTime":"0"},"version":"1","privileges":{"users":[{"userProto":"admin","privileges":"2048","withGrantOption":"2048"},{"userProto":"root","privileges":"2048","withGrantOption":"2048"}],"ownerProto":"node","version":3},"systemDatabaseSchemaVersion":{"majorVal":1000023,"minorVal":2,"internal":26}}} {"table":{"name":"comments","id":24,"version":"1","modificationTime":{"wallTime":"0"},"parentId":1,"unexposedParentSchemaId":29,"columns":[{"name":"type","id":1,"type":{"family":"IntFamily","width":64,"oid":20}},{"name":"object_id","id":2,"type":{"family":"IntFamily","width":64,"oid":20}},{"name":"sub_id","id":3,"type":{"family":"IntFamily","width":64,"oid":20}},{"name":"comment","id":4,"type":{"family":"StringFamily","oid":25}}],"nextColumnId":5,"families":[{"name":"primary","columnNames":["type","object_id","sub_id"],"columnIds":[1,2,3]},{"name":"fam_4_comment","id":4,"columnNames":["comment"],"columnIds":[4],"defaultColumnId":4}],"nextFamilyId":5,"primaryIndex":{"name":"primary","id":1,"unique":true,"version":4,"keyColumnNames":["type","object_id","sub_id"],"keyColumnDirections":["ASC","ASC","ASC"],"storeColumnNames":["comment"],"keyColumnIds":[1,2,3],"storeColumnIds":[4],"foreignKey":{},"interleave":{},"partitioning":{},"encodingType":1,"sharded":{},"geoConfig":{},"constraintId":1},"nextIndexId":2,"privileges":{"users":[{"userProto":"admin","privileges":"480","withGrantOption":"480"},{"userProto":"public","privileges":"32"},{"userProto":"root","privileges":"480","withGrantOption":"480"}],"ownerProto":"node","version":3},"nextMutationId":1,"formatVersion":3,"replacementOf":{"time":{}},"createAsOfTime":{},"nextConstraintId":2}} {"table":{"name":"database_role_settings","id":44,"version":"1","modificationTime":{"wallTime":"0"},"parentId":1,"unexposedParentSchemaId":29,"columns":[{"name":"database_id","id":1,"type":{"family":"OidFamily","oid":26}},{"name":"role_name","id":2,"type":{"family":"StringFamily","oid":25}},{"name":"settings","id":3,"type":{"family":"ArrayFamily","arrayElemType":"StringFamily","oid":1009,"arrayContents":{"family":"StringFamily","oid":25}}},{"name":"role_id","id":4,"type":{"family":"OidFamily","oid":26}}],"nextColumnId":5,"families":[{"name":"primary","columnNames":["database_id","role_name","settings","role_id"],"columnIds":[1,2,3,4]}],"nextFamilyId":1,"primaryIndex":{"name":"primary","id":1,"unique":true,"version":4,"keyColumnNames":["database_id","role_name"],"keyColumnDirections":["ASC","ASC"],"storeColumnNames":["settings","role_id"],"keyColumnIds":[1,2],"storeColumnIds":[3,4],"foreignKey":{},"interleave":{},"partitioning":{},"encodingType":1,"sharded":{},"geoConfig":{},"constraintId":2},"indexes":[{"name":"database_role_settings_database_id_role_id_key","id":2,"unique":true,"version":3,"keyColumnNames":["database_id","role_id"],"keyColumnDirections":["ASC","ASC"],"storeColumnNames":["settings"],"keyColumnIds":[1,4],"keySuffixColumnIds":[2],"storeColumnIds":[3],"foreignKey":{},"interleave":{},"partitioning":{},"sharded":{},"geoConfig":{},"constraintId":1}],"nextIndexId":3,"privileges":{"users":[{"userProto":"admin","privileges":"480","withGrantOption":"480"},{"userProto":"root","privileges":"480","withGrantOption":"480"}],"ownerProto":"node","version":3},"nextMutationId":1,"formatVersion":3,"replacementOf":{"time":{}},"createAsOfTime":{},"nextConstraintId":3}} {"table":{"name":"descriptor","id":3,"version":"1","modificationTime":{"wallTime":"0"},"parentId":1,"unexposedParentSchemaId":29,"columns":[{"name":"id","id":1,"type":{"family":"IntFamily","width":64,"oid":20}},{"name":"descriptor","id":2,"type":{"family":"BytesFamily","oid":17},"nullable":true}],"nextColumnId":3,"families":[{"name":"primary","columnNames":["id"],"columnIds":[1]},{"name":"fam_2_descriptor","id":2,"columnNames":["descriptor"],"columnIds":[2],"defaultColumnId":2}],"nextFamilyId":3,"primaryIndex":{"name":"primary","id":1,"unique":true,"version":4,"keyColumnNames":["id"],"keyColumnDirections":["ASC"],"storeColumnNames":["descriptor"],"keyColumnIds":[1],"storeColumnIds":[2],"foreignKey":{},"interleave":{},"partitioning":{},"encodingType":1,"sharded":{},"geoConfig":{},"constraintId":1},"nextIndexId":2,"privileges":{"users":[{"userProto":"admin","privileges":"32","withGrantOption":"32"},{"userProto":"root","privileges":"32","withGrantOption":"32"}],"ownerProto":"node","version":3},"nextMutationId":1,"formatVersion":3,"replacementOf":{"time":{}},"createAsOfTime":{},"nextConstraintId":2}} diff --git a/pkg/sql/logictest/testdata/logic_test/crdb_internal_catalog b/pkg/sql/logictest/testdata/logic_test/crdb_internal_catalog index adbbee31019b..a7705f67ddf2 100644 --- a/pkg/sql/logictest/testdata/logic_test/crdb_internal_catalog +++ b/pkg/sql/logictest/testdata/logic_test/crdb_internal_catalog @@ -104,7 +104,7 @@ skipif config local-mixed-23.2 query IT SELECT id, strip_volatile(descriptor) FROM crdb_internal.kv_catalog_descriptor ORDER BY id ---- -1 {"database": {"id": 1, "name": "system", "privileges": {"ownerProto": "node", "users": [{"privileges": "2048", "userProto": "admin", "withGrantOption": "2048"}, {"privileges": "2048", "userProto": "root", "withGrantOption": "2048"}], "version": 3}, "systemDatabaseSchemaVersion": {"internal": 14, "majorVal": 1000023, "minorVal": 2}, "version": "1"}} +1 {"database": {"id": 1, "name": "system", "privileges": {"ownerProto": "node", "users": [{"privileges": "2048", "userProto": "admin", "withGrantOption": "2048"}, {"privileges": "2048", "userProto": "root", "withGrantOption": "2048"}], "version": 3}, "systemDatabaseSchemaVersion": {"internal": 26, "majorVal": 1000023, "minorVal": 2}, "version": "1"}} 3 {"table": {"columns": [{"id": 1, "name": "id", "type": {"family": "IntFamily", "oid": 20, "width": 64}}, {"id": 2, "name": "descriptor", "nullable": true, "type": {"family": "BytesFamily", "oid": 17}}], "formatVersion": 3, "id": 3, "name": "descriptor", "nextColumnId": 3, "nextConstraintId": 2, "nextIndexId": 2, "nextMutationId": 1, "parentId": 1, "primaryIndex": {"constraintId": 1, "encodingType": 1, "foreignKey": {}, "geoConfig": {}, "id": 1, "interleave": {}, "keyColumnDirections": ["ASC"], "keyColumnIds": [1], "keyColumnNames": ["id"], "name": "primary", "partitioning": {}, "sharded": {}, "storeColumnIds": [2], "storeColumnNames": ["descriptor"], "unique": true, "version": 4}, "privileges": {"ownerProto": "node", "users": [{"privileges": "32", "userProto": "admin", "withGrantOption": "32"}, {"privileges": "32", "userProto": "root", "withGrantOption": "32"}], "version": 3}, "replacementOf": {"time": {}}, "unexposedParentSchemaId": 29, "version": "1"}} 4 {"table": {"columns": [{"id": 1, "name": "username", "type": {"family": "StringFamily", "oid": 25}}, {"id": 2, "name": "hashedPassword", "nullable": true, "type": {"family": "BytesFamily", "oid": 17}}, {"defaultExpr": "false", "id": 3, "name": "isRole", "type": {"oid": 16}}, {"id": 4, "name": "user_id", "type": {"family": "OidFamily", "oid": 26}}], "formatVersion": 3, "id": 4, "indexes": [{"constraintId": 1, "foreignKey": {}, "geoConfig": {}, "id": 2, "interleave": {}, "keyColumnDirections": ["ASC"], "keyColumnIds": [4], "keyColumnNames": ["user_id"], "keySuffixColumnIds": [1], "name": "users_user_id_idx", "partitioning": {}, "sharded": {}, "unique": true, "version": 3}], "name": "users", "nextColumnId": 5, "nextConstraintId": 3, "nextIndexId": 3, "nextMutationId": 1, "parentId": 1, "primaryIndex": {"constraintId": 2, "encodingType": 1, "foreignKey": {}, "geoConfig": {}, "id": 1, "interleave": {}, "keyColumnDirections": ["ASC"], "keyColumnIds": [1], "keyColumnNames": ["username"], "name": "primary", "partitioning": {}, "sharded": {}, "storeColumnIds": [2, 3, 4], "storeColumnNames": ["hashedPassword", "isRole", "user_id"], "unique": true, "version": 4}, "privileges": {"ownerProto": "node", "users": [{"privileges": "480", "userProto": "admin", "withGrantOption": "480"}, {"privileges": "480", "userProto": "root", "withGrantOption": "480"}], "version": 3}, "replacementOf": {"time": {}}, "unexposedParentSchemaId": 29, "version": "2"}} 5 {"table": {"columns": [{"id": 1, "name": "id", "type": {"family": "IntFamily", "oid": 20, "width": 64}}, {"id": 2, "name": "config", "nullable": true, "type": {"family": "BytesFamily", "oid": 17}}], "formatVersion": 3, "id": 5, "name": "zones", "nextColumnId": 3, "nextConstraintId": 2, "nextIndexId": 2, "nextMutationId": 1, "parentId": 1, "primaryIndex": {"constraintId": 1, "encodingType": 1, "foreignKey": {}, "geoConfig": {}, "id": 1, "interleave": {}, "keyColumnDirections": ["ASC"], "keyColumnIds": [1], "keyColumnNames": ["id"], "name": "primary", "partitioning": {}, "sharded": {}, "storeColumnIds": [2], "storeColumnNames": ["config"], "unique": true, "version": 4}, "privileges": {"ownerProto": "node", "users": [{"privileges": "480", "userProto": "admin", "withGrantOption": "480"}, {"privileges": "480", "userProto": "root", "withGrantOption": "480"}], "version": 3}, "replacementOf": {"time": {}}, "unexposedParentSchemaId": 29, "version": "1"}} diff --git a/pkg/upgrade/BUILD.bazel b/pkg/upgrade/BUILD.bazel index c999bd31a8e1..f546a494aeb3 100644 --- a/pkg/upgrade/BUILD.bazel +++ b/pkg/upgrade/BUILD.bazel @@ -30,6 +30,7 @@ go_library( "//pkg/util/log", "//pkg/util/stop", "//pkg/util/uuid", + "@com_github_cockroachdb_errors//:errors", "@com_github_cockroachdb_logtags//:logtags", ], ) diff --git a/pkg/upgrade/helpers.go b/pkg/upgrade/helpers.go index 7db450fcad61..06ca46369c55 100644 --- a/pkg/upgrade/helpers.go +++ b/pkg/upgrade/helpers.go @@ -14,7 +14,11 @@ import ( "context" "github.com/cockroachdb/cockroach/pkg/clusterversion" + "github.com/cockroachdb/cockroach/pkg/keys" + "github.com/cockroachdb/cockroach/pkg/roachpb" + "github.com/cockroachdb/cockroach/pkg/sql/catalog/descs" "github.com/cockroachdb/cockroach/pkg/util/log" + "github.com/cockroachdb/errors" ) // FenceVersionFor constructs the appropriate "fence version" for the given @@ -42,3 +46,34 @@ func FenceVersionFor( fenceCV.Internal-- return fenceCV } + +// BumpSystemDatabaseSchemaVersion bumps the SystemDatabaseSchemaVersion +// field for the system database descriptor. It is called after every upgrade +// step that has an associated migration, and when upgrading to the final +// clusterversion for a release. +func BumpSystemDatabaseSchemaVersion( + ctx context.Context, version roachpb.Version, descDB descs.DB, +) error { + if err := 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 version.Less(*sv) { + return errors.AssertionFailedf( + "new system schema version (%#v) is lower than previous system schema version (%#v)", + version, + *sv, + ) + } else if version.Equal(sv) { + return nil + } + } + systemDBDesc.SystemDatabaseSchemaVersion = &version + return txn.Descriptors().WriteDesc(ctx, false /* kvTrace */, systemDBDesc, txn.KV()) + }); err != nil { + return err + } + return nil +} diff --git a/pkg/upgrade/upgradejob/upgrade_job.go b/pkg/upgrade/upgradejob/upgrade_job.go index 06d5789c06b1..54a436842eb4 100644 --- a/pkg/upgrade/upgradejob/upgrade_job.go +++ b/pkg/upgrade/upgradejob/upgrade_job.go @@ -130,6 +130,14 @@ func (r resumer) Resume(ctx context.Context, execCtxI interface{}) error { return errors.Wrapf(err, "running migration for %v", v) } + // Bump the version of the system database schema whenever we run a + // non-permanent migration. + if !m.Permanent() { + if err := upgrade.BumpSystemDatabaseSchemaVersion(ctx, v, db); err != nil { + return err + } + } + // Mark the upgrade as having been completed so that subsequent iterations // no-op and new jobs are not created. if err := migrationstable.MarkMigrationCompleted(ctx, ex, v); err != nil { diff --git a/pkg/upgrade/upgrademanager/manager.go b/pkg/upgrade/upgrademanager/manager.go index 79d9f8f1b5b3..a359106951f6 100644 --- a/pkg/upgrade/upgrademanager/manager.go +++ b/pkg/upgrade/upgrademanager/manager.go @@ -522,6 +522,17 @@ func (m *Manager) Migrate( } } + // Bump the version of the system database schema if this is the final + // version for a release. + // NB: The final version never has an associated migration, which is why we + // bump the SystemDatabaseSchemaVersion here; we cannot do it inside of + // runMigration. + if clusterVersion.Equal(clusterversion.Latest.Version()) && clusterVersion.IsFinal() { + if err := upgrade.BumpSystemDatabaseSchemaVersion(ctx, cv.Version, m.deps.DB); err != nil { + return err + } + } + if m.knobs.InterlockPausePoint == upgradebase.AfterMigration { m.postToPauseChannelAndWaitForResume(ctx) } @@ -621,6 +632,7 @@ func (m *Manager) runMigration( // since this code doesn't do retries and we also don't want to complicate // test only code. ctx := startup.WithoutChecks(ctx) + v := mig.Version() alreadyCompleted, err := migrationstable.CheckIfMigrationCompleted( ctx, version, nil /* txn */, m.ie, false /* enterpriseEnabled */, migrationstable.ConsistentRead, ) @@ -630,13 +642,13 @@ func (m *Manager) runMigration( switch upg := mig.(type) { case *upgrade.SystemUpgrade: - if err := upg.Run(ctx, mig.Version(), m.SystemDeps()); err != nil { + if err := upg.Run(ctx, v, m.SystemDeps()); err != nil { return err } case *upgrade.TenantUpgrade: // The TenantDeps used here are incomplete, but enough for the "permanent // upgrades" that run under this testing knob. - if err := upg.Run(ctx, mig.Version(), upgrade.TenantDeps{ + if err := upg.Run(ctx, v, upgrade.TenantDeps{ KVDB: m.deps.DB.KV(), DB: m.deps.DB, Codec: m.codec, @@ -651,9 +663,16 @@ func (m *Manager) runMigration( } } - if err := migrationstable.MarkMigrationCompleted(ctx, m.ie, mig.Version()); err != nil { + if err := migrationstable.MarkMigrationCompleted(ctx, m.ie, v); err != nil { return err } + // Bump the version of the system database schema whenever we run a + // non-permanent migration. + if !mig.Permanent() { + if err := upgrade.BumpSystemDatabaseSchemaVersion(ctx, v, m.deps.DB); err != nil { + return err + } + } return nil } else { // Run a job that, in turn, will run the upgrade. By running upgrades inside diff --git a/pkg/upgrade/upgrades/main_test.go b/pkg/upgrade/upgrades/main_test.go index 74f3e77825f8..aa89a0c71864 100644 --- a/pkg/upgrade/upgrades/main_test.go +++ b/pkg/upgrade/upgrades/main_test.go @@ -33,3 +33,5 @@ func TestMain(m *testing.M) { os.Exit(m.Run()) } + +//go:generate ../../util/leaktest/add-leaktest.sh *_test.go diff --git a/pkg/upgrade/upgrades/permanent_sql_stats_ttl_test.go b/pkg/upgrade/upgrades/permanent_sql_stats_ttl_test.go index 2a3571e58ab7..3360221d3983 100644 --- a/pkg/upgrade/upgrades/permanent_sql_stats_ttl_test.go +++ b/pkg/upgrade/upgrades/permanent_sql_stats_ttl_test.go @@ -28,8 +28,8 @@ import ( // we still need to test that the upgrade happens as expected when creating a // new cluster. func TestSQLStatsTTLChange(t *testing.T) { - skip.UnderStressRace(t) defer leaktest.AfterTest(t)() + skip.UnderStressRace(t) ctx := context.Background() tc := testcluster.StartTestCluster(t, 1, base.TestClusterArgs{}) diff --git a/pkg/upgrade/upgrades/permanent_system_activity_update_job_test.go b/pkg/upgrade/upgrades/permanent_system_activity_update_job_test.go index 9b0687a90c58..2f53f4b8b3f0 100644 --- a/pkg/upgrade/upgrades/permanent_system_activity_update_job_test.go +++ b/pkg/upgrade/upgrades/permanent_system_activity_update_job_test.go @@ -26,8 +26,8 @@ import ( // versions this old, but we still need to test that the upgrade happens as // expected when creating a new cluster. func TestCreateActivityUpdateJobMigration(t *testing.T) { - skip.UnderStressRace(t) defer leaktest.AfterTest(t)() + skip.UnderStressRace(t) ctx := context.Background() tc := testcluster.StartTestCluster(t, 1, base.TestClusterArgs{}) 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/schema_changes_external_test.go b/pkg/upgrade/upgrades/schema_changes_external_test.go index baff94c8ea5a..7687ea989239 100644 --- a/pkg/upgrade/upgrades/schema_changes_external_test.go +++ b/pkg/upgrade/upgrades/schema_changes_external_test.go @@ -69,6 +69,7 @@ type schemaChangeTestCase struct { // exponential backoff to the system.jobs table, but was retrofitted to prevent // regressions. func TestMigrationWithFailures(t *testing.T) { + defer leaktest.AfterTest(t)() const createTableBefore = ` CREATE TABLE test.test_table ( id INT8 DEFAULT unique_rowid() PRIMARY KEY, @@ -173,6 +174,7 @@ CREATE TABLE test.test_table ( // alters a column in a table multiple times with failures at different stages // of the migration. func TestMigrationWithFailuresMultipleAltersOnSameColumn(t *testing.T) { + defer leaktest.AfterTest(t)() const createTableBefore = ` CREATE TABLE test.test_table ( diff --git a/pkg/upgrade/upgrades/upgrades_test.go b/pkg/upgrade/upgrades/upgrades_test.go index db6043f8fa36..0136eadcb2a5 100644 --- a/pkg/upgrade/upgrades/upgrades_test.go +++ b/pkg/upgrade/upgrades/upgrades_test.go @@ -15,12 +15,15 @@ import ( "github.com/cockroachdb/cockroach/pkg/clusterversion" "github.com/cockroachdb/cockroach/pkg/roachpb" + "github.com/cockroachdb/cockroach/pkg/sql/catalog/systemschema" "github.com/cockroachdb/cockroach/pkg/upgrade/upgradebase" + "github.com/cockroachdb/cockroach/pkg/util/leaktest" "github.com/stretchr/testify/require" ) // TestUniqueVersions tests that the registered upgrades have unique versions. func TestUniqueVersions(t *testing.T) { + defer leaktest.AfterTest(t)() versions := make(map[roachpb.Version]upgradebase.Upgrade) for _, m := range upgrades { _, found := versions[m.Version()] @@ -30,6 +33,7 @@ func TestUniqueVersions(t *testing.T) { } func TestRestoreBehaviorIsSet(t *testing.T) { + defer leaktest.AfterTest(t)() for _, m := range upgrades { require.NotEmpty(t, m.RestoreBehavior(), "expected %s to document a restore behavior", m.Name()) } @@ -39,6 +43,7 @@ func TestRestoreBehaviorIsSet(t *testing.T) { // version following each supported pre-existing release has an firstUpgrade // registered for it. func TestFirstUpgradesAfterPreExistingRelease(t *testing.T) { + defer leaktest.AfterTest(t)() // Compute the set of pre-existing releases supported by this binary. // This excludes the latest release if the binary version is a release. preExistingReleases := make(map[roachpb.Version]struct{}) @@ -75,3 +80,19 @@ func TestFirstUpgradesAfterPreExistingRelease(t *testing.T) { v, r) } } + +// TestSystemDatabaseSchemaBootstrapVersionBumped serves as a reminder to bump +// systemschema.SystemDatabaseSchemaBootstrapVersion whenever a new upgrade +// has an associated migration or is the final upgrade. +func TestSystemDatabaseSchemaBootstrapVersionBumped(t *testing.T) { + defer leaktest.AfterTest(t)() + + _, hasMigration := GetUpgrade(clusterversion.Latest.Version()) + if clusterversion.Latest.IsFinal() || hasMigration { + require.Equalf( + t, clusterversion.Latest.Version(), systemschema.SystemDatabaseSchemaBootstrapVersion, + "SystemDatabaseSchemaBootstrapVersion is %s, but it should be %s", + systemschema.SystemDatabaseSchemaBootstrapVersion, clusterversion.Latest.Version(), + ) + } +} 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_add_span_counts_test.go b/pkg/upgrade/upgrades/v24_1_add_span_counts_test.go index 00ebc3676791..4f8ad56aa6b6 100644 --- a/pkg/upgrade/upgrades/v24_1_add_span_counts_test.go +++ b/pkg/upgrade/upgrades/v24_1_add_span_counts_test.go @@ -26,10 +26,9 @@ import ( ) func TestAddSpanCounts(t *testing.T) { - clusterversion.SkipWhenMinSupportedVersionIsAtLeast(t, 24, 1) - defer leaktest.AfterTest(t)() defer log.Scope(t).Close(t) + clusterversion.SkipWhenMinSupportedVersionIsAtLeast(t, 24, 1) clusterArgs := base.TestClusterArgs{ ServerArgs: base.TestServerArgs{ 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 }