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..117cbc24d695 100644 --- a/pkg/sql/sem/builtins/builtins.go +++ b/pkg/sql/sem/builtins/builtins.go @@ -4926,6 +4926,21 @@ 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", + }, + ), + "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