Skip to content

add skip isolation level check variable #10065

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

Merged
jackysp merged 13 commits into from
Apr 11, 2019
Merged

add skip isolation level check variable #10065

Show file tree
Hide file tree
Changes from 12 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
427762b
add strict compatibility variable
marsishandsome Apr 8, 2019
3270cf5
e -> err
marsishandsome Apr 8, 2019
6579b58
make fmt
marsishandsome Apr 8, 2019
4271cd6
add unit test
marsishandsome Apr 8, 2019
8d0a17d
change tidb_strict_compatibility to tidb_skip_isolation_level_check
marsishandsome Apr 9, 2019
8ddd5c5
fix log warning
marsishandsome Apr 9, 2019
4c5eddd
fix error msg
marsishandsome Apr 9, 2019
0bda114
add warning check in ut
marsishandsome Apr 9, 2019
a388359
fix ut
marsishandsome Apr 9, 2019
18957ec
fix ut
marsishandsome Apr 9, 2019
470b0dd
change error message
marsishandsome Apr 11, 2019
2c75356
deduplicate two same warnings
marsishandsome Apr 11, 2019
876342f
Merge branch 'master' into feature/strict_compatibility
jackysp Apr 11, 2019
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
45 changes: 45 additions & 0 deletions executor/set_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -285,6 +285,49 @@ func (s *testSuite2) TestSetVar(c *C) {
c.Assert(err, NotNil)
_, err = tk.Exec("set global tidb_batch_commit = 2")
c.Assert(err, NotNil)

// test skip isolation level check: init
tk.MustExec("SET GLOBAL tidb_skip_isolation_level_check = 0")
tk.MustExec("SET SESSION tidb_skip_isolation_level_check = 0")
tk.MustExec("SET GLOBAL TRANSACTION ISOLATION LEVEL READ COMMITTED")
tk.MustExec("SET SESSION TRANSACTION ISOLATION LEVEL READ COMMITTED")
tk.MustQuery("select @@global.tx_isolation").Check(testkit.Rows("READ-COMMITTED"))
tk.MustQuery("select @@global.transaction_isolation").Check(testkit.Rows("READ-COMMITTED"))
tk.MustQuery("select @@session.tx_isolation").Check(testkit.Rows("READ-COMMITTED"))
tk.MustQuery("select @@session.transaction_isolation").Check(testkit.Rows("READ-COMMITTED"))

// test skip isolation level check: error
_, err = tk.Exec("SET SESSION TRANSACTION ISOLATION LEVEL SERIALIZABLE")
c.Assert(terror.ErrorEqual(err, variable.ErrUnsupportedIsolationLevel), IsTrue, Commentf("err %v", err))
tk.MustQuery("select @@session.tx_isolation").Check(testkit.Rows("READ-COMMITTED"))
tk.MustQuery("select @@session.transaction_isolation").Check(testkit.Rows("READ-COMMITTED"))

_, err = tk.Exec("SET GLOBAL TRANSACTION ISOLATION LEVEL SERIALIZABLE")
c.Assert(terror.ErrorEqual(err, variable.ErrUnsupportedIsolationLevel), IsTrue, Commentf("err %v", err))
tk.MustQuery("select @@global.tx_isolation").Check(testkit.Rows("READ-COMMITTED"))
tk.MustQuery("select @@global.transaction_isolation").Check(testkit.Rows("READ-COMMITTED"))

// test skip isolation level check: success
tk.MustExec("SET GLOBAL tidb_skip_isolation_level_check = 1")
tk.MustExec("SET SESSION tidb_skip_isolation_level_check = 1")
tk.MustExec("SET SESSION TRANSACTION ISOLATION LEVEL SERIALIZABLE")
Comment thread
jackysp marked this conversation as resolved.
Outdated
tk.MustQuery("show warnings").Check(testkit.Rows(
"Warning 1105 The isolation level 'SERIALIZABLE' is not supported. Set tidb_skip_isolation_level_check=1 to skip this error"))
Copy link
Copy Markdown
Contributor

@morgo morgo Apr 11, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would like to see just one warning here.

Copy link
Copy Markdown
Contributor Author

@marsishandsome marsishandsome Apr 11, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@zhexuany @jackysp any suggestions about how to elegantly deduplicate two same warnings?

Copy link
Copy Markdown
Contributor

@jackysp jackysp Apr 11, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should debug it, why the warning was appended twice.

Copy link
Copy Markdown
Contributor Author

@marsishandsome marsishandsome Apr 11, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SET TRANSACTION ISOLATION LEVEL will affect two internal variables:

  1. tx_isolation
  2. transaction_isolation

So the function ValidateSetSystemVar is called twice.

Copy link
Copy Markdown
Contributor

@morgo morgo Apr 11, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because the error message does not mention which variable was set now, it is easy to fix by wrapping an if around transaction_isolation.

Copy link
Copy Markdown
Contributor Author

@marsishandsome marsishandsome Apr 11, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@morgo @jackysp PTAL again

tk.MustQuery("select @@session.tx_isolation").Check(testkit.Rows("SERIALIZABLE"))
tk.MustQuery("select @@session.transaction_isolation").Check(testkit.Rows("SERIALIZABLE"))

// test skip isolation level check: success
tk.MustExec("SET GLOBAL tidb_skip_isolation_level_check = 0")
tk.MustExec("SET SESSION tidb_skip_isolation_level_check = 1")
tk.MustExec("SET GLOBAL TRANSACTION ISOLATION LEVEL READ UNCOMMITTED")
tk.MustQuery("show warnings").Check(testkit.Rows(
"Warning 1105 The isolation level 'READ-UNCOMMITTED' is not supported. Set tidb_skip_isolation_level_check=1 to skip this error"))
Copy link
Copy Markdown
Contributor

@morgo morgo Apr 11, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And here too

tk.MustQuery("select @@global.tx_isolation").Check(testkit.Rows("READ-UNCOMMITTED"))
tk.MustQuery("select @@global.transaction_isolation").Check(testkit.Rows("READ-UNCOMMITTED"))

// test skip isolation level check: reset
tk.MustExec("SET GLOBAL tidb_skip_isolation_level_check = 0")
tk.MustExec("SET SESSION tidb_skip_isolation_level_check = 0")
}

func (s *testSuite2) TestSetCharset(c *C) {
Expand Down Expand Up @@ -584,6 +627,8 @@ func (s *testSuite2) TestValidateSetVar(c *C) {
result = tk.MustQuery("select @@tx_isolation;")
result.Check(testkit.Rows("REPEATABLE-READ"))

tk.MustExec("SET GLOBAL tidb_skip_isolation_level_check = 0")
tk.MustExec("SET SESSION tidb_skip_isolation_level_check = 0")
_, err = tk.Exec("set @@tx_isolation='SERIALIZABLE'")
c.Assert(terror.ErrorEqual(err, variable.ErrUnsupportedValueForVar), IsTrue, Commentf("err %v", err))
}
Expand Down
16 changes: 15 additions & 1 deletion sessionctx/variable/session.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -603,7 +603,21 @@ func (s *SessionVars) SetSystemVar(name string, val string) error {
case TxnIsolationOneShot:
switch val {
case "SERIALIZABLE", "READ-UNCOMMITTED":
return ErrUnsupportedValueForVar.GenWithStackByArgs(name, val)
skipIsolationLevelCheck, err := GetSessionSystemVar(s, TiDBSkipIsolationLevelCheck)
Comment thread
jackysp marked this conversation as resolved.
Outdated
returnErr := ErrUnsupportedIsolationLevel.GenWithStackByArgs(val)
if err != nil {
returnErr = err
}
if !TiDBOptOn(skipIsolationLevelCheck) || err != nil {
return returnErr
}
//SET TRANSACTION ISOLATION LEVEL will affect two internal variables:
// 1. tx_isolation
// 2. transaction_isolation
// The following if condition is used to deduplicate two same warnings.
if name == "transaction_isolation" {
s.StmtCtx.AppendWarning(returnErr)
}
}
s.TxnIsolationLevelOneShot.State = 1
s.TxnIsolationLevelOneShot.Value = val
Expand Down
2 changes: 2 additions & 0 deletions sessionctx/variable/sysvar.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,7 @@ var (
ErrTruncatedWrongValue = terror.ClassVariable.New(CodeTruncatedWrongValue, mysql.MySQLErrName[mysql.ErrTruncatedWrongValue])
ErrMaxPreparedStmtCountReached = terror.ClassVariable.New(CodeMaxPreparedStmtCountReached, mysql.MySQLErrName[mysql.ErrMaxPreparedStmtCountReached])
ErrUnsupportedValueForVar = terror.ClassVariable.New(CodeUnknownStatusVar, "variable '%s' does not yet support value: %s")
ErrUnsupportedIsolationLevel = terror.ClassVariable.New(CodeUnknownStatusVar, "The isolation level '%s' is not supported. Set tidb_skip_isolation_level_check=1 to skip this error")
)

func init() {
Expand Down Expand Up @@ -673,6 +674,7 @@ var defaultSysVars = []*SysVar{
{ScopeSession, TiDBOptimizerSelectivityLevel, strconv.Itoa(DefTiDBOptimizerSelectivityLevel)},
{ScopeGlobal | ScopeSession, TiDBEnableWindowFunction, BoolToIntStr(DefEnableWindowFunction)},
{ScopeGlobal | ScopeSession, TiDBEnableFastAnalyze, BoolToIntStr(DefTiDBUseFastAnalyze)},
{ScopeGlobal | ScopeSession, TiDBSkipIsolationLevelCheck, BoolToIntStr(DefTiDBSkipIsolationLevelCheck)},
/* The following variable is defined as session scope but is actually server scope. */
{ScopeSession, TiDBGeneralLog, strconv.Itoa(DefTiDBGeneralLog)},
{ScopeSession, TiDBSlowLogThreshold, strconv.Itoa(logutil.DefaultSlowThreshold)},
Expand Down
5 changes: 5 additions & 0 deletions sessionctx/variable/tidb_vars.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -131,6 +131,10 @@ const (

// TiDBCheckMb4ValueInUTF8 is used to control whether to enable the check wrong utf8 value.
TiDBCheckMb4ValueInUTF8 = "tidb_check_mb4_value_in_utf8"

// tidb_skip_isolation_level_check is used to control whether to return error when set unsupported transaction
// isolation level.
TiDBSkipIsolationLevelCheck = "tidb_skip_isolation_level_check"
)

// TiDB system variable names that both in session and global scope.
Expand Down Expand Up @@ -295,6 +299,7 @@ const (
DefEnableWindowFunction = false
DefTiDBDDLSlowOprThreshold = 300
DefTiDBUseFastAnalyze = false
DefTiDBSkipIsolationLevelCheck = false
)

// Process global variables.
Expand Down
16 changes: 15 additions & 1 deletion sessionctx/variable/varsutil.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -429,7 +429,21 @@ func ValidateSetSystemVar(vars *SessionVars, name string, value string) (string,
}
switch upVal {
case "SERIALIZABLE", "READ-UNCOMMITTED":
return "", ErrUnsupportedValueForVar.GenWithStackByArgs(name, value)
skipIsolationLevelCheck, err := GetSessionSystemVar(vars, TiDBSkipIsolationLevelCheck)
returnErr := ErrUnsupportedIsolationLevel.GenWithStackByArgs(value)
if err != nil {
returnErr = err
}
if !TiDBOptOn(skipIsolationLevelCheck) || err != nil {
return "", returnErr
}
Comment thread
jackysp marked this conversation as resolved.
Outdated
//SET TRANSACTION ISOLATION LEVEL will affect two internal variables:
// 1. tx_isolation
// 2. transaction_isolation
// The following if condition is used to deduplicate two same warnings.
if name == "transaction_isolation" {
vars.StmtCtx.AppendWarning(returnErr)
}
}
return upVal, nil
case TiDBInitChunkSize:
Expand Down
⚡