-
Notifications
You must be signed in to change notification settings - Fork 5.8k
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
*: Move tikv gc configuration to sysvars #21988
Changes from 19 commits
87c72ea
9c4e9cc
55dc0c6
34395c8
757c201
3944be0
4aea44a
1d931bc
a454ad1
6e3b461
4fcc43d
90993cd
8a73198
2d45af3
510ae7a
96cc636
e1ec48b
cf5585c
f550923
b89cb9e
9bbf7fe
bb8cba4
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -94,8 +94,29 @@ var ( | |
sessionExecuteCompileDurationGeneral = metrics.SessionExecuteCompileDuration.WithLabelValues(metrics.LblGeneral) | ||
sessionExecuteParseDurationInternal = metrics.SessionExecuteParseDuration.WithLabelValues(metrics.LblInternal) | ||
sessionExecuteParseDurationGeneral = metrics.SessionExecuteParseDuration.WithLabelValues(metrics.LblGeneral) | ||
|
||
tiKVGCAutoConcurrency = "tikv_gc_auto_concurrency" | ||
) | ||
|
||
var gcVariableComments = map[string]string{ | ||
variable.TiDBGCRunInterval: "GC run interval, at least 10m, in Go format.", | ||
variable.TiDBGCLifetime: "All versions within life time will not be collected by GC, at least 10m, in Go format.", | ||
variable.TiDBGCConcurrency: "How many goroutines used to do GC parallel, [1, 128], default 2", | ||
variable.TiDBGCEnable: "Current GC enable status", | ||
variable.TiDBGCMode: "Mode of GC, \"central\" or \"distributed\"", | ||
tiKVGCAutoConcurrency: "Let TiDB pick the concurrency automatically. If set false, tikv_gc_concurrency will be used", | ||
variable.TiDBGCScanLockMode: "Mode of scanning locks, \"physical\" or \"legacy\"", | ||
} | ||
|
||
var gcVariableMap = map[string]string{ | ||
variable.TiDBGCRunInterval: "tikv_gc_run_interval", | ||
variable.TiDBGCLifetime: "tikv_gc_life_time", | ||
variable.TiDBGCConcurrency: "tikv_gc_concurrency", | ||
variable.TiDBGCEnable: "tikv_gc_enable", | ||
variable.TiDBGCMode: "tikv_gc_mode", | ||
variable.TiDBGCScanLockMode: "tikv_gc_scan_lock_mode", | ||
} | ||
|
||
// Session context, it is consistent with the lifecycle of a client connection. | ||
type Session interface { | ||
sessionctx.Context | ||
|
@@ -1002,7 +1023,9 @@ func (s *session) GetAllSysVars() (map[string]string, error) { | |
ret := make(map[string]string, len(rows)) | ||
for _, r := range rows { | ||
k, v := r.GetString(0), r.GetString(1) | ||
ret[k] = v | ||
if v, err = s.checkForTiDBTableValue(k, v); err != nil { | ||
ret[k] = v | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If we decides not to introduce the concept of TiKV global sysvar, the function's name seems better to be updated as well. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It passes every variable through the function, but for non-tikv vars, it is effectively a noop. The functions don't need to be exported, so I should fix that. I have also renamed them to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I prefer they are invoked only when the variable is a GC configuration item, which I think has better readability. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. OK, I've made this change. PTAL :-) |
||
} | ||
return ret, nil | ||
} | ||
|
@@ -1029,7 +1052,8 @@ func (s *session) GetGlobalSysVar(name string) (string, error) { | |
} | ||
return "", err | ||
} | ||
return sysVar, nil | ||
// Update mysql.tidb values if required | ||
return s.checkForTiDBTableValue(name, sysVar) | ||
} | ||
|
||
// SetGlobalSysVar implements GlobalVarAccessor.SetGlobalSysVar interface. | ||
|
@@ -1056,13 +1080,118 @@ func (s *session) SetGlobalSysVar(name, value string) error { | |
return err | ||
} | ||
name = strings.ToLower(name) | ||
// update mysql.tidb if required. | ||
if err = s.setTiDBTableValue(name, sVal); err != nil { | ||
return err | ||
} | ||
variable.CheckDeprecationSetSystemVar(s.sessionVars, name) | ||
sql := fmt.Sprintf(`REPLACE %s.%s VALUES ('%s', '%s');`, | ||
mysql.SystemDB, mysql.GlobalVariablesTable, name, sVal) | ||
mysql.SystemDB, mysql.GlobalVariablesTable, name, escapeUserString(sVal)) | ||
_, _, err = s.ExecRestrictedSQL(sql) | ||
return err | ||
} | ||
|
||
// escape user supplied string for internal SQL. Not safe for all cases, since it doesn't | ||
// handle quote-type, sql-mode, character set breakout. | ||
func escapeUserString(str string) string { | ||
return strings.ReplaceAll(str, `'`, `\'`) | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This still doesn't looks safe enough 😕 I think we need better way to do this in the future There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, I agree. I searched and couldn't find anything reliable that could be imported in |
||
|
||
// setTiDBTableValue handles tikv_* sysvars which need to update mysql.tidb | ||
// for backwards compatibility. Validation has already been performed. | ||
func (s *session) setTiDBTableValue(name, val string) error { | ||
switch name { | ||
case variable.TiDBGCConcurrency: | ||
autoConcurrency := "false" | ||
if val == "-1" { | ||
autoConcurrency = "true" | ||
} | ||
sql := fmt.Sprintf(`INSERT INTO mysql.tidb (variable_name, variable_value, comment) VALUES ('%[1]s', '%[2]s', '%[3]s') | ||
ON DUPLICATE KEY UPDATE variable_value = '%[2]s'`, tiKVGCAutoConcurrency, autoConcurrency, gcVariableComments[name]) | ||
MyonKeminta marked this conversation as resolved.
Show resolved
Hide resolved
|
||
_, _, err := s.ExecRestrictedSQL(sql) | ||
if err != nil { | ||
return err | ||
} | ||
fallthrough | ||
case variable.TiDBGCEnable, variable.TiDBGCRunInterval, variable.TiDBGCLifetime, variable.TiDBGCMode, variable.TiDBGCScanLockMode: | ||
val = onOffToTrueFalse(val) | ||
sql := fmt.Sprintf(`INSERT INTO mysql.tidb (variable_name, variable_value, comment) VALUES ('%[1]s', '%[2]s', '%[3]s') | ||
ON DUPLICATE KEY UPDATE variable_value = '%[2]s'`, gcVariableMap[name], escapeUserString(val), gcVariableComments[name]) | ||
_, _, err := s.ExecRestrictedSQL(sql) | ||
return err | ||
} | ||
return nil // not a TiKV sysVar | ||
} | ||
|
||
// In mysql.tidb the convention has been to store the string value "true"/"false", | ||
// but sysvars use the convention ON/OFF. | ||
func trueFalseToOnOff(str string) string { | ||
if strings.EqualFold("true", str) { | ||
return variable.BoolOn | ||
} else if strings.EqualFold("false", str) { | ||
return variable.BoolOff | ||
} | ||
return str | ||
} | ||
|
||
// In mysql.tidb the convention has been to store the string value "true"/"false", | ||
// but sysvars use the convention ON/OFF. | ||
func onOffToTrueFalse(str string) string { | ||
if strings.EqualFold("ON", str) { | ||
return "true" | ||
} else if strings.EqualFold("OFF", str) { | ||
return "false" | ||
} | ||
return str | ||
} | ||
|
||
// checkForTiDBTableValue handles tikv_* sysvars which need | ||
// to read from mysql.tidb for backwards compatibility. | ||
func (s *session) checkForTiDBTableValue(name, val string) (string, error) { | ||
switch name { | ||
case variable.TiDBGCConcurrency: | ||
// Check if autoconcurrency is set | ||
sql := fmt.Sprintf(`SELECT VARIABLE_VALUE FROM mysql.tidb WHERE VARIABLE_NAME='%s';`, tiKVGCAutoConcurrency) | ||
autoConcurrencyVal, err := s.getExecRet(s, sql) | ||
if err == nil && strings.EqualFold(autoConcurrencyVal, "true") { | ||
return "-1", nil // convention for "AUTO" | ||
} | ||
fallthrough | ||
case variable.TiDBGCEnable, variable.TiDBGCRunInterval, variable.TiDBGCLifetime, | ||
variable.TiDBGCMode, variable.TiDBGCScanLockMode: | ||
sql := fmt.Sprintf(`SELECT VARIABLE_VALUE FROM mysql.tidb WHERE VARIABLE_NAME='%s';`, gcVariableMap[name]) | ||
tblValue, err := s.getExecRet(s, sql) | ||
if err != nil { | ||
return val, nil // mysql.tidb value does not exist. | ||
} | ||
// Run validation on the tblValue. This will return an error if it can't be validated, | ||
// but will also make it more consistent: disTribuTeD -> DISTRIBUTED etc | ||
tblValue = trueFalseToOnOff(tblValue) | ||
validatedVal, err := variable.ValidateSetSystemVar(s.sessionVars, name, tblValue, variable.ScopeGlobal) | ||
if err != nil { | ||
logutil.Logger(context.Background()).Warn("restoring sysvar value since validating mysql.tidb value failed", | ||
zap.Error(err), | ||
zap.String("name", name), | ||
zap.String("tblName", gcVariableMap[name]), | ||
zap.String("tblValue", tblValue), | ||
zap.String("restoredValue", val)) | ||
sql := fmt.Sprintf(`REPLACE INTO mysql.tidb (variable_name, variable_value, comment) | ||
VALUES ('%s', '%s', '%s')`, gcVariableMap[name], escapeUserString(val), gcVariableComments[name]) | ||
_, _, err = s.ExecRestrictedSQL(sql) | ||
return val, err | ||
} | ||
if validatedVal != val { | ||
// The sysvar value is out of sync. | ||
sql := fmt.Sprintf(`REPLACE %s.%s VALUES ('%s', '%s');`, | ||
mysql.SystemDB, mysql.GlobalVariablesTable, gcVariableMap[name], escapeUserString(validatedVal)) | ||
_, _, err = s.ExecRestrictedSQL(sql) | ||
return validatedVal, err | ||
} | ||
return validatedVal, nil | ||
} | ||
return val, nil // not a TiKV sysVar | ||
} | ||
|
||
func (s *session) ensureFullGlobalStats() error { | ||
rows, _, err := s.ExecRestrictedSQL(`select count(1) from information_schema.tables t where t.create_options = 'partitioned' | ||
and not exists (select 1 from mysql.stats_meta m where m.table_id = t.tidb_table_id)`) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this behavior changed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was caused by the changes in
util/gcutil/gcutil.go
. It now reads the sysvars to get the state of if GC is running, which check the system tables but use the default if there are no values.