sysvar: fix circular dependency in rebuildSysVarCache leading to dead…

…lock (#40283)

close #40240

domain/domain.go

@@ -2187,7 +2187,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, "")
@@ -2196,8 +2196,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))
+		}
 	}
 }
 

session/session.go

@@ -1441,13 +1441,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
 }
 
@@ -1509,12 +1509,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)
@@ -1525,7 +1526,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.

session/session_test/session_test.go

@@ -3779,7 +3779,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)
@@ -3790,7 +3790,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.
@@ -3801,7 +3801,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.
@@ -3810,7 +3810,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.

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)

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

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'

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
 	}