From 1e0629ae6abbed515e53d524fd8d866cdb787ea0 Mon Sep 17 00:00:00 2001 From: Morgan Tocker Date: Tue, 18 May 2021 10:33:13 -0600 Subject: [PATCH 1/4] fix sysvar datarace with deep copy --- sessionctx/variable/sysvar.go | 33 ++++++++++++++++++++++++++++-- sessionctx/variable/sysvar_test.go | 27 ++++++++++++++++++++++++ 2 files changed, 58 insertions(+), 2 deletions(-) diff --git a/sessionctx/variable/sysvar.go b/sessionctx/variable/sysvar.go index 99c3da8233d68..4dd17ac4b8ccd 100644 --- a/sessionctx/variable/sysvar.go +++ b/sessionctx/variable/sysvar.go @@ -34,6 +34,7 @@ import ( "github.com/pingcap/tidb/util/collate" "github.com/pingcap/tidb/util/logutil" "github.com/pingcap/tidb/util/versioninfo" + atomic2 "go.uber.org/atomic" ) @@ -492,12 +493,40 @@ func UnregisterSysVar(name string) { sysVarsLock.Unlock() } +// copySv deep copies the sysvar struct to avoid a race +func copySv(src *SysVar) (dest *SysVar) { + return &SysVar{ + Scope: src.Scope, + Name: src.Name, + Value: src.Value, + Type: src.Type, + MinValue: src.MinValue, + MaxValue: src.MaxValue, + AutoConvertNegativeBool: src.AutoConvertNegativeBool, + AutoConvertOutOfRange: src.AutoConvertOutOfRange, + ReadOnly: src.ReadOnly, + PossibleValues: src.PossibleValues, + AllowEmpty: src.AllowEmpty, + AllowEmptyAll: src.AllowEmptyAll, + AllowAutoValue: src.AllowAutoValue, + Validation: src.Validation, + SetSession: src.SetSession, + SetGlobal: src.SetGlobal, + IsHintUpdatable: src.IsHintUpdatable, + Hidden: src.Hidden, + Aliases: src.Aliases, + } +} + // GetSysVar returns sys var info for name as key. func GetSysVar(name string) *SysVar { name = strings.ToLower(name) sysVarsLock.RLock() defer sysVarsLock.RUnlock() - return sysVars[name] + if sysVars[name] == nil { + return nil + } + return copySv(sysVars[name]) } // SetSysVar sets a sysvar. This will not propagate to the cluster, so it should only be @@ -515,7 +544,7 @@ func GetSysVars() map[string]*SysVar { defer sysVarsLock.RUnlock() copy := make(map[string]*SysVar, len(sysVars)) for name, sv := range sysVars { - copy[name] = sv + copy[name] = copySv(sv) } return copy } diff --git a/sessionctx/variable/sysvar_test.go b/sessionctx/variable/sysvar_test.go index 71979a57b7eef..d236dc142f1dd 100644 --- a/sessionctx/variable/sysvar_test.go +++ b/sessionctx/variable/sysvar_test.go @@ -555,3 +555,30 @@ func (*testSysVarSuite) TestInstanceScopedVars(c *C) { c.Assert(err, IsNil) 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") +} From 5b085d0f9dfd7cf8a32ca82c88d0a6e0327dbae0 Mon Sep 17 00:00:00 2001 From: Morgan Tocker Date: Tue, 18 May 2021 10:33:13 -0600 Subject: [PATCH 2/4] fix sysvar datarace with deep copy --- sessionctx/variable/sysvar.go | 32 ++++++++++++++++++++++++++++-- sessionctx/variable/sysvar_test.go | 27 +++++++++++++++++++++++++ 2 files changed, 57 insertions(+), 2 deletions(-) diff --git a/sessionctx/variable/sysvar.go b/sessionctx/variable/sysvar.go index 99c3da8233d68..1fa4674b80115 100644 --- a/sessionctx/variable/sysvar.go +++ b/sessionctx/variable/sysvar.go @@ -492,12 +492,40 @@ func UnregisterSysVar(name string) { sysVarsLock.Unlock() } +// copySv deep copies the sysvar struct to avoid a race +func copySv(src *SysVar) (dest *SysVar) { + return &SysVar{ + Scope: src.Scope, + Name: src.Name, + Value: src.Value, + Type: src.Type, + MinValue: src.MinValue, + MaxValue: src.MaxValue, + AutoConvertNegativeBool: src.AutoConvertNegativeBool, + AutoConvertOutOfRange: src.AutoConvertOutOfRange, + ReadOnly: src.ReadOnly, + PossibleValues: src.PossibleValues, + AllowEmpty: src.AllowEmpty, + AllowEmptyAll: src.AllowEmptyAll, + AllowAutoValue: src.AllowAutoValue, + Validation: src.Validation, + SetSession: src.SetSession, + SetGlobal: src.SetGlobal, + IsHintUpdatable: src.IsHintUpdatable, + Hidden: src.Hidden, + Aliases: src.Aliases, + } +} + // GetSysVar returns sys var info for name as key. func GetSysVar(name string) *SysVar { name = strings.ToLower(name) sysVarsLock.RLock() defer sysVarsLock.RUnlock() - return sysVars[name] + if sysVars[name] == nil { + return nil + } + return copySv(sysVars[name]) } // SetSysVar sets a sysvar. This will not propagate to the cluster, so it should only be @@ -515,7 +543,7 @@ func GetSysVars() map[string]*SysVar { defer sysVarsLock.RUnlock() copy := make(map[string]*SysVar, len(sysVars)) for name, sv := range sysVars { - copy[name] = sv + copy[name] = copySv(sv) } return copy } diff --git a/sessionctx/variable/sysvar_test.go b/sessionctx/variable/sysvar_test.go index 71979a57b7eef..d236dc142f1dd 100644 --- a/sessionctx/variable/sysvar_test.go +++ b/sessionctx/variable/sysvar_test.go @@ -555,3 +555,30 @@ func (*testSysVarSuite) TestInstanceScopedVars(c *C) { c.Assert(err, IsNil) 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") +} From e90fe9d18ad5c1d59254729a667d68b711ac9a42 Mon Sep 17 00:00:00 2001 From: Morgan Tocker Date: Tue, 18 May 2021 20:52:28 -0600 Subject: [PATCH 3/4] remove empty line --- sessionctx/variable/sysvar.go | 1 - 1 file changed, 1 deletion(-) diff --git a/sessionctx/variable/sysvar.go b/sessionctx/variable/sysvar.go index 462825abaeab0..e0ef3743c90f5 100644 --- a/sessionctx/variable/sysvar.go +++ b/sessionctx/variable/sysvar.go @@ -34,7 +34,6 @@ import ( "github.com/pingcap/tidb/util/collate" "github.com/pingcap/tidb/util/logutil" "github.com/pingcap/tidb/util/versioninfo" - atomic2 "go.uber.org/atomic" ) From 3ed42bdb134b570cb90fac7dd89f1debcf87bbff Mon Sep 17 00:00:00 2001 From: Morgan Tocker Date: Wed, 19 May 2021 08:44:14 -0600 Subject: [PATCH 4/4] address PR feedback --- sessionctx/variable/sysvar.go | 31 ++++++------------------------- 1 file changed, 6 insertions(+), 25 deletions(-) diff --git a/sessionctx/variable/sysvar.go b/sessionctx/variable/sysvar.go index e0ef3743c90f5..18ebe75eb80dd 100644 --- a/sessionctx/variable/sysvar.go +++ b/sessionctx/variable/sysvar.go @@ -492,29 +492,10 @@ func UnregisterSysVar(name string) { sysVarsLock.Unlock() } -// copySv deep copies the sysvar struct to avoid a race -func copySv(src *SysVar) (dest *SysVar) { - return &SysVar{ - Scope: src.Scope, - Name: src.Name, - Value: src.Value, - Type: src.Type, - MinValue: src.MinValue, - MaxValue: src.MaxValue, - AutoConvertNegativeBool: src.AutoConvertNegativeBool, - AutoConvertOutOfRange: src.AutoConvertOutOfRange, - ReadOnly: src.ReadOnly, - PossibleValues: src.PossibleValues, - AllowEmpty: src.AllowEmpty, - AllowEmptyAll: src.AllowEmptyAll, - AllowAutoValue: src.AllowAutoValue, - Validation: src.Validation, - SetSession: src.SetSession, - SetGlobal: src.SetGlobal, - IsHintUpdatable: src.IsHintUpdatable, - Hidden: src.Hidden, - Aliases: src.Aliases, - } +// 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. @@ -525,7 +506,7 @@ func GetSysVar(name string) *SysVar { if sysVars[name] == nil { return nil } - return copySv(sysVars[name]) + return sysVars[name].Clone() } // SetSysVar sets a sysvar. This will not propagate to the cluster, so it should only be @@ -543,7 +524,7 @@ func GetSysVars() map[string]*SysVar { defer sysVarsLock.RUnlock() copy := make(map[string]*SysVar, len(sysVars)) for name, sv := range sysVars { - copy[name] = copySv(sv) + copy[name] = sv.Clone() } return copy }