From d793e6712596067875a017fa09828111c8369e7f Mon Sep 17 00:00:00 2001 From: Andy Yang Date: Fri, 29 Sep 2023 16:39:19 -0400 Subject: [PATCH] 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 --- pkg/upgrade/upgrades/BUILD.bazel | 1 + .../upgrades/create_region_liveness.go | 4 +-- .../upgrades/create_region_liveness_test.go | 2 ++ pkg/upgrade/upgrades/helpers_test.go | 28 +++++++++++++++++++ .../plan_gist_stmt_diagnostics_requests.go | 2 +- ...lan_gist_stmt_diagnostics_requests_test.go | 2 ++ pkg/upgrade/upgrades/schema_changes.go | 26 +++++++++++++++++ 7 files changed, 62 insertions(+), 3 deletions(-) diff --git a/pkg/upgrade/upgrades/BUILD.bazel b/pkg/upgrade/upgrades/BUILD.bazel index 9fca58592750..8124bd863d96 100644 --- a/pkg/upgrade/upgrades/BUILD.bazel +++ b/pkg/upgrade/upgrades/BUILD.bazel @@ -202,6 +202,7 @@ go_test( "//pkg/util/intsets", "//pkg/util/leaktest", "//pkg/util/log", + "//pkg/util/protoutil", "//pkg/util/syncutil", "//pkg/util/timeutil", "@com_github_cockroachdb_cockroach_go_v2//crdb", diff --git a/pkg/upgrade/upgrades/create_region_liveness.go b/pkg/upgrade/upgrades/create_region_liveness.go index 24214a09d509..3b14b855d8c9 100644 --- a/pkg/upgrade/upgrades/create_region_liveness.go +++ b/pkg/upgrade/upgrades/create_region_liveness.go @@ -25,7 +25,7 @@ import ( // createRegionLivenessTables creates the system.region_liveness table. func createRegionLivenessTables( - ctx context.Context, _ clusterversion.ClusterVersion, d upgrade.TenantDeps, + ctx context.Context, cs clusterversion.ClusterVersion, d upgrade.TenantDeps, ) error { setDBLocality := false // Since this is a re-use of an old key space, invalid data might exists, @@ -96,5 +96,5 @@ func createRegionLivenessTables( } } - return nil + return bumpSystemDatabaseSchemaVersion(ctx, cs, d) } diff --git a/pkg/upgrade/upgrades/create_region_liveness_test.go b/pkg/upgrade/upgrades/create_region_liveness_test.go index d5ebe633340f..fbf926496f36 100644 --- a/pkg/upgrade/upgrades/create_region_liveness_test.go +++ b/pkg/upgrade/upgrades/create_region_liveness_test.go @@ -57,4 +57,6 @@ func TestRegionLivenessTableMigration(t *testing.T) { _, err := db.Exec("SELECT * FROM system.region_liveness") assert.NoError(t, err, "system.region_liveness exists") + + upgrades.ValidateSystemDatabaseSchemaVersionBumped(t, db, clusterversion.V23_2_RegionaLivenessTable) } diff --git a/pkg/upgrade/upgrades/helpers_test.go b/pkg/upgrade/upgrades/helpers_test.go index ef0764fd2e5f..4478ecf9476b 100644 --- a/pkg/upgrade/upgrades/helpers_test.go +++ b/pkg/upgrade/upgrades/helpers_test.go @@ -18,6 +18,7 @@ import ( "github.com/cockroachdb/cockroach-go/v2/crdb" "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" "github.com/cockroachdb/cockroach/pkg/sql/catalog/descpb" @@ -26,6 +27,7 @@ import ( "github.com/cockroachdb/cockroach/pkg/sql/catalog/systemschema" "github.com/cockroachdb/cockroach/pkg/sql/catalog/tabledesc" "github.com/cockroachdb/cockroach/pkg/testutils/serverutils" + "github.com/cockroachdb/cockroach/pkg/util/protoutil" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -198,3 +200,29 @@ func ExecForCountInTxns( require.NoError(t, err) } } + +// ValidateSystemDatabaseSchemaVersionBumped validates that the system database +// schema version has been bumped to the expected version. +func ValidateSystemDatabaseSchemaVersionBumped( + t *testing.T, sqlDB *gosql.DB, expectedVersion clusterversion.Key, +) { + expectedSchemaVersion := clusterversion.ByKey(expectedVersion) + + var actualSchemaVersionBytes []byte + require.NoError(t, sqlDB.QueryRow( + `SELECT crdb_internal.json_to_pb( + 'cockroach.roachpb.Version', + crdb_internal.pb_to_json( + 'cockroach.sql.sqlbase.Descriptor', + descriptor, + false + )->'database'->'systemDatabaseSchemaVersion') +FROM system.descriptor WHERE id = $1`, + keys.SystemDatabaseID, + ).Scan(&actualSchemaVersionBytes)) + + actualSchemaVersion := roachpb.Version{} + require.NoError(t, protoutil.Unmarshal(actualSchemaVersionBytes, &actualSchemaVersion)) + + require.Equal(t, expectedSchemaVersion, actualSchemaVersion) +} diff --git a/pkg/upgrade/upgrades/plan_gist_stmt_diagnostics_requests.go b/pkg/upgrade/upgrades/plan_gist_stmt_diagnostics_requests.go index 62983969884b..8fae809195ad 100644 --- a/pkg/upgrade/upgrades/plan_gist_stmt_diagnostics_requests.go +++ b/pkg/upgrade/upgrades/plan_gist_stmt_diagnostics_requests.go @@ -76,5 +76,5 @@ func stmtDiagForPlanGistMigration( return err } } - return nil + return bumpSystemDatabaseSchemaVersion(ctx, cs, d) } diff --git a/pkg/upgrade/upgrades/plan_gist_stmt_diagnostics_requests_test.go b/pkg/upgrade/upgrades/plan_gist_stmt_diagnostics_requests_test.go index 37acb22e6d1c..0de3b873b374 100644 --- a/pkg/upgrade/upgrades/plan_gist_stmt_diagnostics_requests_test.go +++ b/pkg/upgrade/upgrades/plan_gist_stmt_diagnostics_requests_test.go @@ -98,6 +98,8 @@ func TestStmtDiagForPlanGistMigration(t *testing.T) { ) // Validate that the table has new schema. validateSchemaExists(true) + + upgrades.ValidateSystemDatabaseSchemaVersionBumped(t, sqlDB, clusterversion.V23_2_StmtDiagForPlanGist) } // getOldStmtDiagReqsDescriptor returns the diff --git a/pkg/upgrade/upgrades/schema_changes.go b/pkg/upgrade/upgrades/schema_changes.go index 1b6061fc3ecd..97fd286aad3b 100644 --- a/pkg/upgrade/upgrades/schema_changes.go +++ b/pkg/upgrade/upgrades/schema_changes.go @@ -21,6 +21,7 @@ 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" @@ -411,3 +412,28 @@ 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()) + }) +}