Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

*: support pessimistic transaction (experimental feature) #10297

Merged
merged 13 commits into from
May 11, 2019

Conversation

coocood
Copy link
Member

@coocood coocood commented Apr 28, 2019

What problem does this PR solve?

Avoid transaction failure caused by write conflict, especially when the transaction cannot be retried automatically.

What is changed and how it works?

  • Use BEGIN LOCK syntax to start a new pessimistic transaction.
  • In the pessimistic transaction, write pessimistic lock on INSERT/UPDATE/DELETE/SELECT FOR UPDATE.
  • Pessimistic locks don't have value, don't block read.
  • Pessimistic locks blocks writes to make sure there is no write conflict during Prewrite phase.

Check List

Tests

  • Unit test
  • Integration test

Code changes

  • Has exported function/method change
  • Has exported variable/fields change
  • Has interface methods change
  • Has persistent data change

Side effects

  • Possible performance regression
  • Increased code complexity

Related changes

  • Need to update the documentation
  • Need to be included in the release note

@codecov
Copy link

codecov bot commented Apr 28, 2019

Codecov Report

❗ No coverage uploaded for pull request base (master@5617809). Click here to learn what that means.
The diff coverage is 21.8142%.

@@            Coverage Diff             @@
##             master    #10297   +/-   ##
==========================================
  Coverage          ?   77.012%           
==========================================
  Files             ?       412           
  Lines             ?     86232           
  Branches          ?         0           
==========================================
  Hits              ?     66409           
  Misses            ?     14754           
  Partials          ?      5069

@coocood
Copy link
Member Author

coocood commented Apr 28, 2019

/run-all-tests

@jackysp jackysp changed the title support pessimistic transaction (experimental feature) *: support pessimistic transaction (experimental feature) Apr 29, 2019
config/config.go Outdated Show resolved Hide resolved
@zhouqiang-cl
Copy link
Contributor

/run-all-tests tidb-test=pr/795

config/config.go Show resolved Hide resolved
config/config.toml.example Outdated Show resolved Hide resolved
executor/adapter.go Outdated Show resolved Hide resolved
executor/adapter.go Outdated Show resolved Hide resolved
executor/adapter.go Show resolved Hide resolved

func (s *testSessionSuite) TestPessimisticTxn(c *C) {
if !config.GetGlobalConfig().PessimisticTxn.Enable {
c.Skip("pessimistic transaction is not enabled")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The default conf is false, this test will never run?!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes.

session/session_test.go Show resolved Hide resolved
session/session_test.go Show resolved Hide resolved
store/tikv/2pc.go Outdated Show resolved Hide resolved
store/tikv/2pc.go Outdated Show resolved Hide resolved
@tiancaiamao
Copy link
Contributor

LGTM

@tiancaiamao tiancaiamao added the status/LGT1 Indicates that a PR has LGTM 1. label Apr 30, 2019
executor/adapter.go Outdated Show resolved Hide resolved
executor/adapter.go Outdated Show resolved Hide resolved
executor/adapter.go Outdated Show resolved Hide resolved
executor/adapter.go Outdated Show resolved Hide resolved
executor/adapter.go Show resolved Hide resolved
executor/prepared.go Outdated Show resolved Hide resolved
store/tikv/2pc.go Show resolved Hide resolved
executor/point_get.go Show resolved Hide resolved
@coocood
Copy link
Member Author

coocood commented May 6, 2019

@lysu @jackysp PTAL

@zhouqiang-cl
Copy link
Contributor

The make dev test takes too long and seems something wrong in this pr

@@ -130,7 +160,9 @@ func newTwoPhaseCommitter(txn *tikvTxn, connID uint64) (*twoPhaseCommitter, erro
}
delCnt++
}
keys = append(keys, k)
if isPessimistic && !bytes.Equal(k, c.primaryKey) {
Copy link
Contributor

@lysu lysu May 6, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe should be?

Suggested change
if isPessimistic && !bytes.Equal(k, c.primaryKey) {
if !isPessimistic || !bytes.Equal(k, c.primaryKey) {

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

after dig, maybe old one is ok too, c.primaryKey always be null if isPessimistic=true

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ping @coocood

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lysu I know, but add isPessimistic is more explicit that this code is only related to pessimistic lock.

@lysu
Copy link
Contributor

lysu commented May 6, 2019

rest LGTM for pessimistic disabled logic

}

for _, pair := range txn.assertions {
mutation, ok := mutations[string(pair.key)]
if !ok {
logutil.Logger(context.Background()).Error("ASSERTION FAIL!!! assertion exists but no mutation?",
zap.Stringer("assertion", pair))
// It's possible when a transaction inserted a key then deleted it later.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tiancaiamao is this reasonable?

Copy link
Member

@jackysp jackysp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rest LGTM

…tic-txn

Conflicts:
	go.mod
	go.sum
	store/mockstore/mocktikv/mvcc_leveldb.go
@zhouqiang-cl
Copy link
Contributor

make check always hang for very long time

@coocood
Copy link
Member Author

coocood commented May 10, 2019

/run-all-tests

@jackysp jackysp merged commit 373748a into pingcap:master May 11, 2019
@coocood coocood deleted the pessimistic-txn branch May 13, 2019 04:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
sig/transaction SIG:Transaction status/LGT1 Indicates that a PR has LGTM 1. type/new-feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants