From cc7392dcae6bd787d8ce65ca04617563fa04f3e2 Mon Sep 17 00:00:00 2001 From: Ewan Chou Date: Tue, 18 Jun 2019 22:53:35 +0800 Subject: [PATCH 1/3] store/tikv: fix performance issue under mixed transaction mode --- config/config.go | 11 ++++++----- session/pessimistic_test.go | 2 ++ store/tikv/2pc.go | 13 +++++++++++++ store/tikv/2pc_test.go | 20 ++++++++++++++++++++ store/tikv/lock_resolver.go | 9 ++------- store/tikv/lock_test.go | 5 +++++ 6 files changed, 48 insertions(+), 12 deletions(-) diff --git a/config/config.go b/config/config.go index 4cc8a1f6ff7d9..9d107f6be82b5 100644 --- a/config/config.go +++ b/config/config.go @@ -35,7 +35,9 @@ import ( // Config number limitations const ( - MaxLogFileSize = 4096 // MB + MaxLogFileSize = 4096 // MB + MinPessimisticTTL = time.Second * 15 + MaxPessimisticTTL = time.Second * 60 ) // Valid config maps @@ -568,10 +570,9 @@ func (c *Config) Valid() error { if err != nil { return err } - minDur := time.Second * 15 - maxDur := time.Second * 60 - if dur < minDur || dur > maxDur { - return fmt.Errorf("pessimistic transaction ttl %s out of range [%s, %s]", dur, minDur, maxDur) + if dur < MinPessimisticTTL || dur > MaxPessimisticTTL { + return fmt.Errorf("pessimistic transaction ttl %s out of range [%s, %s]", + dur, MinPessimisticTTL, MaxPessimisticTTL) } } return nil diff --git a/session/pessimistic_test.go b/session/pessimistic_test.go index 331f542a34611..79d1599b7de53 100644 --- a/session/pessimistic_test.go +++ b/session/pessimistic_test.go @@ -27,6 +27,7 @@ import ( "github.com/pingcap/tidb/session" "github.com/pingcap/tidb/store/mockstore" "github.com/pingcap/tidb/store/mockstore/mocktikv" + "github.com/pingcap/tidb/store/tikv" "github.com/pingcap/tidb/tablecodec" "github.com/pingcap/tidb/util/codec" "github.com/pingcap/tidb/util/testkit" @@ -52,6 +53,7 @@ func (s *testPessimisticSuite) SetUpSuite(c *C) { mockstore.WithCluster(s.cluster), mockstore.WithMVCCStore(s.mvccStore), ) + tikv.PessimisticLockTTL = uint64(config.MinPessimisticTTL / time.Millisecond) c.Assert(err, IsNil) s.store = store session.SetSchemaLease(0) diff --git a/store/tikv/2pc.go b/store/tikv/2pc.go index 6b888d4c162fd..cade7c7e2dde6 100644 --- a/store/tikv/2pc.go +++ b/store/tikv/2pc.go @@ -538,6 +538,19 @@ func (c *twoPhaseCommitter) prewriteSingleBatch(bo *Backoffer, batch batchKeys) if err1 != nil { return errors.Trace(err1) } + if !c.isPessimistic && lock.TTL >= uint64(config.MinPessimisticTTL/time.Millisecond) { + // An optimistic prewrite meets pessimistic lock. + // If we wait for the lock, other written optimistic locks would block reads for long time. + // And it is very unlikely this transaction would succeed after wait for the pessimistic lock. + // Return write conflict error to cleanup locks. + return newWriteConflictError(&pb.WriteConflict{ + StartTs: c.startTS, + ConflictTs: lock.TxnID, + ConflictCommitTs: 0, + Key: lock.Key, + Primary: lock.Primary, + }) + } logutil.Logger(context.Background()).Debug("prewrite encounters lock", zap.Uint64("conn", c.connID), zap.Stringer("lock", lock)) diff --git a/store/tikv/2pc_test.go b/store/tikv/2pc_test.go index e602b9ec9248b..de89ca0d476bd 100644 --- a/store/tikv/2pc_test.go +++ b/store/tikv/2pc_test.go @@ -23,6 +23,7 @@ import ( . "github.com/pingcap/check" "github.com/pingcap/errors" "github.com/pingcap/kvproto/pkg/kvrpcpb" + "github.com/pingcap/tidb/config" "github.com/pingcap/tidb/kv" "github.com/pingcap/tidb/store/mockstore/mocktikv" "github.com/pingcap/tidb/store/tikv/tikvrpc" @@ -36,6 +37,10 @@ type testCommitterSuite struct { var _ = Suite(&testCommitterSuite{}) +func (s *testCommitterSuite) SetUpSuite(c *C) { + PessimisticLockTTL = uint64(config.MinPessimisticTTL / time.Millisecond) +} + func (s *testCommitterSuite) SetUpTest(c *C) { s.cluster = mocktikv.NewCluster() mocktikv.BootstrapWithMultiRegions(s.cluster, []byte("a"), []byte("b"), []byte("c")) @@ -468,3 +473,18 @@ func (s *testCommitterSuite) TestPessimisticLockedKeysDedup(c *C) { c.Assert(err, IsNil) c.Assert(txn.lockKeys, HasLen, 2) } + +func (s *testCommitterSuite) TestPessimistOptimisticConflict(c *C) { + txnPes := s.begin(c) + txnPes.SetOption(kv.Pessimistic, true) + err := txnPes.LockKeys(context.Background(), txnPes.startTS, kv.Key("pes")) + c.Assert(err, IsNil) + c.Assert(txnPes.IsPessimistic(), IsTrue) + c.Assert(txnPes.lockKeys, HasLen, 1) + txnOpt := s.begin(c) + err = txnOpt.Set(kv.Key("pes"), []byte("v")) + c.Assert(err, IsNil) + err = txnOpt.Commit(context.Background()) + c.Assert(kv.ErrWriteConflict.Equal(err), IsTrue) + c.Assert(txnPes.Commit(context.Background()), IsNil) +} diff --git a/store/tikv/lock_resolver.go b/store/tikv/lock_resolver.go index e2f0177d57cb0..42bf901eb195d 100644 --- a/store/tikv/lock_resolver.go +++ b/store/tikv/lock_resolver.go @@ -138,17 +138,12 @@ func (l *Lock) String() string { // NewLock creates a new *Lock. func NewLock(l *kvrpcpb.LockInfo) *Lock { - ttl := l.GetLockTtl() - if ttl == 0 { - ttl = defaultLockTTL - } - txnSize := l.GetTxnSize() return &Lock{ Key: l.GetKey(), Primary: l.GetPrimaryLock(), TxnID: l.GetLockVersion(), - TTL: ttl, - TxnSize: txnSize, + TTL: l.GetLockTtl(), + TxnSize: l.GetTxnSize(), } } diff --git a/store/tikv/lock_test.go b/store/tikv/lock_test.go index d507ecaa800da..3b9ed28f3fded 100644 --- a/store/tikv/lock_test.go +++ b/store/tikv/lock_test.go @@ -277,6 +277,11 @@ func (s *testLockSuite) TestLockTTL(c *C) { s.ttlEquals(c, l.TTL, defaultLockTTL+uint64(time.Since(start)/time.Millisecond)) } +func (s *testLockSuite) TestNewLockZeroTTL(c *C) { + l := NewLock(&kvrpcpb.LockInfo{}) + c.Assert(l.TTL, Equals, uint64(0)) +} + func init() { // Speed up tests. defaultLockTTL = 3 From 6df925c8a5ed99a0ce78d85c0e813aae7911bbec Mon Sep 17 00:00:00 2001 From: Ewan Chou Date: Wed, 19 Jun 2019 13:53:53 +0800 Subject: [PATCH 2/3] address comment --- store/tikv/2pc.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/store/tikv/2pc.go b/store/tikv/2pc.go index cade7c7e2dde6..705da738ac03a 100644 --- a/store/tikv/2pc.go +++ b/store/tikv/2pc.go @@ -538,10 +538,10 @@ func (c *twoPhaseCommitter) prewriteSingleBatch(bo *Backoffer, batch batchKeys) if err1 != nil { return errors.Trace(err1) } - if !c.isPessimistic && lock.TTL >= uint64(config.MinPessimisticTTL/time.Millisecond) { - // An optimistic prewrite meets pessimistic lock. + if !c.isPessimistic && c.lockTTL < lock.TTL && lock.TTL >= uint64(config.MinPessimisticTTL/time.Millisecond) { + // An optimistic prewrite meets a pessimistic or large transaction lock. // If we wait for the lock, other written optimistic locks would block reads for long time. - // And it is very unlikely this transaction would succeed after wait for the pessimistic lock. + // And it is very unlikely this transaction would succeed after wait for the long TTL lock. // Return write conflict error to cleanup locks. return newWriteConflictError(&pb.WriteConflict{ StartTs: c.startTS, From f4e6008097bbd728a3cc200fbea0725cad16fbc6 Mon Sep 17 00:00:00 2001 From: Ewan Chou Date: Wed, 19 Jun 2019 14:21:05 +0800 Subject: [PATCH 3/3] try fix integration test --- store/tikv/2pc_test.go | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/store/tikv/2pc_test.go b/store/tikv/2pc_test.go index de89ca0d476bd..34b76c3b27a68 100644 --- a/store/tikv/2pc_test.go +++ b/store/tikv/2pc_test.go @@ -37,12 +37,9 @@ type testCommitterSuite struct { var _ = Suite(&testCommitterSuite{}) -func (s *testCommitterSuite) SetUpSuite(c *C) { - PessimisticLockTTL = uint64(config.MinPessimisticTTL / time.Millisecond) -} - func (s *testCommitterSuite) SetUpTest(c *C) { s.cluster = mocktikv.NewCluster() + PessimisticLockTTL = uint64(config.MinPessimisticTTL / time.Millisecond) mocktikv.BootstrapWithMultiRegions(s.cluster, []byte("a"), []byte("b"), []byte("c")) mvccStore, err := mocktikv.NewMVCCLevelDB("") c.Assert(err, IsNil)