diff --git a/expression/integration_serial_test.go b/expression/integration_serial_test.go index d43a5342d8b23..2ac3e40830ec7 100644 --- a/expression/integration_serial_test.go +++ b/expression/integration_serial_test.go @@ -3851,8 +3851,8 @@ func TestSetVariables(t *testing.T) { tk.MustExec("set global tidb_enable_noop_functions=1") _, err = tk.Exec("set @@global.max_user_connections='';") - require.NoError(t, err) - //require.Error(t, err, variable.ErrWrongTypeForVar.GenWithStackByArgs("max_user_connections").Error()) + require.Error(t, err) + require.Error(t, err, variable.ErrWrongTypeForVar.GenWithStackByArgs("max_user_connections").Error()) _, err = tk.Exec("set @@global.max_prepared_stmt_count='';") require.Error(t, err) require.Error(t, err, variable.ErrWrongTypeForVar.GenWithStackByArgs("max_prepared_stmt_count").Error()) diff --git a/session/session.go b/session/session.go index 7089008367636..b25a2b6515180 100644 --- a/session/session.go +++ b/session/session.go @@ -1253,7 +1253,13 @@ func (s *session) GetGlobalSysVar(name string) (string, error) { } // It might have been written from an earlier TiDB version, so we should do type validation // See https://github.com/pingcap/tidb/issues/30255 for why we don't do full validation. - return sv.ValidateFromType(s.GetSessionVars(), sysVar, variable.ScopeGlobal) + // If validation fails, we should return the default value: + // See: https://github.com/pingcap/tidb/pull/31566 + sysVar, err = sv.ValidateFromType(s.GetSessionVars(), sysVar, variable.ScopeGlobal) + if err != nil { + return sv.Value, nil + } + return sysVar, nil } // SetGlobalSysVar implements GlobalVarAccessor.SetGlobalSysVar interface. diff --git a/session/session_test.go b/session/session_test.go index d39e5b82f584a..0027c144b8a84 100644 --- a/session/session_test.go +++ b/session/session_test.go @@ -691,15 +691,51 @@ func (s *testSessionSuite) TestGlobalVarAccessor(c *C) { _, err = tk.Exec("set global time_zone = 'timezone'") c.Assert(err, NotNil) c.Assert(terror.ErrorEqual(err, variable.ErrUnknownTimeZone), IsTrue) +} + +func (s *testSessionSuite) TestUpgradeSysvars(c *C) { + tk := testkit.NewTestKitWithInit(c, s.store) + se := tk.Se.(variable.GlobalVarAccessor) // Set the global var to a non canonical form of the value // i.e. implying that it was set from an earlier version of TiDB. tk.MustExec(`REPLACE INTO mysql.global_variables (variable_name, variable_value) VALUES ('tidb_enable_noop_functions', '0')`) domain.GetDomain(tk.Se).NotifyUpdateSysVarCache() // update cache - v, err = se.GetGlobalSysVar("tidb_enable_noop_functions") + v, err := se.GetGlobalSysVar("tidb_enable_noop_functions") c.Assert(err, IsNil) c.Assert(v, Equals, "OFF") + + // Set the global var to "" which is the invalid version of this from TiDB 4.0.16 + // the err is quashed by the GetGlobalSysVar, and the default value is restored. + // This helps callers of GetGlobalSysVar(), which can't individually be expected + // to handle upgrade/downgrade issues correctly. + + tk.MustExec(`REPLACE INTO mysql.global_variables (variable_name, variable_value) VALUES ('rpl_semi_sync_slave_enabled', '')`) + domain.GetDomain(tk.Se).NotifyUpdateSysVarCache() // update cache + v, err = se.GetGlobalSysVar("rpl_semi_sync_slave_enabled") + c.Assert(err, IsNil) + c.Assert(v, Equals, "OFF") // the default value is restored. + result := tk.MustQuery("SHOW VARIABLES LIKE 'rpl_semi_sync_slave_enabled'") + result.Check(testkit.Rows("rpl_semi_sync_slave_enabled OFF")) + + // Ensure variable out of range is converted to in range after upgrade. + // This further helps for https://github.com/pingcap/tidb/pull/28842 + + tk.MustExec(`REPLACE INTO mysql.global_variables (variable_name, variable_value) VALUES ('tidb_executor_concurrency', '999')`) + domain.GetDomain(tk.Se).NotifyUpdateSysVarCache() // update cache + v, err = se.GetGlobalSysVar("tidb_executor_concurrency") + c.Assert(err, IsNil) + c.Assert(v, Equals, "256") // the max value is restored. + + // Handle the case of a completely bogus value from an earlier version of TiDB. + // This could be the case if an ENUM sysvar removes a value. + + tk.MustExec(`REPLACE INTO mysql.global_variables (variable_name, variable_value) VALUES ('tidb_enable_noop_functions', 'SOMEVAL')`) + domain.GetDomain(tk.Se).NotifyUpdateSysVarCache() // update cache + v, err = se.GetGlobalSysVar("tidb_enable_noop_functions") + c.Assert(err, IsNil) + c.Assert(v, Equals, "OFF") // the default value is restored. } func (s *testSessionSuite) TestMatchIdentity(c *C) { diff --git a/sessionctx/variable/sysvar_test.go b/sessionctx/variable/sysvar_test.go index 6356492e66c2c..c53e9fe183e27 100644 --- a/sessionctx/variable/sysvar_test.go +++ b/sessionctx/variable/sysvar_test.go @@ -872,6 +872,7 @@ func TestIndexMergeSwitcher(t *testing.T) { require.Equal(t, BoolToOnOff(DefTiDBEnableIndexMerge), val) } +<<<<<<< HEAD func TestNoValidateForNoop(t *testing.T) { vars := NewSessionVars() @@ -883,4 +884,20 @@ func TestNoValidateForNoop(t *testing.T) { // for other variables, error _, err = GetSysVar(TiDBAllowBatchCop).ValidateFromType(vars, "", ScopeGlobal) require.Error(t, err) +======= +func TestNetBufferLength(t *testing.T) { + netBufferLength := GetSysVar(NetBufferLength) + vars := NewSessionVars() + vars.GlobalVarsAccessor = NewMockGlobalAccessor4Tests() + + val, err := netBufferLength.Validate(vars, "1", ScopeGlobal) + require.NoError(t, err) + require.Equal(t, "1024", val) // converts it to min value + val, err = netBufferLength.Validate(vars, "10485760", ScopeGlobal) + require.NoError(t, err) + require.Equal(t, "1048576", val) // converts it to max value + val, err = netBufferLength.Validate(vars, "524288", ScopeGlobal) + require.NoError(t, err) + require.Equal(t, "524288", val) // unchanged +>>>>>>> 22f4c33d4... *: better handle sysvar upgrades from older versions (#31583) } diff --git a/sessionctx/variable/variable.go b/sessionctx/variable/variable.go index c8fdfeadf9ed6..aeb65a8257d9f 100644 --- a/sessionctx/variable/variable.go +++ b/sessionctx/variable/variable.go @@ -261,10 +261,6 @@ func (sv *SysVar) Validate(vars *SessionVars, value string, scope ScopeFlag) (st // ValidateFromType provides automatic validation based on the SysVar's type func (sv *SysVar) ValidateFromType(vars *SessionVars, value string, scope ScopeFlag) (string, error) { - // TODO: this is a temporary solution for issue: https://github.com/pingcap/tidb/issues/31538, an elegant solution is needed. - if value == "" && sv.IsNoop { - return value, nil - } // Some sysvars in TiDB have a special behavior where the empty string means // "use the config file value". This needs to be cleaned up once the behavior // for instance variables is determined.