From 284e9cc2e5d6e05ef952f46e9175710bf7e5ecb7 Mon Sep 17 00:00:00 2001 From: Faizan Qazi Date: Tue, 19 Nov 2024 19:46:08 +0000 Subject: [PATCH] upgrade: adding is_draining to system.sql_instance can fail Previously, the logic to add the is_draining column to the system.sql_instance descriptor would copy the bootstrap descriptor directly. While this works fine for non-multiregion system databases, this approach breaks for multi-region system databases. This is because bootstrap descriptors do not have multi-region modifications applied on top. To address this, this change modifies the upgrade to use ALTER TABLE ADD COLUMN. Fixes: #135736 Release note: None --- .../multiregion_system_table_test.go | 12 +++++-- .../v24_3_sql_instances_add_draining.go | 33 ++++++------------- 2 files changed, 19 insertions(+), 26 deletions(-) diff --git a/pkg/ccl/multiregionccl/multiregion_system_table_test.go b/pkg/ccl/multiregionccl/multiregion_system_table_test.go index 35b99308c853..2a9600b93edf 100644 --- a/pkg/ccl/multiregionccl/multiregion_system_table_test.go +++ b/pkg/ccl/multiregionccl/multiregion_system_table_test.go @@ -19,6 +19,7 @@ import ( "github.com/cockroachdb/cockroach/pkg/roachpb" "github.com/cockroachdb/cockroach/pkg/server" "github.com/cockroachdb/cockroach/pkg/settings/cluster" + "github.com/cockroachdb/cockroach/pkg/sql" "github.com/cockroachdb/cockroach/pkg/sql/enum" "github.com/cockroachdb/cockroach/pkg/sql/sqlinstance/instancestorage" "github.com/cockroachdb/cockroach/pkg/sql/sqlliveness" @@ -562,7 +563,7 @@ func TestMrSystemDatabaseUpgrade(t *testing.T) { // Enable settings required for configuring a tenant's system database as multi-region. makeSettings := func() *cluster.Settings { - cs := cluster.MakeTestingClusterSettingsWithVersions(clusterversion.V24_1.Version(), + cs := cluster.MakeTestingClusterSettingsWithVersions(clusterversion.Latest.Version(), clusterversion.MinSupported.Version(), false) instancestorage.ReclaimLoopInterval.Override(ctx, &cs.SV, 150*time.Millisecond) @@ -578,10 +579,14 @@ func TestMrSystemDatabaseUpgrade(t *testing.T) { }, multiregionccltestutils.WithSettings(makeSettings())) defer cleanup() - id, err := roachpb.MakeTenantID(11) require.NoError(t, err) + // Disable license enforcement for this test. + for _, s := range cluster.Servers { + s.ExecutorConfig().(sql.ExecutorConfig).LicenseEnforcer.Disable(ctx) + } + tenantArgs := base.TestTenantArgs{ Settings: makeSettings(), TestingKnobs: base.TestingKnobs{ @@ -593,7 +598,8 @@ func TestMrSystemDatabaseUpgrade(t *testing.T) { TenantID: id, Locality: cluster.Servers[0].Locality(), } - _, tenantSQL := serverutils.StartTenant(t, cluster.Servers[0], tenantArgs) + appLayer, tenantSQL := serverutils.StartTenant(t, cluster.Servers[0], tenantArgs) + appLayer.ExecutorConfig().(sql.ExecutorConfig).LicenseEnforcer.Disable(ctx) tDB := sqlutils.MakeSQLRunner(tenantSQL) diff --git a/pkg/upgrade/upgrades/v24_3_sql_instances_add_draining.go b/pkg/upgrade/upgrades/v24_3_sql_instances_add_draining.go index d3a184468aa5..518502773c78 100644 --- a/pkg/upgrade/upgrades/v24_3_sql_instances_add_draining.go +++ b/pkg/upgrade/upgrades/v24_3_sql_instances_add_draining.go @@ -9,35 +9,22 @@ import ( "context" "github.com/cockroachdb/cockroach/pkg/clusterversion" - "github.com/cockroachdb/cockroach/pkg/sql/catalog/descpb" - "github.com/cockroachdb/cockroach/pkg/sql/catalog/descs" + "github.com/cockroachdb/cockroach/pkg/keys" "github.com/cockroachdb/cockroach/pkg/sql/catalog/systemschema" "github.com/cockroachdb/cockroach/pkg/upgrade" - "github.com/cockroachdb/cockroach/pkg/util/protoutil" - "github.com/cockroachdb/errors" ) // sqlInstancesAddDrainingMigration adds a new column `is_draining` to the // system.sql_instances table. func sqlInstancesAddDrainingMigration( - ctx context.Context, cs clusterversion.ClusterVersion, deps upgrade.TenantDeps, + ctx context.Context, clusterVersion clusterversion.ClusterVersion, deps upgrade.TenantDeps, ) error { - finalDescriptor := systemschema.SQLInstancesTable() - // Replace the stored descriptor with the bootstrap descriptor. - err := deps.DB.DescsTxn(ctx, func(ctx context.Context, txn descs.Txn) error { - expectedDesc := finalDescriptor.TableDesc() - mutableDesc, err := txn.Descriptors().MutableByID(txn.KV()).Table(ctx, expectedDesc.GetID()) - if err != nil { - return err - } - version := mutableDesc.Version - mutableDesc.TableDescriptor = *protoutil.Clone(expectedDesc).(*descpb.TableDescriptor) - mutableDesc.Version = version - return txn.Descriptors().WriteDesc(ctx, false, mutableDesc, txn.KV()) - }) - if err != nil { - return errors.Wrapf(err, "unable to replace system descriptor for system.%s (%+v)", - finalDescriptor.GetName(), finalDescriptor) - } - return err + return migrateTable(ctx, clusterVersion, deps, operation{ + name: "add-draining-column", + schemaList: []string{"is_draining"}, + schemaExistsFn: columnExists, + query: `ALTER TABLE system.sql_instances ADD COLUMN IF NOT EXISTS is_draining BOOL NULL FAMILY "primary"`, + }, + keys.SQLInstancesTableID, + systemschema.SQLInstancesTable()) }