*: better handle sysvar upgrades from older versions

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())

session/session.go

@@ -1239,7 +1239,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.

session/session_test.go

@@ -691,15 +691,53 @@ 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) {

sessionctx/variable/sysvar_test.go

@@ -834,16 +834,3 @@ func TestIndexMergeSwitcher(t *testing.T) {
  require.Equal(t, DefTiDBEnableIndexMerge, true)
  require.Equal(t, BoolToOnOff(DefTiDBEnableIndexMerge), val)
 }
-
-func TestNoValidateForNoop(t *testing.T) {
- vars := NewSessionVars()
-
- // for noop variables, no error
- val, err := GetSysVar("rpl_semi_sync_slave_enabled").ValidateFromType(vars, "", ScopeGlobal)
- require.NoError(t, err)
- require.Equal(t, val, "")
-
- // for other variables, error
- _, err = GetSysVar(TiDBAllowBatchCop).ValidateFromType(vars, "", ScopeGlobal)
- require.Error(t, err)
-}

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.