Merge #97321 #98264 #98546 #98615

97321: copy: enhance copyfrom tests with kvtrace feature and more tests r=cucaroach a=cucaroach

Epic: CRDB-18892
Informs: #91831
Release note: None


98264: colfetcher: track KV CPU time in the direct columnar scan r=yuzefovich a=yuzefovich

This commit addresses a minor TODO to track the KV CPU time when direct
columnar scans are used. In the regular columnar scan this time is
tracked by the cFetcher, but with the KV projection pushdown the
cFetcher is used on the KV server side, so we need to augment the
ColBatchDirectScan to track it. Notably, this means that the decoding
done on the KV server side is included. Additionally, this commit
clarifies how the KV CPU time is obtained from the cFetcher (we don't
need to use a helper (unlike in the case of `bytesRead` and
`batchRequestsIssued` fields which are written to on `cFetcher.Close`),
and we don't need the mutex protection there either).

Epic: None

Release note: None

98546: multitenant: allow secondary tenants to split/scatter by default r=knz a=arulajmani

AdminSplit and AdminScatter requests are subject to capability checks.
Previously, these capabilities were codified in the "enabled" form. As
such, by default, secondary tenants did not have the ability to perform
these operations. This is in violation of what secondary tenants could
do prior to 23.1, at a time before capabilities existed. Moreover,
RESTORE/IMPORT rely on performing these operations for performance.
This made disallowing these operations by default a performance
regression.

This patch flips the phrasing of how these capabilities are stored on
the proto to use the "disable" verbiage. As such, secondary tenants are
able to perform splits and scatters by default. However, no change is
made to the public interface -- users above the `tenantcapabilitiespb`
package continue to interact with these capabilities as they were
before, oblivious to how these things are stored on disk.

There's a few testing changes here:
- As part of this change, we also clean up a testing knob that was used
by various backup, CDC, and logictests to override capability checks in
the authorizer. This isn't required with the new default behaviour.
- We also add some missing E2E tests for the `CanAdminUnsplit` capability
which were missing when it was introduced.

Fixes #96736

Release note: None

98615: sql_test: re-skip TestShowTraceReplica r=msirek a=msirek

TestShowTraceReplica wasn't failing under stress, but failed in TeamCity
once enabled. This re-skips the test until it can be reliably reproduced
and debugged.

Informs #98598

Release note: None 

Co-authored-by: Tommy Reilly <[email protected]>
Co-authored-by: Yahor Yuzefovich <[email protected]>
Co-authored-by: Raphael 'kena' Poss <[email protected]>
Co-authored-by: Arul Ajmani <[email protected]>
Co-authored-by: Mark Sirek <[email protected]>

pkg/ccl/backupccl/BUILD.bazel

@@ -240,7 +240,6 @@ go_test(
         "//pkg/kv/kvserver/protectedts/ptpb",
         "//pkg/kv/kvserver/protectedts/ptutil",
         "//pkg/multitenant/mtinfopb",
-        "//pkg/multitenant/tenantcapabilities",
         "//pkg/roachpb",
         "//pkg/scheduledjobs",
         "//pkg/scheduledjobs/schedulebase",

pkg/ccl/backupccl/backup_test.go

@@ -67,7 +67,6 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/kv/kvserver/protectedts"
 	"github.com/cockroachdb/cockroach/pkg/kv/kvserver/protectedts/ptutil"
 	"github.com/cockroachdb/cockroach/pkg/multitenant/mtinfopb"
-	"github.com/cockroachdb/cockroach/pkg/multitenant/tenantcapabilities"
 	"github.com/cockroachdb/cockroach/pkg/roachpb"
 	"github.com/cockroachdb/cockroach/pkg/security"
 	"github.com/cockroachdb/cockroach/pkg/security/username"
@@ -2949,13 +2948,6 @@ func TestBackupRestoreCrossTableReferences(t *testing.T) {
 	defer cleanupFn()
 	args := base.TestServerArgs{
 		ExternalIODir: dir,
-		Knobs: base.TestingKnobs{
-			TenantCapabilitiesTestingKnobs: &tenantcapabilities.TestingKnobs{
-				// TODO(arul): This can be removed once
-				// https://github.com/cockroachdb/cockroach/issues/96736  is fixed.
-				AuthorizerSkipAdminSplitCapabilityChecks: true,
-			},
-		},
 	}
 
 	// Generate some testdata and back it up.
@@ -5296,13 +5288,6 @@ func TestBackupRestoreSequence(t *testing.T) {
 	defer cleanupFn()
 	args := base.TestServerArgs{
 		ExternalIODir: dir,
-		Knobs: base.TestingKnobs{
-			TenantCapabilitiesTestingKnobs: &tenantcapabilities.TestingKnobs{
-				// TODO(arul): This can be removed once
-				// https://github.com/cockroachdb/cockroach/issues/96736  is fixed.
-				AuthorizerSkipAdminSplitCapabilityChecks: true,
-			},
-		},
 	}
 
 	backupLoc := localFoo

pkg/ccl/backupccl/datadriven_test.go

@@ -31,7 +31,6 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/kv/kvclient/kvcoord"
 	"github.com/cockroachdb/cockroach/pkg/kv/kvpb"
 	"github.com/cockroachdb/cockroach/pkg/kv/kvserver/closedts"
-	"github.com/cockroachdb/cockroach/pkg/multitenant/tenantcapabilities"
 	"github.com/cockroachdb/cockroach/pkg/roachpb"
 	"github.com/cockroachdb/cockroach/pkg/server"
 	"github.com/cockroachdb/cockroach/pkg/settings/cluster"
@@ -155,14 +154,6 @@ func (d *datadrivenTestState) addCluster(t *testing.T, cfg clusterCfg) error {
 	params.ServerArgs.Knobs = base.TestingKnobs{
 		JobsTestingKnobs: jobs.NewTestingKnobsWithShortIntervals(),
 	}
-	// Backups issue splits underneath the hood, and as such, will fail capability
-	// checks for tests that run as secondary tenants. Skip these checks at a
-	// global level using a testing knob.
-	params.ServerArgs.Knobs.TenantCapabilitiesTestingKnobs = &tenantcapabilities.TestingKnobs{
-		// TODO(arul): This can be removed once
-		// https://github.com/cockroachdb/cockroach/issues/96736  is fixed.
-		AuthorizerSkipAdminSplitCapabilityChecks: true,
-	}
 
 	settings := cluster.MakeTestingClusterSettings()
 

pkg/ccl/backupccl/utils_test.go

@@ -35,7 +35,6 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/keyvisualizer"
 	"github.com/cockroachdb/cockroach/pkg/kv"
 	"github.com/cockroachdb/cockroach/pkg/kv/kvserver"
-	"github.com/cockroachdb/cockroach/pkg/multitenant/tenantcapabilities"
 	"github.com/cockroachdb/cockroach/pkg/roachpb"
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog/desctestutils"
 	"github.com/cockroachdb/cockroach/pkg/sql/sem/tree"
@@ -96,11 +95,6 @@ func backupRestoreTestSetupWithParams(
 		}
 		params.ServerArgs.Knobs.Store.(*kvserver.StoreTestingKnobs).SmallEngineBlocks = true
 	}
-	params.ServerArgs.Knobs.TenantCapabilitiesTestingKnobs = &tenantcapabilities.TestingKnobs{
-		// TODO(arul): This can be removed once
-		// https://github.com/cockroachdb/cockroach/issues/96736  is fixed.
-		AuthorizerSkipAdminSplitCapabilityChecks: true,
-	}
 
 	params.ServerArgs.Knobs.KeyVisualizer = &keyvisualizer.TestingKnobs{
 		SkipJobBootstrap:        true,
@@ -154,13 +148,6 @@ func backupRestoreTestSetup(
 		base.TestClusterArgs{
 			ServerArgs: base.TestServerArgs{
 				DisableDefaultTestTenant: true,
-				Knobs: base.TestingKnobs{
-					TenantCapabilitiesTestingKnobs: &tenantcapabilities.TestingKnobs{
-						// TODO(arul): This can be removed once
-						// https://github.com/cockroachdb/cockroach/issues/96736  is fixed.
-						AuthorizerSkipAdminSplitCapabilityChecks: true,
-					},
-				},
 			}})
 }
 
@@ -173,11 +160,6 @@ func backupRestoreTestSetupEmpty(
 ) (tc *testcluster.TestCluster, sqlDB *sqlutils.SQLRunner, cleanup func()) {
 	// TODO (msbutler): this should be disabled by callers of this function
 	params.ServerArgs.DisableDefaultTestTenant = true
-	params.ServerArgs.Knobs.TenantCapabilitiesTestingKnobs = &tenantcapabilities.TestingKnobs{
-		// TODO(arul): This can be removed once
-		// https://github.com/cockroachdb/cockroach/issues/96736  is fixed.
-		AuthorizerSkipAdminSplitCapabilityChecks: true,
-	}
 	return backupRestoreTestSetupEmptyWithParams(t, clusterSize, tempDir, init, params)
 }
 
@@ -205,11 +187,6 @@ func backupRestoreTestSetupEmptyWithParams(
 		}
 		params.ServerArgs.Knobs.Store.(*kvserver.StoreTestingKnobs).SmallEngineBlocks = true
 	}
-	params.ServerArgs.Knobs.TenantCapabilitiesTestingKnobs = &tenantcapabilities.TestingKnobs{
-		// TODO(arul): This can be removed once
-		// https://github.com/cockroachdb/cockroach/issues/96736  is fixed.
-		AuthorizerSkipAdminSplitCapabilityChecks: true,
-	}
 
 	tc = testcluster.StartTestCluster(t, clusterSize, params)
 	init(tc)
@@ -235,11 +212,6 @@ func createEmptyCluster(
 	params.ServerArgs.Knobs.Store = &kvserver.StoreTestingKnobs{
 		SmallEngineBlocks: smallEngineBlocks,
 	}
-	params.ServerArgs.Knobs.TenantCapabilitiesTestingKnobs = &tenantcapabilities.TestingKnobs{
-		// TODO(arul): This can be removed once
-		// https://github.com/cockroachdb/cockroach/issues/96736  is fixed.
-		AuthorizerSkipAdminSplitCapabilityChecks: true,
-	}
 	tc := testcluster.StartTestCluster(t, clusterSize, params)
 
 	sqlDB = sqlutils.MakeSQLRunner(tc.Conns[0])

pkg/ccl/logictestccl/testdata/logic_test/crdb_internal

@@ -194,8 +194,7 @@ query ITT colnames,retry
 SELECT * FROM crdb_internal.node_tenant_capabilities_cache WHERE capability_id = 'can_admin_split'
 ----
 tenant_id  capability_id    capability_value
-1          can_admin_split  false
-5          can_admin_split  false
+1          can_admin_split  true
 
 statement ok
 ALTER TENANT [5] GRANT CAPABILITY can_admin_split
@@ -205,7 +204,7 @@ query ITT colnames,retry
 SELECT * FROM crdb_internal.node_tenant_capabilities_cache WHERE capability_id = 'can_admin_split'
 ----
 tenant_id  capability_id    capability_value
-1          can_admin_split  false
+1          can_admin_split  true
 5          can_admin_split  true
 
 subtest end

pkg/ccl/logictestccl/testdata/logic_test/multi_region_backup

@@ -1,6 +1,5 @@
 # tenant-cluster-setting-override-opt: sql.zone_configs.allow_for_secondary_tenant.enabled=true sql.multi_region.allow_abstractions_for_secondary_tenants.enabled=true
 # LogicTest: multiregion-9node-3region-3azs multiregion-9node-3region-3azs-no-los
-# cluster-opt: can-admin-split
 
 # Tests in this file assume no multi-region tenant setup as tenants have no
 # access to nodelocal.

pkg/ccl/logictestccl/testdata/logic_test/partitioning_hash_sharded_index_mr

@@ -1,5 +1,4 @@
 # tenant-cluster-setting-override-opt: sql.multi_region.allow_abstractions_for_secondary_tenants.enabled=true
-# cluster-opt: can-admin-split
 # LogicTest: multiregion-9node-3region-3azs multiregion-9node-3region-3azs-tenant multiregion-9node-3region-3azs-no-los
 
 statement ok

pkg/ccl/logictestccl/testdata/logic_test/partitioning_hash_sharded_index_query_plan

@@ -1,4 +1,3 @@
-# cluster-opt: can-admin-split
 # LogicTest: 5node !metamorphic-batch-sizes
 
 statement ok

pkg/ccl/logictestccl/testdata/logic_test/regional_by_row_hash_sharded_index

@@ -1,5 +1,4 @@
 # tenant-cluster-setting-override-opt: sql.multi_region.allow_abstractions_for_secondary_tenants.enabled=true
-# cluster-opt: can-admin-split
 # LogicTest: multiregion-9node-3region-3azs multiregion-9node-3region-3azs-no-los
 
 statement ok

pkg/ccl/logictestccl/testdata/logic_test/tenant_capability

@@ -31,8 +31,8 @@ query TT colnames
 SELECT capability_id, capability_value FROM [SHOW TENANT "no-capabilities-tenant" WITH CAPABILITIES]
 ----
 capability_id          capability_value
-can_admin_scatter      false
-can_admin_split        false
+can_admin_scatter      true
+can_admin_split        true
 can_admin_unsplit      false
 can_view_node_info     false
 can_view_tsdb_metrics  false
@@ -51,7 +51,7 @@ query TT colnames
 SELECT capability_id, capability_value FROM [SHOW TENANT "bool-capability-no-value-tenant" WITH CAPABILITIES]
 ----
 capability_id          capability_value
-can_admin_scatter      false
+can_admin_scatter      true
 can_admin_split        true
 can_admin_unsplit      false
 can_view_node_info     false
@@ -64,7 +64,7 @@ query TT colnames
 SELECT capability_id, capability_value FROM [SHOW TENANT "bool-capability-no-value-tenant" WITH CAPABILITIES]
 ----
 capability_id          capability_value
-can_admin_scatter      false
+can_admin_scatter      true
 can_admin_split        false
 can_admin_unsplit      false
 can_view_node_info     false
@@ -84,7 +84,7 @@ query TT colnames
 SELECT capability_id, capability_value FROM [SHOW TENANT "bool-capability-with-value-tenant" WITH CAPABILITIES]
 ----
 capability_id          capability_value
-can_admin_scatter      false
+can_admin_scatter      true
 can_admin_split        true
 can_admin_unsplit      false
 can_view_node_info     false
@@ -104,7 +104,7 @@ query TT colnames
 SELECT capability_id, capability_value FROM [SHOW TENANT "bool-capability-with-expression-value-tenant" WITH CAPABILITIES]
 ----
 capability_id          capability_value
-can_admin_scatter      false
+can_admin_scatter      true
 can_admin_split        true
 can_admin_unsplit      false
 can_view_node_info     false
@@ -124,7 +124,7 @@ query TT colnames
 SELECT capability_id, capability_value FROM [SHOW TENANT "multiple-capability-tenant" WITH CAPABILITIES]
 ----
 capability_id          capability_value
-can_admin_scatter      false
+can_admin_scatter      true
 can_admin_split        true
 can_admin_unsplit      false
 can_view_node_info     true
@@ -137,7 +137,7 @@ query TT colnames
 SELECT capability_id, capability_value FROM [SHOW TENANT "multiple-capability-tenant" WITH CAPABILITIES]
 ----
 capability_id          capability_value
-can_admin_scatter      false
+can_admin_scatter      true
 can_admin_split        false
 can_admin_unsplit      false
 can_view_node_info     false

pkg/ccl/multitenantccl/tenantcapabilitiesccl/capabilities_test.go

@@ -100,6 +100,7 @@ func TestDataDriven(t *testing.T) {
 		// us from being able to do so.
 		settings := cluster.MakeTestingClusterSettings()
 		sql.SecondaryTenantSplitAtEnabled.Override(ctx, &settings.SV, true)
+		sql.SecondaryTenantScatterEnabled.Override(ctx, &settings.SV, true)
 		tenantArgs := base.TestTenantArgs{
 			TenantID: serverutils.TestTenantID(),
 			Settings: settings,
@@ -115,7 +116,7 @@ func TestDataDriven(t *testing.T) {
 		}()
 		require.NoError(t, err)
 
-		var lastUpdateTS hlc.Timestamp
+		lastUpdateTS := tc.Server(0).Clock().Now() // ensure watcher isn't starting out empty
 		datadriven.RunTest(t, path, func(t *testing.T, d *datadriven.TestData) string {
 
 			switch d.Cmd {

pkg/ccl/multitenantccl/tenantcapabilitiesccl/testdata/can_admin_scatter

@@ -0,0 +1,79 @@
+query-sql-system
+SELECT * FROM [SHOW TENANT [10] WITH CAPABILITIES] WHERE capability_id = 'can_admin_scatter'
+----
+10 tenant-10 ready none can_admin_scatter true
+
+exec-sql-tenant
+CREATE TABLE t(a INT)
+----
+ok
+
+exec-sql-tenant
+CREATE INDEX idx on t(a)
+----
+ok
+
+# By default, we should be able to scatter.
+exec-privileged-op-tenant
+ALTER TABLE t SCATTER
+----
+ok
+
+# ditto for the index.
+exec-privileged-op-tenant
+ALTER INDEX t@idx SCATTER
+----
+ok
+
+
+update-capabilities
+ALTER TENANT [10] GRANT CAPABILITY can_admin_scatter=false
+----
+ok
+
+exec-privileged-op-tenant
+ALTER TABLE t SCATTER
+----
+pq: ba: AdminScatter [/Tenant/10/Table/104/1,/Tenant/10/Table/104/2) RPC error: rpc error: code = Unauthenticated desc = client tenant does not have capability "can_admin_scatter" (*kvpb.AdminScatterRequest)
+
+# Check the index as well.
+exec-privileged-op-tenant
+ALTER INDEX t@idx SCATTER
+----
+pq: ba: AdminScatter [/Tenant/10/Table/104/2,/Tenant/10/Table/104/3) RPC error: rpc error: code = Unauthenticated desc = client tenant does not have capability "can_admin_scatter" (*kvpb.AdminScatterRequest)
+
+# Grant the capability without providing an explicit value.
+update-capabilities
+ALTER TENANT [10] GRANT CAPABILITY can_admin_scatter
+----
+ok
+
+# Scatters should work now.
+exec-privileged-op-tenant
+ALTER TABLE t SCATTER
+----
+ok
+
+# Revoke the capability using REVOKE syntax.
+update-capabilities
+ALTER TENANT [10] REVOKE CAPABILITY can_admin_scatter
+----
+ok
+
+# Scatters should no longer work.
+exec-privileged-op-tenant
+ALTER TABLE t SCATTER
+----
+pq: ba: AdminScatter [/Tenant/10/Table/104/1,/Tenant/10/Table/104/2) RPC error: rpc error: code = Unauthenticated desc = client tenant does not have capability "can_admin_scatter" (*kvpb.AdminScatterRequest)
+
+# Lastly, use the explicitly set to true syntax.
+update-capabilities
+ALTER TENANT [10] GRANT CAPABILITY can_admin_scatter=true
+----
+ok
+
+# Scatters should now work.
+exec-privileged-op-tenant
+ALTER TABLE t SCATTER
+----
+ok