From a542c5868b8674147e2c7a4556d16219126adb88 Mon Sep 17 00:00:00 2001
From: tiancaiamao <tiancaiamao@gmail.com>
Date: Mon, 19 Jul 2021 18:01:34 +0800
Subject: [PATCH] sessionctx/variable: avoid SysVar clone every time when
 visiting system variable (#26308)

---
 sessionctx/variable/sysvar.go      | 28 ++++++++++++----------------
 sessionctx/variable/sysvar_test.go | 27 ---------------------------
 2 files changed, 12 insertions(+), 43 deletions(-)

diff --git a/sessionctx/variable/sysvar.go b/sessionctx/variable/sysvar.go
index 39eab561719db..14ad9952c6a07 100644
--- a/sessionctx/variable/sysvar.go
+++ b/sessionctx/variable/sysvar.go
@@ -80,6 +80,7 @@ const (
 )
 
 // SysVar is for system variable.
+// All the fields of SysVar should be READ ONLY after created.
 type SysVar struct {
 	// Scope is for whether can be changed or not
 	Scope ScopeFlag
@@ -556,30 +557,24 @@ func UnregisterSysVar(name string) {
 	sysVarsLock.Unlock()
 }
 
-// Clone deep copies the sysvar struct to avoid a race
-func (sv *SysVar) Clone() *SysVar {
-	dst := *sv
-	return &dst
-}
-
 // GetSysVar returns sys var info for name as key.
 func GetSysVar(name string) *SysVar {
 	name = strings.ToLower(name)
 	sysVarsLock.RLock()
 	defer sysVarsLock.RUnlock()
-	if sysVars[name] == nil {
-		return nil
-	}
-	return sysVars[name].Clone()
+
+	return sysVars[name]
 }
 
-// SetSysVar sets a sysvar. This will not propagate to the cluster, so it should only be
+// SetSysVar sets a sysvar. In fact, SysVar is immutable.
+// SetSysVar is implemented by register a new SysVar with the same name again.
+// This will not propagate to the cluster, so it should only be
 // used for instance scoped AUTO variables such as system_time_zone.
 func SetSysVar(name string, value string) {
-	name = strings.ToLower(name)
-	sysVarsLock.Lock()
-	defer sysVarsLock.Unlock()
-	sysVars[name].Value = value
+	old := GetSysVar(name)
+	tmp := *old
+	tmp.Value = value
+	RegisterSysVar(&tmp)
 }
 
 // GetSysVars deep copies the sysVars list under a RWLock
@@ -588,7 +583,8 @@ func GetSysVars() map[string]*SysVar {
 	defer sysVarsLock.RUnlock()
 	copy := make(map[string]*SysVar, len(sysVars))
 	for name, sv := range sysVars {
-		copy[name] = sv.Clone()
+		tmp := *sv
+		copy[name] = &tmp
 	}
 	return copy
 }
diff --git a/sessionctx/variable/sysvar_test.go b/sessionctx/variable/sysvar_test.go
index 7c523e2eebef1..a87d12176d690 100644
--- a/sessionctx/variable/sysvar_test.go
+++ b/sessionctx/variable/sysvar_test.go
@@ -571,33 +571,6 @@ func (*testSysVarSuite) TestInstanceScopedVars(c *C) {
 	c.Assert(val, Equals, vars.TxnScope.GetVarValue())
 }
 
-// Calling GetSysVars/GetSysVar needs to return a deep copy, otherwise there will be data races.
-// This is a bit unfortunate, since the only time the race occurs is in the testsuite (Enabling/Disabling SEM) and
-// during startup (setting the .Value of ScopeNone variables). In future it might also be able
-// to fix this by delaying the LoadSysVarCacheLoop start time until after the server is fully initialized.
-func (*testSysVarSuite) TestDeepCopyGetSysVars(c *C) {
-	// Check GetSysVar
-	sv := SysVar{Scope: ScopeGlobal | ScopeSession, Name: "datarace", Value: On, Type: TypeBool}
-	RegisterSysVar(&sv)
-	svcopy := GetSysVar("datarace")
-	svcopy.Name = "datarace2"
-	c.Assert(sv.Name, Equals, "datarace")
-	c.Assert(GetSysVar("datarace").Name, Equals, "datarace")
-	UnregisterSysVar("datarace")
-
-	// Check GetSysVars
-	sv = SysVar{Scope: ScopeGlobal | ScopeSession, Name: "datarace", Value: On, Type: TypeBool}
-	RegisterSysVar(&sv)
-	for name, svcopy := range GetSysVars() {
-		if name == "datarace" {
-			svcopy.Name = "datarace2"
-		}
-	}
-	c.Assert(sv.Name, Equals, "datarace")
-	c.Assert(GetSysVar("datarace").Name, Equals, "datarace")
-	UnregisterSysVar("datarace")
-}
-
 // Test that sysvars defaults are logically valid. i.e.
 // the default itself must validate without error provided the scope and read-only is correct.
 // The default values should also be normalized for consistency.