executor: support select for update no wait #12775

cfzjywxk · 2019-10-17T02:28:29Z

What problem does this PR solve?

support select for update nowait
to implement

# Session 1:

mysql> CREATE TABLE t (i INT, PRIMARY KEY (i)) ENGINE = InnoDB;

mysql> INSERT INTO t (i) VALUES(1),(2),(3);

mysql> START TRANSACTION;

mysql> SELECT * FROM t WHERE i = 2 FOR UPDATE;
+---+
| i |
+---+
| 2 |
+---+

# Session 2:

mysql> START TRANSACTION;

mysql> SELECT * FROM t WHERE i = 2 FOR UPDATE NOWAIT;
ERROR 3572 (HY000): Statement aborted because lock(s) could not be acquired immediately and NOWAIT is set.

What is changed and how it works?

parser and error code here parser: add nowait keyword, add for update no wait grammar, and related error… parser#582
kvproto change add LockType field into proto LockInfo kvproto#472
change the doLockKeys interface add one new parameter lockNoWait
report error immediately when pessimistic lock report error ErrLocked(key locked by others)
and do no more resolveLocks operations

need to discuss: how to process forUpdateTS

for pessimistic txn
in same txn, user tries

session1:
select for update key1 fail(ErrLock or max execution time or sth else.  lock key1 not get)
(the error is not "deadLockErr", rollBack pessmisitc key async)

select for update key1 again(this time succ, key locked like in "nowait" situation)
(the async rollback rollbacked the lock acquired in last step, since the forUpdateTs will not be updated
in "select for update" path)

session2:
after session1 step2
user tries to select for update key1,  lock succ(should fail)

do we need to make kvproto
bool nowait -> int waitSec to make it more flexible to support possible [wait n | nowait]

Check List

Tests

Unit test

Code changes

Has interface methods change

Side effects

Related changes

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

Release note

executor/executor.go

lysu · 2019-10-17T08:05:21Z

store/tikv/2pc.go

+			// Check lock conflict error for nowait, if nowait set and key locked by others
+			// report error immediately and do no resolve locks any more
+			if c.lockNoWait {
+				if keyErr.GetLocked() != nil {


can this lock be a committed or rollbacked lock in pessimistic txn and need check lock state? I'm not familiar with this :D

executor/adapter.go

sessionctx/context.go

store/mockstore/mocktikv/mvcc_leveldb.go

config/config.go

cfzjywxk · 2019-10-18T02:56:19Z

executor/adapter.go

+		//         the async rollback operation rollbacked the lock just acquired
+		newForUpdateTS, tsErr := a.Ctx.GetStore().GetOracle().GetTimestamp(ctx)
+		if tsErr != nil {
+			return nil, tsErr


if fail happened here, UpdateTS will not be updated, and the previous async rollback is possiblely ongoing,
still risk that the later select for update rollbacked ? @coocood

This error rarely happens, so even if this happened, the async rollback causes the transaction to fail, it is acceptable.

@tiancaiamao @coocood @jackysp
I'll add and use getWithRetry, but I think here still risk with nowait. User get this statement result error, next statement pessimistic lock getForUpdateTs not updated(select for update, update, delete operations), ths "async rollback" risk still exists, how should we process the error here?
Or maybe we make rollback sync for "nowait" locked error?

sync rollback may still send duplicated requests later.

executor/executor.go

store/tikv/error.go

executor/adapter.go

store/tikv/2pc.go

go.mod

session/pessimistic_test.go

tiancaiamao · 2019-10-18T08:45:07Z

store/tikv/2pc.go

@@ -112,6 +112,8 @@ type twoPhaseCommitter struct {
 	regionTxnSize map[uint64]int
 	// Used by pessimistic transaction and large transaction.
 	ttlManager
+	// Used for select for update in ms, 0 means always wait, 1 means nowait
+	lockWaitTime uint64


Why should lockWaitTime be a struct field in twoPhaseCommitter?

begin; select xxx for update no wait select yyy for update // The committer is still there but `lockWaitTime` change? commit;

How about pass lockWaitTime as an argument?

pessimisticLockSingleBatch and other proc functions from getProcFuncByType have the same declaration, seems need more change to pass it as a variable into NewRequest call

I did a refactoring and twoPhaseCommitAction can carry more information now. You can see how actionPessimisticLock is done on the master branch.

Never mind, as we can also merge the PR and refactor later.

Roger, I've learned the action refactoration when resolving conflicts, this is refactored passing time param as parameter not member of twoPhaseCommitter

cfzjywxk · 2019-10-25T08:10:25Z

still has some unstable unit tests

[2019-10-25T08:06:02.802Z] ----------------------------------------------------------------------
[2019-10-25T08:06:02.802Z] FAIL: ddl_test.go:442: testSuite6.TestRenameTable
[2019-10-25T08:06:02.802Z] 
[2019-10-25T08:06:02.802Z] ddl_test.go:495:
[2019-10-25T08:06:02.802Z]     result.Check(testkit.Rows("1", "100000", "100001"))
[2019-10-25T08:06:02.802Z] /home/jenkins/agent/workspace/tidb_ghpr_unit_test/go/src/github.com/pingcap/tidb/util/testkit/testkit.go:62:
[2019-10-25T08:06:02.802Z]     res.c.Assert(resBuff.String(), check.Equals, needBuff.String(), res.comment)
[2019-10-25T08:06:02.802Z] ... obtained string = "" +
[2019-10-25T08:06:02.802Z] ...     "[1]\n" +
[2019-10-25T08:06:02.802Z] ...     "[100000]\n" +
[2019-10-25T08:06:02.802Z] ...     "[2000001]\n"
[2019-10-25T08:06:02.802Z] ... expected string = "" +
[2019-10-25T08:06:02.802Z] ...     "[1]\n" +
[2019-10-25T08:06:02.802Z] ...     "[100000]\n" +
[2019-10-25T08:06:02.802Z] ...     "[100001]\n"
[2019-10-25T08:06:02.802Z] ... sql:select * from rename2.t1, args:[]
[2019-10-25T08:06:02.802Z] 
[2019-10-25T08:06:02.802Z] ddl_test.go:495:
[2019-10-25T08:06:02.802Z]     result.Check(testkit.Rows("1", "100000", "100001"))
[2019-10-25T08:06:02.802Z] ddl_test.go:445:
[2019-10-25T08:06:02.802Z]     c.Assert(failpoint.Disable("github.com/pingcap/tidb/meta/autoid/mockAutoIDChange"), IsNil)
[2019-10-25T08:06:02.802Z] ... value *errors.errorString = &errors.errorString{s:"failpoint: failpoint is disabled"} ("failpoint: failpoint is disabled")
[2019-10-25T08:06:02.802Z]

cfzjywxk · 2019-10-25T08:10:38Z

@jackysp @tiancaiamao PTAL

cfzjywxk · 2019-10-25T08:11:34Z

/run-all-tests

tiancaiamao

LGTM

coocood · 2019-10-25T08:28:43Z

/release

crazycs520 · 2019-10-25T08:55:25Z

/run-all-tests

cfzjywxk · 2019-10-28T02:31:50Z

@jackysp PTAL

jackysp

LGTM
Should we wait for tikv/tikv#5680 to merge?

youjiali1995 · 2019-11-01T03:49:19Z

/release

cfzjywxk · 2019-11-01T04:49:20Z

/run-all-tests

cfzjywxk · 2019-11-01T04:53:00Z

/release

This reverts commit 4b3dfce.

cfzjywxk · 2019-11-04T08:31:53Z

LGTM
Should we wait for tikv/tikv#5680 to merge?

tikv pull requests merged, can we merge this now ? @coocood @jackysp

coocood

LGTM

coocood · 2019-11-04T08:57:40Z

/merge

sre-bot · 2019-11-04T08:58:09Z

/run-all-tests

support select for update no wait

f242af2

cfzjywxk added sig/execution SIG execution sig/transaction SIG:Transaction labels Oct 17, 2019

cfzjywxk requested review from lysu, coocood, tiancaiamao, jackysp and youjiali1995 October 17, 2019 02:28

coocood reviewed Oct 17, 2019

View reviewed changes

executor/executor.go Outdated Show resolved Hide resolved

lysu reviewed Oct 17, 2019

View reviewed changes

refactor, handle select for update error will update "updateTS"

9e8d6cf

tiancaiamao reviewed Oct 17, 2019

View reviewed changes

executor/adapter.go Show resolved Hide resolved

tiancaiamao reviewed Oct 17, 2019

View reviewed changes

sessionctx/context.go Outdated Show resolved Hide resolved

tiancaiamao reviewed Oct 17, 2019

View reviewed changes

store/mockstore/mocktikv/mvcc_leveldb.go Outdated Show resolved Hide resolved

coocood reviewed Oct 17, 2019

View reviewed changes

config/config.go Outdated Show resolved Hide resolved

cfzjywxk added 2 commits October 17, 2019 20:28

move const1

70dc1eb

add LockType in LockInfo

d2bc394

cfzjywxk commented Oct 18, 2019

View reviewed changes

lockType use kvrpc.Op

c103a77

cfzjywxk requested review from tiancaiamao, coocood and lysu October 18, 2019 03:33

coocood reviewed Oct 18, 2019

View reviewed changes

executor/executor.go Outdated Show resolved Hide resolved

jackysp reviewed Oct 18, 2019

View reviewed changes

store/tikv/error.go Outdated Show resolved Hide resolved

jackysp reviewed Oct 18, 2019

View reviewed changes

executor/adapter.go Outdated Show resolved Hide resolved

cfzjywxk commented Oct 18, 2019

View reviewed changes

store/tikv/2pc.go Outdated Show resolved Hide resolved

tiancaiamao reviewed Oct 18, 2019

View reviewed changes

go.mod Outdated Show resolved Hide resolved

tiancaiamao reviewed Oct 18, 2019

View reviewed changes

youjiali1995 mentioned this pull request Oct 18, 2019

pessimistic-txn: support lock wait timeout tikv/tikv#5680

Merged

gomod, refactor some logic

b69efa6

Merge branch 'master' into nowait

4565b05

tiancaiamao reviewed Oct 25, 2019

View reviewed changes

Merge branch 'master' into nowait

7f73591

jackysp reviewed Oct 28, 2019

View reviewed changes

cfzjywxk added 2 commits November 1, 2019 12:43

Merge branch 'master' into nowait

99bada6

format

afa7bea

cfzjywxk mentioned this pull request Nov 4, 2019

executor: support innodb_lock_wait_timeout for pessimistic transaction #13103

Merged

change nowait signal to int64

4b3dfce

jackysp added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Nov 4, 2019

Revert "change nowait signal to int64"

edcaf66

This reverts commit 4b3dfce.

cfzjywxk added 2 commits November 4, 2019 16:35

change nowait signal to int64

16b26c4

Merge branch 'master' into nowait

cd7151a

coocood approved these changes Nov 4, 2019

View reviewed changes

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

sre-bot merged commit f12403e into pingcap:master Nov 4, 2019

cfzjywxk mentioned this pull request Nov 6, 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 select for update no wait (pingcap#12775)

00cd539

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

executor: support select for update no wait #12775

executor: support select for update no wait #12775

cfzjywxk commented Oct 17, 2019 •

edited

Loading

lysu Oct 17, 2019

cfzjywxk Oct 18, 2019

coocood Oct 18, 2019

cfzjywxk Oct 18, 2019 •

edited

Loading

coocood Oct 18, 2019

tiancaiamao Oct 18, 2019

cfzjywxk Oct 18, 2019

tiancaiamao Oct 25, 2019

cfzjywxk Oct 25, 2019 •

edited

Loading

cfzjywxk commented Oct 25, 2019 •

edited

Loading

cfzjywxk commented Oct 25, 2019

cfzjywxk commented Oct 25, 2019

tiancaiamao left a comment

coocood commented Oct 25, 2019

crazycs520 commented Oct 25, 2019

cfzjywxk commented Oct 28, 2019

jackysp left a comment

youjiali1995 commented Nov 1, 2019

cfzjywxk commented Nov 1, 2019

cfzjywxk commented Nov 1, 2019

cfzjywxk commented Nov 4, 2019

coocood left a comment

coocood commented Nov 4, 2019

sre-bot commented Nov 4, 2019

executor: support select for update no wait #12775

executor: support select for update no wait #12775

Conversation

cfzjywxk commented Oct 17, 2019 • edited Loading

What problem does this PR solve?

What is changed and how it works?

Check List

lysu Oct 17, 2019

Choose a reason for hiding this comment

cfzjywxk Oct 18, 2019

Choose a reason for hiding this comment

coocood Oct 18, 2019

Choose a reason for hiding this comment

cfzjywxk Oct 18, 2019 • edited Loading

Choose a reason for hiding this comment

coocood Oct 18, 2019

Choose a reason for hiding this comment

tiancaiamao Oct 18, 2019

Choose a reason for hiding this comment

cfzjywxk Oct 18, 2019

Choose a reason for hiding this comment

tiancaiamao Oct 25, 2019

Choose a reason for hiding this comment

cfzjywxk Oct 25, 2019 • edited Loading

Choose a reason for hiding this comment

cfzjywxk commented Oct 25, 2019 • edited Loading

cfzjywxk commented Oct 25, 2019

cfzjywxk commented Oct 25, 2019

tiancaiamao left a comment

Choose a reason for hiding this comment

coocood commented Oct 25, 2019

crazycs520 commented Oct 25, 2019

cfzjywxk commented Oct 28, 2019

jackysp left a comment

Choose a reason for hiding this comment

youjiali1995 commented Nov 1, 2019

cfzjywxk commented Nov 1, 2019

cfzjywxk commented Nov 1, 2019

cfzjywxk commented Nov 4, 2019

coocood left a comment

Choose a reason for hiding this comment

coocood commented Nov 4, 2019

sre-bot commented Nov 4, 2019

cfzjywxk commented Oct 17, 2019 •

edited

Loading

cfzjywxk Oct 18, 2019 •

edited

Loading

cfzjywxk Oct 25, 2019 •

edited

Loading

cfzjywxk commented Oct 25, 2019 •

edited

Loading