Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

sysvar: fix circular dependency in rebuildSysVarCache leading to deadlock (#40283) #40311

Merged
merged 1 commit into from
Jan 4, 2023
Merged

sysvar: fix circular dependency in rebuildSysVarCache leading to deadlock (#40283) #40311

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 5 additions & 3 deletions domain/domain.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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, "")
Expand All @@ -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))
}
}
}

Expand Down
11 changes: 6 additions & 5 deletions session/session.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
}

Expand Down Expand Up @@ -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)
Expand All @@ -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.
Expand Down
8 changes: 4 additions & 4 deletions session/session_test/session_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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.
Expand All @@ -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.
Expand All @@ -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.
Expand Down
2 changes: 1 addition & 1 deletion sessionctx/variable/mock_globalaccessor.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
4 changes: 2 additions & 2 deletions sessionctx/variable/mock_globalaccessor_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
Expand Down
4 changes: 2 additions & 2 deletions sessionctx/variable/variable.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
}
Expand Down Expand Up @@ -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'
Expand Down
2 changes: 1 addition & 1 deletion sessionctx/variable/varsutil.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand Down