-
Notifications
You must be signed in to change notification settings - Fork 5.9k
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.
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
*: modify 'session transaction isolation level' to take effect once #6175
Conversation
'session transaction isolation level' is not the same with 'session session transaction isolation level', it should take effect only once to the next transaction.
session/session_fail_test.go
Outdated
|
||
// set transaction isolation level read committed take effect just one time. | ||
tk2.MustExec("update t set v = 43 where k = 1") | ||
tk1.MustQuery("select v from t where k = 1").Check(testkit.Rows("43")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test does not verify that the desired isolation level is set.
9e9d9dc
to
b5d25ac
Compare
PTAL @coocood |
@tiancaiamao |
|
LGTM |
sessionctx/variable/session.go
Outdated
@@ -448,6 +457,9 @@ func (s *SessionVars) deleteSystemVar(name string) error { | |||
// SetSystemVar sets the value of a system variable. | |||
func (s *SessionVars) SetSystemVar(name string, val string) error { | |||
switch name { | |||
case "tx_isolation_one_shot": |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we define a constant as tx_isolation_one_shot
?
executor/set.go
Outdated
@@ -150,6 +150,9 @@ func (e *SetExecutor) setSysVariable(name string, v *expression.VarAssignment) e | |||
return errors.Trace(err) | |||
} | |||
oldSnapshotTS := sessionVars.SnapshotTS | |||
if name == "tx_isolation_one_shot" && sessionVars.InTxn() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we add a test for it?
@@ -125,7 +125,13 @@ func (builder *RequestBuilder) SetKeepOrder(order bool) *RequestBuilder { | |||
} | |||
|
|||
func (builder *RequestBuilder) getIsolationLevel(sv *variable.SessionVars) kv.IsoLevel { | |||
isoLevel, _ := sv.GetSystemVar(variable.TxnIsolation) | |||
var isoLevel string | |||
if sv.TxnIsolationLevelOneShot.State == 2 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why does it need special handling?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
set statement still run into doCommitWithRetry, if I don't maintain a state for tx_isolation_one_shot, it will be reset immediately, in the set statement itself.
@zimulala
PTAL @zimulala |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
/run-all-tests |
/run-unit-test |
session transaction isolation level
is not the same withsession session transaction isolation level
, it should take effectonly once to the next transaction.
@coocood @zz-jason