From d64ac0843f8752c211779fa6b9bed2a984cf5204 Mon Sep 17 00:00:00 2001 From: Jeff Date: Thu, 17 Nov 2022 16:19:15 -0500 Subject: [PATCH] multiregionccl: add unsafe_optimize_system_database crdb_internal.unsafe_optimize_system_database optimizes the system database for multi region deployments by configuring latency critical tables as global or regional by row. Tables that are sensitive to read latency were converted to global. Table that are sensitive to write latency were converted to regional by row. system.sqlliveness and system.sql_instances were reworked by (#90408) and (#92010) to have a primary key index that is binary compatible with regional by row tables. The conversion to regional by row is implemented as a built in function because it is not possible to use `ALTER TABLE ... SET LOCALITY REGIONAL BY ROW` to perform the conversion. Using alter table to convert the RBR tables has two challenges: 1. There is no way to convert the crdb_region column from []byte to the region enum type. 2. Changing the locality to RBR always rewrites the primary index. The sqlliveness and sql_instance subsystems use the raw kv API and expect a specific index ID. Rewriting the primary index changes the ID and breaks the subsystems. Release note: None Part of #85736 --- pkg/ccl/multiregionccl/BUILD.bazel | 3 - .../multiregion_system_table_test.go | 149 +++++---------- pkg/sql/faketreeeval/evalctx.go | 6 + pkg/sql/importer/import_table_creation.go | 5 + pkg/sql/region_util.go | 172 ++++++++++++++++++ pkg/sql/sem/builtins/builtins.go | 16 ++ pkg/sql/sem/builtins/fixed_oids.go | 1 + pkg/sql/sem/eval/deps.go | 8 + 8 files changed, 256 insertions(+), 104 deletions(-) diff --git a/pkg/ccl/multiregionccl/BUILD.bazel b/pkg/ccl/multiregionccl/BUILD.bazel index 52845d27c5d1..36c0c5e14e6d 100644 --- a/pkg/ccl/multiregionccl/BUILD.bazel +++ b/pkg/ccl/multiregionccl/BUILD.bazel @@ -67,8 +67,6 @@ go_test( "//pkg/sql/catalog/descpb", "//pkg/sql/catalog/descs", "//pkg/sql/catalog/desctestutils", - "//pkg/sql/catalog/tabledesc", - "//pkg/sql/catalog/typedesc", "//pkg/sql/enum", "//pkg/sql/execinfra", "//pkg/sql/parser", @@ -79,7 +77,6 @@ go_test( "//pkg/sql/sqlliveness/slstorage", "//pkg/sql/sqltestutils", "//pkg/sql/tests", - "//pkg/sql/types", "//pkg/testutils", "//pkg/testutils/serverutils", "//pkg/testutils/skip", diff --git a/pkg/ccl/multiregionccl/multiregion_system_table_test.go b/pkg/ccl/multiregionccl/multiregion_system_table_test.go index 6b9cbb16cb81..180ff2484ee6 100644 --- a/pkg/ccl/multiregionccl/multiregion_system_table_test.go +++ b/pkg/ccl/multiregionccl/multiregion_system_table_test.go @@ -17,22 +17,13 @@ import ( "github.com/cockroachdb/apd/v3" "github.com/cockroachdb/cockroach/pkg/base" "github.com/cockroachdb/cockroach/pkg/ccl/multiregionccl/multiregionccltestutils" - "github.com/cockroachdb/cockroach/pkg/keys" - "github.com/cockroachdb/cockroach/pkg/kv" "github.com/cockroachdb/cockroach/pkg/roachpb" "github.com/cockroachdb/cockroach/pkg/settings/cluster" "github.com/cockroachdb/cockroach/pkg/sql" - "github.com/cockroachdb/cockroach/pkg/sql/catalog" - "github.com/cockroachdb/cockroach/pkg/sql/catalog/descpb" - "github.com/cockroachdb/cockroach/pkg/sql/catalog/descs" - "github.com/cockroachdb/cockroach/pkg/sql/catalog/tabledesc" - "github.com/cockroachdb/cockroach/pkg/sql/catalog/typedesc" "github.com/cockroachdb/cockroach/pkg/sql/enum" - "github.com/cockroachdb/cockroach/pkg/sql/sem/tree" "github.com/cockroachdb/cockroach/pkg/sql/sqlinstance/instancestorage" "github.com/cockroachdb/cockroach/pkg/sql/sqlliveness" "github.com/cockroachdb/cockroach/pkg/sql/sqlliveness/slstorage" - "github.com/cockroachdb/cockroach/pkg/sql/types" "github.com/cockroachdb/cockroach/pkg/testutils" "github.com/cockroachdb/cockroach/pkg/testutils/serverutils" "github.com/cockroachdb/cockroach/pkg/testutils/sqlutils" @@ -44,84 +35,6 @@ import ( "github.com/stretchr/testify/require" ) -// alterCrdbRegionType converts the crdb_region []byte column in a system -// database table into the system database's enum type. -func alterCrdbRegionType( - ctx context.Context, tableID descpb.ID, db *kv.DB, executor descs.TxnManager, -) error { - flags := tree.CommonLookupFlags{ - Required: true, - AvoidLeased: true, - } - objFlags := tree.ObjectLookupFlags{ - CommonLookupFlags: flags, - } - - getRegionEnum := func(systemDB catalog.DatabaseDescriptor, txn *kv.Txn, collection *descs.Collection) (*typedesc.Mutable, *types.T, error) { - enumID, err := systemDB.MultiRegionEnumID() - if err != nil { - return nil, nil, err - } - enumTypeDesc, err := collection.GetMutableTypeByID(ctx, txn, enumID, objFlags) - if err != nil { - return nil, nil, err - } - schema, err := collection.GetImmutableSchemaByID(ctx, txn, enumTypeDesc.GetParentSchemaID(), flags) - if err != nil { - return nil, nil, err - } - enumName := tree.MakeQualifiedTypeName(systemDB.GetName(), schema.GetName(), enumTypeDesc.GetName()) - enumType, err := enumTypeDesc.MakeTypesT(ctx, &enumName, nil) - if err != nil { - return nil, nil, err - } - return enumTypeDesc, enumType, nil - } - - getMutableColumn := func(table *tabledesc.Mutable, name string) (*descpb.ColumnDescriptor, error) { - for i := range table.Columns { - if table.Columns[i].Name == name { - return &table.Columns[i], nil - } - } - return nil, errors.New("crdb_region column not found") - } - - err := executor.DescsTxn(ctx, db, func(ctx context.Context, txn *kv.Txn, collection *descs.Collection) error { - _, systemDB, err := collection.GetImmutableDatabaseByID(ctx, txn, keys.SystemDatabaseID, flags) - if err != nil { - return err - } - - enumTypeDesc, enumType, err := getRegionEnum(systemDB, txn, collection) - if err != nil { - return err - } - - // Change the crdb_region column's type to the enum - tableDesc, err := collection.GetMutableTableByID(ctx, txn, tableID, objFlags) - if err != nil { - return err - } - column, err := getMutableColumn(tableDesc, "crdb_region") - if err != nil { - return err - } - column.Type = enumType - if err := collection.WriteDesc(ctx, false, tableDesc, txn); err != nil { - return err - } - - // Add a back reference to the enum - enumTypeDesc.AddReferencingDescriptorID(tableID) - return collection.WriteDesc(ctx, false, enumTypeDesc, txn) - }) - if err != nil { - return errors.Wrapf(err, "unable to change crdb_region from []byte to the multi-region enum for table %d", tableID) - } - return err -} - func TestMrSystemDatabase(t *testing.T) { defer leaktest.AfterTest(t)() defer log.Scope(t).Close(t) @@ -146,29 +59,25 @@ func TestMrSystemDatabase(t *testing.T) { TenantID: id, Locality: *cluster.Servers[0].Locality(), } - tenantServer, tenantSQL := serverutils.StartTenant(t, cluster.Servers[0], tenantArgs) + _, tenantSQL := serverutils.StartTenant(t, cluster.Servers[0], tenantArgs) tDB := sqlutils.MakeSQLRunner(tenantSQL) + // Generate stats for system.sqlinstances. See the "QueryByEnum" test for + // details. + tDB.Exec(t, `ANALYZE system.sqlliveness;`) + tDB.Exec(t, `ALTER DATABASE system SET PRIMARY REGION "us-east1"`) tDB.Exec(t, `ALTER DATABASE system ADD REGION "us-east2"`) tDB.Exec(t, `ALTER DATABASE system ADD REGION "us-east3"`) - executor := tenantServer.ExecutorConfig().(sql.ExecutorConfig) - - // Changing the type of the crdb_region field is required to modify the - // types with SET LOCALITY REGIONAL BY ROW. - require.NoError(t, alterCrdbRegionType(ctx, keys.SqllivenessID, executor.DB, executor.InternalExecutorFactory)) - require.NoError(t, alterCrdbRegionType(ctx, keys.SQLInstancesTableID, executor.DB, executor.InternalExecutorFactory)) + tDB.Exec(t, `SELECT crdb_internal.unsafe_optimize_system_database()`) // Run schema validations to ensure the manual descriptor modifications are // okay. tDB.CheckQueryResults(t, `SELECT * FROM crdb_internal.invalid_objects`, [][]string{}) t.Run("Sqlliveness", func(t *testing.T) { - // TODO(jeffswenson): Setting the locality does not work because it causes the schema - // changer to rewrite the primary key index. - // tDB.Exec(t, `ALTER TABLE system.sqlliveness SET LOCALITY REGIONAL BY ROW`) row := tDB.QueryRow(t, `SELECT crdb_region, session_uuid, expiration FROM system.sqlliveness LIMIT 1`) var sessionUUID string var crdbRegion string @@ -178,10 +87,6 @@ func TestMrSystemDatabase(t *testing.T) { }) t.Run("Sqlinstances", func(t *testing.T) { - // TODO(jeffswenson): Setting the locality does not work because it causes the schema - // changer to rewrite the primary key index. - // tDB.Exec(t, `ALTER TABLE system.sql_instances SET LOCALITY REGIONAL BY ROW`) - t.Run("InUse", func(t *testing.T) { query := ` SELECT id, addr, session_id, locality, crdb_region @@ -302,6 +207,48 @@ func TestMrSystemDatabase(t *testing.T) { }) }) }) + + t.Run("GlobalTables", func(t *testing.T) { + query := ` + SELECT target + FROM [SHOW ALL ZONE CONFIGURATIONS] + WHERE target LIKE 'TABLE system.public.%' + AND raw_config_sql LIKE '%global_reads = true%' + ORDER BY target; + ` + tDB.CheckQueryResults(t, query, [][]string{ + {"TABLE system.public.comments"}, + {"TABLE system.public.database_role_settings"}, + {"TABLE system.public.descriptor"}, + {"TABLE system.public.namespace"}, + {"TABLE system.public.privileges"}, + {"TABLE system.public.role_members"}, + {"TABLE system.public.role_options"}, + {"TABLE system.public.settings"}, + {"TABLE system.public.table_statistics"}, + {"TABLE system.public.users"}, + {"TABLE system.public.web_sessions"}, + {"TABLE system.public.zones"}, + }) + }) + + t.Run("QueryByEnum", func(t *testing.T) { + // This is a regression test for a bug triggered by + // unsafe_optimize_system_database. If usnafe_optimize_system_database + // does not clear table statistics, this query will fail in the + // optimizer, because the stats will have the wrong type for the + // crdb_column. + row := tDB.QueryRow(t, ` + SELECT crdb_region, session_uuid, expiration + FROM system.sqlliveness + WHERE crdb_region = 'us-east1' + LIMIT 1;`) + var sessionUUID string + var crdbRegion string + var rawExpiration apd.Decimal + row.Scan(&crdbRegion, &sessionUUID, &rawExpiration) + require.Equal(t, "us-east1", crdbRegion) + }) } func TestTenantStartupWithMultiRegionEnum(t *testing.T) { diff --git a/pkg/sql/faketreeeval/evalctx.go b/pkg/sql/faketreeeval/evalctx.go index 4e8eb973068c..f70d1bc41cf2 100644 --- a/pkg/sql/faketreeeval/evalctx.go +++ b/pkg/sql/faketreeeval/evalctx.go @@ -157,6 +157,12 @@ func (so *DummyRegionOperator) ResetMultiRegionZoneConfigsForDatabase( return errors.WithStack(errRegionOperator) } +// OptimizeSystemDatabase is part of the eval.RegionOperator +// interface. +func (so *DummyRegionOperator) OptimizeSystemDatabase(_ context.Context) error { + return errors.WithStack(errRegionOperator) +} + // DummyStreamManagerFactory implements the eval.StreamManagerFactory interface by // returning errors. type DummyStreamManagerFactory struct{} diff --git a/pkg/sql/importer/import_table_creation.go b/pkg/sql/importer/import_table_creation.go index 2dfd5a59b88f..5474b2c27ce0 100644 --- a/pkg/sql/importer/import_table_creation.go +++ b/pkg/sql/importer/import_table_creation.go @@ -275,6 +275,11 @@ func (so *importRegionOperator) ResetMultiRegionZoneConfigsForDatabase( return errors.WithStack(errRegionOperator) } +// OptimizeSystemDatabase is part of the eval.RegionOperator interface. +func (so *importRegionOperator) OptimizeSystemDatabase(_ context.Context) error { + return errors.WithStack(errRegionOperator) +} + // Implements the eval.SequenceOperators interface. type importSequenceOperators struct{} diff --git a/pkg/sql/region_util.go b/pkg/sql/region_util.go index 0525f37ff6a5..00acbb665aef 100644 --- a/pkg/sql/region_util.go +++ b/pkg/sql/region_util.go @@ -17,6 +17,7 @@ import ( "strings" "github.com/cockroachdb/cockroach/pkg/config/zonepb" + "github.com/cockroachdb/cockroach/pkg/keys" "github.com/cockroachdb/cockroach/pkg/kv" "github.com/cockroachdb/cockroach/pkg/roachpb" "github.com/cockroachdb/cockroach/pkg/security/username" @@ -27,6 +28,8 @@ import ( "github.com/cockroachdb/cockroach/pkg/sql/catalog/descpb" "github.com/cockroachdb/cockroach/pkg/sql/catalog/descs" "github.com/cockroachdb/cockroach/pkg/sql/catalog/multiregion" + "github.com/cockroachdb/cockroach/pkg/sql/catalog/resolver" + "github.com/cockroachdb/cockroach/pkg/sql/catalog/systemschema" "github.com/cockroachdb/cockroach/pkg/sql/catalog/tabledesc" "github.com/cockroachdb/cockroach/pkg/sql/catalog/zone" "github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgcode" @@ -2446,3 +2449,172 @@ func (p *planner) GetMultiregionConfig( func (p *planner) IsANSIDML() bool { return p.stmt.IsANSIDML() } + +// OptimizeSystemDatabase is part of the eval.RegionOperator interface. +func (p *planner) OptimizeSystemDatabase(ctx context.Context) error { + globalTables := []string{ + "users", + "zones", + "privileges", + "comments", + "role_options", + "role_members", + "database_role_settings", + "settings", + "descriptor", + "namespace", + "table_statistics", + "web_sessions", + } + + rbrTables := []string{ + "sqlliveness", + "sql_instances", + } + + if !systemschema.TestSupportMultiRegion() { + return errors.New("multi region system database is not supported") + } + + // Retrieve the system database descriptor and ensure it supports + // multi-region + options := tree.CommonLookupFlags{ + AvoidLeased: true, + Required: true, + } + _, systemDB, err := p.Descriptors().GetImmutableDatabaseByID(ctx, p.txn, keys.SystemDatabaseID, options) + if err != nil { + return err + } + regionEnumID, err := systemDB.MultiRegionEnumID() + if err != nil { + return errors.Wrap(err, "system database is not multi-region") + } + enumTypeDesc, err := p.Descriptors().GetMutableTypeByID(ctx, p.txn, regionEnumID, tree.ObjectLookupFlags{ + CommonLookupFlags: options, + }) + if err != nil { + return err + } + + // Convert the enum descriptor into a type + enumName := tree.MakeQualifiedTypeName(systemDB.GetName(), "public", enumTypeDesc.GetName()) + enumType, err := enumTypeDesc.MakeTypesT(ctx, &enumName, nil) + if err != nil { + return err + } + + getDescriptor := func(name string) (*tabledesc.Mutable, error) { + tableName := tree.MakeTableNameWithSchema(tree.Name("system"), tree.Name("public"), tree.Name(name)) + required := true + _, desc, err := resolver.ResolveMutableExistingTableObject( + ctx, p, &tableName, required, tree.ResolveRequireTableDesc) + return desc, err + } + + applyLocalityChange := func(desc *tabledesc.Mutable, locality string) error { + if err := p.ResetMultiRegionZoneConfigsForTable(ctx, int64(desc.GetID())); err != nil { + return err + } + + jobName := fmt.Sprintf("convert system.%s to %s locality", desc.GetName(), locality) + return p.writeSchemaChange(ctx, desc, descpb.InvalidMutationID, jobName) + } + + getMutableColumn := func(table *tabledesc.Mutable, name string) (*descpb.ColumnDescriptor, error) { + for i := range table.Columns { + if table.Columns[i].Name == name { + return &table.Columns[i], nil + } + } + return nil, errors.Newf("%s column not found", name) + } + + partitionByRegion := func(table *tabledesc.Mutable) error { + regionConfig, err := SynthesizeRegionConfig( + ctx, p.txn, table.GetParentID(), p.Descriptors(), + ) + if err != nil { + return err + } + partitionAllBy := partitionByForRegionalByRow( + regionConfig, + "crdb_region", + ) + + unexpectedColumns, partitioning, err := CreatePartitioning( + ctx, + p.ExecCfg().Settings, + p.EvalContext(), + table, + table.PrimaryIndex, + partitionAllBy, + nil, /*do not allow implicit columns, the existing crdb_region column must be used*/ + true /*allow implicit partitioning */) + if err != nil { + return err + } + if 0 < len(unexpectedColumns) { + return errors.AssertionFailedf("unexpected implicit partitioning columns for table %s", table.GetName()) + } + tabledesc.UpdateIndexPartitioning(&table.PrimaryIndex, true, nil, partitioning) + table.PartitionAllBy = true + return nil + } + + // Configure global system tables + for _, tableName := range globalTables { + descriptor, err := getDescriptor(tableName) + if err != nil { + return err + } + + descriptor.SetTableLocalityGlobal() + + if err := applyLocalityChange(descriptor, "global"); err != nil { + return err + } + } + + // Configure regional by row system tables + for _, tableName := range rbrTables { + descriptor, err := getDescriptor(tableName) + if err != nil { + return err + } + + // Change crdb_region type to the multi-region enum + column, err := getMutableColumn(descriptor, "crdb_region") + if err != nil { + return err + } + column.Type = enumType + + // Add a back reference to the table + backReferenceJob := fmt.Sprintf("add back ref on mr-enum for system table %s", tableName) + if err = p.addTypeBackReference(ctx, regionEnumID, descriptor.GetID(), backReferenceJob); err != nil { + return err + } + + descriptor.SetTableLocalityRegionalByRow(tree.Name(column.Name)) + if err := partitionByRegion(descriptor); err != nil { + return err + } + + if err := applyLocalityChange(descriptor, "regional by row"); err != nil { + return err + } + + // Delete statistics for the table because the statistics materialize + // the column type for `crdb_region` and the column type is changing + // from bytes to an enum. + if _, err := p.ExecCfg().InternalExecutor.Exec(ctx, "delete-stats", p.txn, + `DELETE FROM system.table_statistics WHERE "tableID" = $1;`, + descriptor.GetID(), + ); err != nil { + return errors.Wrap(err, "unable to delete statistics") + } + } + + return nil +} diff --git a/pkg/sql/sem/builtins/builtins.go b/pkg/sql/sem/builtins/builtins.go index 326d0b7a8eb8..cc473a1ddecf 100644 --- a/pkg/sql/sem/builtins/builtins.go +++ b/pkg/sql/sem/builtins/builtins.go @@ -4926,6 +4926,22 @@ value if you rely on the HLC for accuracy.`, }, ), + "crdb_internal.unsafe_optimize_system_database": makeBuiltin( + tree.FunctionProperties{ + Category: builtinconstants.CategoryMultiRegion, + Undocumented: true, + }, + tree.Overload{ + Types: tree.ParamTypes{}, + ReturnType: tree.FixedReturnType(types.Bool), + Fn: func(ctx context.Context, evalCtx *eval.Context, args tree.Datums) (tree.Datum, error) { + return tree.MakeDBool(tree.DBool(true)), evalCtx.Regions.OptimizeSystemDatabase(ctx) + }, + Info: "Configures the sytem database to reduce latency during start up", + Volatility: volatility.Volatile, + }, + ), + "crdb_internal.destroy_tenant": makeBuiltin( tree.FunctionProperties{ Category: builtinconstants.CategoryMultiTenancy, diff --git a/pkg/sql/sem/builtins/fixed_oids.go b/pkg/sql/sem/builtins/fixed_oids.go index ce409811a412..a70d92f6c2f7 100644 --- a/pkg/sql/sem/builtins/fixed_oids.go +++ b/pkg/sql/sem/builtins/fixed_oids.go @@ -493,6 +493,7 @@ var builtinOidsBySignature = map[string]oid.Oid{ `crdb_internal.trim_tenant_prefix(key: bytes) -> bytes`: 1318, `crdb_internal.trim_tenant_prefix(keys: bytes[]) -> bytes[]`: 1319, `crdb_internal.unary_table() -> tuple`: 329, + `crdb_internal.unsafe_optimize_system_database() -> bool`: 2057, `crdb_internal.unsafe_clear_gossip_info(key: string) -> bool`: 1306, `crdb_internal.unsafe_delete_descriptor(id: int) -> bool`: 1347, `crdb_internal.unsafe_delete_descriptor(id: int, force: bool) -> bool`: 1348, diff --git a/pkg/sql/sem/eval/deps.go b/pkg/sql/sem/eval/deps.go index 655a3cb823da..36b4bdf61b37 100644 --- a/pkg/sql/sem/eval/deps.go +++ b/pkg/sql/sem/eval/deps.go @@ -505,6 +505,14 @@ type RegionOperator interface { // ResetMultiRegionZoneConfigsForDatabase resets the given database's zone // configuration to its multi-region default. ResetMultiRegionZoneConfigsForDatabase(ctx context.Context, id int64) error + + // OptimizeSystemDatabase configures some tables in the system data as + // global and regional by row. The locality changes reduce how long it + // takes a server to start up in a multi-region deployment. + // + // TODO(jeffswenson): remove OptimizeSystemDatabase after cleaning up the + // unsafe_optimize_system_database built in. + OptimizeSystemDatabase(ctx context.Context) error } // SequenceOperators is used for various sql related functions that can