From dd593b2e032b0aa92ad1cdeaf9666f156d37a40f Mon Sep 17 00:00:00 2001 From: djshow832 Date: Tue, 22 Aug 2023 16:30:34 +0800 Subject: [PATCH] session, sessionctx: consider the dependency of sysvars for session states (#46244) close pingcap/tidb#46214 --- session/session.go | 5 +++- .../sessionstates/session_states_test.go | 22 +++++++++++++++ sessionctx/variable/sysvar.go | 8 +++--- sessionctx/variable/variable.go | 20 ++++++++++++++ sessionctx/variable/variable_test.go | 27 +++++++++++++++++++ 5 files changed, 77 insertions(+), 5 deletions(-) diff --git a/session/session.go b/session/session.go index 5d74b720b1..ffb343aeb8 100644 --- a/session/session.go +++ b/session/session.go @@ -4315,7 +4315,10 @@ func (s *session) DecodeSessionStates(ctx context.Context, } // Decode session variables. - for name, val := range sessionStates.SystemVars { + names := variable.OrderByDependency(sessionStates.SystemVars) + // Some variables must be set before others, e.g. tidb_enable_noop_functions should be before noop variables. + for _, name := range names { + val := sessionStates.SystemVars[name] // Experimental system variables may change scope, data types, or even be removed. // We just ignore the errors and continue. if err := s.sessionVars.SetSystemVar(name, val); err != nil { diff --git a/sessionctx/sessionstates/session_states_test.go b/sessionctx/sessionstates/session_states_test.go index 66ad5f7bc7..bcf0da7561 100644 --- a/sessionctx/sessionstates/session_states_test.go +++ b/sessionctx/sessionstates/session_states_test.go @@ -171,6 +171,28 @@ func TestSystemVars(t *testing.T) { checkStmt: "select rand()", expectedValue: "0.11641535266900002", }, + { + // tidb_enforce_mpp depends on tidb_allow_mpp. + stmts: []string{ + "set @@global.tidb_allow_mpp=0", + "set @@tidb_allow_mpp=1", + "set @@tidb_enforce_mpp=1", + }, + inSessionStates: true, + varName: variable.TiDBEnforceMPPExecution, + expectedValue: "1", + }, + { + // tx_read_only depends on tidb_enable_noop_functions. + stmts: []string{ + "set @@global.tidb_enable_noop_functions=0", + "set @@tidb_enable_noop_functions=1", + "set @@tx_read_only=1", + }, + inSessionStates: true, + varName: variable.TxReadOnly, + expectedValue: "1", + }, } if !sem.IsEnabled() { diff --git a/sessionctx/variable/sysvar.go b/sessionctx/variable/sysvar.go index 9a31b9eb15..5292d6badb 100644 --- a/sessionctx/variable/sysvar.go +++ b/sessionctx/variable/sysvar.go @@ -678,7 +678,7 @@ var defaultSysVars = []*SysVar{ (*SetPDClientDynamicOption.Load())(TiDBEnableTSOFollowerProxy, val) return nil }}, - {Scope: ScopeGlobal, Name: TiDBEnableLocalTxn, Value: BoolToOnOff(DefTiDBEnableLocalTxn), Hidden: true, Type: TypeBool, GetGlobal: func(_ context.Context, sv *SessionVars) (string, error) { + {Scope: ScopeGlobal, Name: TiDBEnableLocalTxn, Value: BoolToOnOff(DefTiDBEnableLocalTxn), Hidden: true, Type: TypeBool, Depended: true, GetGlobal: func(_ context.Context, sv *SessionVars) (string, error) { return BoolToOnOff(EnableLocalTxn.Load()), nil }, SetGlobal: func(_ context.Context, s *SessionVars, val string) error { oldVal := EnableLocalTxn.Load() @@ -803,7 +803,7 @@ var defaultSysVars = []*SysVar{ return nil }}, {Scope: ScopeGlobal, Name: TiDBEnableTelemetry, Value: BoolToOnOff(DefTiDBEnableTelemetry), Type: TypeBool}, - {Scope: ScopeGlobal, Name: TiDBEnableHistoricalStats, Value: On, Type: TypeBool}, + {Scope: ScopeGlobal, Name: TiDBEnableHistoricalStats, Value: On, Type: TypeBool, Depended: true}, /* tikv gc metrics */ {Scope: ScopeGlobal, Name: TiDBGCEnable, Value: On, Type: TypeBool, GetGlobal: func(_ context.Context, s *SessionVars) (string, error) { return getTiDBTableValue(s, "tikv_gc_enable", On) @@ -1549,7 +1549,7 @@ var defaultSysVars = []*SysVar{ }}, {Scope: ScopeGlobal | ScopeSession, Name: BlockEncryptionMode, Value: "aes-128-ecb", Type: TypeEnum, PossibleValues: []string{"aes-128-ecb", "aes-192-ecb", "aes-256-ecb", "aes-128-cbc", "aes-192-cbc", "aes-256-cbc", "aes-128-ofb", "aes-192-ofb", "aes-256-ofb", "aes-128-cfb", "aes-192-cfb", "aes-256-cfb"}}, /* TiDB specific variables */ - {Scope: ScopeGlobal | ScopeSession, Name: TiDBAllowMPPExecution, Type: TypeBool, Value: BoolToOnOff(DefTiDBAllowMPPExecution), SetSession: func(s *SessionVars, val string) error { + {Scope: ScopeGlobal | ScopeSession, Name: TiDBAllowMPPExecution, Type: TypeBool, Value: BoolToOnOff(DefTiDBAllowMPPExecution), Depended: true, SetSession: func(s *SessionVars, val string) error { s.allowMPPExecution = TiDBOptOn(val) return nil }}, @@ -1893,7 +1893,7 @@ var defaultSysVars = []*SysVar{ s.TiDBOptJoinReorderThreshold = tidbOptPositiveInt32(val, DefTiDBOptJoinReorderThreshold) return nil }}, - {Scope: ScopeGlobal | ScopeSession, Name: TiDBEnableNoopFuncs, Value: DefTiDBEnableNoopFuncs, Type: TypeEnum, PossibleValues: []string{Off, On, Warn}, Validation: func(vars *SessionVars, normalizedValue string, originalValue string, scope ScopeFlag) (string, error) { + {Scope: ScopeGlobal | ScopeSession, Name: TiDBEnableNoopFuncs, Value: DefTiDBEnableNoopFuncs, Type: TypeEnum, PossibleValues: []string{Off, On, Warn}, Depended: true, Validation: func(vars *SessionVars, normalizedValue string, originalValue string, scope ScopeFlag) (string, error) { // The behavior is very weird if someone can turn TiDBEnableNoopFuncs OFF, but keep any of the following on: // TxReadOnly, TransactionReadOnly, OfflineMode, SuperReadOnly, serverReadOnly, SQLAutoIsNull // To prevent this strange position, prevent setting to OFF when any of these sysVars are ON of the same scope. diff --git a/sessionctx/variable/variable.go b/sessionctx/variable/variable.go index 38457a4f56..ea570963d4 100644 --- a/sessionctx/variable/variable.go +++ b/sessionctx/variable/variable.go @@ -155,6 +155,10 @@ type SysVar struct { // GetStateValue gets the value for session states, which is used for migrating sessions. // We need a function to override GetSession sometimes, because GetSession may not return the real value. GetStateValue func(*SessionVars) (string, bool, error) + // Depended indicates whether other variables depend on this one. That is, if this one is not correctly set, + // another variable cannot be set either. + // This flag is used to decide the order to replay session variables. + Depended bool // skipInit defines if the sysvar should be loaded into the session on init. // This is only important to set for sysvars that include session scope, // since global scoped sysvars are not-applicable. @@ -614,6 +618,22 @@ func GetSysVars() map[string]*SysVar { return m } +// OrderByDependency orders the vars by dependency. The depended sys vars are in the front. +// Unknown sys vars are treated as not depended. +func OrderByDependency(names map[string]string) []string { + depended, notDepended := make([]string, 0, len(names)), make([]string, 0, len(names)) + sysVarsLock.RLock() + defer sysVarsLock.RUnlock() + for name := range names { + if sv, ok := sysVars[name]; ok && sv.Depended { + depended = append(depended, name) + } else { + notDepended = append(notDepended, name) + } + } + return append(depended, notDepended...) +} + func init() { sysVars = make(map[string]*SysVar) for _, v := range defaultSysVars { diff --git a/sessionctx/variable/variable_test.go b/sessionctx/variable/variable_test.go index 0c61d4e85b..89da5597ee 100644 --- a/sessionctx/variable/variable_test.go +++ b/sessionctx/variable/variable_test.go @@ -19,11 +19,13 @@ import ( "encoding/json" "fmt" "runtime" + "slices" "strings" "testing" "time" "github.com/pingcap/tidb/config" + "github.com/pingcap/tidb/kv" "github.com/pingcap/tidb/parser/mysql" "github.com/pingcap/tidb/parser/terror" "github.com/pingcap/tidb/types" @@ -687,3 +689,28 @@ func TestTimeValidationWithTimezone(t *testing.T) { require.NoError(t, err) require.Equal(t, "23:59 +0800", val) } + +func TestOrderByDependency(t *testing.T) { + // Some other exceptions: + // - tidb_snapshot and tidb_read_staleness can not be set at the same time. It doesn't affect dependency. + vars := map[string]string{ + "unknown": "1", + TxReadOnly: "1", + SQLAutoIsNull: "1", + TiDBEnableNoopFuncs: "1", + TiDBEnforceMPPExecution: "1", + TiDBAllowMPPExecution: "1", + TiDBTxnScope: kv.LocalTxnScope, + TiDBEnableLocalTxn: "1", + TiDBEnablePlanReplayerContinuousCapture: "1", + TiDBEnableHistoricalStats: "1", + } + names := OrderByDependency(vars) + require.Greater(t, slices.Index(names, TxReadOnly), slices.Index(names, TiDBEnableNoopFuncs)) + require.Greater(t, slices.Index(names, SQLAutoIsNull), slices.Index(names, TiDBEnableNoopFuncs)) + require.Greater(t, slices.Index(names, TiDBEnforceMPPExecution), slices.Index(names, TiDBAllowMPPExecution)) + // Depended variables below are global variables, so actually it doesn't matter. + require.Greater(t, slices.Index(names, TiDBTxnScope), slices.Index(names, TiDBEnableLocalTxn)) + require.Greater(t, slices.Index(names, TiDBEnablePlanReplayerContinuousCapture), slices.Index(names, TiDBEnableHistoricalStats)) + require.Contains(t, names, "unknown") +}