Skip to content

Commit 175bbf3

Browse files
craig[bot]xinhaoz
craig[bot]
and
xinhaoz
committed
Merge #144417
144417: sqlstats: remove sqlstats controller structs r=xinhaoz a=xinhaoz ### sqlstats: remove move persistedsqlstats controller Let's remove this controller struct and just use the PersistedSQLStats provider directly for clarity. Epic: none Release note: None ### sqlstats: remove sslocal controller This struct is not very useful as it just provides an interface to reset local sql stats. This commit removes the sql stats controller defined in sslocal as we can access the local sql stats directly. Epic: none Release note: None -------------- Please note only the latest 2 commits should be reviewed in this PR. Co-authored-by: Xin Hao Zhang <[email protected]>
2 parents 6470faf + 114a395 commit 175bbf3

File tree

17 files changed

+109
-228
lines changed

17 files changed

+109
-228
lines changed

‎pkg/ccl/serverccl/diagnosticsccl/reporter_test.go‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -68,7 +68,7 @@ func TestTenantReport(t *testing.T) {
6868
setupCluster(t, tenantDB)
6969

7070
// Clear the SQL stat pool before getting diagnostics.
71-
rt.server.SQLServer().(*sql.Server).GetSQLStatsController().ResetLocalSQLStats(ctx)
71+
require.NoError(t, rt.server.SQLServer().(*sql.Server).GetLocalSQLStatsProvider().Reset(ctx))
7272
reporter.ReportDiagnostics(ctx)
7373

7474
require.Equal(t, 1, rt.diagServer.NumRequests())
@@ -171,7 +171,7 @@ func TestServerReport(t *testing.T) {
171171

172172
node := rt.server.MetricsRecorder().GenerateNodeStatus(ctx)
173173
// Clear the SQL stat pool before getting diagnostics.
174-
rt.server.SQLServer().(*sql.Server).GetSQLStatsController().ResetLocalSQLStats(ctx)
174+
require.NoError(t, rt.server.SQLServer().(*sql.Server).GetLocalSQLStatsProvider().Reset(ctx))
175175
rt.server.DiagnosticsReporter().(*diagnostics.Reporter).ReportDiagnostics(ctx)
176176

177177
keyCounts := make(map[roachpb.StoreID]int64)
@@ -495,7 +495,7 @@ func TestUsageQuantization(t *testing.T) {
495495
ts := s.ApplicationLayer()
496496

497497
// Flush the SQL stat pool.
498-
ts.SQLServer().(*sql.Server).GetSQLStatsController().ResetLocalSQLStats(ctx)
498+
require.NoError(t, ts.SQLServer().(*sql.Server).GetLocalSQLStatsProvider().Reset(ctx))
499499

500500
// Collect a round of statistics.
501501
ts.DiagnosticsReporter().(*diagnostics.Reporter).ReportDiagnostics(ctx)

‎pkg/server/application_api/stats_test.go‎

Lines changed: 10 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -63,8 +63,8 @@ CREATE TABLE t.test (x INT PRIMARY KEY);
6363
sqlServer := s.SQLServer().(*sql.Server)
6464

6565
// Flush stats at the beginning of the test.
66-
sqlServer.GetSQLStatsController().ResetLocalSQLStats(ctx)
67-
sqlServer.GetReportedSQLStatsController().ResetLocalSQLStats(ctx)
66+
require.NoError(t, sqlServer.GetLocalSQLStatsProvider().Reset(ctx))
67+
require.NoError(t, sqlServer.GetReportedSQLStatsProvider().Reset(ctx))
6868

6969
// Run some queries mixed with diagnostics, and ensure that the statistics
7070
// are unaffected by the calls to report diagnostics.
@@ -178,8 +178,8 @@ func TestSQLStatCollection(t *testing.T) {
178178
sqlServer := srv.ApplicationLayer().SQLServer().(*sql.Server)
179179

180180
// Flush stats at the beginning of the test.
181-
sqlServer.GetSQLStatsController().ResetLocalSQLStats(ctx)
182-
sqlServer.GetReportedSQLStatsController().ResetLocalSQLStats(ctx)
181+
require.NoError(t, sqlServer.GetLocalSQLStatsProvider().Reset(ctx))
182+
require.NoError(t, sqlServer.GetReportedSQLStatsProvider().Reset(ctx))
183183

184184
// Execute some queries against the sqlDB to build up some stats.
185185
// As we are scrubbing the stats, we want to make sure the app name
@@ -213,7 +213,7 @@ func TestSQLStatCollection(t *testing.T) {
213213

214214
// Reset the SQL statistics, which will dump stats into the
215215
// reported statistics pool.
216-
sqlServer.GetSQLStatsController().ResetLocalSQLStats(ctx)
216+
require.NoError(t, sqlServer.GetLocalSQLStatsProvider().Reset(ctx))
217217

218218
// Query the reported statistics.
219219
stats, err = sqlServer.GetScrubbedReportingStats(ctx, 1000, true)
@@ -271,7 +271,7 @@ func TestSQLStatCollection(t *testing.T) {
271271
}
272272

273273
// Flush the SQL stats again.
274-
sqlServer.GetSQLStatsController().ResetLocalSQLStats(ctx)
274+
require.NoError(t, sqlServer.GetLocalSQLStatsProvider().Reset(ctx))
275275

276276
// Find our statement stat from the reported stats pool.
277277
stats, err = sqlServer.GetScrubbedReportingStats(ctx, 1000, true)
@@ -396,8 +396,8 @@ func TestScrubbedReportingStatsLimit(t *testing.T) {
396396
sqlRunner := sqlutils.MakeSQLRunner(sqlDB)
397397
sqlServer := srv.ApplicationLayer().SQLServer().(*sql.Server)
398398
// Flush stats at the beginning of the test.
399-
sqlServer.GetSQLStatsController().ResetLocalSQLStats(ctx)
400-
sqlServer.GetReportedSQLStatsController().ResetLocalSQLStats(ctx)
399+
require.NoError(t, sqlServer.GetLocalSQLStatsProvider().Reset(ctx))
400+
require.NoError(t, sqlServer.GetReportedSQLStatsProvider().Reset(ctx))
401401

402402
hashedAppName := "hashed app name"
403403
sqlRunner.Exec(t, `SET application_name = $1;`, hashedAppName)
@@ -409,13 +409,13 @@ func TestScrubbedReportingStatsLimit(t *testing.T) {
409409
sqlRunner.Exec(t, `DELETE FROM t.test WHERE x=5`)
410410

411411
// verify that with low limit, number of stats is within that limit
412-
sqlServer.GetSQLStatsController().ResetLocalSQLStats(ctx)
412+
require.NoError(t, sqlServer.GetLocalSQLStatsProvider().Reset(ctx))
413413
stats, err := sqlServer.GetScrubbedReportingStats(ctx, 5, true)
414414
require.NoError(t, err)
415415
require.LessOrEqual(t, len(stats), 5)
416416

417417
// verify that with high limit, the number of queries is as much as the above
418-
sqlServer.GetSQLStatsController().ResetLocalSQLStats(ctx)
418+
require.NoError(t, sqlServer.GetLocalSQLStatsProvider().Reset(ctx))
419419
stats, err = sqlServer.GetScrubbedReportingStats(ctx, 1000, true)
420420
require.NoError(t, err)
421421
require.GreaterOrEqual(t, len(stats), 7)

‎pkg/server/diagnostics/reporter.go‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -281,7 +281,10 @@ func (r *Reporter) ReportDiagnostics(ctx context.Context) {
281281
log.Warningf(ctx, "failed to report node usage metrics: status: %s, body: %s", res.Status, b)
282282
return
283283
}
284-
r.SQLServer.GetReportedSQLStatsController().ResetLocalSQLStats(ctx)
284+
err = r.SQLServer.GetReportedSQLStatsProvider().Reset(ctx)
285+
if err != nil {
286+
log.Warningf(ctx, "failed to reset SQL stats: %s", err)
287+
}
285288
}
286289

287290
// GetLastSuccessfulTelemetryPing will return the timestamp of when we last got

‎pkg/server/server_sql.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1146,7 +1146,7 @@ func newSQLServer(ctx context.Context, cfg sqlServerArgs) (*SQLServer, error) {
11461146
execCfg,
11471147
)
11481148

1149-
distSQLServer.ServerConfig.SQLStatsController = pgServer.SQLServer.GetSQLStatsController()
1149+
distSQLServer.ServerConfig.SQLStatsController = pgServer.SQLServer.GetSQLStatsProvider()
11501150
distSQLServer.ServerConfig.SchemaTelemetryController = pgServer.SQLServer.GetSchemaTelemetryController()
11511151
distSQLServer.ServerConfig.IndexUsageStatsController = pgServer.SQLServer.GetIndexUsageStatsController()
11521152

‎pkg/server/sql_stats.go‎

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -89,12 +89,13 @@ func (s *statusServer) ResetSQLStats(
8989
}
9090

9191
response := &serverpb.ResetSQLStatsResponse{}
92-
controller := s.sqlServer.pgServer.SQLServer.GetSQLStatsController()
92+
localSQLStats := s.sqlServer.pgServer.SQLServer.GetLocalSQLStatsProvider()
93+
persistedSQLStats := s.sqlServer.pgServer.SQLServer.GetSQLStatsProvider()
9394

94-
// If we need to reset persisted stats, we delegate to SQLStatsController,
95+
// If we need to reset persisted stats, we delegate to persisted sql stats,
9596
// which will trigger a system table truncation and RPC fanout under the hood.
9697
if req.ResetPersistedStats {
97-
if err := controller.ResetClusterSQLStats(ctx); err != nil {
98+
if err := persistedSQLStats.ResetClusterSQLStats(ctx); err != nil {
9899
return nil, err
99100
}
100101

@@ -113,8 +114,8 @@ func (s *statusServer) ResetSQLStats(
113114
return nil, status.Error(codes.InvalidArgument, err.Error())
114115
}
115116
if local {
116-
controller.ResetLocalSQLStats(ctx)
117-
return response, nil
117+
err := localSQLStats.Reset(ctx)
118+
return response, err
118119
}
119120
status, err := s.dialNode(ctx, requestedNodeID)
120121
if err != nil {

‎pkg/sql/conn_executor.go‎

Lines changed: 17 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -340,9 +340,6 @@ type Server struct {
340340
// node. Newly collected statistics flow into localSqlStats.
341341
localSqlStats *sslocal.SQLStats
342342

343-
// sqlStatsController is the control-plane interface for sqlStats.
344-
sqlStatsController *persistedsqlstats.Controller
345-
346343
// sqlStatsIngester provides the interface to consume stats about a sql execution.
347344
sqlStatsIngester *sslocal.SQLStatsIngester
348345

@@ -359,10 +356,6 @@ type Server struct {
359356
// into reported stats when sqlStats is cleared.
360357
reportedStats *sslocal.SQLStats
361358

362-
// reportedStatsController is the control-plane interface for
363-
// reportedStatsController.
364-
reportedStatsController *sslocal.Controller
365-
366359
insights *insights.Provider
367360

368361
reCache *tree.RegexpCache
@@ -452,7 +445,6 @@ func NewServer(cfg *ExecutorConfig, pool *mon.BytesMonitor) *Server {
452445
nil, /* reportedProvider */
453446
cfg.SQLStatsTestingKnobs,
454447
)
455-
reportedSQLStatsController := reportedSQLStats.GetController(cfg.SQLStatusServer)
456448
memSQLStats := sslocal.New(
457449
cfg.Settings,
458450
sqlstats.MaxMemSQLStatsStmtFingerprints,
@@ -471,18 +463,17 @@ func NewServer(cfg *ExecutorConfig, pool *mon.BytesMonitor) *Server {
471463
}
472464
})
473465
s := &Server{
474-
cfg: cfg,
475-
Metrics: metrics,
476-
InternalMetrics: makeMetrics(true /* internal */, &cfg.Settings.SV),
477-
ServerMetrics: serverMetrics,
478-
pool: pool,
479-
localSqlStats: memSQLStats,
480-
reportedStats: reportedSQLStats,
481-
sqlStatsIngester: sqlStatsIngester,
482-
reportedStatsController: reportedSQLStatsController,
483-
insights: insightsProvider,
484-
reCache: tree.NewRegexpCache(512),
485-
toCharFormatCache: tochar.NewFormatCache(512),
466+
cfg: cfg,
467+
Metrics: metrics,
468+
InternalMetrics: makeMetrics(true /* internal */, &cfg.Settings.SV),
469+
ServerMetrics: serverMetrics,
470+
pool: pool,
471+
localSqlStats: memSQLStats,
472+
reportedStats: reportedSQLStats,
473+
sqlStatsIngester: sqlStatsIngester,
474+
insights: insightsProvider,
475+
reCache: tree.NewRegexpCache(512),
476+
toCharFormatCache: tochar.NewFormatCache(512),
486477
indexUsageStats: idxusage.NewLocalIndexUsageStats(&idxusage.Config{
487478
ChannelSize: idxusage.DefaultChannelSize,
488479
Setting: cfg.Settings,
@@ -507,6 +498,7 @@ func NewServer(cfg *ExecutorConfig, pool *mon.BytesMonitor) *Server {
507498
ClusterID: s.cfg.NodeInfo.LogicalClusterID,
508499
SQLIDContainer: cfg.NodeInfo.NodeID,
509500
JobRegistry: s.cfg.JobRegistry,
501+
FanoutServer: cfg.SQLStatusServer,
510502
Knobs: cfg.SQLStatsTestingKnobs,
511503
FlushesSuccessful: serverMetrics.StatsMetrics.SQLStatsFlushesSuccessful,
512504
FlushDoneSignalsIgnored: serverMetrics.StatsMetrics.SQLStatsFlushDoneSignalsIgnored,
@@ -516,7 +508,6 @@ func NewServer(cfg *ExecutorConfig, pool *mon.BytesMonitor) *Server {
516508
}, memSQLStats)
517509

518510
s.sqlStats = persistedSQLStats
519-
s.sqlStatsController = persistedSQLStats.GetController(cfg.SQLStatusServer)
520511
schemaTelemetryIEMonitor := MakeInternalExecutorMemMonitor(MemoryMetrics{}, s.GetExecutorConfig().Settings)
521512
schemaTelemetryIEMonitor.StartNoReserved(context.Background(), s.GetBytesMonitor())
522513
s.schemaTelemetryController = schematelemetrycontroller.NewController(
@@ -700,12 +691,6 @@ func (s *Server) Start(ctx context.Context, stopper *stop.Stopper) {
700691
s.txnIDCache.Start(ctx, stopper)
701692
}
702693

703-
// GetSQLStatsController returns the persistedsqlstats.Controller for current
704-
// sql.Server's SQL Stats.
705-
func (s *Server) GetSQLStatsController() *persistedsqlstats.Controller {
706-
return s.sqlStatsController
707-
}
708-
709694
// GetSchemaTelemetryController returns the schematelemetryschedule.Controller
710695
// for current sql.Server's schema telemetry.
711696
func (s *Server) GetSchemaTelemetryController() *schematelemetrycontroller.Controller {
@@ -734,10 +719,10 @@ func (s *Server) GetLocalSQLStatsProvider() *sslocal.SQLStats {
734719
return s.localSqlStats
735720
}
736721

737-
// GetReportedSQLStatsController returns the sqlstats.Controller for the current
738-
// sql.Server's reported SQL Stats.
739-
func (s *Server) GetReportedSQLStatsController() *sslocal.Controller {
740-
return s.reportedStatsController
722+
// GetReportedSQLStatsProvider returns the provider for the in-memory reported
723+
// sql stats sink.
724+
func (s *Server) GetReportedSQLStatsProvider() *sslocal.SQLStats {
725+
return s.reportedStats
741726
}
742727

743728
// GetTxnIDCache returns the txnidcache.Cache for the current sql.Server.
@@ -3840,7 +3825,7 @@ func (ex *connExecutor) initEvalCtx(ctx context.Context, evalCtx *extendedEvalCo
38403825
SessionDataStack: ex.sessionDataStack,
38413826
ReCache: ex.server.reCache,
38423827
ToCharFormatCache: ex.server.toCharFormatCache,
3843-
SQLStatsController: ex.server.sqlStatsController,
3828+
SQLStatsController: ex.server.sqlStats,
38443829
SchemaTelemetryController: ex.server.schemaTelemetryController,
38453830
IndexUsageStatsController: ex.server.indexUsageStatsController,
38463831
ConsistencyChecker: p.execCfg.ConsistencyChecker,

‎pkg/sql/planner.go‎

Lines changed: 1 addition & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -497,17 +497,14 @@ func internalExtendedEvalCtx(
497497
evalContextTestingKnobs := execCfg.EvalContextTestingKnobs
498498

499499
var indexUsageStats *idxusage.LocalIndexUsageStats
500-
var sqlStatsController eval.SQLStatsController
501500
var schemaTelemetryController eval.SchemaTelemetryController
502501
var indexUsageStatsController eval.IndexUsageStatsController
503502
var sqlStatsProvider *persistedsqlstats.PersistedSQLStats
504503
var localSqlStatsProvider *sslocal.SQLStats
505504
if ief := execCfg.InternalDB; ief != nil {
506505
if ief.server != nil {
507506
indexUsageStats = ief.server.indexUsageStats
508-
sqlStatsController = ief.server.sqlStatsController
509507
schemaTelemetryController = ief.server.schemaTelemetryController
510-
indexUsageStatsController = ief.server.indexUsageStatsController
511508
sqlStatsProvider = ief.server.sqlStats
512509
localSqlStatsProvider = ief.server.localSqlStats
513510
} else {
@@ -517,7 +514,6 @@ func internalExtendedEvalCtx(
517514
indexUsageStats = idxusage.NewLocalIndexUsageStats(&idxusage.Config{
518515
Setting: execCfg.Settings,
519516
})
520-
sqlStatsController = &persistedsqlstats.Controller{}
521517
schemaTelemetryController = &schematelemetrycontroller.Controller{}
522518
indexUsageStatsController = &idxusage.Controller{}
523519
sqlStatsProvider = &persistedsqlstats.PersistedSQLStats{}
@@ -534,7 +530,7 @@ func internalExtendedEvalCtx(
534530
TestingKnobs: evalContextTestingKnobs,
535531
StmtTimestamp: stmtTimestamp,
536532
TxnTimestamp: txnTimestamp,
537-
SQLStatsController: sqlStatsController,
533+
SQLStatsController: sqlStatsProvider,
538534
SchemaTelemetryController: schemaTelemetryController,
539535
IndexUsageStatsController: indexUsageStatsController,
540536
ConsistencyChecker: execCfg.ConsistencyChecker,

‎pkg/sql/sqlstats/persistedsqlstats/BUILD.bazel‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,6 @@ go_library(
88
"cluster_settings.go",
99
"compaction_exec.go",
1010
"compaction_scheduling.go",
11-
"controller.go",
1211
"flush.go",
1312
"provider.go",
1413
"scheduled_job_monitor.go",

‎pkg/sql/sqlstats/persistedsqlstats/bench_test.go‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -385,7 +385,6 @@ func BenchmarkSqlStatsMaxFlushTime(b *testing.B) {
385385

386386
sqlStats := s.SQLServer().(*sql.Server).GetLocalSQLStatsProvider()
387387
pss := s.SQLServer().(*sql.Server).GetSQLStatsProvider()
388-
controller := s.SQLServer().(*sql.Server).GetSQLStatsController()
389388
stmtFingerprintLimit := sqlstats.MaxMemSQLStatsStmtFingerprints.Get(&s.ClusterSettings().SV)
390389
txnFingerprintLimit := sqlstats.MaxMemSQLStatsTxnFingerprints.Get(&s.ClusterSettings().SV)
391390

@@ -435,7 +434,7 @@ func BenchmarkSqlStatsMaxFlushTime(b *testing.B) {
435434
b.Run(fmt.Sprintf("single-application/writes=insert/%d-fingerprints", totalFingerprints), func(b *testing.B) {
436435
b.StopTimer()
437436
for i := 0; i < b.N; i++ {
438-
require.NoError(b, controller.ResetClusterSQLStats(ctx))
437+
require.NoError(b, pss.ResetClusterSQLStats(ctx))
439438
fillBenchAppMemStats()
440439
b.StartTimer()
441440
require.True(b, pss.MaybeFlush(ctx, s.AppStopper()))

0 commit comments

Comments
 (0)