Skip to content

Commit

Permalink
multiregionccl: add unsafe_optimize_system_database
Browse files Browse the repository at this point in the history 
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 (cockroachdb#90408)
and (cockroachdb#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 cockroachdb#85736
  • Loading branch information
@jeffswenson
jeffswenson committed Dec 6, 2022
1 parent c4c1d20 commit bb388da
Show file tree
Hide file tree
Showing 8 changed files with 256 additions and 104 deletions.
3 changes: 0 additions & 3 deletions pkg/ccl/multiregionccl/BUILD.bazel
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand All @@ -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",
Expand Down
149 changes: 48 additions & 101 deletions pkg/ccl/multiregionccl/multiregion_system_table_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -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)
Expand All @@ -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
Expand All @@ -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
Expand Down Expand Up @@ -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) {
Expand Down
6 changes: 6 additions & 0 deletions pkg/sql/faketreeeval/evalctx.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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{}
Expand Down
5 changes: 5 additions & 0 deletions pkg/sql/importer/import_table_creation.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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{}

Expand Down
Loading

0 comments on commit bb388da

Please sign in to comment.