tikv/txn: support local latch in transaction #6418

AndreMouche · 2018-04-28T01:53:23Z

coocood · 2018-04-28T02:28:33Z

store/tikv/txn.go

+		if err == nil {
+			commitTS = committer.commitTS
+		}
+		txn.store.txnLatches.UnLock(lock, commitTS)


The commitTS is not used in UnLock.

Can we put commitTS into lock?

coocood · 2018-04-28T04:59:00Z

store/tikv/latch/scheduler.go

@@ -85,6 +85,7 @@ func (scheduler *LatchesScheduler) Lock(startTS uint64, keys [][]byte) *Lock {

 // UnLock unlocks a lock with commitTS.
 func (scheduler *LatchesScheduler) UnLock(lock *Lock, commitTS uint64) {
+	lock.commitTS = commitTS


We can remove the argument, and set commitTS before UnLock.

coocood · 2018-04-28T05:10:23Z

store/tikv/kv.go

@@ -165,7 +168,7 @@ func (s *tikvStore) CheckVisibility(startTime uint64) error {
 	return nil
 }

-func newTikvStore(uuid string, pdClient pd.Client, spkv SafePointKV, client Client, enableGC bool) (*tikvStore, error) {
+func newTikvStore(uuid string, pdClient pd.Client, spkv SafePointKV, client Client, enableGC, enableTxnLocalLatches bool) (*tikvStore, error) {


There are too many arguments to new TiKVStore.
Can we remove enableGC and enableTxnLocalLatches, and set them in a method when we want the non-default value?

coocood · 2018-04-28T05:48:19Z

@AndreMouche
We can enable the latch in unit tests.

coocood · 2018-04-28T07:18:04Z

@lamxTyler PTAL

alivxxx · 2018-04-28T07:45:20Z

sessionctx/context.go

+	return context.WithValue(ctx, Retryable, retryAble)
+}
+
+// GetRetryable returns the value of GetRetryable from the ctx.


value of retryable.

alivxxx · 2018-04-28T07:45:29Z

sessionctx/context.go

+
+// GetRetryable returns the value of GetRetryable from the ctx.
+func GetRetryable(ctx context.Context) bool {
+	var retryAble bool


I think there is no need to capitalize the a.

alivxxx · 2018-04-28T07:47:39Z

store/tikv/2pc.go

+	err := c.execute(ctx)
+	if err != nil {
+		c.writeFinishBinlog(binlog.BinlogType_Rollback, 0)
+


redundant blank line.

alivxxx · 2018-04-28T08:01:32Z

store/tikv/txn.go

+	// for transactions not retryable, commit directly.
+	if !sessionctx.GetRetryable(ctx) {
+		err = committer.executeAndWriteFinishBinlog(ctx)
+		txn.store.txnLatches.RefreshCommitTS(committer.keys, committer.startTS)


Why use the startTS to refresh? Can we refresh if an error occurred when commit?

tiancaiamao · 2018-04-28T09:12:25Z

store/tikv/txn.go

@@ -185,20 +184,41 @@ func (txn *tikvTxn) Commit(ctx context.Context) error {
 		connID = val.(uint64)
 	}
 	committer, err := newTwoPhaseCommitter(txn, connID)
-	if err != nil {
+	if err != nil || committer == nil {


When will commiter == nil ? and if it's really nil, the error trace would be oddly.

When no row needs to update in this transaction.

tiancaiamao · 2018-04-28T09:17:51Z

sessionctx/context.go

+const Retryable contextKey = "Retryable"
+
+// SetCommitCtx sets the variables for context before commit a transaction.
+func SetCommitCtx(ctx context.Context, sessCtx Context) context.Context {


Pass such important information through context.Context is not reliable.
If the call just pass a context.Background, those information will loss.

Any suggestion?

tiancaiamao · 2018-05-02T01:58:01Z

/run-all-tests

disksing · 2018-05-02T02:09:44Z

LGTM.

AndreMouche · 2018-05-02T03:15:49Z

/run-all-tests

AndreMouche · 2018-05-02T07:30:52Z

/run-all-tests

jackysp · 2018-05-02T10:49:32Z

/run-unit-test

AndreMouche · 2018-05-03T02:09:58Z

/run-all-tests

tiancaiamao

LGTM

tiancaiamao · 2018-05-03T07:21:05Z

kv/txn.go

@@ -54,7 +60,7 @@ func RunInNewTxn(store Storage, retryable bool, f func(txn Transaction) error) e
 			return errors.Trace(err)
 		}

-		err = txn.Commit(context.Background())
+		err = txn.Commit(context.WithValue(context.Background(), Retryable, retryable))


I really dislike passing those value through context, even txn.SetOption(retryable) is better than this.
If you insist, I'll refactor the code here after it's merged.

I've tried your suggestion and it's a little complex. I think to refactor the code in another PR is
ok for me.

tiancaiamao · 2018-05-03T07:27:41Z

store/tikv/latch/scheduler.go

@@ -84,7 +84,7 @@ func (scheduler *LatchesScheduler) Lock(startTS uint64, keys [][]byte) *Lock {
 }

 // UnLock unlocks a lock with commitTS.
-func (scheduler *LatchesScheduler) UnLock(lock *Lock, commitTS uint64) {
+func (scheduler *LatchesScheduler) UnLock(lock *Lock) {


IMHO, this API design is worse than before, because the user may forget to call lock.SetCommitTS after UnLock, while previous API make a stronger contract.

Both are ok for me. It's suggested by @coocood , maybe I could update it when you reach an agreement?

tiancaiamao · 2018-05-03T07:31:33Z

store/tikv/txn.go

-		return nil
+
+	defer func() {
+		if err == nil {


I guess this can be moved into executeAndWriteFinishBinlog, so defer is unnecessary

AndreMouche · 2018-05-04T01:47:23Z

/run-all-tests

AndreMouche · 2018-05-04T01:47:51Z

friendly ping @coocood

coocood · 2018-05-07T02:41:31Z

LGTM

tikv/txn: support latch in transaction

656dd7b

coocood reviewed Apr 28, 2018

View reviewed changes

address comments

b51c483

coocood reviewed Apr 28, 2018

View reviewed changes

address comments

ed8154b

AndreMouche added 2 commits April 28, 2018 14:19

address comments

41dc21c

Merge branch 'master' into tikv/txn_latch

2678449

AndreMouche requested review from tiancaiamao and disksing April 28, 2018 06:53

alivxxx reviewed Apr 28, 2018

View reviewed changes

tiancaiamao added the require-LGT3 Indicates that the PR requires three LGTM. label Apr 28, 2018

tiancaiamao reviewed Apr 28, 2018

View reviewed changes

AndreMouche added 3 commits April 28, 2018 20:41

address comments

25a893f

Merge branch 'master' into tikv/txn_latch

4a8e29b

address comments

cb90239

Merge branch 'master' into tikv/txn_latch

2502227

AndreMouche added 2 commits May 2, 2018 15:27

kv/txn: set retryable in context for commit

6f3263b

Merge branch 'master' into tikv/txn_latch

9b465c4

Merge branch 'master' into tikv/txn_latch

25faf0d

AndreMouche added 2 commits May 3, 2018 10:09

tikv/txn: add debug log

7657fa4

Merge branch 'master' into tikv/txn_latch

e8c279b

jackysp added the all-tests-passed label May 3, 2018

tiancaiamao reviewed May 3, 2018

View reviewed changes

tiancaiamao added the status/LGT2 Indicates that a PR has LGTM 2. label May 3, 2018

AndreMouche added 4 commits May 4, 2018 09:33

mvcc/reader: address comments

c6a0002

Merge branch 'master' into tikv/txn_latch

0c5caaa

update comments

37695a6

merge master

79fdafd

AndreMouche added 3 commits May 4, 2018 11:02

Merge branch 'master' into tikv/txn_latch

47d6420

Merge branch 'master' into tikv/txn_latch

f522381

Merge branch 'master' into tikv/txn_latch

1c0ab63

coocood approved these changes May 7, 2018

View reviewed changes

coocood added status/LGT3 The PR has already had 3 LGTM. and removed status/LGT2 Indicates that a PR has LGTM 2. labels May 7, 2018

Merge branch 'master' into tikv/txn_latch

40da727

jackysp merged commit 4185e53 into pingcap:master May 7, 2018

LinuxGit mentioned this pull request May 13, 2018

tidb txn-local-latches config pingcap/tidb-ansible#387

Closed

tiancaiamao mentioned this pull request Jun 28, 2018

*: 'load data' should not use latches #6927

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

tikv/txn: support local latch in transaction #6418

tikv/txn: support local latch in transaction #6418

AndreMouche commented Apr 28, 2018

coocood Apr 28, 2018

coocood Apr 28, 2018

coocood Apr 28, 2018

coocood Apr 28, 2018

coocood commented Apr 28, 2018

coocood commented Apr 28, 2018

alivxxx Apr 28, 2018

alivxxx Apr 28, 2018

alivxxx Apr 28, 2018

alivxxx Apr 28, 2018 •

edited

Loading

tiancaiamao Apr 28, 2018

AndreMouche Apr 28, 2018

tiancaiamao Apr 28, 2018

AndreMouche Apr 28, 2018

tiancaiamao commented May 2, 2018

disksing commented May 2, 2018

AndreMouche commented May 2, 2018

AndreMouche commented May 2, 2018

jackysp commented May 2, 2018

AndreMouche commented May 3, 2018

tiancaiamao left a comment

tiancaiamao May 3, 2018

AndreMouche May 4, 2018

tiancaiamao May 3, 2018

AndreMouche May 4, 2018

tiancaiamao May 3, 2018

AndreMouche commented May 4, 2018

AndreMouche commented May 4, 2018

coocood commented May 7, 2018

tikv/txn: support local latch in transaction #6418

tikv/txn: support local latch in transaction #6418

Conversation

AndreMouche commented Apr 28, 2018

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 commented Apr 28, 2018

coocood commented Apr 28, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alivxxx Apr 28, 2018 • 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

Choose a reason for hiding this comment

tiancaiamao commented May 2, 2018

disksing commented May 2, 2018

AndreMouche commented May 2, 2018

AndreMouche commented May 2, 2018

jackysp commented May 2, 2018

AndreMouche commented May 3, 2018

tiancaiamao left a 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

AndreMouche commented May 4, 2018

AndreMouche commented May 4, 2018

coocood commented May 7, 2018

alivxxx Apr 28, 2018 •

edited

Loading