*: replace pessimistic default config option with global variable (#1…

…2049)

config/config.go

@@ -299,8 +299,6 @@ type Plugin struct {
 type PessimisticTxn struct {
 	// Enable must be true for 'begin lock' or session variable to start a pessimistic transaction.
 	Enable bool `toml:"enable" json:"enable"`
-	// Starts a pessimistic transaction by default when Enable is true.
-	Default bool `toml:"default" json:"default"`
 	// The max count of retry for a single statement in a pessimistic transaction.
 	MaxRetryCount uint `toml:"max-retry-count" json:"max-retry-count"`
 	// The pessimistic lock ttl.
@@ -392,8 +390,7 @@ var defaultConf = Config{
 		Strategy:     "range",
 	},
 	PessimisticTxn: PessimisticTxn{
-		Enable:        false,
-		Default:       false,
+		Enable:        true,
 		MaxRetryCount: 256,
 		TTL:           "40s",
 	},

config/config.toml.example

@@ -289,10 +289,7 @@ strategy = "range"
 
 [pessimistic-txn]
 # enable pessimistic transaction.
-enable = false
-
-# start pessimistic transaction by default.
-default = false
+enable = true
 
 # max retry count for a statement in a pessimistic transaction.
 max-retry-count = 256

executor/executor_test.go

@@ -137,15 +137,7 @@ func (s *testSuite) TearDownSuite(c *C) {
 	s.store.Close()
 }
 
-func enablePessimisticTxn(enable bool) {
-	newConf := config.NewConfig()
-	newConf.PessimisticTxn.Enable = enable
-	config.StoreGlobalConfig(newConf)
-}
-
 func (s *testSuite) TestPessimisticSelectForUpdate(c *C) {
-	defer func() { enablePessimisticTxn(false) }()
-	enablePessimisticTxn(true)
 	tk := testkit.NewTestKit(c, s.store)
 	tk.MustExec("use test")
 	tk.MustExec("drop table if exists t")

executor/simple.go

@@ -518,9 +518,6 @@ func (e *SimpleExec) executeBegin(ctx context.Context, s *ast.BeginStmt) error {
 		txnMode := s.Mode
 		if txnMode == "" {
 			txnMode = e.ctx.GetSessionVars().TxnMode
-			if txnMode == "" && pTxnConf.Default {
-				txnMode = ast.Pessimistic
-			}
 		}
 		if txnMode == ast.Pessimistic {
 			e.ctx.GetSessionVars().TxnCtx.IsPessimistic = true

session/pessimistic_test.go

@@ -21,7 +21,6 @@ import (
 	"github.com/pingcap/errors"
 	"github.com/pingcap/parser/mysql"
 	"github.com/pingcap/parser/terror"
-	"github.com/pingcap/tidb/config"
 	"github.com/pingcap/tidb/domain"
 	"github.com/pingcap/tidb/kv"
 	"github.com/pingcap/tidb/session"
@@ -45,7 +44,6 @@ type testPessimisticSuite struct {
 
 func (s *testPessimisticSuite) SetUpSuite(c *C) {
 	testleak.BeforeTest()
-	config.GetGlobalConfig().PessimisticTxn.Enable = true
 	// Set it to 300ms for testing lock resolve.
 	tikv.PessimisticLockTTL = 300
 	s.cluster = mocktikv.NewCluster()
@@ -60,13 +58,13 @@ func (s *testPessimisticSuite) SetUpSuite(c *C) {
 	session.SetSchemaLease(0)
 	session.DisableStats4Test()
 	s.dom, err = session.BootstrapSession(s.store)
+	s.dom.GetGlobalVarsCache().Disable()
 	c.Assert(err, IsNil)
 }
 
 func (s *testPessimisticSuite) TearDownSuite(c *C) {
 	s.dom.Close()
 	s.store.Close()
-	config.GetGlobalConfig().PessimisticTxn.Enable = false
 	testleak.AfterTest(c)()
 }
 
@@ -121,30 +119,19 @@ func (s *testPessimisticSuite) TestTxnMode(c *C) {
 	tests := []struct {
 		beginStmt     string
 		txnMode       string
-		configDefault bool
 		isPessimistic bool
 	}{
-		{"pessimistic", "pessimistic", false, true},
-		{"pessimistic", "pessimistic", true, true},
-		{"pessimistic", "optimistic", false, true},
-		{"pessimistic", "optimistic", true, true},
-		{"pessimistic", "", false, true},
-		{"pessimistic", "", true, true},
-		{"optimistic", "pessimistic", false, false},
-		{"optimistic", "pessimistic", true, false},
-		{"optimistic", "optimistic", false, false},
-		{"optimistic", "optimistic", true, false},
-		{"optimistic", "", false, false},
-		{"optimistic", "", true, false},
-		{"", "pessimistic", false, true},
-		{"", "pessimistic", true, true},
-		{"", "optimistic", false, false},
-		{"", "optimistic", true, false},
-		{"", "", false, false},
-		{"", "", true, true},
+		{"pessimistic", "pessimistic", true},
+		{"pessimistic", "optimistic", true},
+		{"pessimistic", "", true},
+		{"optimistic", "pessimistic", false},
+		{"optimistic", "optimistic", false},
+		{"optimistic", "", false},
+		{"", "pessimistic", true},
+		{"", "optimistic", false},
+		{"", "", false},
 	}
 	for _, tt := range tests {
-		config.GetGlobalConfig().PessimisticTxn.Default = tt.configDefault
 		tk.MustExec(fmt.Sprintf("set @@tidb_txn_mode = '%s'", tt.txnMode))
 		tk.MustExec("begin " + tt.beginStmt)
 		c.Check(tk.Se.GetSessionVars().TxnCtx.IsPessimistic, Equals, tt.isPessimistic)
@@ -155,24 +142,30 @@ func (s *testPessimisticSuite) TestTxnMode(c *C) {
 	tk.MustExec("create table if not exists txn_mode (a int)")
 	tests2 := []struct {
 		txnMode       string
-		configDefault bool
 		isPessimistic bool
 	}{
-		{"pessimistic", false, true},
-		{"pessimistic", true, true},
-		{"optimistic", false, false},
-		{"optimistic", true, false},
-		{"", false, false},
-		{"", true, true},
+		{"pessimistic", true},
+		{"optimistic", false},
+		{"", false},
 	}
 	for _, tt := range tests2 {
-		config.GetGlobalConfig().PessimisticTxn.Default = tt.configDefault
 		tk.MustExec(fmt.Sprintf("set @@tidb_txn_mode = '%s'", tt.txnMode))
 		tk.MustExec("rollback")
 		tk.MustExec("insert txn_mode values (1)")
 		c.Check(tk.Se.GetSessionVars().TxnCtx.IsPessimistic, Equals, tt.isPessimistic)
 		tk.MustExec("rollback")
 	}
+	tk.MustExec("set @@global.tidb_txn_mode = 'pessimistic'")
+	tk1 := testkit.NewTestKitWithInit(c, s.store)
+	tk1.MustQuery("select @@tidb_txn_mode").Check(testkit.Rows("pessimistic"))
+	tk1.MustExec("set @@autocommit = 0")
+	tk1.MustExec("insert txn_mode values (2)")
+	c.Check(tk1.Se.GetSessionVars().TxnCtx.IsPessimistic, IsTrue)
+	tk1.MustExec("set @@tidb_txn_mode = ''")
+	tk1.MustExec("rollback")
+	tk1.MustExec("insert txn_mode values (2)")
+	c.Check(tk1.Se.GetSessionVars().TxnCtx.IsPessimistic, IsFalse)
+	tk1.MustExec("rollback")
 }
 
 func (s *testPessimisticSuite) TestDeadlock(c *C) {

session/session.go

@@ -1691,6 +1691,7 @@ var builtinGlobalVariable = []string{
 	variable.TiDBEnableWindowFunction,
 	variable.TiDBEnableFastAnalyze,
 	variable.TiDBExpensiveQueryTimeThreshold,
+	variable.TiDBTxnMode,
 }
 
 var (
@@ -1779,11 +1780,7 @@ func (s *session) PrepareTxnCtx(ctx context.Context) {
 	if !s.sessionVars.IsAutocommit() {
 		pessTxnConf := config.GetGlobalConfig().PessimisticTxn
 		if pessTxnConf.Enable {
-			txnMode := s.sessionVars.TxnMode
-			if txnMode == "" && pessTxnConf.Default {
-				txnMode = ast.Pessimistic
-			}
-			if txnMode == ast.Pessimistic {
+			if s.sessionVars.TxnMode == ast.Pessimistic {
 				s.sessionVars.TxnCtx.IsPessimistic = true
 			}
 		}

sessionctx/variable/session.go

@@ -822,9 +822,7 @@ func (s *SessionVars) SetSystemVar(name string, val string) error {
 	case TiDBExpensiveQueryTimeThreshold:
 		atomic.StoreUint64(&ExpensiveQueryTimeThreshold, uint64(tidbOptPositiveInt32(val, DefTiDBExpensiveQueryTimeThreshold)))
 	case TiDBTxnMode:
-		if err := s.setTxnMode(val); err != nil {
-			return err
-		}
+		s.TxnMode = strings.ToUpper(val)
 	case TiDBLowResolutionTSO:
 		s.LowResolutionTSO = TiDBOptOn(val)
 	}

sessionctx/variable/sysvar.go

@@ -676,7 +676,7 @@ var defaultSysVars = []*SysVar{
 	{ScopeGlobal | ScopeSession, TiDBRetryLimit, strconv.Itoa(DefTiDBRetryLimit)},
 	{ScopeGlobal | ScopeSession, TiDBDisableTxnAutoRetry, BoolToIntStr(DefTiDBDisableTxnAutoRetry)},
 	{ScopeGlobal | ScopeSession, TiDBConstraintCheckInPlace, BoolToIntStr(DefTiDBConstraintCheckInPlace)},
-	{ScopeSession, TiDBTxnMode, DefTiDBTxnMode},
+	{ScopeGlobal | ScopeSession, TiDBTxnMode, DefTiDBTxnMode},
 	{ScopeSession, TiDBOptimizerSelectivityLevel, strconv.Itoa(DefTiDBOptimizerSelectivityLevel)},
 	{ScopeGlobal | ScopeSession, TiDBEnableWindowFunction, BoolToIntStr(DefEnableWindowFunction)},
 	{ScopeGlobal | ScopeSession, TiDBEnableFastAnalyze, BoolToIntStr(DefTiDBUseFastAnalyze)},

sessionctx/variable/tidb_vars.go

@@ -29,7 +29,8 @@ import (
 	5. Update the `NewSessionVars` function to set the field to its default value.
 	6. Update the `variable.SetSessionSystemVar` function to use the new value when SET statement is executed.
 	7. If it is a global variable, add it in `session.loadCommonGlobalVarsSQL`.
-	8. Use this variable to control the behavior in code.
+	8. Update ValidateSetSystemVar if the variable's value need to be validated.
+	9. Use this variable to control the behavior in code.
 */
 
 // TiDB system variable names that only in session scope.

sessionctx/variable/varsutil.go

@@ -23,6 +23,7 @@ import (
 	"time"
 
 	"github.com/pingcap/errors"
+	"github.com/pingcap/parser/ast"
 	"github.com/pingcap/parser/mysql"
 	"github.com/pingcap/tidb/config"
 	"github.com/pingcap/tidb/types"
@@ -562,6 +563,12 @@ func ValidateSetSystemVar(vars *SessionVars, name string, value string) (string,
 		if v <= 0 {
 			return value, errors.Errorf("tidb_wait_split_region_timeout(%d) cannot be smaller than 1", v)
 		}
+	case TiDBTxnMode:
+		switch strings.ToUpper(value) {
+		case ast.Pessimistic, ast.Optimistic, "":
+		default:
+			return value, ErrWrongValueForVar.GenWithStackByArgs(TiDBTxnMode, value)
+		}
 	}
 	return value, nil
 }

sessionctx/variable/varsutil_test.go

@@ -295,3 +295,67 @@ func (s *testVarsutilSuite) TestVarsutil(c *C) {
 	c.Assert(val, Equals, "0")
 	c.Assert(v.CorrelationThreshold, Equals, float64(0))
 }
+
+func (s *testVarsutilSuite) TestValidate(c *C) {
+	v := NewSessionVars()
+	v.GlobalVarsAccessor = NewMockGlobalAccessor()
+	v.TimeZone = time.UTC
+
+	tests := []struct {
+		key   string
+		value string
+		error bool
+	}{
+		{TiDBAutoAnalyzeStartTime, "15:04", false},
+		{TiDBAutoAnalyzeStartTime, "15:04 -0700", false},
+		{DelayKeyWrite, "ON", false},
+		{DelayKeyWrite, "OFF", false},
+		{DelayKeyWrite, "ALL", false},
+		{DelayKeyWrite, "3", true},
+		{ForeignKeyChecks, "3", true},
+		{MaxSpRecursionDepth, "256", false},
+		{SessionTrackGtids, "OFF", false},
+		{SessionTrackGtids, "OWN_GTID", false},
+		{SessionTrackGtids, "ALL_GTIDS", false},
+		{SessionTrackGtids, "ON", true},
+		{EnforceGtidConsistency, "OFF", false},
+		{EnforceGtidConsistency, "ON", false},
+		{EnforceGtidConsistency, "WARN", false},
+		{QueryCacheType, "OFF", false},
+		{QueryCacheType, "ON", false},
+		{QueryCacheType, "DEMAND", false},
+		{QueryCacheType, "3", true},
+		{SecureAuth, "1", false},
+		{SecureAuth, "3", true},
+		{MyISAMUseMmap, "ON", false},
+		{MyISAMUseMmap, "OFF", false},
+		{TiDBEnableTablePartition, "ON", false},
+		{TiDBEnableTablePartition, "OFF", false},
+		{TiDBEnableTablePartition, "AUTO", false},
+		{TiDBEnableTablePartition, "UN", true},
+		{TiDBOptCorrelationExpFactor, "a", true},
+		{TiDBOptCorrelationExpFactor, "-10", true},
+		{TiDBOptCorrelationThreshold, "a", true},
+		{TiDBOptCorrelationThreshold, "-2", true},
+		{TxnIsolation, "READ-UNCOMMITTED", true},
+		{TiDBInitChunkSize, "a", true},
+		{TiDBInitChunkSize, "-1", true},
+		{TiDBMaxChunkSize, "a", true},
+		{TiDBMaxChunkSize, "-1", true},
+		{TiDBOptJoinReorderThreshold, "a", true},
+		{TiDBOptJoinReorderThreshold, "-1", true},
+		{TiDBTxnMode, "invalid", true},
+		{TiDBTxnMode, "pessimistic", false},
+		{TiDBTxnMode, "optimistic", false},
+		{TiDBTxnMode, "", false},
+	}
+
+	for _, t := range tests {
+		_, err := ValidateSetSystemVar(v, t.key, t.value)
+		if t.error {
+			c.Assert(err, NotNil, Commentf("%v got err=%v", t, err))
+		} else {
+			c.Assert(err, IsNil, Commentf("%v got err=%v", t, err))
+		}
+	}
+}