From 23230edf3610ef82196486ccfde0bcdd35b76535 Mon Sep 17 00:00:00 2001 From: Ewan Chou Date: Thu, 5 Sep 2019 18:32:58 +0800 Subject: [PATCH] *: replace pessimistic default config option with global variable --- config/config.go | 5 +-- config/config.toml.example | 5 +-- executor/executor_test.go | 8 ---- executor/simple.go | 3 -- session/pessimistic_test.go | 55 +++++++++++------------- session/session.go | 7 +-- sessionctx/variable/session.go | 4 +- sessionctx/variable/sysvar.go | 2 +- sessionctx/variable/tidb_vars.go | 3 +- sessionctx/variable/varsutil.go | 7 +++ sessionctx/variable/varsutil_test.go | 64 ++++++++++++++++++++++++++++ 11 files changed, 103 insertions(+), 60 deletions(-) diff --git a/config/config.go b/config/config.go index 023435cb87d9b..9235f753d5aa0 100644 --- a/config/config.go +++ b/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", }, diff --git a/config/config.toml.example b/config/config.toml.example index 356d4d9f8716d..93a5454196ed7 100644 --- a/config/config.toml.example +++ b/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 diff --git a/executor/executor_test.go b/executor/executor_test.go index 3f278cc156291..69fcf128d490b 100644 --- a/executor/executor_test.go +++ b/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") diff --git a/executor/simple.go b/executor/simple.go index 1408aa8339ea6..4f6ed1528969f 100644 --- a/executor/simple.go +++ b/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 diff --git a/session/pessimistic_test.go b/session/pessimistic_test.go index 2f638b1ae0699..2d2f233680760 100644 --- a/session/pessimistic_test.go +++ b/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) { diff --git a/session/session.go b/session/session.go index a1f2024afa0d8..14c2d1e3fd3dd 100644 --- a/session/session.go +++ b/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 } } diff --git a/sessionctx/variable/session.go b/sessionctx/variable/session.go index 6ca9dccb3e243..1c2b201d5ac48 100644 --- a/sessionctx/variable/session.go +++ b/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) } diff --git a/sessionctx/variable/sysvar.go b/sessionctx/variable/sysvar.go index 199e9c5779c48..233484f99af45 100644 --- a/sessionctx/variable/sysvar.go +++ b/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)}, diff --git a/sessionctx/variable/tidb_vars.go b/sessionctx/variable/tidb_vars.go index 365c7df16265c..002ebf722b38a 100644 --- a/sessionctx/variable/tidb_vars.go +++ b/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. diff --git a/sessionctx/variable/varsutil.go b/sessionctx/variable/varsutil.go index 71424cea407f5..8196b2315aa51 100644 --- a/sessionctx/variable/varsutil.go +++ b/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 } diff --git a/sessionctx/variable/varsutil_test.go b/sessionctx/variable/varsutil_test.go index f4af0357a682b..ec97078c4d849 100644 --- a/sessionctx/variable/varsutil_test.go +++ b/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)) + } + } +}