-
Notifications
You must be signed in to change notification settings - Fork 5.8k
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
session: add a global/session variable 'tidb_disable_txn_auto_retry' #6872
session: add a global/session variable 'tidb_disable_txn_auto_retry' #6872
Conversation
@shenli @tiancaiamao @jackysp PTAL |
session/session_test.go
Outdated
tk1.Se.NewTxn() | ||
// session 2 update the value. | ||
tk2.MustExec("update no_retry set id = 4") | ||
// Non-autocommit update will retry, so it would not fail. |
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.
It should be Autocommit
.
session/session_test.go
Outdated
|
||
// session 1 starts a transaction early. | ||
// execute a select statement to . | ||
tk1.MustExec("select 1") |
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 add select 1
here?
@@ -198,6 +201,7 @@ const ( | |||
DefTiDBMemQuotaNestedLoopApply = 32 << 30 // 32GB. | |||
DefTiDBGeneralLog = 0 | |||
DefTiDBRetryLimit = 10 | |||
DefTiDBDisableTxnAutoRetry = false |
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.
I think it could be true as default.
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 should be false by default for now. Because many users rely on the auto-retry.
session/session.go
Outdated
@@ -346,6 +346,15 @@ func (s *session) doCommitWithRetry(ctx context.Context) error { | |||
err := s.doCommit(ctx) | |||
if err != nil { | |||
commitRetryLimit := s.sessionVars.RetryLimit | |||
if s.sessionVars.DisableTxnAutoRetry { |
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.
For the internal sessions, we need always enable the auto-retry.
// For explicit transactions, the statement count is more than 1. | ||
history := GetHistory(s) | ||
if history.Count() > 1 { | ||
commitRetryLimit = 0 |
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.
Can we return here.
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.
No, there are extra works to do below.
/rebuild |
/run-all-tests |
LGTM |
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-all-tests |
What have you changed? (mandatory)
What are the type of the changes (mandatory)?
How has this PR been tested (mandatory)?
Does this PR affect documentation (docs/docs-cn) update? (optional)