From 12f24d2738317af35444eabb540065a8c6e33dc8 Mon Sep 17 00:00:00 2001 From: Ti Chi Robot Date: Wed, 4 Jan 2023 18:10:19 +0800 Subject: [PATCH] sysvar: fix circular dependency in rebuildSysVarCache leading to deadlock (#40283) (#40311) close pingcap/tidb#40240 --- domain/domain.go | 8 +++++--- session/session.go | 11 ++++++----- session/session_test/session_test.go | 8 ++++---- sessionctx/variable/mock_globalaccessor.go | 2 +- sessionctx/variable/mock_globalaccessor_test.go | 4 ++-- sessionctx/variable/variable.go | 4 ++-- sessionctx/variable/varsutil.go | 2 +- 7 files changed, 21 insertions(+), 18 deletions(-) diff --git a/domain/domain.go b/domain/domain.go index c68f6c1349d31..0d406a20d099e 100644 --- a/domain/domain.go +++ b/domain/domain.go @@ -2110,7 +2110,7 @@ func (do *Domain) NotifyUpdatePrivilege() error { // NotifyUpdateSysVarCache updates the sysvar cache key in etcd, which other TiDB // clients are subscribed to for updates. For the caller, the cache is also built // synchronously so that the effect is immediate. -func (do *Domain) NotifyUpdateSysVarCache() { +func (do *Domain) NotifyUpdateSysVarCache(updateLocal bool) { if do.etcdClient != nil { row := do.etcdClient.KV _, err := row.Put(context.Background(), sysVarCacheKey, "") @@ -2119,8 +2119,10 @@ func (do *Domain) NotifyUpdateSysVarCache() { } } // update locally - if err := do.rebuildSysVarCache(nil); err != nil { - logutil.BgLogger().Error("rebuilding sysvar cache failed", zap.Error(err)) + if updateLocal { + if err := do.rebuildSysVarCache(nil); err != nil { + logutil.BgLogger().Error("rebuilding sysvar cache failed", zap.Error(err)) + } } } diff --git a/session/session.go b/session/session.go index e681606323fe1..b9a205bacc4b9 100644 --- a/session/session.go +++ b/session/session.go @@ -1442,13 +1442,13 @@ func (s *session) getTableValue(ctx context.Context, tblName string, varName str // replaceGlobalVariablesTableValue executes restricted sql updates the variable value // It will then notify the etcd channel that the value has changed. -func (s *session) replaceGlobalVariablesTableValue(ctx context.Context, varName, val string) error { +func (s *session) replaceGlobalVariablesTableValue(ctx context.Context, varName, val string, updateLocal bool) error { ctx = kv.WithInternalSourceType(ctx, kv.InternalTxnSysVar) _, _, err := s.ExecRestrictedSQL(ctx, nil, `REPLACE INTO %n.%n (variable_name, variable_value) VALUES (%?, %?)`, mysql.SystemDB, mysql.GlobalVariablesTable, varName, val) if err != nil { return err } - domain.GetDomain(s).NotifyUpdateSysVarCache() + domain.GetDomain(s).NotifyUpdateSysVarCache(updateLocal) return err } @@ -1510,12 +1510,13 @@ func (s *session) SetGlobalSysVar(ctx context.Context, name string, value string if sv.GlobalConfigName != "" { domain.GetDomain(s).NotifyGlobalConfigChange(sv.GlobalConfigName, variable.OnOffToTrueFalse(value)) } - return s.replaceGlobalVariablesTableValue(context.TODO(), sv.Name, value) + return s.replaceGlobalVariablesTableValue(context.TODO(), sv.Name, value, true) } // SetGlobalSysVarOnly updates the sysvar, but does not call the validation function or update aliases. // This is helpful to prevent duplicate warnings being appended from aliases, or recursion. -func (s *session) SetGlobalSysVarOnly(ctx context.Context, name string, value string) (err error) { +// updateLocal indicates whether to rebuild the local SysVar Cache. This is helpful to prevent recursion. +func (s *session) SetGlobalSysVarOnly(ctx context.Context, name string, value string, updateLocal bool) (err error) { sv := variable.GetSysVar(name) if sv == nil { return variable.ErrUnknownSystemVar.GenWithStackByArgs(name) @@ -1526,7 +1527,7 @@ func (s *session) SetGlobalSysVarOnly(ctx context.Context, name string, value st if sv.HasInstanceScope() { // skip for INSTANCE scope return nil } - return s.replaceGlobalVariablesTableValue(ctx, sv.Name, value) + return s.replaceGlobalVariablesTableValue(ctx, sv.Name, value, updateLocal) } // SetTiDBTableValue implements GlobalVarAccessor.SetTiDBTableValue interface. diff --git a/session/session_test/session_test.go b/session/session_test/session_test.go index 60b265b3caa6a..2c211a74b5f66 100644 --- a/session/session_test/session_test.go +++ b/session/session_test/session_test.go @@ -3778,7 +3778,7 @@ func TestUpgradeSysvars(t *testing.T) { // 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.Session()).NotifyUpdateSysVarCache() // update cache + domain.GetDomain(tk.Session()).NotifyUpdateSysVarCache(true) // update cache v, err := se.GetGlobalSysVar("tidb_enable_noop_functions") require.NoError(t, err) require.Equal(t, "OFF", v) @@ -3789,7 +3789,7 @@ func TestUpgradeSysvars(t *testing.T) { // 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.Session()).NotifyUpdateSysVarCache() // update cache + domain.GetDomain(tk.Session()).NotifyUpdateSysVarCache(true) // update cache v, err = se.GetGlobalSysVar("rpl_semi_sync_slave_enabled") require.NoError(t, err) require.Equal(t, "OFF", v) // the default value is restored. @@ -3800,7 +3800,7 @@ func TestUpgradeSysvars(t *testing.T) { // 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.Session()).NotifyUpdateSysVarCache() // update cache + domain.GetDomain(tk.Session()).NotifyUpdateSysVarCache(true) // update cache v, err = se.GetGlobalSysVar("tidb_executor_concurrency") require.NoError(t, err) require.Equal(t, "256", v) // the max value is restored. @@ -3809,7 +3809,7 @@ func TestUpgradeSysvars(t *testing.T) { // 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.Session()).NotifyUpdateSysVarCache() // update cache + domain.GetDomain(tk.Session()).NotifyUpdateSysVarCache(true) // update cache v, err = se.GetGlobalSysVar("tidb_enable_noop_functions") require.NoError(t, err) require.Equal(t, "OFF", v) // the default value is restored. diff --git a/sessionctx/variable/mock_globalaccessor.go b/sessionctx/variable/mock_globalaccessor.go index e452c398da424..5477c054257e4 100644 --- a/sessionctx/variable/mock_globalaccessor.go +++ b/sessionctx/variable/mock_globalaccessor.go @@ -87,7 +87,7 @@ func (m *MockGlobalAccessor) SetGlobalSysVar(ctx context.Context, name string, v } // SetGlobalSysVarOnly implements GlobalVarAccessor.SetGlobalSysVarOnly interface. -func (m *MockGlobalAccessor) SetGlobalSysVarOnly(ctx context.Context, name string, value string) error { +func (m *MockGlobalAccessor) SetGlobalSysVarOnly(ctx context.Context, name string, value string, _ bool) error { sv := GetSysVar(name) if sv == nil { return ErrUnknownSystemVar.GenWithStackByArgs(name) diff --git a/sessionctx/variable/mock_globalaccessor_test.go b/sessionctx/variable/mock_globalaccessor_test.go index 3372a92790275..76f4b1d39b608 100644 --- a/sessionctx/variable/mock_globalaccessor_test.go +++ b/sessionctx/variable/mock_globalaccessor_test.go @@ -36,7 +36,7 @@ func TestMockAPI(t *testing.T) { // invalid option name err = mock.SetGlobalSysVar(context.Background(), "illegalopt", "val") require.Error(t, err) - err = mock.SetGlobalSysVarOnly(context.Background(), "illegalopt", "val") + err = mock.SetGlobalSysVarOnly(context.Background(), "illegalopt", "val", true) require.Error(t, err) // valid option, invalid value @@ -46,7 +46,7 @@ func TestMockAPI(t *testing.T) { // valid option, valid value err = mock.SetGlobalSysVar(context.Background(), DefaultAuthPlugin, "mysql_native_password") require.NoError(t, err) - err = mock.SetGlobalSysVarOnly(context.Background(), DefaultAuthPlugin, "mysql_native_password") + err = mock.SetGlobalSysVarOnly(context.Background(), DefaultAuthPlugin, "mysql_native_password", true) require.NoError(t, err) // Test GetTiDBTableValue diff --git a/sessionctx/variable/variable.go b/sessionctx/variable/variable.go index b47036bfdd6d1..48eb86a45c2a2 100644 --- a/sessionctx/variable/variable.go +++ b/sessionctx/variable/variable.go @@ -256,7 +256,7 @@ func (sv *SysVar) SetGlobalFromHook(ctx context.Context, s *SessionVars, val str if !skipAliases && sv.Aliases != nil { for _, aliasName := range sv.Aliases { - if err := s.GlobalVarsAccessor.SetGlobalSysVarOnly(ctx, aliasName, val); err != nil { + if err := s.GlobalVarsAccessor.SetGlobalSysVarOnly(ctx, aliasName, val, true); err != nil { return err } } @@ -631,7 +631,7 @@ type GlobalVarAccessor interface { // SetGlobalSysVar sets the global system variable name to value. SetGlobalSysVar(ctx context.Context, name string, value string) error // SetGlobalSysVarOnly sets the global system variable without calling the validation function or updating aliases. - SetGlobalSysVarOnly(ctx context.Context, name string, value string) error + SetGlobalSysVarOnly(ctx context.Context, name string, value string, updateLocal bool) error // GetTiDBTableValue gets a value from mysql.tidb for the key 'name' GetTiDBTableValue(name string) (string, error) // SetTiDBTableValue sets a value+comment for the mysql.tidb key 'name' diff --git a/sessionctx/variable/varsutil.go b/sessionctx/variable/varsutil.go index 7329ac6300253..959bb9fb07e52 100644 --- a/sessionctx/variable/varsutil.go +++ b/sessionctx/variable/varsutil.go @@ -533,7 +533,7 @@ func collectAllowFuncName4ExpressionIndex() string { } func updatePasswordValidationLength(s *SessionVars, length int32) error { - err := s.GlobalVarsAccessor.SetGlobalSysVarOnly(context.Background(), ValidatePasswordLength, strconv.FormatInt(int64(length), 10)) + err := s.GlobalVarsAccessor.SetGlobalSysVarOnly(context.Background(), ValidatePasswordLength, strconv.FormatInt(int64(length), 10), false) if err != nil { return err }