From 56175655486ac94c892dee1669b85b7469018c0d Mon Sep 17 00:00:00 2001 From: Evan Wall Date: Fri, 18 Nov 2022 06:37:46 -0500 Subject: [PATCH] sql: support crdb_internal.ranges{_no_leases} for secondary tenants This PR makes crdb_internal.ranges{_no_leases} and SHOW RANGES work for secondary tenants. Release note (sql change): crdb_internal.ranges{_no_leases} and SHOW RANGES statements now work on secondary tenants. Epic: CRDB-14522 --- .../tenant_scan_range_descriptors_test.go | 7 +- .../testdata/logic_test/crdb_internal_tenant | 12 +- pkg/keys/sql.go | 6 + pkg/sql/crdb_internal.go | 46 +++--- pkg/sql/multitenant_admin_function_test.go | 135 ++++++++++-------- pkg/util/errorutil/tenant.go | 2 +- pkg/workload/workloadsql/workloadsql.go | 12 ++ 7 files changed, 125 insertions(+), 95 deletions(-) diff --git a/pkg/ccl/kvccl/kvtenantccl/tenant_scan_range_descriptors_test.go b/pkg/ccl/kvccl/kvtenantccl/tenant_scan_range_descriptors_test.go index d669242175ea..47e133145eab 100644 --- a/pkg/ccl/kvccl/kvtenantccl/tenant_scan_range_descriptors_test.go +++ b/pkg/ccl/kvccl/kvtenantccl/tenant_scan_range_descriptors_test.go @@ -46,7 +46,7 @@ func TestScanRangeDescriptors(t *testing.T) { }) require.NoError(t, err) - //Split some ranges within tenant2 that we'll scan over. + // Split some ranges within tenant2 that we'll scan over. ten2Codec := keys.MakeSQLCodec(ten2ID) ten2Split1 := append(ten2Codec.TenantPrefix(), 'a') ten2Split2 := append(ten2Codec.TenantPrefix(), 'b') @@ -57,10 +57,7 @@ func TestScanRangeDescriptors(t *testing.T) { } iteratorFactory := tenant2.RangeDescIteratorFactory().(rangedesc.IteratorFactory) - iter, err := iteratorFactory.NewIterator(ctx, roachpb.Span{ - Key: ten2Codec.TenantPrefix(), - EndKey: ten2Codec.TenantPrefix().PrefixEnd(), - }) + iter, err := iteratorFactory.NewIterator(ctx, ten2Codec.TenantSpan()) require.NoError(t, err) var rangeDescs []roachpb.RangeDescriptor diff --git a/pkg/ccl/logictestccl/testdata/logic_test/crdb_internal_tenant b/pkg/ccl/logictestccl/testdata/logic_test/crdb_internal_tenant index d60c2378fc0a..b7b6627f9511 100644 --- a/pkg/ccl/logictestccl/testdata/logic_test/crdb_internal_tenant +++ b/pkg/ccl/logictestccl/testdata/logic_test/crdb_internal_tenant @@ -301,11 +301,15 @@ SELECT * FROM crdb_internal.node_inflight_trace_spans WHERE span_id < 0 ---- trace_id parent_span_id span_id goroutine_id finished start_time duration operation -statement error not fully contained in tenant keyspace -SELECT * FROM crdb_internal.ranges WHERE range_id < 0 +query ITTI +SELECT range_id, start_pretty, end_pretty, lease_holder FROM crdb_internal.ranges +---- +55 /Tenant/10 /Max 1 -statement error not fully contained in tenant keyspace -SELECT * FROM crdb_internal.ranges_no_leases WHERE range_id < 0 +query ITT +SELECT range_id, start_pretty, end_pretty FROM crdb_internal.ranges_no_leases +---- +55 /Tenant/10 /Max query IT SELECT zone_id, target FROM crdb_internal.zones ORDER BY 1 diff --git a/pkg/keys/sql.go b/pkg/keys/sql.go index eecc70e21889..8b21314b86de 100644 --- a/pkg/keys/sql.go +++ b/pkg/keys/sql.go @@ -121,6 +121,12 @@ func (e sqlEncoder) TenantPrefix() roachpb.Key { return *e.buf } +// TenantPrefix returns the key prefix used for the tenants's data. +func (e sqlEncoder) TenantSpan() roachpb.Span { + key := *e.buf + return roachpb.Span{Key: key, EndKey: key.PrefixEnd()} +} + // TablePrefix returns the key prefix used for the table's data. func (e sqlEncoder) TablePrefix(tableID uint32) roachpb.Key { k := e.TenantPrefix() diff --git a/pkg/sql/crdb_internal.go b/pkg/sql/crdb_internal.go index f5976a079774..9859ee5d2484 100644 --- a/pkg/sql/crdb_internal.go +++ b/pkg/sql/crdb_internal.go @@ -31,7 +31,6 @@ import ( "github.com/cockroachdb/cockroach/pkg/jobs/jobspb" "github.com/cockroachdb/cockroach/pkg/keys" "github.com/cockroachdb/cockroach/pkg/kv" - "github.com/cockroachdb/cockroach/pkg/kv/kvclient" "github.com/cockroachdb/cockroach/pkg/kv/kvclient/kvcoord" "github.com/cockroachdb/cockroach/pkg/kv/kvserver/concurrency/lock" "github.com/cockroachdb/cockroach/pkg/kv/kvserver/liveness/livenesspb" @@ -3660,34 +3659,27 @@ CREATE TABLE crdb_internal.ranges_no_leases ( if !hasPermission { return nil, nil, pgerror.Newf(pgcode.InsufficientPrivilege, "only users with the ZONECONFIG privilege or the admin role can read crdb_internal.ranges_no_leases") } - ranges, err := kvclient.ScanMetaKVs(ctx, p.txn, roachpb.Span{ - Key: keys.MinKey, - EndKey: keys.MaxKey, - }) + + execCfg := p.ExecCfg() + rangeDescIterator, err := execCfg.RangeDescIteratorFactory.NewIterator(ctx, execCfg.Codec.TenantSpan()) if err != nil { return nil, nil, err } - var desc roachpb.RangeDescriptor - - i := 0 - return func() (tree.Datums, error) { - if i >= len(ranges) { + if !rangeDescIterator.Valid() { return nil, nil } - r := ranges[i] - i++ + rangeDesc := rangeDescIterator.CurRangeDescriptor() - if err := r.ValueProto(&desc); err != nil { - return nil, err - } + rangeDescIterator.Next() + replicas := rangeDesc.Replicas() votersAndNonVoters := append([]roachpb.ReplicaDescriptor(nil), - desc.Replicas().VoterAndNonVoterDescriptors()...) + replicas.VoterAndNonVoterDescriptors()...) var learnerReplicaStoreIDs []int - for _, rd := range desc.Replicas().LearnerDescriptors() { + for _, rd := range replicas.LearnerDescriptors() { learnerReplicaStoreIDs = append(learnerReplicaStoreIDs, int(rd.StoreID)) } sort.Slice(votersAndNonVoters, func(i, j int) bool { @@ -3701,13 +3693,13 @@ CREATE TABLE crdb_internal.ranges_no_leases ( } } votersArr := tree.NewDArray(types.Int) - for _, replica := range desc.Replicas().VoterDescriptors() { + for _, replica := range replicas.VoterDescriptors() { if err := votersArr.Append(tree.NewDInt(tree.DInt(replica.StoreID))); err != nil { return nil, err } } nonVotersArr := tree.NewDArray(types.Int) - for _, replica := range desc.Replicas().NonVoterDescriptors() { + for _, replica := range replicas.NonVoterDescriptors() { if err := nonVotersArr.Append(tree.NewDInt(tree.DInt(replica.StoreID))); err != nil { return nil, err } @@ -3732,21 +3724,21 @@ CREATE TABLE crdb_internal.ranges_no_leases ( } tableID, dbName, schemaName, tableName, indexName := lookupNamesByKey( - p, desc.StartKey.AsRawKey(), dbNames, tableNames, schemaNames, + p, rangeDesc.StartKey.AsRawKey(), dbNames, tableNames, schemaNames, indexNames, schemaParents, parents, ) splitEnforcedUntil := tree.DNull - if !desc.StickyBit.IsEmpty() { - splitEnforcedUntil = eval.TimestampToInexactDTimestamp(desc.StickyBit) + if !rangeDesc.StickyBit.IsEmpty() { + splitEnforcedUntil = eval.TimestampToInexactDTimestamp(rangeDesc.StickyBit) } return tree.Datums{ - tree.NewDInt(tree.DInt(desc.RangeID)), - tree.NewDBytes(tree.DBytes(desc.StartKey)), - tree.NewDString(keys.PrettyPrint(nil /* valDirs */, desc.StartKey.AsRawKey())), - tree.NewDBytes(tree.DBytes(desc.EndKey)), - tree.NewDString(keys.PrettyPrint(nil /* valDirs */, desc.EndKey.AsRawKey())), + tree.NewDInt(tree.DInt(rangeDesc.RangeID)), + tree.NewDBytes(tree.DBytes(rangeDesc.StartKey)), + tree.NewDString(keys.PrettyPrint(nil /* valDirs */, rangeDesc.StartKey.AsRawKey())), + tree.NewDBytes(tree.DBytes(rangeDesc.EndKey)), + tree.NewDString(keys.PrettyPrint(nil /* valDirs */, rangeDesc.EndKey.AsRawKey())), tree.NewDInt(tree.DInt(tableID)), tree.NewDString(dbName), tree.NewDString(schemaName), diff --git a/pkg/sql/multitenant_admin_function_test.go b/pkg/sql/multitenant_admin_function_test.go index 5840ef14b3da..7ba1b020ff74 100644 --- a/pkg/sql/multitenant_admin_function_test.go +++ b/pkg/sql/multitenant_admin_function_test.go @@ -67,91 +67,98 @@ func TestMultiTenantAdminFunction(t *testing.T) { defer leaktest.AfterTest(t)() defer log.Scope(t).Close(t) + // TODO(ewall): When moving tenant capability check to KV, + // merge test w/ SkipSQLSystemTenantCheck variant into single test case if + // 1) test case has expectedErrorMessage: errorutil.UnsupportedWithMultiTenancyMessage + // and + // 2) SkipSQLSystemTenantCheck variant has no error message testCases := []struct { desc string setup string setups []string query string - errorMessage string + expectedErrorMessage string allowSplitAndScatter bool skipSQLSystemTenantCheck bool }{ { - desc: "ALTER RANGE x RELOCATE LEASE", - query: "ALTER RANGE (SELECT min(range_id) FROM [SHOW RANGES FROM TABLE t]) RELOCATE LEASE TO 1;", + desc: "ALTER RANGE x RELOCATE LEASE", + query: "ALTER RANGE (SELECT min(range_id) FROM [SHOW RANGES FROM TABLE t]) RELOCATE LEASE TO 1;", + expectedErrorMessage: errorutil.UnsupportedWithMultiTenancyMessage, }, { desc: "ALTER RANGE x RELOCATE LEASE SkipSQLSystemTenantCheck", query: "ALTER RANGE (SELECT min(range_id) FROM [SHOW RANGES FROM TABLE t]) RELOCATE LEASE TO 1;", - errorMessage: rangeErrorMessage, skipSQLSystemTenantCheck: true, }, { - desc: "ALTER RANGE RELOCATE LEASE", - query: "ALTER RANGE RELOCATE LEASE TO 1 FOR (SELECT min(range_id) FROM [SHOW RANGES FROM TABLE t]);", + desc: "ALTER RANGE RELOCATE LEASE", + query: "ALTER RANGE RELOCATE LEASE TO 1 FOR (SELECT min(range_id) FROM [SHOW RANGES FROM TABLE t]);", + expectedErrorMessage: errorutil.UnsupportedWithMultiTenancyMessage, }, { desc: "ALTER RANGE RELOCATE LEASE SkipSQLSystemTenantCheck", query: "ALTER RANGE RELOCATE LEASE TO 1 FOR (SELECT min(range_id) FROM [SHOW RANGES FROM TABLE t]);", - errorMessage: rangeErrorMessage, skipSQLSystemTenantCheck: true, }, { - desc: "ALTER RANGE x RELOCATE VOTERS", - query: "ALTER RANGE (SELECT min(range_id) FROM [SHOW RANGES FROM TABLE t]) RELOCATE VOTERS FROM 1 TO 1;", + desc: "ALTER RANGE x RELOCATE VOTERS", + query: "ALTER RANGE (SELECT min(range_id) FROM [SHOW RANGES FROM TABLE t]) RELOCATE VOTERS FROM 1 TO 1;", + expectedErrorMessage: errorutil.UnsupportedWithMultiTenancyMessage, }, { desc: "ALTER RANGE x RELOCATE VOTERS SkipSQLSystemTenantCheck", query: "ALTER RANGE (SELECT min(range_id) FROM [SHOW RANGES FROM TABLE t]) RELOCATE VOTERS FROM 1 TO 1;", - errorMessage: rangeErrorMessage, skipSQLSystemTenantCheck: true, }, { - desc: "ALTER RANGE RELOCATE VOTERS", - query: "ALTER RANGE RELOCATE VOTERS FROM 1 TO 1 FOR (SELECT min(range_id) FROM [SHOW RANGES FROM TABLE t]);", + desc: "ALTER RANGE RELOCATE VOTERS", + query: "ALTER RANGE RELOCATE VOTERS FROM 1 TO 1 FOR (SELECT min(range_id) FROM [SHOW RANGES FROM TABLE t]);", + expectedErrorMessage: errorutil.UnsupportedWithMultiTenancyMessage, }, { desc: "ALTER RANGE RELOCATE VOTERS SkipSQLSystemTenantCheck", query: "ALTER RANGE RELOCATE VOTERS FROM 1 TO 1 FOR (SELECT min(range_id) FROM [SHOW RANGES FROM TABLE t]);", - errorMessage: rangeErrorMessage, skipSQLSystemTenantCheck: true, }, { - desc: "ALTER TABLE x EXPERIMENTAL_RELOCATE LEASE", - query: "ALTER TABLE t EXPERIMENTAL_RELOCATE LEASE SELECT 1, 1;", + desc: "ALTER TABLE x EXPERIMENTAL_RELOCATE LEASE", + query: "ALTER TABLE t EXPERIMENTAL_RELOCATE LEASE SELECT 1, 1;", + expectedErrorMessage: errorutil.UnsupportedWithMultiTenancyMessage, }, { desc: "ALTER TABLE x EXPERIMENTAL_RELOCATE LEASE SkipSQLSystemTenantCheck", query: "ALTER TABLE t EXPERIMENTAL_RELOCATE LEASE SELECT 1, 1;", - errorMessage: rangeErrorMessage, + expectedErrorMessage: rangeErrorMessage, skipSQLSystemTenantCheck: true, }, { - desc: "ALTER TABLE x EXPERIMENTAL_RELOCATE VOTERS", - query: "ALTER TABLE t EXPERIMENTAL_RELOCATE VOTERS SELECT ARRAY[1], 1;", + desc: "ALTER TABLE x EXPERIMENTAL_RELOCATE VOTERS", + query: "ALTER TABLE t EXPERIMENTAL_RELOCATE VOTERS SELECT ARRAY[1], 1;", + expectedErrorMessage: errorutil.UnsupportedWithMultiTenancyMessage, }, { desc: "ALTER TABLE x EXPERIMENTAL_RELOCATE VOTERS SkipSQLSystemTenantCheck", query: "ALTER TABLE t EXPERIMENTAL_RELOCATE VOTERS SELECT ARRAY[1], 1;", - errorMessage: rangeErrorMessage, + expectedErrorMessage: rangeErrorMessage, skipSQLSystemTenantCheck: true, }, { - desc: "ALTER TABLE x SPLIT AT", - query: "ALTER TABLE t SPLIT AT VALUES (1);", - errorMessage: "request [1 AdmSplit] not permitted", + desc: "ALTER TABLE x SPLIT AT", + query: "ALTER TABLE t SPLIT AT VALUES (1);", + expectedErrorMessage: "request [1 AdmSplit] not permitted", }, { - desc: "ALTER INDEX x SPLIT AT", - setup: "CREATE INDEX idx on t(i);", - query: "ALTER INDEX t@idx SPLIT AT VALUES (1);", - errorMessage: "request [1 AdmSplit] not permitted", + desc: "ALTER INDEX x SPLIT AT", + setup: "CREATE INDEX idx on t(i);", + query: "ALTER INDEX t@idx SPLIT AT VALUES (1);", + expectedErrorMessage: "request [1 AdmSplit] not permitted", }, { desc: "ALTER TABLE x UNSPLIT AT", setup: "ALTER TABLE t SPLIT AT VALUES (1);", query: "ALTER TABLE t UNSPLIT AT VALUES (1);", - errorMessage: "request [1 AdmUnsplit] not permitted", + expectedErrorMessage: "request [1 AdmUnsplit] not permitted", allowSplitAndScatter: true, }, { @@ -162,45 +169,46 @@ func TestMultiTenantAdminFunction(t *testing.T) { "ALTER INDEX t@idx SPLIT AT VALUES (1);", }, query: "ALTER INDEX t@idx UNSPLIT AT VALUES (1);", - errorMessage: "request [1 AdmUnsplit] not permitted", + expectedErrorMessage: "request [1 AdmUnsplit] not permitted", allowSplitAndScatter: true, }, { - desc: "ALTER TABLE x UNSPLIT ALL", - query: "ALTER TABLE t UNSPLIT ALL;", + desc: "ALTER TABLE x UNSPLIT ALL", + query: "ALTER TABLE t UNSPLIT ALL;", + expectedErrorMessage: errorutil.UnsupportedWithMultiTenancyMessage, }, { desc: "ALTER TABLE x UNSPLIT ALL SkipSQLSystemTenantCheck", query: "ALTER TABLE t UNSPLIT ALL;", - errorMessage: rangeErrorMessage, skipSQLSystemTenantCheck: true, }, { - desc: "ALTER INDEX x UNSPLIT ALL", - setup: "CREATE INDEX idx on t(i);", - query: "ALTER INDEX t@idx UNSPLIT ALL;", + desc: "ALTER INDEX x UNSPLIT ALL", + setup: "CREATE INDEX idx on t(i);", + query: "ALTER INDEX t@idx UNSPLIT ALL;", + expectedErrorMessage: errorutil.UnsupportedWithMultiTenancyMessage, }, { desc: "ALTER INDEX x UNSPLIT ALL SkipSQLSystemTenantCheck", setup: "CREATE INDEX idx on t(i);", query: "ALTER INDEX t@idx UNSPLIT ALL;", - errorMessage: rangeErrorMessage, skipSQLSystemTenantCheck: true, }, { - desc: "ALTER TABLE x SCATTER", - query: "ALTER TABLE t SCATTER;", - errorMessage: "request [1 AdmScatter] not permitted", + desc: "ALTER TABLE x SCATTER", + query: "ALTER TABLE t SCATTER;", + expectedErrorMessage: "request [1 AdmScatter] not permitted", }, { - desc: "ALTER INDEX x SCATTER", - setup: "CREATE INDEX idx on t(i);", - query: "ALTER INDEX t@idx SCATTER;", - errorMessage: "request [1 AdmScatter] not permitted", + desc: "ALTER INDEX x SCATTER", + setup: "CREATE INDEX idx on t(i);", + query: "ALTER INDEX t@idx SCATTER;", + expectedErrorMessage: "request [1 AdmScatter] not permitted", }, { - desc: "CONFIGURE ZONE", - query: "ALTER TABLE t CONFIGURE ZONE DISCARD;", + desc: "CONFIGURE ZONE", + query: "ALTER TABLE t CONFIGURE ZONE DISCARD;", + expectedErrorMessage: errorutil.UnsupportedWithMultiTenancyMessage, }, } @@ -239,12 +247,13 @@ func TestMultiTenantAdminFunction(t *testing.T) { defer cleanup() db := createSecondaryTenantDB(t, testServer, tc.allowSplitAndScatter, tc.skipSQLSystemTenantCheck) err := execQueries(db) - require.Error(t, err) - errorMessage := tc.errorMessage - if errorMessage == "" { - errorMessage = errorutil.UnsupportedWithMultiTenancyMessage + expectedErrorMessage := tc.expectedErrorMessage + if expectedErrorMessage == "" { + require.NoError(t, err) + } else { + require.Error(t, err) + require.Contains(t, err.Error(), expectedErrorMessage) } - require.Contains(t, err.Error(), errorMessage) }() }) } @@ -255,16 +264,28 @@ func TestTruncateTable(t *testing.T) { defer log.Scope(t).Close(t) ctx := context.Background() - execQueries := func(db *gosql.DB) error { + execQueries := func(db *gosql.DB, expectedStartKeys []string, expectedEndKeys []string) { _, err := db.ExecContext(ctx, createTable) require.NoError(t, err) _, err = db.ExecContext(ctx, "ALTER TABLE t SPLIT AT VALUES (1);") require.NoError(t, err) _, err = db.ExecContext(ctx, "TRUNCATE TABLE t;") require.NoError(t, err) - _, err = db.QueryContext(ctx, "SHOW RANGES FROM TABLE t;") - // TODO(ewall): Verify splits are not retained after `SHOW RANGES` is supported by secondary tenants. - return err + rows, err := db.QueryContext(ctx, "SELECT start_key, end_key from [SHOW RANGES FROM TABLE t];") + require.NoError(t, err) + i := 0 + for ; rows.Next(); i++ { + var startKey, endKey gosql.NullString + err := rows.Scan(&startKey, &endKey) + require.NoError(t, err, i) + require.Less(t, i, len(expectedStartKeys), i) + require.Equal(t, startKey.String, expectedStartKeys[i], i) + require.Less(t, i, len(expectedEndKeys), i) + require.Equal(t, endKey.String, expectedEndKeys[i], i) + } + require.NoError(t, rows.Err()) + require.Equal(t, len(expectedStartKeys), i) + require.Equal(t, len(expectedEndKeys), i) } // Test system tenant. @@ -272,8 +293,7 @@ func TestTruncateTable(t *testing.T) { testServer, cleanup := createTestServer(t) defer cleanup() db := createSystemTenantDB(t, testServer) - err := execQueries(db) - require.NoError(t, err) + execQueries(db, []string{"", "/1"}, []string{"/1", ""}) }() // Test secondary tenant. @@ -286,8 +306,7 @@ func TestTruncateTable(t *testing.T) { true, /* allowSplitAndScatter */ false, /* skipSQLSystemTenantCheck */ ) - err := execQueries(db) - require.Error(t, err) - require.Contains(t, err.Error(), rangeErrorMessage) + // TODO(ewall): Retain splits after `TRUNCATE` for secondary tenants. + execQueries(db, []string{""}, []string{""}) }() } diff --git a/pkg/util/errorutil/tenant.go b/pkg/util/errorutil/tenant.go index 5efefa0d2b29..b099ca5b4eed 100644 --- a/pkg/util/errorutil/tenant.go +++ b/pkg/util/errorutil/tenant.go @@ -12,7 +12,7 @@ package errorutil import "github.com/cockroachdb/cockroach/pkg/util/errorutil/unimplemented" -// UnsupportedWithMultiTenancyMessage is the message used by UnsupportedWithMultiTenancy error +// UnsupportedWithMultiTenancyMessage is the message used by UnsupportedWithMultiTenancy error. const UnsupportedWithMultiTenancyMessage = "operation is unsupported in multi-tenancy mode" // UnsupportedWithMultiTenancy returns an error suitable for returning when an diff --git a/pkg/workload/workloadsql/workloadsql.go b/pkg/workload/workloadsql/workloadsql.go index 5c8fdf301d83..d8efb220d7a7 100644 --- a/pkg/workload/workloadsql/workloadsql.go +++ b/pkg/workload/workloadsql/workloadsql.go @@ -118,6 +118,18 @@ func Split(ctx context.Context, db *gosql.DB, table workload.Table, concurrency if table.Splits.NumBatches <= 0 { return nil } + + // Test that we can actually perform a scatter. + if _, err := db.Exec("ALTER TABLE system.jobs SCATTER"); err != nil { + if strings.Contains(err.Error(), "not fully contained in tenant") || + strings.Contains(err.Error(), "request [1 AdmScatter] not permitted") { + log.Infof(ctx, `skipping workload splits; can't scatter on tenants'`) + //nolint:returnerrcheck + return nil + } + return err + } + splitPoints := make([][]interface{}, 0, table.Splits.NumBatches) for splitIdx := 0; splitIdx < table.Splits.NumBatches; splitIdx++ { splitPoints = append(splitPoints, table.Splits.BatchRows(splitIdx)...)