topsql: shouldn't evict the SQL meta, since the evicted SQL can be ap…

…pear on Other components (TiKV) TopN records (#27050)
pingcap · Dec 13, 2021 · 12e2288 · 12e2288
1 parent d8a48d9
commit 12e2288
Show file tree

Hide file tree

Showing 8 changed files with 35 additions and 35 deletions.
diff --git a/executor/set_test.go b/executor/set_test.go
@@ -1530,22 +1530,22 @@ func (s *testSerialSuite) TestSetTopSQLVariables(c *C) {
 	tk.MustQuery("select @@global.tidb_top_sql_max_statement_count;").Check(testkit.Rows("20"))
 	c.Assert(variable.TopSQLVariable.MaxStatementCount.Load(), Equals, int64(20))
 
-	tk.MustExec("set @@global.tidb_top_sql_max_collect=20000;")
-	tk.MustQuery("select @@global.tidb_top_sql_max_collect;").Check(testkit.Rows("20000"))
-	c.Assert(variable.TopSQLVariable.MaxCollect.Load(), Equals, int64(20000))
+	tk.MustExec("set @@global.tidb_top_sql_max_collect=10000;")
+	tk.MustQuery("select @@global.tidb_top_sql_max_collect;").Check(testkit.Rows("10000"))
+	c.Assert(variable.TopSQLVariable.MaxCollect.Load(), Equals, int64(10000))
 	_, err = tk.Exec("set @@global.tidb_top_sql_max_collect='abc';")
 	c.Assert(err.Error(), Equals, "[variable:1232]Incorrect argument type to variable 'tidb_top_sql_max_collect'")
 	tk.MustExec("set @@global.tidb_top_sql_max_collect='-1';")
 	tk.MustQuery(`show warnings`).Check(testkit.Rows("Warning 1292 Truncated incorrect tidb_top_sql_max_collect value: '-1'"))
 	tk.MustQuery("select @@global.tidb_top_sql_max_collect;").Check(testkit.Rows("1"))
 
-	tk.MustExec("set @@global.tidb_top_sql_max_collect='500001';")
-	tk.MustQuery(`show warnings`).Check(testkit.Rows("Warning 1292 Truncated incorrect tidb_top_sql_max_collect value: '500001'"))
-	tk.MustQuery("select @@global.tidb_top_sql_max_collect;").Check(testkit.Rows("500000"))
+	tk.MustExec("set @@global.tidb_top_sql_max_collect='10001';")
+	tk.MustQuery(`show warnings`).Check(testkit.Rows("Warning 1292 Truncated incorrect tidb_top_sql_max_collect value: '10001'"))
+	tk.MustQuery("select @@global.tidb_top_sql_max_collect;").Check(testkit.Rows("10000"))
 
-	tk.MustExec("set @@global.tidb_top_sql_max_collect=20000;")
-	tk.MustQuery("select @@global.tidb_top_sql_max_collect;").Check(testkit.Rows("20000"))
-	c.Assert(variable.TopSQLVariable.MaxCollect.Load(), Equals, int64(20000))
+	tk.MustExec("set @@global.tidb_top_sql_max_collect=5000;")
+	tk.MustQuery("select @@global.tidb_top_sql_max_collect;").Check(testkit.Rows("5000"))
+	c.Assert(variable.TopSQLVariable.MaxCollect.Load(), Equals, int64(5000))
 
 	tk.MustExec("set @@global.tidb_top_sql_report_interval_seconds=120;")
 	tk.MustQuery("select @@global.tidb_top_sql_report_interval_seconds;").Check(testkit.Rows("120"))

diff --git a/sessionctx/variable/sysvar.go b/sessionctx/variable/sysvar.go
@@ -1276,7 +1276,7 @@ var defaultSysVars = []*SysVar{
 		TopSQLVariable.MaxStatementCount.Store(val)
 		return nil
 	}},
-	{Scope: ScopeGlobal, Name: TiDBTopSQLMaxCollect, Value: strconv.Itoa(DefTiDBTopSQLMaxCollect), Type: TypeInt, Hidden: true, MinValue: 1, MaxValue: 500000, GetGlobal: func(s *SessionVars) (string, error) {
+	{Scope: ScopeGlobal, Name: TiDBTopSQLMaxCollect, Value: strconv.Itoa(DefTiDBTopSQLMaxCollect), Type: TypeInt, Hidden: true, MinValue: 1, MaxValue: 10000, GetGlobal: func(s *SessionVars) (string, error) {
 		return strconv.FormatInt(TopSQLVariable.MaxCollect.Load(), 10), nil
 	}, SetGlobal: func(vars *SessionVars, s string) error {
 		val, err := strconv.ParseInt(s, 10, 64)

diff --git a/sessionctx/variable/tidb_vars.go b/sessionctx/variable/tidb_vars.go
@@ -766,7 +766,7 @@ const (
 	DefTiDBTopSQLEnable                   = false
 	DefTiDBTopSQLPrecisionSeconds         = 1
 	DefTiDBTopSQLMaxStatementCount        = 200
-	DefTiDBTopSQLMaxCollect               = 10000
+	DefTiDBTopSQLMaxCollect               = 5000
 	DefTiDBTopSQLReportIntervalSeconds    = 60
 	DefTiDBTmpTableMaxSize                = 64 << 20 // 64MB.
 	DefTiDBEnableLocalTxn                 = false

diff --git a/util/topsql/reporter/mock/server.go b/util/topsql/reporter/mock/server.go
@@ -192,6 +192,16 @@ func (svr *mockAgentServer) GetLatestRecords() []*tipb.CPUTimeRecord {
 	return records[len(records)-1]
 }
 
+func (svr *mockAgentServer) GetTotalSQLMetas() []tipb.SQLMeta {
+	svr.Lock()
+	defer svr.Unlock()
+	metas := make([]tipb.SQLMeta, 0, len(svr.sqlMetas))
+	for _, meta := range svr.sqlMetas {
+		metas = append(metas, meta)
+	}
+	return metas
+}
+
 func (svr *mockAgentServer) Address() string {
 	return svr.addr
 }

diff --git a/util/topsql/reporter/reporter.go b/util/topsql/reporter/reporter.go
@@ -411,25 +411,11 @@ func (tsr *RemoteTopSQLReporter) doCollect(
 	if len(evicted) == 0 {
 		return
 	}
-	// Clean up non Top N data and merge them as "others" (keyed by in the `keyOthers`) in `collectTarget`.
-	normalizedSQLMap := tsr.normalizedSQLMap.Load().(*sync.Map)
-	normalizedPlanMap := tsr.normalizedPlanMap.Load().(*sync.Map)
+	// Merge non Top N data as "others" (keyed by in the `keyOthers`) in `collectTarget`.
+	// SQL meta will not be evicted, since the evicted SQL can be appear on Other components (TiKV) TopN records.
 	totalEvictedCPUTime := uint32(0)
 	for _, evict := range evicted {
 		totalEvictedCPUTime += evict.CPUTimeMs
-		key := encodeKey(keyBuf, evict.SQLDigest, evict.PlanDigest)
-		_, ok := collectTarget[key]
-		if ok {
-			continue
-		}
-		_, loaded := normalizedSQLMap.LoadAndDelete(string(evict.SQLDigest))
-		if loaded {
-			tsr.sqlMapLength.Add(-1)
-		}
-		_, loaded = normalizedPlanMap.LoadAndDelete(string(evict.PlanDigest))
-		if loaded {
-			tsr.planMapLength.Add(-1)
-		}
 	}
 	addEvictedCPUTime(collectTarget, timestamp, totalEvictedCPUTime)
 }
@@ -529,8 +515,7 @@ func (tsr *RemoteTopSQLReporter) getReportData(collected collectedData) reportDa
 		sort.Sort(others)
 	}
 	for _, evict := range evicted {
-		collected.normalizedSQLMap.LoadAndDelete(string(evict.SQLDigest))
-		collected.normalizedPlanMap.LoadAndDelete(string(evict.PlanDigest))
+		// SQL meta will not be evicted, since the evicted SQL can be appear on Other components (TiKV) TopN records.
 		others = addEvictedIntoSortedDataPoints(others, evict)
 	}
 

diff --git a/util/topsql/reporter/reporter_test.go b/util/topsql/reporter/reporter_test.go
@@ -66,6 +66,7 @@ func mockPlanBinaryDecoderFunc(plan string) (string, error) {
 
 func setupRemoteTopSQLReporter(maxStatementsNum, interval int, addr string) *RemoteTopSQLReporter {
 	variable.TopSQLVariable.MaxStatementCount.Store(int64(maxStatementsNum))
+	variable.TopSQLVariable.MaxCollect.Store(10000)
 	variable.TopSQLVariable.ReportIntervalSeconds.Store(int64(interval))
 	config.UpdateGlobal(func(conf *config.Config) {
 		conf.TopSQL.ReceiverAddress = addr
@@ -249,6 +250,10 @@ func TestCollectAndTopN(t *testing.T) {
 	require.Equal(t, 5, getTotalCPUTime(results[1]))
 	require.Equal(t, []byte("sqlDigest3"), results[2].SqlDigest)
 	require.Equal(t, 3, getTotalCPUTime(results[2]))
+	// sleep to wait for all SQL meta received.
+	time.Sleep(50 * time.Millisecond)
+	totalMetas := agentServer.GetTotalSQLMetas()
+	require.Equal(t, 6, len(totalMetas))
 }
 
 func TestCollectCapacity(t *testing.T) {
@@ -302,8 +307,8 @@ func TestCollectCapacity(t *testing.T) {
 	collectedData := make(map[string]*dataPoints)
 	tsr.doCollect(collectedData, 1, genRecord(20000))
 	require.Equal(t, 5001, len(collectedData))
-	require.Equal(t, int64(5000), tsr.sqlMapLength.Load())
-	require.Equal(t, int64(5000), tsr.planMapLength.Load())
+	require.Equal(t, int64(20000), tsr.sqlMapLength.Load())
+	require.Equal(t, int64(20000), tsr.planMapLength.Load())
 }
 
 func TestCollectOthers(t *testing.T) {

diff --git a/util/topsql/topsql.go b/util/topsql/topsql.go
@@ -32,8 +32,8 @@ import (
 const (
 	// MaxSQLTextSize exports for testing.
 	MaxSQLTextSize = 4 * 1024
-	// MaxPlanTextSize exports for testing.
-	MaxPlanTextSize = 32 * 1024
+	// MaxBinaryPlanSize exports for testing.
+	MaxBinaryPlanSize = 2 * 1024
 )
 
 var globalTopSQLReport reporter.TopSQLReporter
@@ -122,7 +122,7 @@ func linkSQLTextWithDigest(sqlDigest []byte, normalizedSQL string, isInternal bo
 }
 
 func linkPlanTextWithDigest(planDigest []byte, normalizedBinaryPlan string) {
-	if len(normalizedBinaryPlan) > MaxPlanTextSize {
+	if len(normalizedBinaryPlan) > MaxBinaryPlanSize {
 		// ignore the huge size plan
 		return
 	}

diff --git a/util/topsql/topsql_test.go b/util/topsql/topsql_test.go
@@ -199,7 +199,7 @@ func TestMaxSQLAndPlanTest(t *testing.T) {
 	sql = genStr(topsql.MaxSQLTextSize + 10)
 	sqlDigest = mock.GenSQLDigest(sql)
 	topsql.AttachSQLInfo(ctx, sql, sqlDigest, "", nil, false)
-	plan = genStr(topsql.MaxPlanTextSize + 10)
+	plan = genStr(topsql.MaxBinaryPlanSize + 10)
 	planDigest = genDigest(plan)
 	topsql.AttachSQLInfo(ctx, sql, sqlDigest, plan, planDigest, false)