Merge #157799

157799: testutils: remove ForceTableGC from ApplicationLayerInterface r=yuzefovich a=yuzefovich

In 9d0d231 we made `ForceTableGC` helper method of the test server work with secondary tenants and moved it into the ApplicationLayer interface. The method works by issuing the KV GCRequest command on the range containing the user table. However, with secondary tenants (both in shared-process and external-process modes) IIUC we don't add split points between all tables, so it's possible that when trying to GC the "user" table we also GC the "system" table. This showed up as a failure in TestStatsWithLowTTL where an update to a cluster setting was never observed by the tenant because the settings-watcher rangefeed failed since it ran into the GC threshold and exited.

I think if we're mucking with KV GC requests it makes sense that such tests would be specific to the system tenant, so this commit removes ForceTableGC method from the ApplicationLayer interface and adjusts a couple of tests to only work with system tenants.

Fixes: #157126.
Release note: None

Co-authored-by: Yahor Yuzefovich <yahor@cockroachlabs.com>

Author: craig[bot]

pkg/backup/backup_test.go

@@ -10502,6 +10502,7 @@ func TestBackupDBWithViewOnAdjacentDBRange(t *testing.T) {
 			},
 		})
 	defer cleanupFn()
+	srv0 := tc.StorageLayer(0)
 	s0 := tc.ApplicationLayer(0)
 
 	// Speeds up the test.
@@ -10532,7 +10533,7 @@ func TestBackupDBWithViewOnAdjacentDBRange(t *testing.T) {
 	time.Sleep(5 * time.Second)
 
 	// Force GC on da.t2 to advance its GC threshold.
-	err := s0.ForceTableGC(context.Background(), "da", "t2", s0.Clock().Now().Add(-int64(1*time.Second), 0))
+	err := srv0.ForceTableGC(context.Background(), "da", "t2", s0.Clock().Now().Add(-int64(1*time.Second), 0))
 	require.NoError(t, err)
 
 	// This statement should succeed as we are not backing up the span for dbview,

pkg/server/testserver.go

@@ -1297,13 +1297,6 @@ func (t *testTenant) TracerI() interface{} {
 	return t.Tracer()
 }
 
-// ForceTableGC is part of the serverutils.ApplicationLayerInterface.
-func (t *testTenant) ForceTableGC(
-	ctx context.Context, database, table string, timestamp hlc.Timestamp,
-) error {
-	return internalForceTableGC(ctx, t, database, table, timestamp)
-}
-
 // DefaultZoneConfig is part of the serverutils.ApplicationLayerInterface.
 func (t *testTenant) DefaultZoneConfig() zonepb.ZoneConfig {
 	return *t.SystemConfigProvider().GetSystemConfig().DefaultZoneConfig
@@ -2294,33 +2287,24 @@ func (ts *testServer) Tracer() *tracing.Tracer {
 	return ts.node.storeCfg.AmbientCtx.Tracer
 }
 
-// ForceTableGC is part of the serverutils.ApplicationLayerInterface.
+// ForceTableGC is part of the serverutils.StorageLayerInterface.
 func (ts *testServer) ForceTableGC(
 	ctx context.Context, database, table string, timestamp hlc.Timestamp,
 ) error {
-	return internalForceTableGC(ctx, ts, database, table, timestamp)
-}
-
-func internalForceTableGC(
-	ctx context.Context,
-	app serverutils.ApplicationLayerInterface,
-	database, table string,
-	timestamp hlc.Timestamp,
-) error {
-	tableID, err := app.QueryTableID(ctx, username.RootUserName(), database, table)
+	tableID, err := ts.QueryTableID(ctx, username.RootUserName(), database, table)
 	if err != nil {
 		return err
 	}
 
-	tblKey := app.Codec().TablePrefix(uint32(tableID))
+	tblKey := ts.Codec().TablePrefix(uint32(tableID))
 	gcr := kvpb.GCRequest{
 		RequestHeader: kvpb.RequestHeader{
 			Key:    tblKey,
 			EndKey: tblKey.PrefixEnd(),
 		},
 		Threshold: timestamp,
 	}
-	_, pErr := kv.SendWrapped(ctx, app.DistSenderI().(kv.Sender), &gcr)
+	_, pErr := kv.SendWrapped(ctx, ts.DistSenderI().(kv.Sender), &gcr)
 	return pErr.GoError()
 }
 

pkg/sql/create_stats_test.go

@@ -42,7 +42,7 @@ func TestStatsWithLowTTL(t *testing.T) {
 	var blockTableReader atomic.Bool
 	blockCh := make(chan struct{})
 
-	s, db, _ := serverutils.StartServer(t, base.TestServerArgs{
+	srv, db, _ := serverutils.StartServer(t, base.TestServerArgs{
 		Knobs: base.TestingKnobs{
 			DistSQL: &execinfra.TestingKnobs{
 				// Set the batch size small to avoid having to use a large
@@ -59,12 +59,11 @@ func TestStatsWithLowTTL(t *testing.T) {
 				},
 			},
 		},
-		// In external-process mode the tenant doesn't have kvpb.GCRequest
-		// capability (and this capability can't be granted at the time of
-		// writing either), so we skip the external mode only.
-		DefaultTestTenant: base.TestSkipForExternalProcessMode(),
+		// ForceTableGC is only available for the system tenant.
+		DefaultTestTenant: base.TestIsSpecificToStorageLayerAndNeedsASystemTenant,
 	})
-	defer s.Stopper().Stop(context.Background())
+	defer srv.Stopper().Stop(context.Background())
+	s := srv.ApplicationLayer()
 
 	r := sqlutils.MakeSQLRunner(db)
 	r.Exec(t, `SET CLUSTER SETTING sql.stats.automatic_collection.enabled = false;`)
@@ -141,7 +140,7 @@ func TestStatsWithLowTTL(t *testing.T) {
 				nextPK++
 			}
 			// Force a table GC of values older than 2 seconds.
-			if err := s.ForceTableGC(
+			if err := srv.ForceTableGC(
 				context.Background(), "defaultdb", "t", s.Clock().Now().Add(-int64(2*time.Second), 0),
 			); err != nil {
 				goroutineErr = err

pkg/sql/user_test.go

@@ -36,10 +36,8 @@ func TestUserLoginAfterGC(t *testing.T) {
 	ctx := context.Background()
 
 	srv, db, _ := serverutils.StartServer(t, base.TestServerArgs{
-		// In external-process mode the tenant doesn't have kvpb.GCRequest
-		// capability (and this capability can't be granted at the time of
-		// writing either), so we skip the external mode only.
-		DefaultTestTenant: base.TestSkipForExternalProcessMode(),
+		// ForceTableGC is only available for the system tenant.
+		DefaultTestTenant: base.TestIsSpecificToStorageLayerAndNeedsASystemTenant,
 	})
 	defer srv.Stopper().Stop(ctx)
 	s := srv.ApplicationLayer()
@@ -55,7 +53,7 @@ func TestUserLoginAfterGC(t *testing.T) {
 	time.Sleep(2 * time.Second)
 
 	// Force a table GC with a threshold of 500ms in the past.
-	err = s.ForceTableGC(ctx, "system", "role_members", s.Clock().Now().Add(-int64(500*time.Millisecond), 0))
+	err = srv.ForceTableGC(ctx, "system", "role_members", s.Clock().Now().Add(-int64(500*time.Millisecond), 0))
 	require.NoError(t, err)
 
 	// Verify that newuser can still log in.

pkg/testutils/serverutils/api.go

@@ -463,11 +463,6 @@ type ApplicationLayerInterface interface {
 		ctx context.Context, span roachpb.Span,
 	) (*serverpb.TableStatsResponse, error)
 
-	// ForceTableGC forces a KV GC round on the key range for the given table.
-	ForceTableGC(
-		ctx context.Context, database, table string, timestamp hlc.Timestamp,
-	) error
-
 	// DefaultZoneConfig is a convenience function that accesses
 	// .SystemConfigProvider().GetSystemConfig().DefaultZoneConfig.
 	DefaultZoneConfig() zonepb.ZoneConfig
@@ -732,6 +727,17 @@ type StorageLayerInterface interface {
 
 	// RaftConfig retrieves a copy of the raft configuration.
 	RaftConfig() base.RaftConfig
+
+	// ForceTableGC forces a KV GC round on the key range for the given table.
+	//
+	// Note that we don't provide this functionality as part of the
+	// ApplicationLayerInterface because on secondary tenants we don't put
+	// split points between all tables, so the KV GC request could affect other
+	// system tables (since they could share the underlying range with the user
+	// table).
+	ForceTableGC(
+		ctx context.Context, database, table string, timestamp hlc.Timestamp,
+	) error
 }
 
 // TestServerFactory encompasses the actual implementation of the shim