domain, sessionctx/variable: make code DRY (pingcap#33897)

close pingcap#33896

domain/sysvar_cache.go

@@ -25,9 +25,6 @@ import (
 	"github.com/pingcap/tidb/sessionctx/variable"
 	"github.com/pingcap/tidb/util/logutil"
 	"github.com/pingcap/tidb/util/sqlexec"
-	"github.com/pingcap/tidb/util/stmtsummary"
-	topsqlstate "github.com/pingcap/tidb/util/topsql/state"
-	storekv "github.com/tikv/client-go/v2/kv"
 	pd "github.com/tikv/pd/client"
 	"go.uber.org/zap"
 )
@@ -142,9 +139,24 @@ func (do *Domain) rebuildSysVarCache(ctx sessionctx.Context) error {
 		}
 		if sv.HasGlobalScope() {
 			newGlobalCache[sv.Name] = sVal
+
+			// Call the SetGlobal func for this sysvar if it exists.
+			// SET GLOBAL only calls the SetGlobal func on the calling instances.
+			// This ensures it is run on all tidb servers.
+			// This does not apply to INSTANCE scoped vars (HasGlobalScope() is false)
+			if sv.SetGlobal != nil && !sv.SkipSysvarCache() {
+				sVal = sv.ValidateWithRelaxedValidation(ctx.GetSessionVars(), sVal, variable.ScopeGlobal)
+				err = sv.SetGlobal(ctx.GetSessionVars(), sVal)
+				if err != nil {
+					logutil.BgLogger().Error(fmt.Sprintf("load global variable %s error", sv.Name), zap.Error(err))
+				}
+			}
 		}
-		// Propagate any changes to the server scoped variables
-		do.checkEnableServerGlobalVar(sv.Name, sVal)
+
+		// Some PD options need to be checked outside of the SetGlobal func.
+		// This is also done for the SET GLOBAL caller in executor/set.go,
+		// but here we check for other tidb instances.
+		do.checkPDClientDynamicOption(sv.Name, sVal)
 	}
 
 	logutil.BgLogger().Debug("rebuilding sysvar cache")
@@ -156,19 +168,10 @@ func (do *Domain) rebuildSysVarCache(ctx sessionctx.Context) error {
 	return nil
 }
 
-// checkEnableServerGlobalVar processes variables that acts in server and global level.
-// This is required because the SetGlobal function on the sysvar struct only executes on
-// the initiating tidb-server. There is no current method to say "run this function on all
-// tidb servers when the value of this variable changes". If you do not require changes to
-// be applied on all servers, use a getter/setter instead! You don't need to add to this list.
-func (do *Domain) checkEnableServerGlobalVar(name, sVal string) {
-	var err error
+func (do *Domain) checkPDClientDynamicOption(name, sVal string) {
 	switch name {
-	case variable.TiDBMemQuotaBindingCache:
-		variable.MemQuotaBindingCache.Store(variable.TidbOptInt64(sVal, variable.DefTiDBMemQuotaBindingCache))
 	case variable.TiDBTSOClientBatchMaxWaitTime:
-		var val float64
-		val, err = strconv.ParseFloat(sVal, 64)
+		val, err := strconv.ParseFloat(sVal, 64)
 		if err != nil {
 			break
 		}
@@ -179,82 +182,11 @@ func (do *Domain) checkEnableServerGlobalVar(name, sVal string) {
 		variable.MaxTSOBatchWaitInterval.Store(val)
 	case variable.TiDBEnableTSOFollowerProxy:
 		val := variable.TiDBOptOn(sVal)
-		err = do.SetPDClientDynamicOption(pd.EnableTSOFollowerProxy, val)
+		err := do.SetPDClientDynamicOption(pd.EnableTSOFollowerProxy, val)
 		if err != nil {
 			break
 		}
 		variable.EnableTSOFollowerProxy.Store(val)
-	case variable.TiDBEnableLocalTxn:
-		variable.EnableLocalTxn.Store(variable.TiDBOptOn(sVal))
-	case variable.TiDBEnableStmtSummary:
-		err = stmtsummary.StmtSummaryByDigestMap.SetEnabled(variable.TiDBOptOn(sVal))
-	case variable.TiDBStmtSummaryInternalQuery:
-		err = stmtsummary.StmtSummaryByDigestMap.SetEnabledInternalQuery(variable.TiDBOptOn(sVal))
-	case variable.TiDBStmtSummaryRefreshInterval:
-		err = stmtsummary.StmtSummaryByDigestMap.SetRefreshInterval(variable.TidbOptInt64(sVal, variable.DefTiDBStmtSummaryRefreshInterval))
-	case variable.TiDBStmtSummaryHistorySize:
-		err = stmtsummary.StmtSummaryByDigestMap.SetHistorySize(variable.TidbOptInt(sVal, variable.DefTiDBStmtSummaryHistorySize))
-	case variable.TiDBStmtSummaryMaxStmtCount:
-		err = stmtsummary.StmtSummaryByDigestMap.SetMaxStmtCount(uint(variable.TidbOptInt(sVal, variable.DefTiDBStmtSummaryMaxStmtCount)))
-	case variable.TiDBStmtSummaryMaxSQLLength:
-		err = stmtsummary.StmtSummaryByDigestMap.SetMaxSQLLength(variable.TidbOptInt(sVal, variable.DefTiDBStmtSummaryMaxSQLLength))
-	case variable.TiDBTopSQLMaxTimeSeriesCount:
-		var val int64
-		val, err = strconv.ParseInt(sVal, 10, 64)
-		if err != nil {
-			break
-		}
-		topsqlstate.GlobalState.MaxStatementCount.Store(val)
-	case variable.TiDBTopSQLMaxMetaCount:
-		var val int64
-		val, err = strconv.ParseInt(sVal, 10, 64)
-		if err != nil {
-			break
-		}
-		topsqlstate.GlobalState.MaxCollect.Store(val)
-	case variable.TiDBRestrictedReadOnly:
-		variable.RestrictedReadOnly.Store(variable.TiDBOptOn(sVal))
-	case variable.TiDBSuperReadOnly:
-		variable.VarTiDBSuperReadOnly.Store(variable.TiDBOptOn(sVal))
-	case variable.TiDBStoreLimit:
-		var val int64
-		val, err = strconv.ParseInt(sVal, 10, 64)
-		if err != nil {
-			break
-		}
-		storekv.StoreLimit.Store(val)
-	case variable.TiDBTableCacheLease:
-		var val int64
-		val, err = strconv.ParseInt(sVal, 10, 64)
-		if err != nil {
-			break
-		}
-		variable.TableCacheLease.Store(val)
-	case variable.TiDBGCMaxWaitTime:
-		var val int64
-		val, err = strconv.ParseInt(sVal, 10, 64)
-		if err != nil {
-			break
-		}
-		variable.GCMaxWaitTime.Store(val)
-	case variable.TiDBPersistAnalyzeOptions:
-		variable.PersistAnalyzeOptions.Store(variable.TiDBOptOn(sVal))
-	case variable.TiDBEnableColumnTracking:
-		variable.EnableColumnTracking.Store(variable.TiDBOptOn(sVal))
-	case variable.TiDBStatsLoadSyncWait:
-		var val int64
-		val, err = strconv.ParseInt(sVal, 10, 64)
-		if err != nil {
-			break
-		}
-		variable.StatsLoadSyncWait.Store(val)
-	case variable.TiDBStatsLoadPseudoTimeout:
-		variable.StatsLoadPseudoTimeout.Store(variable.TiDBOptOn(sVal))
-	case variable.TiDBTxnCommitBatchSize:
-		storekv.TxnCommitBatchSize.Store(uint64(variable.TidbOptInt64(sVal, int64(storekv.DefTxnCommitBatchSize))))
-	}
-	if err != nil {
-		logutil.BgLogger().Error(fmt.Sprintf("load global variable %s error", name), zap.Error(err))
 	}
 }
 

executor/set_test.go

@@ -1460,6 +1460,30 @@ func TestDefaultBehavior(t *testing.T) {
 	tk.MustExec("SET GLOBAL sql_mode = DEFAULT")
 }
 
+func TestTiDBReadOnly(t *testing.T) {
+	store, clean := testkit.CreateMockStore(t)
+	defer clean()
+	tk := testkit.NewTestKit(t, store)
+
+	// turn on tidb_restricted_read_only should turn on tidb_super_read_only
+	tk.MustExec("SET GLOBAL tidb_restricted_read_only = ON")
+	tk.MustQuery("SELECT @@GLOBAL.tidb_super_read_only").Check(testkit.Rows("1"))
+
+	// can't turn off tidb_super_read_only if tidb_restricted_read_only is on
+	err := tk.ExecToErr("SET GLOBAL tidb_super_read_only = OFF")
+	require.Error(t, err)
+	require.Equal(t, "can't turn off tidb_super_read_only when tidb_restricted_read_only is on", err.Error())
+
+	// turn off tidb_restricted_read_only won't affect tidb_super_read_only
+	tk.MustExec("SET GLOBAL tidb_restricted_read_only = OFF")
+	tk.MustQuery("SELECT @@GLOBAL.tidb_restricted_read_only").Check(testkit.Rows("0"))
+	tk.MustQuery("SELECT @@GLOBAL.tidb_super_read_only").Check(testkit.Rows("1"))
+
+	// it is ok to turn off tidb_super_read_only now
+	tk.MustExec("SET GLOBAL tidb_super_read_only = OFF")
+	tk.MustQuery("SELECT @@GLOBAL.tidb_super_read_only").Check(testkit.Rows("0"))
+}
+
 func TestRemovedSysVars(t *testing.T) {
 	store, clean := testkit.CreateMockStore(t)
 	defer clean()

sessionctx/variable/sysvar.go

@@ -539,7 +539,8 @@ var defaultSysVars = []*SysVar{
 		}},
 	{Scope: ScopeGlobal, Name: TiDBRestrictedReadOnly, Value: BoolToOnOff(DefTiDBRestrictedReadOnly), Type: TypeBool, SetGlobal: func(s *SessionVars, val string) error {
 		on := TiDBOptOn(val)
-		if on {
+		// For user initiated SET GLOBAL, also change the value of TiDBSuperReadOnly
+		if on && s.StmtCtx.StmtType == "Set" {
 			err := s.GlobalVarsAccessor.SetGlobalSysVar(TiDBSuperReadOnly, "ON")
 			if err != nil {
 				return err
@@ -548,10 +549,10 @@ var defaultSysVars = []*SysVar{
 		RestrictedReadOnly.Store(on)
 		return nil
 	}},
-	{Scope: ScopeGlobal, Name: TiDBSuperReadOnly, Value: BoolToOnOff(DefTiDBSuperReadOnly), Type: TypeBool, Validation: func(vars *SessionVars, normalizedValue string, _ string, _ ScopeFlag) (string, error) {
+	{Scope: ScopeGlobal, Name: TiDBSuperReadOnly, Value: BoolToOnOff(DefTiDBSuperReadOnly), Type: TypeBool, Validation: func(s *SessionVars, normalizedValue string, _ string, _ ScopeFlag) (string, error) {
 		on := TiDBOptOn(normalizedValue)
-		if !on {
-			result, err := vars.GlobalVarsAccessor.GetGlobalSysVar(TiDBRestrictedReadOnly)
+		if !on && s.StmtCtx.StmtType == "Set" {
+			result, err := s.GlobalVarsAccessor.GetGlobalSysVar(TiDBRestrictedReadOnly)
 			if err != nil {
 				return normalizedValue, err
 			}
@@ -610,8 +611,7 @@ var defaultSysVars = []*SysVar{
 		Validation: func(vars *SessionVars, normalizedValue string, originalValue string, scope ScopeFlag) (string, error) {
 			return checkGCTxnMaxWaitTime(vars, normalizedValue, originalValue, scope)
 		}, SetGlobal: func(s *SessionVars, val string) error {
-			ival, _ := strconv.Atoi(val)
-			GCMaxWaitTime.Store((int64)(ival))
+			GCMaxWaitTime.Store(TidbOptInt64(val, DefTiDBGCMaxWaitTime))
 			return nil
 		}},
 	{Scope: ScopeGlobal, Name: TiDBTableCacheLease, Value: strconv.Itoa(DefTiDBTableCacheLease), Type: TypeUnsigned, MinValue: 1, MaxValue: 10, SetGlobal: func(s *SessionVars, sVal string) error {
@@ -662,7 +662,9 @@ var defaultSysVars = []*SysVar{
 		return BoolToOnOff(EnableColumnTracking.Load()), nil
 	}, SetGlobal: func(s *SessionVars, val string) error {
 		v := TiDBOptOn(val)
-		if !v {
+		// If this is a user initiated statement,
+		// we log that column tracking is disabled.
+		if s.StmtCtx.StmtType == "Set" && !v {
 			// Set the location to UTC to avoid time zone interference.
 			disableTime := time.Now().UTC().Format(types.UTCTimeFormat)
 			if err := setTiDBTableValue(s, TiDBDisableColumnTrackingTime, disableTime, "Record the last time tidb_enable_column_tracking is set off"); err != nil {

sessionctx/variable/sysvar_test.go

@@ -536,43 +536,6 @@ func TestIsNoop(t *testing.T) {
 	require.True(t, sv.IsNoop)
 }
 
-func TestTiDBReadOnly(t *testing.T) {
-	rro := GetSysVar(TiDBRestrictedReadOnly)
-	sro := GetSysVar(TiDBSuperReadOnly)
-
-	vars := NewSessionVars()
-	mock := NewMockGlobalAccessor4Tests()
-	mock.SessionVars = vars
-	vars.GlobalVarsAccessor = mock
-
-	// turn on tidb_restricted_read_only should turn on tidb_super_read_only
-	require.NoError(t, mock.SetGlobalSysVar(rro.Name, "ON"))
-	result, err := mock.GetGlobalSysVar(sro.Name)
-	require.NoError(t, err)
-	require.Equal(t, "ON", result)
-
-	// can't turn off tidb_super_read_only if tidb_restricted_read_only is on
-	err = mock.SetGlobalSysVar(sro.Name, "OFF")
-	require.Error(t, err)
-	require.Equal(t, "can't turn off tidb_super_read_only when tidb_restricted_read_only is on", err.Error())
-
-	// turn off tidb_restricted_read_only won't affect tidb_super_read_only
-	require.NoError(t, mock.SetGlobalSysVar(rro.Name, "OFF"))
-	result, err = mock.GetGlobalSysVar(rro.Name)
-	require.NoError(t, err)
-	require.Equal(t, "OFF", result)
-
-	result, err = mock.GetGlobalSysVar(sro.Name)
-	require.NoError(t, err)
-	require.Equal(t, "ON", result)
-
-	// it is ok to turn off tidb_super_read_only now
-	require.NoError(t, mock.SetGlobalSysVar(sro.Name, "OFF"))
-	result, err = mock.GetGlobalSysVar(sro.Name)
-	require.NoError(t, err)
-	require.Equal(t, "OFF", result)
-}
-
 func TestInstanceScopedVars(t *testing.T) {
 	// This tests instance scoped variables through GetSessionOrGlobalSystemVar().
 	// Eventually these should be changed to use getters so that the switch

sessionctx/variable/variable.go

@@ -521,6 +521,22 @@ func (sv *SysVar) SkipInit() bool {
 	return !sv.HasSessionScope()
 }
 
+// SkipSysvarCache returns true if the sysvar should not re-execute on peers
+// This doesn't make sense for the GC variables because they are based in tikv
+// tables. We'd effectively be reading and writing to the same table, which
+// could be in an unsafe manner. In future these variables might be converted
+// to not use a different table internally, but to do that we need to first
+// fix upgrade/downgrade so we know that older servers won't be in the cluster
+// which update only these values.
+func (sv *SysVar) SkipSysvarCache() bool {
+	switch sv.Name {
+	case TiDBGCEnable, TiDBGCRunInterval, TiDBGCLifetime,
+		TiDBGCConcurrency, TiDBGCScanLockMode, TiDBGCMaxWaitTime:
+		return true
+	}
+	return false
+}
+
 var sysVars map[string]*SysVar
 var sysVarsLock sync.RWMutex