Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

*: add global/instance variable to config top sql #24934

Merged
merged 16 commits into from
May 31, 2021
Merged

*: add global/instance variable to config top sql #24934

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
dc18204
*: add global/instance variable to config top sql
crazycs520 May 27, 2021
4b00877
fix test
crazycs520 May 27, 2021
2b5a7eb
Merge branch 'master' of https://github.com/pingcap/tidb into top-sql…
crazycs520 May 27, 2021
6522259
Merge branch 'master' of https://github.com/pingcap/tidb into top-sql…
crazycs520 May 28, 2021
cc0837a
address comment and remove useless code
crazycs520 May 28, 2021
dd1b451
move top sql variable from config to variable
crazycs520 May 28, 2021
fa1b5c3
Merge branch 'master' of https://github.com/pingcap/tidb into top-sql…
crazycs520 May 28, 2021
a553c05
Merge branch 'master' into top-sql-config
crazycs520 May 28, 2021
4c70e71
address comment to change default value and max value
crazycs520 May 31, 2021
0f8274c
Merge branch 'top-sql-config' of https://github.com/crazycs520/tidb i…
crazycs520 May 31, 2021
e86fa65
Merge branch 'master' into top-sql-config
crazycs520 May 31, 2021
3dd6ac8
Merge branch 'master' into top-sql-config
ti-chi-bot May 31, 2021
0dbcec5
Merge branch 'master' into top-sql-config
ti-chi-bot May 31, 2021
c0ac680
Merge branch 'master' into top-sql-config
ti-chi-bot May 31, 2021
38f257a
use serial test
crazycs520 May 31, 2021
4b953d9
Merge branch 'master' into top-sql-config
ti-chi-bot May 31, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 20 additions & 8 deletions config/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,14 @@ const (
DefTableColumnCountLimit = 1017
// DefMaxOfTableColumnCountLimit is maximum limitation of the number of columns in a table
DefMaxOfTableColumnCountLimit = 4096
// DefTopSQLEnable is the default value of TopSQL.Enable
DefTopSQLEnable = false
// DefTopSQLAgentAddress is the default value of TopSQL.AgentAddress
DefTopSQLAgentAddress = ""
// DefTopSQLPrecisionSeconds is the default value of TopSQL.PrecisionSeconds
DefTopSQLPrecisionSeconds = 1
// DefTopSQLMaxStatementCount is the default value of TopSQL.MaxStatementCount
DefTopSQLMaxStatementCount = 5000
)

// Valid config maps
Expand Down Expand Up @@ -137,7 +145,7 @@ type Config struct {
DelayCleanTableLock uint64 `toml:"delay-clean-table-lock" json:"delay-clean-table-lock"`
SplitRegionMaxNum uint64 `toml:"split-region-max-num" json:"split-region-max-num"`
StmtSummary StmtSummary `toml:"stmt-summary" json:"stmt-summary"`
TopSQL TopSQL `toml:"top-sql" json:"top-sql"`
TopSQL TopSQL `toml:"-" json:"-"`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you explain why the toml and json names are blanked out - is the goal to no longer specify them in toml, but instead use them in sysvars?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because the TopSQL feature doesn't want to provide a config item in the TiDB config file, only provide the instance/global variable to control.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@breeswish PTAL

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because I think it is hard for the user to understand in this situation:

The user changes the config item in config file and restart the TiDB server, then which value takes effect? the config value or the global variable value? It will confuse the user.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As an instance config that is not placed in the config file, is it necessary to be inside the config.go?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just for coding convenience. Of course, we can use a unique global variable to do it. Should we ?

Copy link
Contributor Author

@crazycs520 crazycs520 May 28, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will add a new global variable to store the TopSQL config, and remove it from config pkg.

// RepairMode indicates that the TiDB is in the repair mode for table meta.
RepairMode bool `toml:"repair-mode" json:"repair-mode"`
RepairTableList []string `toml:"repair-table-list" json:"repair-table-list"`
Expand Down Expand Up @@ -530,11 +538,13 @@ type StmtSummary struct {
// TopSQL is the config for top sql.
type TopSQL struct {
// Enable statement summary or not.
Enable bool `toml:"enable" json:"enable"`
Enable bool `toml:"-" json:"-"`
// AgentAddress indicate the collect agent address.
AgentAddress string `toml:"-" json:"-"`
// The refresh interval of statement summary.
RefreshInterval int `toml:"refresh-interval" json:"refresh-interval"`
PrecisionSeconds int `toml:"-" json:"-"`
// The maximum number of statements kept in memory.
MaxStmtCount uint `toml:"max-stmt-count" json:"max-stmt-count"`
MaxStatementCount int `toml:"-" json:"-"`
}

// IsolationRead is the config for isolation read.
Expand Down Expand Up @@ -667,9 +677,10 @@ var defaultConf = Config{
HistorySize: 24,
},
TopSQL: TopSQL{
Enable: true,
RefreshInterval: 1,
MaxStmtCount: 5000,
Enable: DefTopSQLEnable,
AgentAddress: DefTopSQLAgentAddress,
PrecisionSeconds: DefTopSQLPrecisionSeconds,
MaxStatementCount: DefTopSQLMaxStatementCount,
},
IsolationRead: IsolationRead{
Engines: []string{"tikv", "tiflash", "tidb"},
Expand Down Expand Up @@ -960,7 +971,8 @@ func TableLockEnabled() bool {

// TopSQLEnabled uses to check whether enabled the top SQL feature.
func TopSQLEnabled() bool {
return GetGlobalConfig().TopSQL.Enable
cfg := GetGlobalConfig()
return cfg.TopSQL.Enable && cfg.TopSQL.AgentAddress != ""
}

// TableLockDelayClean uses to get the time of delay clean table lock.
Expand Down
27 changes: 27 additions & 0 deletions domain/sysvar_cache.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,10 @@ package domain
import (
"context"
"fmt"
"strconv"
"sync"

"github.com/pingcap/tidb/config"
"github.com/pingcap/tidb/sessionctx"
"github.com/pingcap/tidb/sessionctx/variable"
"github.com/pingcap/tidb/util/logutil"
Expand Down Expand Up @@ -162,6 +164,31 @@ func checkEnableServerGlobalVar(name, sVal string) {
err = stmtsummary.StmtSummaryByDigestMap.SetMaxSQLLength(sVal, false)
case variable.TiDBCapturePlanBaseline:
variable.CapturePlanBaseline.Set(sVal, false)
case variable.TiDBEnableTopSQL:
config.UpdateGlobal(func(conf *config.Config) {
conf.TopSQL.Enable = variable.TiDBOptOn(sVal)
})
case variable.TiDBTopSQLAgentAddress:
config.UpdateGlobal(func(conf *config.Config) {
// todo: add validate
conf.TopSQL.AgentAddress = sVal
})
case variable.TiDBTopSQLPrecisionSeconds:
var val int
val, err = strconv.Atoi(sVal)
if err == nil {
config.UpdateGlobal(func(conf *config.Config) {
conf.TopSQL.PrecisionSeconds = val
})
}
case variable.TiDBTopSQLMaxStatementCount:
var val int
val, err = strconv.Atoi(sVal)
if err == nil {
config.UpdateGlobal(func(conf *config.Config) {
conf.TopSQL.MaxStatementCount = val
})
}
morgo marked this conversation as resolved.
Show resolved Hide resolved
}
if err != nil {
logutil.BgLogger().Error(fmt.Sprintf("load global variable %s error", name), zap.Error(err))
Expand Down
8 changes: 4 additions & 4 deletions executor/executor_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8362,10 +8362,10 @@ func (s *testResourceTagSuite) TestResourceGroupTag(c *C) {
tbInfo := testGetTableByName(c, tk.Se, "test", "t")

// Enable Top SQL
cfg := config.GetGlobalConfig()
newCfg := *cfg
newCfg.TopSQL.Enable = true
config.StoreGlobalConfig(&newCfg)
config.UpdateGlobal(func(conf *config.Config) {
conf.TopSQL.Enable = true
conf.TopSQL.AgentAddress = "mock-agent"
})

c.Assert(failpoint.Enable("github.com/pingcap/tidb/store/mockstore/unistore/unistoreRPCClientSendHook", `return(true)`), IsNil)
defer failpoint.Disable("github.com/pingcap/tidb/store/mockstore/unistore/unistoreRPCClientSendHook")
Expand Down
28 changes: 27 additions & 1 deletion executor/set.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,12 +15,14 @@ package executor

import (
"context"
"strconv"
"strings"

"github.com/pingcap/errors"
"github.com/pingcap/parser/ast"
"github.com/pingcap/parser/charset"
"github.com/pingcap/parser/mysql"
"github.com/pingcap/tidb/config"
"github.com/pingcap/tidb/domain"
"github.com/pingcap/tidb/expression"
"github.com/pingcap/tidb/plugin"
Expand Down Expand Up @@ -181,8 +183,32 @@ func (e *SetExecutor) setSysVariable(name string, v *expression.VarAssignment) e
return stmtsummary.StmtSummaryByDigestMap.SetMaxSQLLength(valStr, !v.IsGlobal)
case variable.TiDBCapturePlanBaseline:
variable.CapturePlanBaseline.Set(valStrToBoolStr, !v.IsGlobal)
case variable.TiDBEnableTopSQL:
config.UpdateGlobal(func(conf *config.Config) {
conf.TopSQL.Enable = variable.TiDBOptOn(valStr)
})
case variable.TiDBTopSQLAgentAddress:
// todo: add validate
config.UpdateGlobal(func(conf *config.Config) {
conf.TopSQL.AgentAddress = valStr
})
case variable.TiDBTopSQLPrecisionSeconds:
var val int
val, err = strconv.Atoi(valStr)
if err == nil {
config.UpdateGlobal(func(conf *config.Config) {
conf.TopSQL.PrecisionSeconds = val
})
}
case variable.TiDBTopSQLMaxStatementCount:
var val int
val, err = strconv.Atoi(valStr)
if err == nil {
config.UpdateGlobal(func(conf *config.Config) {
conf.TopSQL.MaxStatementCount = val
})
}
morgo marked this conversation as resolved.
Show resolved Hide resolved
}

return nil
}

Expand Down
69 changes: 69 additions & 0 deletions executor/set_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1376,3 +1376,72 @@ func (s *testSuite5) TestSetClusterConfigJSONData(c *C) {
}
}
}

func (s *testSuite5) TestSetTopSQLVariables(c *C) {
tk := testkit.NewTestKit(c, s.store)
tk.MustExec("set @@tidb_enable_top_sql='On';")
tk.MustQuery("select @@tidb_enable_top_sql;").Check(testkit.Rows("1"))
c.Assert(config.GetGlobalConfig().TopSQL.Enable, IsTrue)
tk.MustExec("set @@tidb_enable_top_sql='off';")
tk.MustQuery("select @@tidb_enable_top_sql;").Check(testkit.Rows("0"))
c.Assert(config.GetGlobalConfig().TopSQL.Enable, IsFalse)
tk.MustExec("set @@global.tidb_enable_top_sql='On';")
tk.MustQuery("select @@global.tidb_enable_top_sql;").Check(testkit.Rows("1"))
c.Assert(config.GetGlobalConfig().TopSQL.Enable, IsTrue)
tk.MustExec("set @@global.tidb_enable_top_sql='off';")
tk.MustQuery("select @@global.tidb_enable_top_sql;").Check(testkit.Rows("0"))
c.Assert(config.GetGlobalConfig().TopSQL.Enable, IsFalse)

tk.MustExec("set @@tidb_top_sql_agent_address='127.0.0.1:4001';")
tk.MustQuery("select @@tidb_top_sql_agent_address;").Check(testkit.Rows("127.0.0.1:4001"))
c.Assert(config.GetGlobalConfig().TopSQL.AgentAddress, Equals, "127.0.0.1:4001")
tk.MustExec("set @@tidb_top_sql_agent_address='';")
tk.MustQuery("select @@tidb_top_sql_agent_address;").Check(testkit.Rows(""))
c.Assert(config.GetGlobalConfig().TopSQL.AgentAddress, Equals, "")
tk.MustExec("set @@global.tidb_top_sql_agent_address='127.0.0.1:4001';")
tk.MustQuery("select @@global.tidb_top_sql_agent_address;").Check(testkit.Rows("127.0.0.1:4001"))
c.Assert(config.GetGlobalConfig().TopSQL.AgentAddress, Equals, "127.0.0.1:4001")
tk.MustExec("set @@global.tidb_top_sql_agent_address='';")
tk.MustQuery("select @@global.tidb_top_sql_agent_address;").Check(testkit.Rows(""))
c.Assert(config.GetGlobalConfig().TopSQL.AgentAddress, Equals, "")

tk.MustExec("set @@tidb_top_sql_precision_seconds=60;")
tk.MustQuery("select @@tidb_top_sql_precision_seconds;").Check(testkit.Rows("60"))
c.Assert(config.GetGlobalConfig().TopSQL.PrecisionSeconds, Equals, 60)
_, err := tk.Exec("set @@tidb_top_sql_precision_seconds='abc';")
c.Assert(err.Error(), Equals, "[variable:1232]Incorrect argument type to variable 'tidb_top_sql_precision_seconds'")
_, err = tk.Exec("set @@tidb_top_sql_precision_seconds='-1';")
c.Assert(err.Error(), Equals, "[variable:1231]Variable 'tidb_top_sql_precision_seconds' can't be set to the value of '-1'")
tk.MustQuery("select @@tidb_top_sql_precision_seconds;").Check(testkit.Rows("60"))
c.Assert(config.GetGlobalConfig().TopSQL.PrecisionSeconds, Equals, 60)
tk.MustExec("set @@global.tidb_top_sql_precision_seconds=2;")
tk.MustQuery("select @@global.tidb_top_sql_precision_seconds;").Check(testkit.Rows("2"))
c.Assert(config.GetGlobalConfig().TopSQL.PrecisionSeconds, Equals, 2)
_, err = tk.Exec("set @@global.tidb_top_sql_precision_seconds='abc';")
c.Assert(err.Error(), Equals, "[variable:1232]Incorrect argument type to variable 'tidb_top_sql_precision_seconds'")
_, err = tk.Exec("set @@global.tidb_top_sql_precision_seconds='-1';")
c.Assert(err.Error(), Equals, "[variable:1231]Variable 'tidb_top_sql_precision_seconds' can't be set to the value of '-1'")
tk.MustQuery("select @@global.tidb_top_sql_precision_seconds;").Check(testkit.Rows("2"))
c.Assert(config.GetGlobalConfig().TopSQL.PrecisionSeconds, Equals, 2)

tk.MustExec("set @@tidb_top_sql_max_statement_count=10000;")
tk.MustQuery("select @@tidb_top_sql_max_statement_count;").Check(testkit.Rows("10000"))
c.Assert(config.GetGlobalConfig().TopSQL.MaxStatementCount, Equals, 10000)
_, err = tk.Exec("set @@tidb_top_sql_max_statement_count='abc';")
c.Assert(err.Error(), Equals, "[variable:1232]Incorrect argument type to variable 'tidb_top_sql_max_statement_count'")
_, err = tk.Exec("set @@tidb_top_sql_max_statement_count='-1';")
c.Assert(err.Error(), Equals, "[variable:1231]Variable 'tidb_top_sql_max_statement_count' can't be set to the value of '-1'")
tk.MustQuery("select @@tidb_top_sql_max_statement_count;").Check(testkit.Rows("10000"))
c.Assert(config.GetGlobalConfig().TopSQL.MaxStatementCount, Equals, 10000)
tk.MustExec("set @@global.tidb_top_sql_max_statement_count=2;")
tk.MustQuery("select @@global.tidb_top_sql_max_statement_count;").Check(testkit.Rows("2"))
c.Assert(config.GetGlobalConfig().TopSQL.MaxStatementCount, Equals, 2)
_, err = tk.Exec("set @@global.tidb_top_sql_max_statement_count='abc';")
c.Assert(err.Error(), Equals, "[variable:1232]Incorrect argument type to variable 'tidb_top_sql_max_statement_count'")
_, err = tk.Exec("set @@global.tidb_top_sql_max_statement_count='-1';")
c.Assert(err.Error(), Equals, "[variable:1231]Variable 'tidb_top_sql_max_statement_count' can't be set to the value of '-1'")
_, err = tk.Exec("set @@global.tidb_top_sql_max_statement_count='10001';")
c.Assert(err.Error(), Equals, "[variable:1231]Variable 'tidb_top_sql_max_statement_count' can't be set to the value of '10001'")
tk.MustQuery("select @@global.tidb_top_sql_precision_seconds;").Check(testkit.Rows("2"))
c.Assert(config.GetGlobalConfig().TopSQL.MaxStatementCount, Equals, 2)
}
14 changes: 14 additions & 0 deletions sessionctx/variable/sysvar.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1628,6 +1628,20 @@ var defaultSysVars = []*SysVar{
{Scope: ScopeGlobal, Name: TiDBGCLifetime, Value: "10m0s", Type: TypeDuration, MinValue: int64(time.Minute * 10), MaxValue: math.MaxInt64},
{Scope: ScopeGlobal, Name: TiDBGCConcurrency, Value: "-1", Type: TypeInt, MinValue: 1, MaxValue: 128, AllowAutoValue: true},
{Scope: ScopeGlobal, Name: TiDBGCScanLockMode, Value: "PHYSICAL", Type: TypeEnum, PossibleValues: []string{"PHYSICAL", "LEGACY"}},

// variable for top SQL feature.
{Scope: ScopeGlobal | ScopeSession, Name: TiDBEnableTopSQL, Value: BoolToOnOff(config.DefTopSQLEnable), Type: TypeBool, AllowEmpty: true, GetSession: func(s *SessionVars) (string, error) {
return BoolToOnOff(config.GetGlobalConfig().TopSQL.Enable), nil
}},
{Scope: ScopeGlobal | ScopeSession, Name: TiDBTopSQLAgentAddress, Value: config.DefTopSQLAgentAddress, Type: TypeStr, AllowEmpty: true, GetSession: func(s *SessionVars) (string, error) {
return config.GetGlobalConfig().TopSQL.AgentAddress, nil
}},
{Scope: ScopeGlobal | ScopeSession, Name: TiDBTopSQLPrecisionSeconds, Value: strconv.Itoa(config.DefTopSQLPrecisionSeconds), Type: TypeInt, MinValue: 1, MaxValue: math.MaxInt64, AllowEmpty: true, GetSession: func(s *SessionVars) (string, error) {
return strconv.Itoa(config.GetGlobalConfig().TopSQL.PrecisionSeconds), nil
}},
{Scope: ScopeGlobal | ScopeSession, Name: TiDBTopSQLMaxStatementCount, Value: strconv.Itoa(config.DefTopSQLMaxStatementCount), Type: TypeInt, MinValue: 0, MaxValue: 10000, AllowEmpty: true, GetSession: func(s *SessionVars) (string, error) {
return strconv.Itoa(config.GetGlobalConfig().TopSQL.MaxStatementCount), nil
}},
}

// FeedbackProbability points to the FeedbackProbability in statistics package.
Expand Down
12 changes: 12 additions & 0 deletions sessionctx/variable/tidb_vars.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -531,6 +531,18 @@ const (

// TiDBEnableDynamicPrivileges enables MySQL 8.0 compatible dynamic privileges (experimental).
TiDBEnableDynamicPrivileges = "tidb_enable_dynamic_privileges"

// TiDBEnableTopSQL indicates whether the top SQL is enabled.
TiDBEnableTopSQL = "tidb_enable_top_sql"

// TiDBTopSQLAgentAddress indicates the top SQL agent address.
TiDBTopSQLAgentAddress = "tidb_top_sql_agent_address"

// TiDBTopSQLPrecisionSeconds indicates the top SQL precision seconds.
TiDBTopSQLPrecisionSeconds = "tidb_top_sql_precision_seconds"

// TiDBTopSQLMaxStatementCount indicates the max number of statements been collected.
TiDBTopSQLMaxStatementCount = "tidb_top_sql_max_statement_count"
)

// TiDB vars that have only global scope
Expand Down