From 91cb86e3cf60d53d3b9e0a6b0aa7bde66012371b Mon Sep 17 00:00:00 2001 From: Morgan Tocker Date: Thu, 28 Apr 2022 06:26:52 -0600 Subject: [PATCH] domain, sessionctx/variable: make code DRY (#33897) close pingcap/tidb#33896 --- domain/sysvar_cache.go | 108 ++++++----------------------- executor/set_test.go | 24 +++++++ sessionctx/variable/sysvar.go | 16 +++-- sessionctx/variable/sysvar_test.go | 37 ---------- sessionctx/variable/variable.go | 16 +++++ 5 files changed, 69 insertions(+), 132 deletions(-) diff --git a/domain/sysvar_cache.go b/domain/sysvar_cache.go index a0389b7e4bff4..5f3862567056e 100644 --- a/domain/sysvar_cache.go +++ b/domain/sysvar_cache.go @@ -25,9 +25,6 @@ import ( "github.com/pingcap/tidb/sessionctx/variable" "github.com/pingcap/tidb/util/logutil" "github.com/pingcap/tidb/util/sqlexec" - "github.com/pingcap/tidb/util/stmtsummary" - topsqlstate "github.com/pingcap/tidb/util/topsql/state" - storekv "github.com/tikv/client-go/v2/kv" pd "github.com/tikv/pd/client" "go.uber.org/zap" ) @@ -142,9 +139,24 @@ func (do *Domain) rebuildSysVarCache(ctx sessionctx.Context) error { } if sv.HasGlobalScope() { newGlobalCache[sv.Name] = sVal + + // Call the SetGlobal func for this sysvar if it exists. + // SET GLOBAL only calls the SetGlobal func on the calling instances. + // This ensures it is run on all tidb servers. + // This does not apply to INSTANCE scoped vars (HasGlobalScope() is false) + if sv.SetGlobal != nil && !sv.SkipSysvarCache() { + sVal = sv.ValidateWithRelaxedValidation(ctx.GetSessionVars(), sVal, variable.ScopeGlobal) + err = sv.SetGlobal(ctx.GetSessionVars(), sVal) + if err != nil { + logutil.BgLogger().Error(fmt.Sprintf("load global variable %s error", sv.Name), zap.Error(err)) + } + } } - // Propagate any changes to the server scoped variables - do.checkEnableServerGlobalVar(sv.Name, sVal) + + // Some PD options need to be checked outside of the SetGlobal func. + // This is also done for the SET GLOBAL caller in executor/set.go, + // but here we check for other tidb instances. + do.checkPDClientDynamicOption(sv.Name, sVal) } logutil.BgLogger().Debug("rebuilding sysvar cache") @@ -156,19 +168,10 @@ func (do *Domain) rebuildSysVarCache(ctx sessionctx.Context) error { return nil } -// checkEnableServerGlobalVar processes variables that acts in server and global level. -// This is required because the SetGlobal function on the sysvar struct only executes on -// the initiating tidb-server. There is no current method to say "run this function on all -// tidb servers when the value of this variable changes". If you do not require changes to -// be applied on all servers, use a getter/setter instead! You don't need to add to this list. -func (do *Domain) checkEnableServerGlobalVar(name, sVal string) { - var err error +func (do *Domain) checkPDClientDynamicOption(name, sVal string) { switch name { - case variable.TiDBMemQuotaBindingCache: - variable.MemQuotaBindingCache.Store(variable.TidbOptInt64(sVal, variable.DefTiDBMemQuotaBindingCache)) case variable.TiDBTSOClientBatchMaxWaitTime: - var val float64 - val, err = strconv.ParseFloat(sVal, 64) + val, err := strconv.ParseFloat(sVal, 64) if err != nil { break } @@ -179,82 +182,11 @@ func (do *Domain) checkEnableServerGlobalVar(name, sVal string) { variable.MaxTSOBatchWaitInterval.Store(val) case variable.TiDBEnableTSOFollowerProxy: val := variable.TiDBOptOn(sVal) - err = do.SetPDClientDynamicOption(pd.EnableTSOFollowerProxy, val) + err := do.SetPDClientDynamicOption(pd.EnableTSOFollowerProxy, val) if err != nil { break } variable.EnableTSOFollowerProxy.Store(val) - case variable.TiDBEnableLocalTxn: - variable.EnableLocalTxn.Store(variable.TiDBOptOn(sVal)) - case variable.TiDBEnableStmtSummary: - err = stmtsummary.StmtSummaryByDigestMap.SetEnabled(variable.TiDBOptOn(sVal)) - case variable.TiDBStmtSummaryInternalQuery: - err = stmtsummary.StmtSummaryByDigestMap.SetEnabledInternalQuery(variable.TiDBOptOn(sVal)) - case variable.TiDBStmtSummaryRefreshInterval: - err = stmtsummary.StmtSummaryByDigestMap.SetRefreshInterval(variable.TidbOptInt64(sVal, variable.DefTiDBStmtSummaryRefreshInterval)) - case variable.TiDBStmtSummaryHistorySize: - err = stmtsummary.StmtSummaryByDigestMap.SetHistorySize(variable.TidbOptInt(sVal, variable.DefTiDBStmtSummaryHistorySize)) - case variable.TiDBStmtSummaryMaxStmtCount: - err = stmtsummary.StmtSummaryByDigestMap.SetMaxStmtCount(uint(variable.TidbOptInt(sVal, variable.DefTiDBStmtSummaryMaxStmtCount))) - case variable.TiDBStmtSummaryMaxSQLLength: - err = stmtsummary.StmtSummaryByDigestMap.SetMaxSQLLength(variable.TidbOptInt(sVal, variable.DefTiDBStmtSummaryMaxSQLLength)) - case variable.TiDBTopSQLMaxTimeSeriesCount: - var val int64 - val, err = strconv.ParseInt(sVal, 10, 64) - if err != nil { - break - } - topsqlstate.GlobalState.MaxStatementCount.Store(val) - case variable.TiDBTopSQLMaxMetaCount: - var val int64 - val, err = strconv.ParseInt(sVal, 10, 64) - if err != nil { - break - } - topsqlstate.GlobalState.MaxCollect.Store(val) - case variable.TiDBRestrictedReadOnly: - variable.RestrictedReadOnly.Store(variable.TiDBOptOn(sVal)) - case variable.TiDBSuperReadOnly: - variable.VarTiDBSuperReadOnly.Store(variable.TiDBOptOn(sVal)) - case variable.TiDBStoreLimit: - var val int64 - val, err = strconv.ParseInt(sVal, 10, 64) - if err != nil { - break - } - storekv.StoreLimit.Store(val) - case variable.TiDBTableCacheLease: - var val int64 - val, err = strconv.ParseInt(sVal, 10, 64) - if err != nil { - break - } - variable.TableCacheLease.Store(val) - case variable.TiDBGCMaxWaitTime: - var val int64 - val, err = strconv.ParseInt(sVal, 10, 64) - if err != nil { - break - } - variable.GCMaxWaitTime.Store(val) - case variable.TiDBPersistAnalyzeOptions: - variable.PersistAnalyzeOptions.Store(variable.TiDBOptOn(sVal)) - case variable.TiDBEnableColumnTracking: - variable.EnableColumnTracking.Store(variable.TiDBOptOn(sVal)) - case variable.TiDBStatsLoadSyncWait: - var val int64 - val, err = strconv.ParseInt(sVal, 10, 64) - if err != nil { - break - } - variable.StatsLoadSyncWait.Store(val) - case variable.TiDBStatsLoadPseudoTimeout: - variable.StatsLoadPseudoTimeout.Store(variable.TiDBOptOn(sVal)) - case variable.TiDBTxnCommitBatchSize: - storekv.TxnCommitBatchSize.Store(uint64(variable.TidbOptInt64(sVal, int64(storekv.DefTxnCommitBatchSize)))) - } - if err != nil { - logutil.BgLogger().Error(fmt.Sprintf("load global variable %s error", name), zap.Error(err)) } } diff --git a/executor/set_test.go b/executor/set_test.go index 3afc09d661a43..f7472ebad4894 100644 --- a/executor/set_test.go +++ b/executor/set_test.go @@ -1460,6 +1460,30 @@ func TestDefaultBehavior(t *testing.T) { tk.MustExec("SET GLOBAL sql_mode = DEFAULT") } +func TestTiDBReadOnly(t *testing.T) { + store, clean := testkit.CreateMockStore(t) + defer clean() + tk := testkit.NewTestKit(t, store) + + // turn on tidb_restricted_read_only should turn on tidb_super_read_only + tk.MustExec("SET GLOBAL tidb_restricted_read_only = ON") + tk.MustQuery("SELECT @@GLOBAL.tidb_super_read_only").Check(testkit.Rows("1")) + + // can't turn off tidb_super_read_only if tidb_restricted_read_only is on + err := tk.ExecToErr("SET GLOBAL tidb_super_read_only = OFF") + require.Error(t, err) + require.Equal(t, "can't turn off tidb_super_read_only when tidb_restricted_read_only is on", err.Error()) + + // turn off tidb_restricted_read_only won't affect tidb_super_read_only + tk.MustExec("SET GLOBAL tidb_restricted_read_only = OFF") + tk.MustQuery("SELECT @@GLOBAL.tidb_restricted_read_only").Check(testkit.Rows("0")) + tk.MustQuery("SELECT @@GLOBAL.tidb_super_read_only").Check(testkit.Rows("1")) + + // it is ok to turn off tidb_super_read_only now + tk.MustExec("SET GLOBAL tidb_super_read_only = OFF") + tk.MustQuery("SELECT @@GLOBAL.tidb_super_read_only").Check(testkit.Rows("0")) +} + func TestRemovedSysVars(t *testing.T) { store, clean := testkit.CreateMockStore(t) defer clean() diff --git a/sessionctx/variable/sysvar.go b/sessionctx/variable/sysvar.go index bdf4b44486b28..68dedb58c9e40 100644 --- a/sessionctx/variable/sysvar.go +++ b/sessionctx/variable/sysvar.go @@ -539,7 +539,8 @@ var defaultSysVars = []*SysVar{ }}, {Scope: ScopeGlobal, Name: TiDBRestrictedReadOnly, Value: BoolToOnOff(DefTiDBRestrictedReadOnly), Type: TypeBool, SetGlobal: func(s *SessionVars, val string) error { on := TiDBOptOn(val) - if on { + // For user initiated SET GLOBAL, also change the value of TiDBSuperReadOnly + if on && s.StmtCtx.StmtType == "Set" { err := s.GlobalVarsAccessor.SetGlobalSysVar(TiDBSuperReadOnly, "ON") if err != nil { return err @@ -548,10 +549,10 @@ var defaultSysVars = []*SysVar{ RestrictedReadOnly.Store(on) return nil }}, - {Scope: ScopeGlobal, Name: TiDBSuperReadOnly, Value: BoolToOnOff(DefTiDBSuperReadOnly), Type: TypeBool, Validation: func(vars *SessionVars, normalizedValue string, _ string, _ ScopeFlag) (string, error) { + {Scope: ScopeGlobal, Name: TiDBSuperReadOnly, Value: BoolToOnOff(DefTiDBSuperReadOnly), Type: TypeBool, Validation: func(s *SessionVars, normalizedValue string, _ string, _ ScopeFlag) (string, error) { on := TiDBOptOn(normalizedValue) - if !on { - result, err := vars.GlobalVarsAccessor.GetGlobalSysVar(TiDBRestrictedReadOnly) + if !on && s.StmtCtx.StmtType == "Set" { + result, err := s.GlobalVarsAccessor.GetGlobalSysVar(TiDBRestrictedReadOnly) if err != nil { return normalizedValue, err } @@ -610,8 +611,7 @@ var defaultSysVars = []*SysVar{ Validation: func(vars *SessionVars, normalizedValue string, originalValue string, scope ScopeFlag) (string, error) { return checkGCTxnMaxWaitTime(vars, normalizedValue, originalValue, scope) }, SetGlobal: func(s *SessionVars, val string) error { - ival, _ := strconv.Atoi(val) - GCMaxWaitTime.Store((int64)(ival)) + GCMaxWaitTime.Store(TidbOptInt64(val, DefTiDBGCMaxWaitTime)) return nil }}, {Scope: ScopeGlobal, Name: TiDBTableCacheLease, Value: strconv.Itoa(DefTiDBTableCacheLease), Type: TypeUnsigned, MinValue: 1, MaxValue: 10, SetGlobal: func(s *SessionVars, sVal string) error { @@ -662,7 +662,9 @@ var defaultSysVars = []*SysVar{ return BoolToOnOff(EnableColumnTracking.Load()), nil }, SetGlobal: func(s *SessionVars, val string) error { v := TiDBOptOn(val) - if !v { + // If this is a user initiated statement, + // we log that column tracking is disabled. + if s.StmtCtx.StmtType == "Set" && !v { // Set the location to UTC to avoid time zone interference. disableTime := time.Now().UTC().Format(types.UTCTimeFormat) if err := setTiDBTableValue(s, TiDBDisableColumnTrackingTime, disableTime, "Record the last time tidb_enable_column_tracking is set off"); err != nil { diff --git a/sessionctx/variable/sysvar_test.go b/sessionctx/variable/sysvar_test.go index 17046ad5df9d4..32b94ea0260b0 100644 --- a/sessionctx/variable/sysvar_test.go +++ b/sessionctx/variable/sysvar_test.go @@ -536,43 +536,6 @@ func TestIsNoop(t *testing.T) { require.True(t, sv.IsNoop) } -func TestTiDBReadOnly(t *testing.T) { - rro := GetSysVar(TiDBRestrictedReadOnly) - sro := GetSysVar(TiDBSuperReadOnly) - - vars := NewSessionVars() - mock := NewMockGlobalAccessor4Tests() - mock.SessionVars = vars - vars.GlobalVarsAccessor = mock - - // turn on tidb_restricted_read_only should turn on tidb_super_read_only - require.NoError(t, mock.SetGlobalSysVar(rro.Name, "ON")) - result, err := mock.GetGlobalSysVar(sro.Name) - require.NoError(t, err) - require.Equal(t, "ON", result) - - // can't turn off tidb_super_read_only if tidb_restricted_read_only is on - err = mock.SetGlobalSysVar(sro.Name, "OFF") - require.Error(t, err) - require.Equal(t, "can't turn off tidb_super_read_only when tidb_restricted_read_only is on", err.Error()) - - // turn off tidb_restricted_read_only won't affect tidb_super_read_only - require.NoError(t, mock.SetGlobalSysVar(rro.Name, "OFF")) - result, err = mock.GetGlobalSysVar(rro.Name) - require.NoError(t, err) - require.Equal(t, "OFF", result) - - result, err = mock.GetGlobalSysVar(sro.Name) - require.NoError(t, err) - require.Equal(t, "ON", result) - - // it is ok to turn off tidb_super_read_only now - require.NoError(t, mock.SetGlobalSysVar(sro.Name, "OFF")) - result, err = mock.GetGlobalSysVar(sro.Name) - require.NoError(t, err) - require.Equal(t, "OFF", result) -} - func TestInstanceScopedVars(t *testing.T) { // This tests instance scoped variables through GetSessionOrGlobalSystemVar(). // Eventually these should be changed to use getters so that the switch diff --git a/sessionctx/variable/variable.go b/sessionctx/variable/variable.go index b5269a99f1cf7..f4e7c386406cc 100644 --- a/sessionctx/variable/variable.go +++ b/sessionctx/variable/variable.go @@ -521,6 +521,22 @@ func (sv *SysVar) SkipInit() bool { return !sv.HasSessionScope() } +// SkipSysvarCache returns true if the sysvar should not re-execute on peers +// This doesn't make sense for the GC variables because they are based in tikv +// tables. We'd effectively be reading and writing to the same table, which +// could be in an unsafe manner. In future these variables might be converted +// to not use a different table internally, but to do that we need to first +// fix upgrade/downgrade so we know that older servers won't be in the cluster +// which update only these values. +func (sv *SysVar) SkipSysvarCache() bool { + switch sv.Name { + case TiDBGCEnable, TiDBGCRunInterval, TiDBGCLifetime, + TiDBGCConcurrency, TiDBGCScanLockMode, TiDBGCMaxWaitTime: + return true + } + return false +} + var sysVars map[string]*SysVar var sysVarsLock sync.RWMutex