server: ignore spanconfig limit for shared-process tenants

This change installs a noop spanconfig limiter when starting
shared-process tenants.  Since shared-process tenants are currently
used for "unified architecture" in which the tenant is expected to
behave as close as possible to the system tenant.

When we have a tenant-side capabilities reader, we should tie this to
a capability instead.

This has an unfortunate side-effect of making any tenant-level
override for the span limit a lie for shared-process tenants.

Fixes #93562

Release note: None

pkg/ccl/serverccl/BUILD.bazel

@@ -40,6 +40,7 @@ go_test(
         "server_controller_test.go",
         "server_sql_test.go",
         "server_startup_guardrails_test.go",
+        "shared_process_tenant_test.go",
         "tenant_decommissioned_host_test.go",
         "tenant_migration_test.go",
         "tenant_vars_test.go",

pkg/ccl/serverccl/shared_process_tenant_test.go

@@ -0,0 +1,61 @@
+// Copyright 2023 The Cockroach Authors.
+//
+// Licensed as a CockroachDB Enterprise file under the Cockroach Community
+// License (the "License"); you may not use this file except in compliance with
+// the License. You may obtain a copy of the License at
+//
+//     https://github.com/cockroachdb/cockroach/blob/master/licenses/CCL.txt
+
+package serverccl
+
+import (
+	"context"
+	gosql "database/sql"
+	"testing"
+
+	"github.com/cockroachdb/cockroach/pkg/base"
+	"github.com/cockroachdb/cockroach/pkg/testutils"
+	"github.com/cockroachdb/cockroach/pkg/testutils/serverutils"
+	"github.com/cockroachdb/cockroach/pkg/util/leaktest"
+	"github.com/cockroachdb/cockroach/pkg/util/log"
+	"github.com/stretchr/testify/require"
+)
+
+func TestSharedProcessTenantNoSpanLimit(t *testing.T) {
+	defer leaktest.AfterTest(t)()
+	defer log.Scope(t).Close(t)
+
+	ctx := context.Background()
+
+	tc := serverutils.StartNewTestCluster(t, 1, base.TestClusterArgs{
+		ServerArgs: base.TestServerArgs{
+			DisableDefaultTestTenant: true,
+		}})
+	defer tc.Stopper().Stop(ctx)
+
+	db := tc.ServerConn(0)
+	_, err := db.Exec("CREATE TENANT hello; ALTER TENANT hello START SERVICE SHARED")
+	require.NoError(t, err)
+
+	_, err = db.Exec("ALTER TENANT ALL SET CLUSTER SETTING spanconfig.tenant_limit = 1000")
+	require.NoError(t, err)
+
+	sqlAddr := tc.Server(0).ServingSQLAddr()
+	var tenantDB *gosql.DB
+	testutils.SucceedsSoon(t, func() error {
+		var err error
+		tenantDB, err = serverutils.OpenDBConnE(sqlAddr, "cluster:hello", false, tc.Stopper())
+		if err != nil {
+			return err
+		}
+
+		if err := tenantDB.Ping(); err != nil {
+			return err
+		}
+		return nil
+	})
+	defer tenantDB.Close()
+
+	_, err = tenantDB.Exec("SELECT crdb_internal.generate_test_objects('foo', 1001)")
+	require.NoError(t, err)
+}

pkg/server/server_sql.go

@@ -248,7 +248,9 @@ type sqlServerOptionalKVArgs struct {
 // sqlServerOptionalTenantArgs are the arguments supplied to newSQLServer which
 // are only available if the SQL server runs as part of a standalone SQL node.
 type sqlServerOptionalTenantArgs struct {
-	tenantConnect    kvtenant.Connector
+	tenantConnect      kvtenant.Connector
+	spanLimiterFactory spanLimiterFactory
+
 	promRuleExporter *metric.PrometheusRuleExporter
 }
 
@@ -741,13 +743,18 @@ func newSQLServer(ctx context.Context, cfg sqlServerArgs) (*SQLServer, error) {
 		splitter             spanconfig.Splitter
 		limiter              spanconfig.Limiter
 	}{}
+
+	spanConfigKnobs, _ := cfg.TestingKnobs.SpanConfig.(*spanconfig.TestingKnobs)
 	if codec.ForSystemTenant() {
-		spanConfig.limiter = spanconfiglimiter.NoopLimiter{}
 		spanConfig.splitter = spanconfigsplitter.NoopSplitter{}
 	} else {
-		spanConfigKnobs, _ := cfg.TestingKnobs.SpanConfig.(*spanconfig.TestingKnobs)
 		spanConfig.splitter = spanconfigsplitter.New(codec, spanConfigKnobs)
-		spanConfig.limiter = spanconfiglimiter.New(
+	}
+
+	if cfg.spanLimiterFactory == nil {
+		spanConfig.limiter = spanconfiglimiter.NoopLimiter{}
+	} else {
+		spanConfig.limiter = cfg.spanLimiterFactory(
 			cfg.circularInternalExecutor,
 			cfg.Settings,
 			spanConfigKnobs,

pkg/server/tenant.go

@@ -49,8 +49,11 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/server/status"
 	"github.com/cockroachdb/cockroach/pkg/server/systemconfigwatcher"
 	"github.com/cockroachdb/cockroach/pkg/settings/cluster"
+	"github.com/cockroachdb/cockroach/pkg/spanconfig"
+	"github.com/cockroachdb/cockroach/pkg/spanconfig/spanconfiglimiter"
 	"github.com/cockroachdb/cockroach/pkg/sql"
 	"github.com/cockroachdb/cockroach/pkg/sql/flowinfra"
+	"github.com/cockroachdb/cockroach/pkg/sql/isql"
 	"github.com/cockroachdb/cockroach/pkg/sql/optionalnodeliveness"
 	"github.com/cockroachdb/cockroach/pkg/sql/pgwire"
 	"github.com/cockroachdb/cockroach/pkg/sql/sessiondatapb"
@@ -150,6 +153,19 @@ func (s *SQLServerWrapper) Drain(
 	return s.drainServer.runDrain(ctx, verbose)
 }
 
+// tenantServerDeps holds dependencies for the SQL server that we want
+// to vary based on whether we are in a shared process or separate
+// process tenant.
+type tenantServerDeps struct {
+	instanceIDContainer *base.SQLIDContainer
+
+	// The following should eventually be connected to tenant
+	// capabilities.
+	costControllerFactory costControllerFactory
+	spanLimiterFactory    spanLimiterFactory
+}
+
+type spanLimiterFactory func(isql.Executor, *cluster.Settings, *spanconfig.TestingKnobs) spanconfig.Limiter
 type costControllerFactory func(*cluster.Settings, roachpb.TenantID, kvtenant.TokenBucketProvider) (multitenant.TenantSideCostController, error)
 
 // NewSeparateProcessTenantServer creates a tenant-specific, SQL-only
@@ -165,9 +181,15 @@ func NewSeparateProcessTenantServer(
 	sqlCfg SQLConfig,
 	tenantNameContainer *roachpb.TenantNameContainer,
 ) (*SQLServerWrapper, error) {
-	instanceIDContainer := baseCfg.IDContainer.SwitchToSQLIDContainerForStandaloneSQLInstance()
-	costControllerBuilder := NewTenantSideCostController
-	return newTenantServer(ctx, stopper, baseCfg, sqlCfg, tenantNameContainer, instanceIDContainer, costControllerBuilder)
+	deps := tenantServerDeps{
+		instanceIDContainer:   baseCfg.IDContainer.SwitchToSQLIDContainerForStandaloneSQLInstance(),
+		costControllerFactory: NewTenantSideCostController,
+		spanLimiterFactory: func(ie isql.Executor, st *cluster.Settings, knobs *spanconfig.TestingKnobs) spanconfig.Limiter {
+			return spanconfiglimiter.New(ie, st, knobs)
+		},
+	}
+
+	return newTenantServer(ctx, stopper, baseCfg, sqlCfg, tenantNameContainer, deps)
 }
 
 // NewSharedProcessTenantServer creates a tenant-specific, SQL-only
@@ -186,16 +208,22 @@ func NewSharedProcessTenantServer(
 	if baseCfg.IDContainer.Get() == 0 {
 		return nil, errors.AssertionFailedf("programming error: NewSharedProcessTenantServer called before NodeID was assigned.")
 	}
-	instanceIDContainer := base.NewSQLIDContainerForNode(baseCfg.IDContainer)
-	// TODO(ssd): The cost controller should instead be able to
-	// read from the capability system and return immediately if
-	// the tenant is exempt. For now we are turning off the
-	// tenant-side cost controller for shared-memory tenants until
-	// we have the abilility to read capabilities tenant-side.
-	//
-	// https://github.com/cockroachdb/cockroach/issues/84586
-	costControllerBuilder := NewNoopTenantSideCostController
-	return newTenantServer(ctx, stopper, baseCfg, sqlCfg, tenantNameContainer, instanceIDContainer, costControllerBuilder)
+
+	deps := tenantServerDeps{
+		instanceIDContainer: base.NewSQLIDContainerForNode(baseCfg.IDContainer),
+		// TODO(ssd): The cost controller should instead be able to
+		// read from the capability system and return immediately if
+		// the tenant is exempt. For now we are turning off the
+		// tenant-side cost controller for shared-memory tenants until
+		// we have the abilility to read capabilities tenant-side.
+		//
+		// https://github.com/cockroachdb/cockroach/issues/84586
+		costControllerFactory: NewNoopTenantSideCostController,
+		spanLimiterFactory: func(isql.Executor, *cluster.Settings, *spanconfig.TestingKnobs) spanconfig.Limiter {
+			return spanconfiglimiter.NoopLimiter{}
+		},
+	}
+	return newTenantServer(ctx, stopper, baseCfg, sqlCfg, tenantNameContainer, deps)
 }
 
 // newTenantServer constructs a SQLServerWrapper.
@@ -207,8 +235,7 @@ func newTenantServer(
 	baseCfg BaseConfig,
 	sqlCfg SQLConfig,
 	tenantNameContainer *roachpb.TenantNameContainer,
-	instanceIDContainer *base.SQLIDContainer,
-	costControllerFactory costControllerFactory,
+	deps tenantServerDeps,
 ) (*SQLServerWrapper, error) {
 	// TODO(knz): Make the license application a per-server thing
 	// instead of a global thing.
@@ -220,7 +247,7 @@ func newTenantServer(
 	// Inform the server identity provider that we're operating
 	// for a tenant server.
 	baseCfg.idProvider.SetTenant(sqlCfg.TenantID)
-	args, err := makeTenantSQLServerArgs(ctx, stopper, baseCfg, sqlCfg, tenantNameContainer, instanceIDContainer, costControllerFactory)
+	args, err := makeTenantSQLServerArgs(ctx, stopper, baseCfg, sqlCfg, tenantNameContainer, deps)
 	if err != nil {
 		return nil, err
 	}
@@ -905,8 +932,7 @@ func makeTenantSQLServerArgs(
 	baseCfg BaseConfig,
 	sqlCfg SQLConfig,
 	tenantNameContainer *roachpb.TenantNameContainer,
-	instanceIDContainer *base.SQLIDContainer,
-	costControllerFactory costControllerFactory,
+	deps tenantServerDeps,
 ) (sqlServerArgs, error) {
 	st := baseCfg.Settings
 
@@ -999,7 +1025,7 @@ func makeTenantSQLServerArgs(
 		tenantKnobs.OverrideTokenBucketProvider != nil {
 		provider = tenantKnobs.OverrideTokenBucketProvider(provider)
 	}
-	costController, err := costControllerFactory(st, sqlCfg.TenantID, provider)
+	costController, err := deps.costControllerFactory(st, sqlCfg.TenantID, provider)
 	if err != nil {
 		return sqlServerArgs{}, err
 	}
@@ -1041,7 +1067,7 @@ func makeTenantSQLServerArgs(
 	)
 
 	dbCtx := kv.DefaultDBContext(stopper)
-	dbCtx.NodeID = instanceIDContainer
+	dbCtx.NodeID = deps.instanceIDContainer
 	db := kv.NewDBWithContext(baseCfg.AmbientCtx, tcsFactory, clock, dbCtx)
 
 	rangeFeedKnobs, _ := baseCfg.TestingKnobs.RangeFeed.(*rangefeed.TestingKnobs)
@@ -1156,13 +1182,14 @@ func makeTenantSQLServerArgs(
 			externalStorageFromURI: externalStorageFromURI,
 			// Set instance ID to 0 and node ID to nil to indicate
 			// that the instance ID will be bound later during preStart.
-			nodeIDContainer:      instanceIDContainer,
+			nodeIDContainer:      deps.instanceIDContainer,
 			spanConfigKVAccessor: tenantConnect,
 			kvStoresIterator:     kvserverbase.UnsupportedStoresIterator{},
 		},
 		sqlServerOptionalTenantArgs: sqlServerOptionalTenantArgs{
-			tenantConnect:    tenantConnect,
-			promRuleExporter: promRuleExporter,
+			spanLimiterFactory: deps.spanLimiterFactory,
+			tenantConnect:      tenantConnect,
+			promRuleExporter:   promRuleExporter,
 		},
 		SQLConfig:                &sqlCfg,
 		BaseConfig:               &baseCfg,