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

txn: Implement check_txn_status API #5390

Merged
merged 32 commits into from Oct 14, 2019

Conversation

@MyonKeminta
Copy link
Contributor

MyonKeminta commented Sep 3, 2019

What have you changed?

This is part of work of supporting large transaction. This Adds the check_txn_status API to TiKV, which is kind of similar to cleanup API, but also checks Lock's TTL, rollback the lock if TTL has expired, and returns the lock_ttl (if still locking) or commit_ts (if already committed).

This PR is corresponding to pingcap/tidb#11974 .

  • Implement the API
  • Tests
  • Documentation
  • Metrics
  • Wait for merging kvproto

What is the type of the changes?

Pick one of the following and delete the others:

  • New feature (a change which adds functionality)

How is the PR tested?

By unit tests and integration tests (WIP)

Does this PR affect documentation (docs) or should it be mentioned in the release notes?

No

Does this PR affect tidb-ansible?

No

Refer to a related PR or issue link (optional)

Corresponding PR in TiDB repo: pingcap/tidb#11974
Required PR in kvproto: pingcap/kvproto#443

MyonKeminta added 2 commits Sep 2, 2019
Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>
Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>
MyonKeminta added 9 commits Sep 3, 2019
Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>
Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>
Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>
Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>
Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>
Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>
Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>
Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>
@MyonKeminta MyonKeminta removed the S: WIP label Sep 5, 2019
@MyonKeminta MyonKeminta changed the title [WIP] txn: Implement check_txn_status API txn: Implement check_txn_status API Sep 5, 2019
@MyonKeminta

This comment has been minimized.

Copy link
Contributor Author

MyonKeminta commented Sep 5, 2019

Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>
@coocood

This comment has been minimized.

Copy link
Contributor

coocood commented Sep 5, 2019

Need to update gen_command_lock too.

self.rollback_lock(primary_key, lock)?;
}
MVCC_CHECK_TXN_STATUS_COUNTER_VEC.rollback.inc();
return Ok((0, 0, is_pessimistic_txn));

This comment has been minimized.

Copy link
@AndreMouche

AndreMouche Sep 6, 2019

Member

Is it possible that is_pessimistic_txn is true while the lock type is not pessimistic?

This comment has been minimized.

Copy link
@MyonKeminta

MyonKeminta Sep 9, 2019

Author Contributor

It's possible since a pessimistic transaction may contains keys that don't use pessimistic locks (usually index keys).

This comment has been minimized.

Copy link
@AndreMouche

AndreMouche Sep 17, 2019

Member

So if is an index keys in the pessimistic transaction, the for_update_ts shouldn't be 0 while the type is usually Lock/PUT/DELETE?

This comment has been minimized.

Copy link
@youjiali1995

youjiali1995 Sep 17, 2019

Contributor

All locks in a pessimistic transaction have non-zero for_update_ts.

if lock.min_commit_ts > 0 && caller_start_ts >= lock.min_commit_ts {
lock.min_commit_ts = caller_start_ts + 1;

if lock.min_commit_ts < current_ts {

This comment has been minimized.

Copy link
@AndreMouche

AndreMouche Sep 6, 2019

Member

Can you give some description for current_ts and caller_start_ts? they really confused me

This comment has been minimized.

Copy link
@MyonKeminta

MyonKeminta Sep 9, 2019

Author Contributor

I've added some description in the function's comments.

This comment has been minimized.

Copy link
@AndreMouche

AndreMouche Sep 17, 2019

Member

Seems we will always meet this condition? since the current ts will always bigger than (caller_start_ts)?

This comment has been minimized.

Copy link
@youjiali1995

youjiali1995 Sep 17, 2019

Contributor

I think current_ts is only used to check lock expiration. It's not related to min_commit_ts.

This comment has been minimized.

Copy link
@MyonKeminta

MyonKeminta Sep 17, 2019

Author Contributor

There may be transactions whose start_ts is between lock.ts and current_ts. Update it to the current_ts (if it's larger) can reduce updating. @coocood

@MyonKeminta

This comment has been minimized.

Copy link
Contributor Author

MyonKeminta commented Sep 6, 2019

@coocood Yes. I haven't update this PR yet.

@MyonKeminta

This comment has been minimized.

Copy link
Contributor Author

MyonKeminta commented Sep 6, 2019

Since there are common code in this PR and #5407 , I'd like to update this PR after merging #5407 , after resolving conflicts.

MyonKeminta added 3 commits Sep 9, 2019
Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>
Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>
MyonKeminta added 2 commits Sep 12, 2019
Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>
@youjiali1995

This comment has been minimized.

Copy link
Contributor

youjiali1995 commented Sep 16, 2019

Is it ready for review?

@MyonKeminta

This comment has been minimized.

Copy link
Contributor Author

MyonKeminta commented Sep 16, 2019

@youjiali1995 I think yes

src/storage/mvcc/lock.rs Outdated Show resolved Hide resolved
self.delete_value(key.clone(), lock.ts);
}
let write = Write::new(WriteType::Rollback, self.start_ts, None);
let ts = self.start_ts;

This comment has been minimized.

Copy link
@sticnarf

sticnarf Oct 11, 2019

Contributor

not a meaningful let

@@ -620,6 +626,75 @@ impl<S: Snapshot> MvccTxn<S> {
})
}

/// Check the status of a transaction.
///
/// This operation checks whether a transaction has expired it's Lock's TTL, rollback the

This comment has been minimized.

Copy link
@sticnarf

sticnarf Oct 11, 2019

Contributor

"it's" -> "its primary"

/// Check the status of a transaction.
///
/// This operation checks whether a transaction has expired it's Lock's TTL, rollback the
/// transaction if expired, and update the transaction's min_commit_ts according to the metadata

This comment has been minimized.

Copy link
@sticnarf

sticnarf Oct 11, 2019

Contributor

"and" -> "or"/"otherwise"

{
// When current_ts == 0 or the lock is expired, clean it up.
if lock.lock_type == LockType::Pessimistic {
self.unlock_key(primary_key);

This comment has been minimized.

Copy link
@sticnarf

sticnarf Oct 11, 2019

Contributor

Why can we avoid writing Rollback when it's a pessimistic lock? What if there comes a stale acquire command?

This comment has been minimized.

Copy link
@youjiali1995

youjiali1995 Oct 11, 2019

Contributor

Oh, it needs to write a rollback.

This comment has been minimized.

Copy link
@MyonKeminta

MyonKeminta Oct 12, 2019

Author Contributor

I didn't write rollback because I found that pessimistic_rollback didn't write rollback either. I'm not very understand now. Does pessimistic_rollback need to write rollback too?

This comment has been minimized.

Copy link
@sticnarf

sticnarf Oct 12, 2019

Contributor

pessimistic_rollback is called by the lock owner transaction itself. Other transactions still use cleanup to clean pessimistic locks.

MyonKeminta added 3 commits Oct 12, 2019
Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>
fix tests
Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>
Some(ref mut lock) if lock.ts == self.start_ts => {
let is_pessimistic_txn = lock.for_update_ts != 0;

if current_ts == 0

This comment has been minimized.

Copy link
@sticnarf

sticnarf Oct 12, 2019

Contributor

Can we remove this now?

Copy link
Contributor

sticnarf left a comment

LGTM

Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>
Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>
@MyonKeminta

This comment has been minimized.

Copy link
Contributor Author

MyonKeminta commented Oct 14, 2019

Sorry I pushed one more commit to add tests. PTAL again. @youjiali1995 @sticnarf

Copy link
Contributor

sticnarf left a comment

Actually I don't think current_ts == u64::max_value() is a special case now. And the added case does not help a lot when some presumptions don't hold in the future. (For example, if lock's TTL can be u64::MAX in the future, the tests here with hardcoded start_ts and TTL won't fail then)

Nevertheless, having more tests is never bad...

@sre-bot

This comment has been minimized.

Copy link
Collaborator

sre-bot commented Oct 14, 2019

Your auto merge job has been accepted, waiting for 5629

@sre-bot

This comment has been minimized.

Copy link
Collaborator

sre-bot commented Oct 14, 2019

/run-all-tests

@sre-bot

This comment has been minimized.

Copy link
Collaborator

sre-bot commented Oct 14, 2019

@MyonKeminta merge failed.

@sticnarf

This comment has been minimized.

Copy link
Contributor

sticnarf commented Oct 14, 2019

/merge

@sre-bot

This comment has been minimized.

Copy link
Collaborator

sre-bot commented Oct 14, 2019

/run-all-tests

@sre-bot sre-bot merged commit cf550ee into tikv:master Oct 14, 2019
6 checks passed
6 checks passed
DCO All commits are signed off!
Details
idc-jenkins-ci-tikv/integration-common-test Jenkins job succeeded.
Details
idc-jenkins-ci-tikv/integration-compatibility-test Jenkins job succeeded.
Details
idc-jenkins-ci-tikv/integration-cop-push-down-test Jenkins job succeeded.
Details
idc-jenkins-ci-tikv/integration-ddl-test Jenkins job succeeded.
Details
idc-jenkins-ci/test Jenkins job succeeded.
Details
@AndreMouche

This comment has been minimized.

Copy link
Member

AndreMouche commented Oct 14, 2019

@coocood PTAL

@MyonKeminta MyonKeminta deleted the MyonKeminta:misono/check-txn-status branch Oct 17, 2019
sticnarf pushed a commit to sticnarf/tikv that referenced this pull request Oct 27, 2019
Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.