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

store/tikv: support single statement rollback for pessimistic transaction #10654

Merged
merged 5 commits into from Jun 5, 2019

Conversation

Projects
None yet
4 participants
@coocood
Copy link
Member

commented May 30, 2019

What problem does this PR solve?

In a pessimistic transaction, a single statement should never cause a deadlock, but current implementation cannot guarantee it.

What is changed and how it works?

Detect if the deadlock key hash is in the keys of the current statement.
Rollback the current statement keys and retry the statement.

Check List

Tests

  • Unit Test

@coocood coocood added the status/WIP label May 30, 2019

@coocood coocood force-pushed the coocood:pessimistic-rollback branch 2 times, most recently from 1119822 to 4728304 May 30, 2019

@coocood

This comment has been minimized.

Copy link
Member Author

commented May 30, 2019

@codecov

This comment has been minimized.

Copy link

commented May 30, 2019

Codecov Report

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

@@             Coverage Diff             @@
##             master     #10654   +/-   ##
===========================================
  Coverage          ?   78.3608%           
===========================================
  Files             ?        414           
  Lines             ?      87836           
  Branches          ?          0           
===========================================
  Hits              ?      68829           
  Misses            ?      13871           
  Partials          ?       5136
// Clean up pessimistic lock.
if txn.IsPessimistic() && txn.committer != nil {
err := txn.rollbackPessimisticLock()
err := txn.rollbackPessimisticLocks()

This comment has been minimized.

Copy link
@youjiali1995

youjiali1995 May 31, 2019

Is it possible rollback a non-pessimistic lock in the pessimistic transaction?

This comment has been minimized.

Copy link
@tiancaiamao

tiancaiamao Jun 4, 2019

Contributor

There is a txn.IsPessimistic() @youjiali1995

This comment has been minimized.

Copy link
@youjiali1995

youjiali1995 Jun 4, 2019

Sorry, I didn't make myself clear. Is it possible the pessimistic transaction holds some non-pessimistic locks here?

This comment has been minimized.

Copy link
@coocood

coocood Jun 5, 2019

Author Member

If Rollback is called, Commit will not be called, non-pessimistic keys is not written.

zap.Uint64("txn", txnCtx.StartTS),
zap.Uint64("lockTS", deadlock.LockTs),
zap.Binary("lockKey", deadlock.LockKey),
zap.Uint64("deadlockKeyHash", deadlock.DeadlockKeyHash))

This comment has been minimized.

Copy link
@youjiali1995

youjiali1995 May 31, 2019

Will get a new forUpdateTs to prevent asyncPessimisticRollback rollbacks the new lock if it is a retryable Deadlock error. Do I understand this correctly?

This comment has been minimized.

Copy link
@coocood

coocood Jun 5, 2019

Author Member

yes

@youjiali1995

This comment has been minimized.

Copy link

commented May 31, 2019

It is possible that the rollbacked statement acquires the lock again. Does it need to sleep a moment before retry?

@@ -45,6 +45,7 @@ const (
CmdGC
CmdDeleteRange
CmdPessimisticLock
CmdPessimisticRollback

This comment has been minimized.

Copy link
@youjiali1995

youjiali1995 Jun 3, 2019

SetContext doesn't handle CmdPessimisticRollback.

This comment has been minimized.

Copy link
@youjiali1995

youjiali1995 Jun 3, 2019

emm, many functions don't handle it.

@coocood coocood force-pushed the coocood:pessimistic-rollback branch from 4728304 to 63083aa Jun 3, 2019

@coocood

This comment has been minimized.

Copy link
Member Author

commented Jun 3, 2019

@coocood coocood removed the status/WIP label Jun 3, 2019

@coocood

This comment has been minimized.

Copy link
Member Author

commented Jun 3, 2019

zap.Uint64("deadlockKeyHash", deadlock.DeadlockKeyHash))
} else if terror.ErrorEqual(kv.ErrWriteConflict, err) {
conflictCommitTS := extractConflictCommitTS(err.Error())
if conflictCommitTS == 0 {

This comment has been minimized.

Copy link
@jackysp

jackysp Jun 4, 2019

Member

when will conflictCommitTS == 0?

This comment has been minimized.

Copy link
@coocood

coocood Jun 4, 2019

Author Member

When failed to extract conflictCommitTS.

This comment has been minimized.

Copy link
@jackysp

jackysp Jun 4, 2019

Member

So should we stop handling PessimisticLockError?

This comment has been minimized.

Copy link
@youjiali1995

youjiali1995 Jun 4, 2019

If the transaction is rollbacked, TiKV will set conflictCommitTs to 0. The log here is not accurate, or I can set conflictCommitTs to conflictStartTs if it is rollbacked?

}
if ok {
lock := dec.lock
if lock.op == kvrpcpb.Op_PessimisticLock && lock.startTS == startTS && lock.forUpdateTS == forUpdateTS {

This comment has been minimized.

Copy link
@youjiali1995

youjiali1995 Jun 4, 2019

lock.forUpdateTS <= forUpdateTS

if anyError {
return errs
}
if err := mvcc.db.Write(batch, nil); err != nil {

This comment has been minimized.

Copy link
@tiancaiamao

tiancaiamao Jun 4, 2019

Contributor

err != nil return nil ?

// Clean up pessimistic lock.
if txn.IsPessimistic() && txn.committer != nil {
err := txn.rollbackPessimisticLock()
err := txn.rollbackPessimisticLocks()

This comment has been minimized.

Copy link
@tiancaiamao

tiancaiamao Jun 4, 2019

Contributor

There is a txn.IsPessimistic() @youjiali1995

return nil
}

return c.cleanupKeys(NewBackoffer(context.Background(), cleanupMaxBackoff), c.keys)
return txn.committer.pessimisticRollbackKeys(NewBackoffer(context.Background(), cleanupMaxBackoff), txn.lockKeys)

This comment has been minimized.

Copy link
@tiancaiamao

tiancaiamao Jun 4, 2019

Contributor

Maybe cleanupKeys is more suitable here?
cleanupKeys checks startTS (the whole txn) while pessimisticRollbackKeys checks startTS + forUpdateTS(only the statement)

This comment has been minimized.

Copy link
@coocood

coocood Jun 4, 2019

Author Member

We use pessimisticRollbackKeys, so there will not be rollback records written.

This comment has been minimized.

Copy link
@coocood

coocood Jun 4, 2019

Author Member

The check has changed to lock.forUpdateTS <= forUpdateTS.

@@ -381,6 +376,14 @@ func (txn *tikvTxn) LockKeys(ctx context.Context, forUpdateTS uint64, keys ...kv
txn.committer.isFirstLock = len(txn.lockKeys) == 0 && len(keys1) == 1
err := txn.committer.pessimisticLockKeys(bo, keys1)
if err != nil {
wg := txn.asyncPessimisticRollback(ctx, keys1)

This comment has been minimized.

Copy link
@tiancaiamao

tiancaiamao Jun 4, 2019

Contributor

If asyncPessimisticRollback fail, this transaction is still locking the key, while it doesn't think itself hold the lock...

This comment has been minimized.

Copy link
@coocood

coocood Jun 4, 2019

Author Member

It doesn't affect the correctness.

@coocood

This comment has been minimized.

Copy link
Member Author

commented Jun 4, 2019

@coocood coocood force-pushed the coocood:pessimistic-rollback branch from 05752a4 to 084b6c9 Jun 5, 2019

@youjiali1995

This comment has been minimized.

Copy link

commented Jun 5, 2019

LGTM

1 similar comment
@tiancaiamao

This comment has been minimized.

Copy link
Contributor

commented Jun 5, 2019

LGTM

@tiancaiamao

This comment has been minimized.

Copy link
Contributor

commented Jun 5, 2019

/run-all-tests

@tiancaiamao tiancaiamao merged commit 08e4bd9 into pingcap:master Jun 5, 2019

14 checks passed

ci/circleci Your tests passed on CircleCI!
Details
idc-jenkins-ci-tidb/build Jenkins job succeeded.
Details
idc-jenkins-ci-tidb/build_check_race Jenkins job succeeded.
Details
idc-jenkins-ci-tidb/check_dev Jenkins job succeeded.
Details
idc-jenkins-ci-tidb/check_dev_2 Jenkins job succeeded.
Details
idc-jenkins-ci-tidb/common-test job succeeded
Details
idc-jenkins-ci-tidb/integration-common-test Jenkins job succeeded.
Details
idc-jenkins-ci-tidb/integration-compatibility-test Jenkins job succeeded.
Details
idc-jenkins-ci-tidb/integration-ddl-test Jenkins job succeeded.
Details
idc-jenkins-ci-tidb/mybatis-test job succeeded
Details
idc-jenkins-ci-tidb/sqllogic-test-1 Jenkins job succeeded.
Details
idc-jenkins-ci-tidb/sqllogic-test-2 Jenkins job succeeded.
Details
idc-jenkins-ci-tidb/unit-test Jenkins job succeeded.
Details
license/cla Contributor License Agreement is signed.
Details

@coocood coocood deleted the coocood:pessimistic-rollback branch Jun 5, 2019

coocood added a commit to coocood/tidb that referenced this pull request Jun 5, 2019

coocood added a commit that referenced this pull request Jun 5, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.