transaction: lock unique key for delete operation (#19220) #20705

ti-srebot · 2020-10-29T04:09:39Z

cherry-pick #19220 to release-4.0

What problem does this PR solve?

Issue Number: close #18798, close #20535

Problem Summary:

What is changed and how it works?

What's Changed:

Lock index for delete operation, so that some transactions can be serialized.

Related changes

Need to cherry-pick to the release branch

Check List

Tests

Unit test

Release note

No release note

Signed-off-by: ti-srebot <ti-srebot@pingcap.com>

ti-srebot · 2020-10-29T04:09:40Z

/run-all-tests

Signed-off-by: you06 <you1474600@gmail.com>

you06 · 2020-10-29T11:15:20Z

/run-all-tests

Signed-off-by: you06 <you1474600@gmail.com>

you06 · 2020-10-29T12:22:13Z

/run-all-tests

Signed-off-by: you06 <you1474600@gmail.com>

you06 · 2020-10-30T05:47:45Z

/run-all-tests

bobotu

LGTM

bobotu · 2020-10-30T08:47:54Z

session/txn.go

@@ -428,7 +434,7 @@ func (st *TxnState) KeysNeedToLock() ([]kv.Key, error) {
 	}
 	keys := make([]kv.Key, 0, st.stmtBufLen())
 	if err := kv.WalkMemBuffer(st.stmtBuf, func(k kv.Key, v []byte) error {
-		if !keyNeedToLock(k, v) {
+		if !st.stmtBuf.IfKeyNeedLock(k) && !keyNeedToLock(k, v) {


stmtBuf shared the same needLockKey map with txnBuf. After RollbackStmt the new keys added into stmtBuf.needLockKey will not be removed.

But it will not cause correctness issue:

If the key still existed after retry, it will be locked;

Otherwise, it will not be collected at here.

Maybe it will lock more keys than need in some scenarios? But this way seems much simpler than implementing a rollbackable needLockKey.

Is MemBuffer.Discard called when rollback? Seems we can move needLockKey into Sandbox, then we can rollback it.

Yep. I have already done something simliar as a part of #18372.

tidb/kv/memdb.go

Lines 32 to 71 in 761a961

type KeyFlags uint8

const (

flagPresumeKeyNotExists KeyFlags = 1 << iota

// flagTouched is a internal flag to help `Mark` operation.

// The default value of `KeyFlags` is 0 and the flags returned by any operation will set `flagTouched`.

// When merge two flags, if the newer one haven't set the `flagTouched`, the newer value will be ignored.

flagTouched KeyFlags = 0x80

)

// MarkPresumeKeyNotExists marks the existence of the associated key is checked lazily.

func (m KeyFlags) MarkPresumeKeyNotExists() KeyFlags {

return (m | flagPresumeKeyNotExists) | flagTouched

}

// UnmarkPresumeKeyNotExists reverts MarkPresumeKeyNotExists.

func (m KeyFlags) UnmarkPresumeKeyNotExists() KeyFlags {

return (m & ^flagPresumeKeyNotExists) | flagTouched

}

// HasPresumeKeyNotExists retruns whether the associated key use lazy check.

func (m KeyFlags) HasPresumeKeyNotExists() bool {

return m&flagPresumeKeyNotExists != 0

}

func (m KeyFlags) isTouched() bool {

return m&flagTouched != 0

}

// Merge used to merge two KeyFlags.

func (m KeyFlags) Merge(old KeyFlags) KeyFlags {

// Only consider flagPresumeKeyNotExists in merge operation for now.

// We should always respect to the older setting,

// the delete operation will overwrite flags in root tree instead of invoke merge.

if old.isTouched() {

return old

}

return m

}

tidb/kv/sandbox.go

Line 109 in 761a961

x.flags = flags

I'll try it following your implementation in the master.

Better not to share objects in stmtbuf with txnBuf, we need make sure no unwanted keys get locked, or there will be other incompatbile behaviours issues.

session/txn.go

cfzjywxk · 2020-10-31T05:41:44Z

session/tidb_test.go

@@ -188,7 +188,7 @@ func (s *testMainSuite) TestKeysNeedLock(c *C) {
 		need bool
 	}{
 		{rowKey, rowVal, true},
-		{rowKey, deleteVal, true},
+		{rowKey, deleteVal, false},


Old deleted row keys need to be locked.

cfzjywxk · 2020-10-31T05:57:23Z

store/tikv/txn.go

@@ -214,6 +214,15 @@ func (txn *tikvTxn) Delete(k kv.Key) error {
 	return txn.us.Delete(k)
 }

+func (txn *tikvTxn) DeleteWithNeedLock(k kv.Key) error {


Seems we will not use this path?

We'd better write this method explicitly, otherwise, the txn.dirty may incorrect.

cfzjywxk · 2020-10-31T05:59:06Z

session/txn.go

@@ -428,7 +434,7 @@ func (st *TxnState) KeysNeedToLock() ([]kv.Key, error) {
 	}
 	keys := make([]kv.Key, 0, st.stmtBufLen())
 	if err := kv.WalkMemBuffer(st.stmtBuf, func(k kv.Key, v []byte) error {
-		if !keyNeedToLock(k, v) {
+		if !st.stmtBuf.IfKeyNeedLock(k) && !keyNeedToLock(k, v) {


Better not to share objects in stmtbuf with txnBuf, we need make sure no unwanted keys get locked, or there will be other incompatbile behaviours issues.

cfzjywxk · 2020-10-31T06:01:43Z

executor/delete_test.go

+				tk2.MustExec(t.tk2Stmt)
+				doneCh <- struct{}{}
+			}()
+			time.Sleep(20 * time.Millisecond)


It's a bit short, may not be stable.

executor/delete_test.go

Signed-off-by: you06 <you1474600@gmail.com>

bobotu · 2020-10-31T06:38:28Z

kv/memdb_buffer.go

-func (m *memDbBuffer) IfKeyNeedLock(k Key) bool {
-	_, ok := m.needLockKey[k.String()]
-	return ok
+	//m.needLockKey[k.String()] = struct{}{}


Delete this line.

kv/memdb/memdb.go

Signed-off-by: you06 <you1474600@gmail.com>

bobotu · 2020-10-31T07:10:53Z

LGTM

you06 · 2020-10-31T07:14:31Z

/run-all-tests

cfzjywxk · 2020-10-31T07:14:44Z

LGTM

you06 · 2020-10-31T07:52:35Z

kv/memdb/memdb.go

@@ -475,6 +508,7 @@ func (ms *mergeState) calculateRecomputeHeight(key []byte, sb *Sandbox) int {
 }

 type nodeHeader struct {
+	flags  KeyFlags
 	height uint16


If we can reduce height to uint8, it'll be more memory friendly.

tidb/kv/sandbox.go

Lines 503 to 508 in 761a961

type nodeHeader struct {

height uint8

flags KeyFlags

keyLen uint16

valLen uint32

}

Afraid of the overflow risk.

Never overflow. The upper bound is

tidb/kv/sandbox.go

Line 25 in 761a961

maxHeight = 16

Signed-off-by: you06 <you1474600@gmail.com>

you06 · 2020-10-31T08:26:20Z

/run-all-tests

you06 · 2020-11-02T02:43:15Z

@cfzjywxk I think this PR can be merged now.

coocood

LGTM

you06 · 2020-11-03T09:18:32Z

What's wrong with the action :(

cfzjywxk · 2020-11-03T09:37:47Z

/merge

ti-srebot · 2020-11-03T09:37:53Z

Your auto merge job has been accepted, waiting for:

20356

ti-srebot · 2020-11-03T09:58:48Z

/run-all-tests

cherry pick pingcap#19220 to release-4.0

1520440

Signed-off-by: ti-srebot <ti-srebot@pingcap.com>

ti-srebot mentioned this pull request Oct 29, 2020

transaction: lock unique key for delete operation #19220

Merged

ti-srebot added sig/sql-infra SIG: SQL Infra sig/execution SIG execution sig/transaction SIG:Transaction type/4.0-cherry-pick labels Oct 29, 2020

ti-srebot requested review from bobotu, cfzjywxk, coocood, jackysp and wshwsh12 October 29, 2020 04:09

ti-srebot added this to the v4.0.8 milestone Oct 29, 2020

ti-srebot assigned you06 Oct 29, 2020

you06 added 5 commits October 29, 2020 15:19

resolve conflict

bceefc6

Signed-off-by: you06 <you1474600@gmail.com>

resolve conflict

8975c8b

Signed-off-by: you06 <you1474600@gmail.com>

fix bug

47c44da

Signed-off-by: you06 <you1474600@gmail.com>

fix bug

ca88911

Signed-off-by: you06 <you1474600@gmail.com>

fix bug, overwrite functions

dd0f536

Signed-off-by: you06 <you1474600@gmail.com>

fix bug

8eec5a8

Signed-off-by: you06 <you1474600@gmail.com>

you06 added 2 commits October 30, 2020 11:44

remove SetBuffer

0e8a5df

Signed-off-by: you06 <you1474600@gmail.com>

remove invalid session variable

2e8ac0c

Signed-off-by: you06 <you1474600@gmail.com>

bobotu reviewed Oct 30, 2020

View reviewed changes

ti-srebot added the status/LGT1 Indicates that a PR has LGTM 1. label Oct 30, 2020

coocood reviewed Oct 31, 2020

View reviewed changes

session/txn.go Outdated Show resolved Hide resolved

cfzjywxk reviewed Oct 31, 2020

View reviewed changes

another implementation by setting flags

dea1da5

Signed-off-by: you06 <you1474600@gmail.com>

bobotu reviewed Oct 31, 2020

View reviewed changes

cfzjywxk modified the milestones: v4.0.8, v4.0.9 Oct 31, 2020

you06 added 3 commits October 31, 2020 14:54

Merge branch 'release-4.0' into release-4.0-de4612597c70

22db2ef

remove unused code & update err template

3fe1c24

Signed-off-by: you06 <you1474600@gmail.com>

tidy mod

cc866a9

Signed-off-by: you06 <you1474600@gmail.com>

ti-srebot added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Oct 31, 2020

you06 reviewed Oct 31, 2020

View reviewed changes

reduce memory usage and make it alignment

c6e98a2

Signed-off-by: you06 <you1474600@gmail.com>

coocood approved these changes Nov 2, 2020

View reviewed changes

ti-srebot added status/LGT3 The PR has already had 3 LGTM. and removed status/LGT2 Indicates that a PR has LGTM 2. labels Nov 2, 2020

wshwsh12 removed request for jackysp and wshwsh12 November 3, 2020 03:55

Merge branch 'release-4.0' into release-4.0-de4612597c70

4a3ed13

wjhuang2016 removed the sig/sql-infra SIG: SQL Infra label Nov 3, 2020

github-actions bot added the sig/sql-infra SIG: SQL Infra label Nov 3, 2020

ti-srebot added the status/can-merge Indicates a PR has been approved by a committer. label Nov 3, 2020

ti-srebot merged commit b77153d into pingcap:release-4.0 Nov 3, 2020

you06 removed the sig/sql-infra SIG: SQL Infra label Nov 3, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

transaction: lock unique key for delete operation (#19220) #20705

transaction: lock unique key for delete operation (#19220) #20705

ti-srebot commented Oct 29, 2020

ti-srebot commented Oct 29, 2020

you06 commented Oct 29, 2020

you06 commented Oct 29, 2020

you06 commented Oct 30, 2020

bobotu left a comment

bobotu Oct 30, 2020 •

edited

Loading

you06 Oct 30, 2020

bobotu Oct 30, 2020

you06 Oct 30, 2020

cfzjywxk Oct 31, 2020 •

edited

Loading

cfzjywxk Oct 31, 2020

cfzjywxk Oct 31, 2020

bobotu Oct 31, 2020

cfzjywxk Oct 31, 2020 •

edited

Loading

cfzjywxk Oct 31, 2020

bobotu Oct 31, 2020

bobotu commented Oct 31, 2020

you06 commented Oct 31, 2020

cfzjywxk commented Oct 31, 2020

you06 Oct 31, 2020

bobotu Oct 31, 2020

you06 Oct 31, 2020

bobotu Oct 31, 2020

you06 commented Oct 31, 2020

you06 commented Nov 2, 2020

coocood left a comment

you06 commented Nov 3, 2020

cfzjywxk commented Nov 3, 2020

ti-srebot commented Nov 3, 2020

ti-srebot commented Nov 3, 2020

	type KeyFlags uint8

	const (
	flagPresumeKeyNotExists KeyFlags = 1 << iota

	// flagTouched is a internal flag to help `Mark` operation.
	// The default value of `KeyFlags` is 0 and the flags returned by any operation will set `flagTouched`.
	// When merge two flags, if the newer one haven't set the `flagTouched`, the newer value will be ignored.
	flagTouched KeyFlags = 0x80
	)

	// MarkPresumeKeyNotExists marks the existence of the associated key is checked lazily.
	func (m KeyFlags) MarkPresumeKeyNotExists() KeyFlags {
	return (m \| flagPresumeKeyNotExists) \| flagTouched
	}

	// UnmarkPresumeKeyNotExists reverts MarkPresumeKeyNotExists.
	func (m KeyFlags) UnmarkPresumeKeyNotExists() KeyFlags {
	return (m & ^flagPresumeKeyNotExists) \| flagTouched
	}

	// HasPresumeKeyNotExists retruns whether the associated key use lazy check.
	func (m KeyFlags) HasPresumeKeyNotExists() bool {
	return m&flagPresumeKeyNotExists != 0
	}

	func (m KeyFlags) isTouched() bool {
	return m&flagTouched != 0
	}

	// Merge used to merge two KeyFlags.
	func (m KeyFlags) Merge(old KeyFlags) KeyFlags {
	// Only consider flagPresumeKeyNotExists in merge operation for now.
	// We should always respect to the older setting,
	// the delete operation will overwrite flags in root tree instead of invoke merge.
	if old.isTouched() {
	return old
	}
	return m
	}

	type nodeHeader struct {
	height uint8
	flags KeyFlags
	keyLen uint16
	valLen uint32
	}

transaction: lock unique key for delete operation (#19220) #20705

transaction: lock unique key for delete operation (#19220) #20705

Conversation

ti-srebot commented Oct 29, 2020

What problem does this PR solve?

What is changed and how it works?

Related changes

Check List

Release note

ti-srebot commented Oct 29, 2020

you06 commented Oct 29, 2020

you06 commented Oct 29, 2020

you06 commented Oct 30, 2020

bobotu left a comment

Choose a reason for hiding this comment

bobotu Oct 30, 2020 • 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

cfzjywxk Oct 31, 2020 • 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

cfzjywxk Oct 31, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bobotu commented Oct 31, 2020

you06 commented Oct 31, 2020

cfzjywxk commented Oct 31, 2020

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

you06 commented Oct 31, 2020

you06 commented Nov 2, 2020

coocood left a comment

Choose a reason for hiding this comment

you06 commented Nov 3, 2020

cfzjywxk commented Nov 3, 2020

ti-srebot commented Nov 3, 2020

ti-srebot commented Nov 3, 2020

bobotu Oct 30, 2020 •

edited

Loading

cfzjywxk Oct 31, 2020 •

edited

Loading

cfzjywxk Oct 31, 2020 •

edited

Loading