cherry pick pingcap#31583 to release-5.4

Signed-off-by: ti-srebot <ti-srebot@pingcap.com>

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

@@ -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.

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

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

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.