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.

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

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

Merged
merged 16 commits into from
May 31, 2021
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
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
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

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
8 changes: 4 additions & 4 deletions executor/executor_test.go
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
1 change: 0 additions & 1 deletion executor/set.go
Original file line number Diff line number Diff line change
Expand Up @@ -182,7 +182,6 @@ func (e *SetExecutor) setSysVariable(name string, v *expression.VarAssignment) e
case variable.TiDBCapturePlanBaseline:
variable.CapturePlanBaseline.Set(valStrToBoolStr, !v.IsGlobal)
}

return nil
}

Expand Down
69 changes: 69 additions & 0 deletions executor/set_test.go
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)
}
71 changes: 71 additions & 0 deletions sessionctx/variable/sysvar.go
Original file line number Diff line number Diff line change
Expand Up @@ -1628,6 +1628,77 @@ 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
}, SetSession: func(vars *SessionVars, s string) error {
config.UpdateGlobal(func(conf *config.Config) {
conf.TopSQL.Enable = TiDBOptOn(s)
})
return nil
}, SetGlobal: func(vars *SessionVars, s string) error {
config.UpdateGlobal(func(conf *config.Config) {
crazycs520 marked this conversation as resolved.
Show resolved Hide resolved
conf.TopSQL.Enable = TiDBOptOn(s)
})
return nil
}},
// todo: add validation
crazycs520 marked this conversation as resolved.
Show resolved Hide resolved
{Scope: ScopeGlobal | ScopeSession, Name: TiDBTopSQLAgentAddress, Value: config.DefTopSQLAgentAddress, Type: TypeStr, AllowEmpty: true, GetSession: func(s *SessionVars) (string, error) {
return config.GetGlobalConfig().TopSQL.AgentAddress, nil
}, SetSession: func(vars *SessionVars, s string) error {
config.UpdateGlobal(func(conf *config.Config) {
conf.TopSQL.AgentAddress = s
})
return nil
}, SetGlobal: func(vars *SessionVars, s string) error {
config.UpdateGlobal(func(conf *config.Config) {
conf.TopSQL.AgentAddress = s
})
return 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
}, SetSession: func(vars *SessionVars, s string) error {
val, err := strconv.Atoi(s)
if err != nil {
return err
}
config.UpdateGlobal(func(conf *config.Config) {
conf.TopSQL.PrecisionSeconds = val
})
return nil
}, SetGlobal: func(vars *SessionVars, s string) error {
val, err := strconv.Atoi(s)
if err != nil {
return err
}
config.UpdateGlobal(func(conf *config.Config) {
conf.TopSQL.PrecisionSeconds = val
})
return 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
}, SetSession: func(vars *SessionVars, s string) error {
val, err := strconv.Atoi(s)
if err != nil {
return err
}
config.UpdateGlobal(func(conf *config.Config) {
conf.TopSQL.MaxStatementCount = val
})
return nil
}, SetGlobal: func(vars *SessionVars, s string) error {
val, err := strconv.Atoi(s)
if err != nil {
return err
}
config.UpdateGlobal(func(conf *config.Config) {
conf.TopSQL.MaxStatementCount = val
})
return nil
}},
}

// FeedbackProbability points to the FeedbackProbability in statistics package.
Expand Down
12 changes: 12 additions & 0 deletions sessionctx/variable/tidb_vars.go
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