Merge #150562

150562: sql/stats: prevent stats collection in read-only tenants r=petermattis a=petermattis

This change prevents both automatic and manual statistics collection
in PCR reader tenants by detecting read-only tenant status at the
tenant level rather than checking individual table descriptors.

Key changes:
- Add TenantReadonly field to SQLConfig and ExecutorConfig to track
  tenant-level read-only status
- Modify tenant initialization to detect read-only tenants by checking
  mtinfopb.TenantInfoWithUsage.ReadFromTenant \!= nil
- Update MakeRefresher to accept readOnlyTenant parameter
- Prevent automatic stats collection in autoStatsEnabled()
- Prevent manual stats collection in makeJobRecord()
- Add TenantReadonly field to TestSharedProcessTenantArgs for testing
- Update tests to use tenant-level detection instead of table-level
  ReplicatedPCRVersion checks
- Enhance test coverage for both autoStatsEnabled() and
  autoStatsEnabledForTableID() functions

The tenant-level approach is more reliable than checking individual
table descriptors because some tables within read-only tenants are
not replicated (e.g. system.statement_statistics), yet we cannot
record stats for any tables since the system.table_statistics table
itself is read-only.

Fixes #135828

Release note: None

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <[email protected]>

Co-authored-by: Peter Mattis <[email protected]>

Author: craig[bot]

pkg/base/test_server_args.go

@@ -635,6 +635,10 @@ type TestSharedProcessTenantArgs struct {
 	// TenantID is the ID of the tenant to be created. If not set, an ID is
 	// assigned automatically.
 	TenantID roachpb.TenantID
+	// TenantReadOnly indicates if this tenant should be created as read-only
+	// (for testing PCR reader tenants). This field is used for testing purposes
+	// and overrides the tenant record check.
+	TenantReadOnly bool
 
 	Knobs TestingKnobs
 

pkg/server/config.go

@@ -487,6 +487,9 @@ type SQLConfig struct {
 	TenantID   roachpb.TenantID
 	TenantName roachpb.TenantName
 
+	// TenantReadOnly indicates if this tenant is read-only (PCR reader tenant).
+	TenantReadOnly bool
+
 	// If set, will to be called at server startup to obtain the tenant id and
 	// locality.
 	DelayedSetTenantID func(context.Context) (roachpb.TenantID, roachpb.Locality, error)
@@ -554,8 +557,9 @@ func MakeSQLConfig(
 	tenID roachpb.TenantID, tenName roachpb.TenantName, tempStorageCfg base.TempStorageConfig,
 ) SQLConfig {
 	sqlCfg := SQLConfig{
-		TenantID:   tenID,
-		TenantName: tenName,
+		TenantID:       tenID,
+		TenantName:     tenName,
+		TenantReadOnly: false, // Default to false, will be set during tenant initialization
 	}
 	sqlCfg.SetDefaults(tempStorageCfg)
 	return sqlCfg

pkg/server/server_controller_new_server.go

@@ -64,13 +64,18 @@ func (s *topLevelServer) newTenantServer(
 	portStartHint int,
 	testArgs base.TestSharedProcessTenantArgs,
 ) (onDemandServer, error) {
-	tenantID, err := s.getTenantID(ctx, tenantNameContainer.Get())
+	tenantID, tenantReadOnly, err := s.getTenantID(ctx, tenantNameContainer.Get())
 	if err != nil {
 		return nil, err
 	}
 
+	// Use test override for tenant read-only status if provided.
+	if testArgs.TenantReadOnly {
+		tenantReadOnly = true
+	}
+
 	baseCfg, sqlCfg, err := s.makeSharedProcessTenantConfig(ctx, tenantID, tenantNameContainer.Get(), portStartHint,
-		tenantStopper, testArgs.Settings)
+		tenantStopper, testArgs.Settings, tenantReadOnly)
 	if err != nil {
 		return nil, err
 	}
@@ -100,23 +105,27 @@ var ErrInvalidTenant error = errInvalidTenantMarker{}
 
 func (s *topLevelServer) getTenantID(
 	ctx context.Context, tenantName roachpb.TenantName,
-) (roachpb.TenantID, error) {
+) (roachpb.TenantID, bool, error) {
 	var rec *mtinfopb.TenantInfo
 	if err := s.sqlServer.internalDB.Txn(ctx, func(ctx context.Context, txn isql.Txn) error {
 		var err error
 		rec, err = sql.GetTenantRecordByName(ctx, s.cfg.Settings, txn, tenantName)
 		return err
 	}); err != nil {
-		return roachpb.TenantID{}, errors.Mark(err, ErrInvalidTenant)
+		return roachpb.TenantID{}, false, errors.Mark(err, ErrInvalidTenant)
 	}
 
 	tenantID, err := roachpb.MakeTenantID(rec.ID)
 	if err != nil {
-		return roachpb.TenantID{}, errors.Mark(
+		return roachpb.TenantID{}, false, errors.Mark(
 			errors.NewAssertionErrorWithWrappedErrf(err, "stored tenant ID %d does not convert to TenantID", rec.ID),
 			ErrInvalidTenant)
 	}
-	return tenantID, nil
+
+	// Check if tenant is read-only (PCR reader tenant).
+	readOnlyTenant := rec.ReadFromTenant != nil
+
+	return tenantID, readOnlyTenant, nil
 }
 
 // newTenantServerInternal instantiates a server for the given target
@@ -151,6 +160,7 @@ func (s *topLevelServer) makeSharedProcessTenantConfig(
 	portStartHint int,
 	stopper *stop.Stopper,
 	testSettings *cluster.Settings,
+	tenantReadOnly bool,
 ) (BaseConfig, SQLConfig, error) {
 	// Create a configuration for the new tenant.
 	parentCfg := s.cfg
@@ -167,7 +177,7 @@ func (s *topLevelServer) makeSharedProcessTenantConfig(
 	}
 
 	baseCfg, sqlCfg, err := makeSharedProcessTenantServerConfig(ctx, tenantID, tenantName, portStartHint, parentCfg,
-		localServerInfo, st, stopper, s.recorder)
+		localServerInfo, st, stopper, s.recorder, tenantReadOnly)
 	if err != nil {
 		return BaseConfig{}, SQLConfig{}, err
 	}
@@ -186,6 +196,7 @@ func makeSharedProcessTenantServerConfig(
 	st *cluster.Settings,
 	stopper *stop.Stopper,
 	nodeMetricsRecorder *status.MetricsRecorder,
+	tenantReadOnly bool,
 ) (baseCfg BaseConfig, sqlCfg SQLConfig, err error) {
 	tr := tracing.NewTracerWithOpt(ctx, tracing.WithClusterSettings(&st.SV))
 
@@ -333,6 +344,7 @@ func makeSharedProcessTenantServerConfig(
 	}
 
 	sqlCfg = MakeSQLConfig(tenantID, tenantName, tempStorageCfg)
+	sqlCfg.TenantReadOnly = tenantReadOnly
 	baseCfg.ExternalIODirConfig = kvServerCfg.BaseConfig.ExternalIODirConfig
 
 	baseCfg.ExternalIODir = kvServerCfg.BaseConfig.ExternalIODir

pkg/server/server_sql.go

@@ -1055,6 +1055,7 @@ func newSQLServer(ctx context.Context, cfg sqlServerArgs) (*SQLServer, error) {
 		RangeStatsFetcher:          rangeStatsFetcher,
 		NodeDescs:                  cfg.nodeDescs,
 		TenantCapabilitiesReader:   cfg.tenantCapabilitiesReader,
+		TenantReadOnly:             cfg.SQLConfig.TenantReadOnly,
 		CidrLookup:                 cfg.BaseConfig.CidrLookup,
 		LicenseEnforcer:            cfg.SQLConfig.LicenseEnforcer,
 	}
@@ -1203,6 +1204,7 @@ func newSQLServer(ctx context.Context, cfg sqlServerArgs) (*SQLServer, error) {
 		execCfg.TableStatsCache,
 		stats.DefaultAsOfTime,
 		tableStatsTestingKnobs,
+		execCfg.TenantReadOnly,
 	)
 	execCfg.StatsRefresher = statsRefresher
 	distSQLServer.ServerConfig.StatsRefresher = statsRefresher

pkg/sql/catalog/replication/reader_catalog_test.go

@@ -70,10 +70,11 @@ func newReaderCatalogTest(
 	})
 	require.NoError(t, err)
 	destTenant, _, err := ts.StartSharedProcessTenant(ctx, base.TestSharedProcessTenantArgs{
-		TenantID:   serverutils.TestTenantID2(),
-		TenantName: "dest",
-		Knobs:      destTestingKnobs,
-		Settings:   destSettings,
+		TenantID:       serverutils.TestTenantID2(),
+		TenantName:     "dest",
+		Knobs:          destTestingKnobs,
+		Settings:       destSettings,
+		TenantReadOnly: true, // Mark the dest tenant as read-only for testing
 	})
 	require.NoError(t, err)
 	srcRunner := sqlutils.MakeSQLRunner(srcTenant.SQLConn(t))
@@ -550,6 +551,34 @@ func TestReaderCatalogTSAdvanceWithLongTxn(t *testing.T) {
 	require.NoError(t, tx.Commit())
 }
 
+func TestReaderCatalogAutoStatsDisabled(t *testing.T) {
+	defer leaktest.AfterTest(t)()
+	skip.UnderDuress(t)
+
+	ctx := context.Background()
+	r, cleanup := newReaderCatalogTest(t, ctx, base.TestingKnobs{}, nil)
+	defer cleanup()
+
+	// Create a table and insert some data in the source tenant.
+	r.srcRunner.Exec(t, `
+		CREATE TABLE t1(n int);
+		INSERT INTO t1 VALUES (1), (2), (3);
+	`)
+
+	// Enable auto stats collection in the source tenant.
+	r.srcRunner.Exec(t, `SET CLUSTER SETTING sql.stats.automatic_collection.enabled = true`)
+
+	// Advance the reader catalog timestamp to replicate the table.
+	require.NoError(t, r.advanceTS(ctx, r.ts.Clock().Now(), true))
+
+	// Verify the table exists in the reader catalog.
+	r.compareEqual(t, "SELECT * FROM t1 ORDER BY n")
+
+	// Now verify that stats collection is disabled for the read-only tenant.
+	// Manual CREATE STATISTICS should fail with our tenant-level read-only error.
+	r.destRunner.ExpectErr(t, "cannot create statistics in read-only tenant", "CREATE STATISTICS test_stats FROM t1")
+}
+
 func TestMain(m *testing.M) {
 	securityassets.SetLoader(securitytest.EmbeddedAssets)
 	randutil.SeedForTests()

pkg/sql/create_stats.go

@@ -229,6 +229,12 @@ func (n *createStatsNode) runJob(ctx context.Context) error {
 // makeJobRecord creates a CreateStats job record which can be used to plan and
 // execute statistics creation.
 func (n *createStatsNode) makeJobRecord(ctx context.Context) (*jobs.Record, error) {
+	// Check tenant-level read-only status first (applies to all tables in tenant).
+	if n.p.ExecCfg().TenantReadOnly {
+		return nil, pgerror.Newf(
+			pgcode.WrongObjectType, "cannot create statistics in read-only tenant")
+	}
+
 	var tableDesc catalog.TableDescriptor
 	var fqTableName string
 	var err error

pkg/sql/exec_util.go

@@ -1678,6 +1678,9 @@ type ExecutorConfig struct {
 
 	TenantCapabilitiesReader SystemTenantOnly[tenantcapabilities.Reader]
 
+	// TenantReadOnly indicates if this tenant is read-only (PCR reader tenant).
+	TenantReadOnly bool
+
 	// VirtualClusterName contains the name of the virtual cluster
 	// (tenant).
 	VirtualClusterName roachpb.TenantName

pkg/sql/stats/automatic_stats.go

@@ -267,11 +267,12 @@ const (
 // sent.
 type Refresher struct {
 	log.AmbientContext
-	st         *cluster.Settings
-	internalDB descs.DB
-	cache      *TableStatisticsCache
-	randGen    autoStatsRand
-	knobs      *TableStatsTestingKnobs
+	st             *cluster.Settings
+	internalDB     descs.DB
+	cache          *TableStatisticsCache
+	randGen        autoStatsRand
+	knobs          *TableStatsTestingKnobs
+	readOnlyTenant bool
 
 	// mutations is the buffered channel used to pass messages containing
 	// metadata about SQL mutations to the background Refresher thread.
@@ -353,6 +354,7 @@ func MakeRefresher(
 	cache *TableStatisticsCache,
 	asOfTime time.Duration,
 	knobs *TableStatsTestingKnobs,
+	readOnlyTenant bool,
 ) *Refresher {
 	randSource := rand.NewSource(rand.Int63())
 
@@ -363,6 +365,7 @@ func MakeRefresher(
 		cache:            cache,
 		randGen:          makeAutoStatsRand(randSource),
 		knobs:            knobs,
+		readOnlyTenant:   readOnlyTenant,
 		mutations:        make(chan mutation, refreshChanBufferLen),
 		settings:         make(chan settingOverride, refreshChanBufferLen),
 		asOfTime:         asOfTime,
@@ -378,6 +381,11 @@ func (r *Refresher) getNumTablesEnsured() int {
 }
 
 func (r *Refresher) autoStatsEnabled(desc catalog.TableDescriptor) bool {
+	// Check tenant-level read-only status first (applies to all tables in tenant).
+	if r.readOnlyTenant {
+		return false
+	}
+
 	if desc == nil {
 		// If the descriptor could not be accessed, defer to the cluster setting.
 		return AutomaticStatisticsClusterMode.Get(&r.st.SV)
@@ -432,6 +440,11 @@ func (r *Refresher) autoFullStatsEnabled(desc catalog.TableDescriptor) bool {
 func (r *Refresher) autoStatsEnabledForTableID(
 	tableID descpb.ID, settingOverrides map[descpb.ID]catpb.AutoStatsSettings,
 ) bool {
+	// Check tenant-level read-only status first (applies to all tables in tenant).
+	if r.readOnlyTenant {
+		return false
+	}
+
 	var setting catpb.AutoStatsSettings
 	var ok bool
 	if settingOverrides == nil {
@@ -533,6 +546,12 @@ func (r *Refresher) SetDraining() {
 func (r *Refresher) Start(
 	ctx context.Context, stopper *stop.Stopper, refreshInterval time.Duration,
 ) error {
+	// If the tenant is read-only, we don't need to start the stats refresher
+	// goroutines as we can't persist collected stats.
+	if r.readOnlyTenant {
+		return nil
+	}
+
 	// The refresher has the same lifetime as the server, so the cancellation
 	// function can be ignored and it'll be called by the stopper.
 	stoppingCtx, _ := stopper.WithCancelOnQuiesce(context.Background()) // nolint:quiesce