executor: support innodb_lock_wait_timeout for pessimistic transaction #13103

cfzjywxk · 2019-11-04T02:25:41Z

What problem does this PR solve?

make innodb_lock_wait_timeout work for pessimistic transactions in tidb

in MySQL, this is a system variable to set lock wait time for transactions,
The length of time in seconds an InnoDB transaction waits for a row lock before giving up. The default value is 50 seconds. A transaction that tries to access a row that is locked by another InnoDB transaction waits at most this many seconds for write access to the row before issuing the following error:
ERROR 1205 (HY000): Lock wait timeout exceeded; try restarting transaction
When a lock wait timeout occurs, the current statement is rolled back (not the entire transaction).

mysql-doc

What is changed and how it works?

make system variable innodb_lock_wait_timeout effective，the default value is same with mysql
check wait time when met with ErrLocked error from kv, and report lock wait timout error if wait time greater than lockWaitTime, the current statement will be rollbacked like mysql behaviors
innodb_lock_wait_timeout does affect only user sessions dmls

TODO:

refactor handlePessimiticError to always update for forUpdateTs
make lock wait action in mvcc_leveldb same with action in tikv.

Check List

Tests

Unit test

Code changes

Has exported variable/fields change

Side effects

Related changes

Need to cherry-pick to the release branch
Need to update the documentation

Release note

Write release note for bug-fix or new feature.

codecov · 2019-11-04T02:31:09Z

Codecov Report

Merging #13103 into master will not change coverage.
The diff coverage is n/a.

@@             Coverage Diff             @@
##             master     #13103   +/-   ##
===========================================
  Coverage   80.6801%   80.6801%           
===========================================
  Files           468        468           
  Lines        113153     113153           
===========================================
  Hits          91292      91292           
  Misses        15039      15039           
  Partials       6822       6822

coocood · 2019-11-04T02:33:13Z

session/pessimistic_test.go

+ tk3.MustExec("begin pessimistic")
+ start := time.Now()
+ _, err := tk3.Exec("select * from tk where c1 = 1 for update")
+ c.Check(time.Since(start), GreaterEqual, time.Duration(1*time.Second))


We also need to check the duration is not too long.

coocood · 2019-11-04T11:51:24Z

sessionctx/variable/session.go

@@ -447,6 +447,9 @@ type SessionVars struct {
 isolationReadEngines map[kv.StoreType]struct{}

 PlannerSelectBlockAsName []ast.HintTable
+
+ // Lock wait timeout for pessimistic transaction in milliseconds, `innodb_lock_wait_timeout` is in seconds


put is in seconds at the last is too easy to be misunderstood as LockWaitTimeout is in seconds.

jackysp · 2019-11-04T11:57:33Z

sessionctx/variable/session.go

@@ -447,6 +447,9 @@ type SessionVars struct {
 isolationReadEngines map[kv.StoreType]struct{}

 PlannerSelectBlockAsName []ast.HintTable
+
+ // Lock wait timeout for pessimistic transaction in milliseconds, `innodb_lock_wait_timeout` is in seconds


Suggested change

// Lock wait timeout for pessimistic transaction in milliseconds, `innodb_lock_wait_timeout` is in seconds

// LockWaitTimeout is used for pessimistic transaction in milliseconds, `innodb_lock_wait_timeout` is in seconds.

jackysp · 2019-11-04T11:58:41Z

executor/executor.go

@@ -815,7 +815,7 @@ func (e *SelectLockExec) Next(ctx context.Context, req *chunk.Chunk) error {
 }
 return nil
 }
- var lockWaitTime = kv.LockAlwaysWait
+ var lockWaitTime = e.ctx.GetSessionVars().LockWaitTimeout


Suggested change

var lockWaitTime = e.ctx.GetSessionVars().LockWaitTimeout

lockWaitTime := e.ctx.GetSessionVars().LockWaitTimeout

coocood · 2019-11-04T12:02:59Z

store/tikv/2pc.go

+ if !c.store.oracle.IsExpired(lock.TxnID, lock.TTL) {
+ return ErrLockAcquireFailAndNoWaitSet
+ }
+ } else if action.lockWaitTime == kv.LockAlwaysWait {


We don't need to handle LockAlwaysWait, innodb_wait_timeout is always positive.

@coocood some inner usages which calls doLockKeys (like RecoverIndexExec, addIndexWorker) will set this param LockAlwaysWait

jackysp · 2019-11-04T12:03:24Z

store/tikv/2pc.go

+ if !c.store.oracle.IsExpired(lock.TxnID, lock.TTL) {
+ return ErrLockAcquireFailAndNoWaitSet
+ }
+ } else if action.lockWaitTime == kv.LockAlwaysWait {


How about

Suggested change

} else if action.lockWaitTime == kv.LockAlwaysWait {

} else if action.lockWaitTime != kv.LockAlwaysWait {

then do something.

current we have 3 paths(negative, 0, positive value), I think it's more clear to make them like noodle style, and we just need to add/remove one more else if path if we want to modify the protocol

coocood · 2019-11-04T12:05:18Z

sessionctx/variable/session.go

@@ -812,6 +816,9 @@ func (s *SessionVars) SetSystemVar(name string, val string) error {
 case MaxExecutionTime:
 timeoutMS := tidbOptPositiveInt32(val, 0)
 s.MaxExecutionTime = uint64(timeoutMS)
+ case InnodbLockWaitTimeout:
+ lockWaitSec := tidbOptInt64(val, DefInnodbLockWaitTimeout)


We need to validate the range.

this is done in function ValidateSetSystemVar

case InnodbLockWaitTimeout: return checkUInt64SystemVar(name, value, 1, 1073741824, vars)

mysql> set global innodb_lock_wait_timeout = 0; Query OK, 0 rows affected, 1 warning (0.00 sec) mysql> show warnings; +---------+------+---------------------------------------------------------+ | Level | Code | Message | +---------+------+---------------------------------------------------------+ | Warning | 1292 | Truncated incorrect innodb_lock_wait_timeout value: '0' | +---------+------+---------------------------------------------------------+ 1 row in set (0.00 sec)

jackysp · 2019-11-04T12:05:20Z

session/pessimistic_test.go

+ tk3.MustExec("set innodb_lock_wait_timeout = 1")
+ tk3.MustQuery(`show variables like "innodb_lock_wait_timeout"`).Check(testkit.Rows("innodb_lock_wait_timeout 1"))
+
+ tk2.MustExec("set @@autocommit = 0")


why set autocommit here?

set this before next begin statement below

coocood · 2019-11-04T12:08:15Z

store/tikv/2pc.go

@@ -676,6 +676,7 @@ func (action actionPessimisticLock) handleSingleBatch(c *twoPhaseCommitter, bo *
 IsFirstLock: c.isFirstLock,
 WaitTimeout: action.lockWaitTime,
 }, pb.Context{Priority: c.priority, SyncLog: c.syncLog})
+ lockWaitStartTime := time.Now()
 for {


We need to update the req.WaitTimeout . so if the wait timeout is less than 3s，we can return sooner.

jackysp · 2019-11-04T12:32:32Z

store/tikv/2pc.go

- return ErrLockAcquireFailAndNoWaitSet
+ if lock.LockType == pb.Op_PessimisticLock {
+ if action.lockWaitTime == kv.LockNoWait {
+ // the pessimistic lock found could be lock left behind(timeout but not recycled yet)


It is better to improve the English of this comment.

jackysp · 2019-11-04T12:34:59Z

store/tikv/2pc.go

+ } else if action.lockWaitTime == kv.LockAlwaysWait {
+ // do nothing but keep wait
+ } else {
+ // user has set the `InnodbLockWaitTimeout`, check timeout


It is better to improve the English of this comment.

jackysp

LGTM

coocood · 2019-11-05T02:43:52Z

LGTM

cfzjywxk · 2019-11-05T03:04:44Z

@tiancaiamao @lysu PTAL

coocood · 2019-11-05T08:21:44Z

@youjiali1995 PTAL

youjiali1995

LGTM

coocood · 2019-11-05T09:21:25Z

/merge

sre-bot · 2019-11-05T09:23:42Z

/run-all-tests

sre-bot · 2019-11-05T09:27:52Z

cherry pick to release-3.0 failed

pingcap#13103)

sre-bot · 2020-04-07T10:24:44Z

It seems that, not for sure, we failed to cherry-pick this commit to release-3.0. Please comment '/run-cherry-picker' to try to trigger the cherry-picker if we did fail to cherry-pick this commit before. @cfzjywxk PTAL.

cfzjywxk added status/WIP sig/execution SIG execution sig/transaction SIG:Transaction labels Nov 4, 2019

cfzjywxk requested a review from coocood November 4, 2019 02:25

cfzjywxk force-pushed the wait_timeout_new_proto branch from 324fea2 to 022273d Compare November 4, 2019 02:30

coocood reviewed Nov 4, 2019

View reviewed changes

support lock wait timeout

5ac2253

cfzjywxk force-pushed the wait_timeout_new_proto branch from 022273d to 5ac2253 Compare November 4, 2019 11:21

cfzjywxk removed the status/WIP label Nov 4, 2019

cfzjywxk requested review from tiancaiamao, jackysp and lysu November 4, 2019 11:28

cfzjywxk added the needs-cherry-pick-3.0 label Nov 4, 2019

cfzjywxk requested a review from coocood November 4, 2019 11:29

coocood reviewed Nov 4, 2019

View reviewed changes

jackysp reviewed Nov 4, 2019

View reviewed changes

coocood reviewed Nov 4, 2019

View reviewed changes

jackysp reviewed Nov 4, 2019

View reviewed changes

coocood reviewed Nov 4, 2019

View reviewed changes

jackysp reviewed Nov 4, 2019

View reviewed changes

coocood reviewed Nov 4, 2019

View reviewed changes

jackysp reviewed Nov 4, 2019

View reviewed changes

refine rpc reuqest timeout param

a05099a

cfzjywxk requested review from jackysp and coocood November 4, 2019 12:59

comment

eb05a0f

jackysp reviewed Nov 5, 2019

View reviewed changes

jackysp added the status/LGT1 Indicates that a PR has LGTM 1. label Nov 5, 2019

cfzjywxk requested a review from youjiali1995 November 5, 2019 09:08

youjiali1995 reviewed Nov 5, 2019

View reviewed changes

sre-bot added the status/can-merge Indicates a PR has been approved by a committer. label Nov 5, 2019

Merge branch 'master' into wait_timeout_new_proto

d5947d6

coocood approved these changes Nov 5, 2019

View reviewed changes

sre-bot merged commit 5fd8b4d into pingcap:master Nov 5, 2019

cfzjywxk mentioned this pull request Nov 5, 2019

executor: support innodb_lock_wait_timeout for pessimistic transaction(#13103) #13165

Merged

XiaTianliang pushed a commit to XiaTianliang/tidb that referenced this pull request Dec 21, 2019

executor: support innodb_lock_wait_timeout for pessimistic transaction (

1609b2c

pingcap#13103)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

executor: support innodb_lock_wait_timeout for pessimistic transaction #13103

executor: support innodb_lock_wait_timeout for pessimistic transaction #13103

cfzjywxk commented Nov 4, 2019 •

edited

Loading

codecov bot commented Nov 4, 2019 •

edited

Loading

coocood Nov 4, 2019

coocood Nov 4, 2019

jackysp Nov 4, 2019

jackysp Nov 4, 2019

coocood Nov 4, 2019

cfzjywxk Nov 4, 2019

jackysp Nov 4, 2019

cfzjywxk Nov 4, 2019

coocood Nov 4, 2019

cfzjywxk Nov 4, 2019

jackysp Nov 4, 2019

cfzjywxk Nov 4, 2019

coocood Nov 4, 2019 •

edited

Loading

cfzjywxk Nov 4, 2019

jackysp Nov 4, 2019

jackysp Nov 4, 2019

jackysp left a comment

coocood commented Nov 5, 2019

cfzjywxk commented Nov 5, 2019

coocood commented Nov 5, 2019

youjiali1995 left a comment

coocood commented Nov 5, 2019

sre-bot commented Nov 5, 2019

sre-bot commented Nov 5, 2019

sre-bot commented Apr 7, 2020

	// Lock wait timeout for pessimistic transaction in milliseconds, `innodb_lock_wait_timeout` is in seconds
	// LockWaitTimeout is used for pessimistic transaction in milliseconds, `innodb_lock_wait_timeout` is in seconds.

	var lockWaitTime = e.ctx.GetSessionVars().LockWaitTimeout
	lockWaitTime := e.ctx.GetSessionVars().LockWaitTimeout

	} else if action.lockWaitTime == kv.LockAlwaysWait {
	} else if action.lockWaitTime != kv.LockAlwaysWait {

executor: support innodb_lock_wait_timeout for pessimistic transaction #13103

executor: support innodb_lock_wait_timeout for pessimistic transaction #13103

Conversation

cfzjywxk commented Nov 4, 2019 • edited Loading

What problem does this PR solve?

What is changed and how it works?

Check List

codecov bot commented Nov 4, 2019 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coocood Nov 4, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jackysp left a comment

Choose a reason for hiding this comment

coocood commented Nov 5, 2019

cfzjywxk commented Nov 5, 2019

coocood commented Nov 5, 2019

youjiali1995 left a comment

Choose a reason for hiding this comment

coocood commented Nov 5, 2019

sre-bot commented Nov 5, 2019

sre-bot commented Nov 5, 2019

sre-bot commented Apr 7, 2020

cfzjywxk commented Nov 4, 2019 •

edited

Loading

codecov bot commented Nov 4, 2019 •

edited

Loading

coocood Nov 4, 2019 •

edited

Loading