txn: Do constraint check when handling repeated acqurie_pessimsitic_lock request #14037

MyonKeminta · 2023-01-11T12:34:42Z

What is changed and how it works?

Issue Number: Close #14038
Close pingcap/tidb#40114

What's Changed:

Fixes the problem that when handling repeated acquire_pessimistic_lock requests is recevied, should_not_exist is ignored. 

TiKV provides idempotency for these RPC requests, but for acquire_pessimistic_lock, it ignored the possibility that the client may expect a pessimistic_rollback between two acquire_pessimistic_lock request on the same key. In this case the second request may come from another statement and carries `should_not_exist` that wasn't set in the previously finished pessimistic lock request. If the first request successfully acquired the lock and the pessimistic_rollback failed, TiKV may return a sucessful response, making the client believe that the key doesn't exist before. In some rare cases, this has risk to cause data inconsistency.

Related changes

PR to update pingcap/docs/pingcap/docs-cn:
Need to cherry-pick to the release branch

Check List

Tests

Unit test

Side effects

Performance regression
- Consumes more CPU

Release note

Fixed a problem that when a transaction in TiDB fails to execute a pessimistic DML and then executes another DML, if there are random network failures between TiDB and TiKV, it has risk to cause data inconsistency.

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

ti-chi-bot · 2023-01-11T12:34:44Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

cfzjywxk
sticnarf

To complete the pull request process, please ask the reviewers in the list to review by filling /cc @reviewer in the comment.
After your PR has acquired the required number of LGTMs, you can assign this pull request to the committer in the list by filling /assign @committer in the comment to help you merge this pull request.

The full list of commands accepted by this bot can be found here.

Reviewer can indicate their review by submitting an approval review.
Reviewer can cancel approval by submitting a request changes review.

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

MyonKeminta · 2023-01-11T13:42:24Z

/release

sre-bot · 2023-01-11T14:28:04Z

download tikv at http://fileserver.pingcap.net/download/builds/pingcap/tikv/pr/6cab3d614356f74e3a508c29489b2e032ba18d56/centos7/tikv-server.tar.gz

sticnarf · 2023-01-12T06:48:16Z

src/storage/txn/actions/acquire_pessimistic_lock.rs

+            if let Some((write, commit_ts)) = write {
+                // Here `get_write_with_commit_ts` returns only the latest PUT if it exists and
+                // is not deleted. It's still ok to pass it into `check_data_constraint`.
+                if locked_with_conflict_ts.is_none() {


Can you add a comment about why locked_with_conflict_ts cases can be skipped? I cannot think of any problem but I also want to hear how you think about it.

Would it be a problem for this case:

Raise an Insert pessimistic lock

Locking is successful with conflict and the constraint check is not done

Response of 2 is lost, the client retry

The locked_with_conflict_ts is Some and the constraint check is skipped unexpectedly

It acquires the lock and lets the client (TiDB) retry the statement in this case. TiDB will find the key already exists when it retries the statement. It's a part of avoiding the key being locked by another transaction when the current transaction does a statement retry, making the retry a waste.
But actually I want to adjust this behavior later: When should_not_exist is set and the key exists and the latest version is newer than for_update_ts, return write conflict error even allow_lock_with_conflict is set. I think in most cases, when the statement retries, it's likely that it still want to insert the same key and it should still fail.

Would it be a problem for this case:

Raise an Insert pessimistic lock

Locking is successful with conflict and the constraint check is not done

Response of 2 is lost, the client retry

The locked_with_conflict_ts is Some and the constraint check is skipped unexpectedly

In the current implementation (which I'm planning to adjust later), the retried request in the 4th step will still produce a locked_with_conflict result, and TiDB will still retry. It will notice the key already exists when retrying the statement and abort the statement then, at which time the lock will be pessimistic_rollback-ed

Got it. So the correctness depends on that the returned pessimistic lock result must contain "value existing" information, maybe we could add this in the comment too in case we forget to return the required information?

The Value field is required in PessimisticLockKeyResult::LockedWithConflict. I think it's just fine for locked_with_conflict cases? I'm actually not very sure where you prefer to add the comment you said

Ok, it's fine.

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

sticnarf

LGTM

MyonKeminta · 2023-01-12T13:41:32Z

/merge

ti-chi-bot · 2023-01-12T13:41:33Z

@MyonKeminta: It seems you want to merge this PR, I will help you trigger all the tests:

/run-all-tests

You only need to trigger /merge once, and if the CI test fails, you just re-trigger the test that failed and the bot will merge the PR for you after the CI passes.

If you have any questions about the PR merge process, please refer to pr process.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository.

MyonKeminta · 2023-01-13T03:37:14Z

/merge

ti-chi-bot · 2023-01-13T03:37:15Z

@MyonKeminta: It seems you want to merge this PR, I will help you trigger all the tests:

/run-all-tests

You only need to trigger /merge once, and if the CI test fails, you just re-trigger the test that failed and the bot will merge the PR for you after the CI passes.

If you have any questions about the PR merge process, please refer to pr process.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository.

ti-chi-bot · 2023-01-13T03:37:16Z

This pull request has been accepted and is ready to merge.

Commit hash: 6824bba

…ence

ti-chi-bot · 2023-01-13T03:37:29Z

@MyonKeminta: Your PR was out of date, I have automatically updated it for you.

If the CI test fails, you just re-trigger the test that failed and the bot will merge the PR for you after the CI passes.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository.

ti-chi-bot · 2023-01-13T03:50:57Z

In response to a cherrypick label: new pull request created to branch release-5.3: #14047.

close tikv#14038, close pingcap/tidb#40114 Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>

ti-chi-bot · 2023-01-13T03:51:36Z

In response to a cherrypick label: new pull request created to branch release-5.4: #14048.

ti-chi-bot · 2023-01-13T03:52:14Z

In response to a cherrypick label: new pull request created to branch release-6.1: #14049.

close tikv#14038, close pingcap/tidb#40114 Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>

ti-chi-bot · 2023-01-13T03:52:52Z

In response to a cherrypick label: new pull request created to branch release-6.5: #14050.

…ock request (#14037) (#14049) ref #14037, close #14038, close pingcap/tidb#40114 Fixes the problem that when handling repeated acquire_pessimistic_lock requests is recevied, should_not_exist is ignored. TiKV provides idempotency for these RPC requests, but for acquire_pessimistic_lock, it ignored the possibility that the client may expect a pessimistic_rollback between two acquire_pessimistic_lock request on the same key. In this case the second request may come from another statement and carries `should_not_exist` that wasn't set in the previously finished pessimistic lock request. If the first request successfully acquired the lock and the pessimistic_rollback failed, TiKV may return a sucessful response, making the client believe that the key doesn't exist before. In some rare cases, this has risk to cause data inconsistency. Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io> Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com> Co-authored-by: MyonKeminta <9948422+MyonKeminta@users.noreply.github.com> Co-authored-by: MyonKeminta <MyonKeminta@users.noreply.github.com>

…ock request (#14037) (#14050) ref #14037, close #14038, close pingcap/tidb#40114 Fixes the problem that when handling repeated acquire_pessimistic_lock requests is recevied, should_not_exist is ignored. TiKV provides idempotency for these RPC requests, but for acquire_pessimistic_lock, it ignored the possibility that the client may expect a pessimistic_rollback between two acquire_pessimistic_lock request on the same key. In this case the second request may come from another statement and carries `should_not_exist` that wasn't set in the previously finished pessimistic lock request. If the first request successfully acquired the lock and the pessimistic_rollback failed, TiKV may return a sucessful response, making the client believe that the key doesn't exist before. In some rare cases, this has risk to cause data inconsistency. Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com> Co-authored-by: MyonKeminta <MyonKeminta@users.noreply.github.com>

MyonKeminta added 2 commits January 11, 2023 19:24

Doconstraint check when handling idempotent pessimsitic lock requests

604ae60

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

Fix warning and format

ee12f37

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

Fix lint

c1264a9

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

ti-chi-bot added do-not-merge/needs-triage-completed and removed do-not-merge/needs-linked-issue labels Jan 11, 2023

Fix lint

6cab3d6

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

MyonKeminta mentioned this pull request Jan 11, 2023

tests: Add test case for the constraint check on repeated pessimistic lock request on left pessimistic lock pingcap/tidb#40520

Merged

9 tasks

MyonKeminta marked this pull request as ready for review January 11, 2023 13:48

ti-chi-bot removed the do-not-merge/work-in-progress label Jan 11, 2023

MyonKeminta requested review from cfzjywxk and ekexium January 12, 2023 05:07

cfzjywxk requested a review from sticnarf January 12, 2023 05:27

sticnarf reviewed Jan 12, 2023

View reviewed changes

Address comments

6824bba

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

MyonKeminta requested a review from sticnarf January 12, 2023 12:27

sticnarf approved these changes Jan 12, 2023

View reviewed changes

cfzjywxk approved these changes Jan 12, 2023

View reviewed changes

ti-chi-bot removed the do-not-merge/needs-triage-completed label Jan 13, 2023

cfzjywxk approved these changes Jan 13, 2023

View reviewed changes

ti-chi-bot added status/LGT2 Status: PR - There are already 2 approvals and removed status/LGT1 Status: PR - There is already 1 approval labels Jan 13, 2023

ti-chi-bot added the status/can-merge Status: Can merge to base branch label Jan 13, 2023

Merge branch 'master' into m/pessimistic-lock-idempotency-check-exist…

4d371d0

…ence

ti-chi-bot merged commit 321aa83 into tikv:master Jan 13, 2023

ti-chi-bot added this to the Pool milestone Jan 13, 2023

ti-chi-bot mentioned this pull request Jan 13, 2023

txn: Do constraint check when handling repeated acqurie_pessimsitic_lock request (#14037) #14047

Closed

ti-chi-bot pushed a commit to ti-chi-bot/tikv that referenced this pull request Jan 13, 2023

This is an automated cherry-pick of tikv#14037

61d14c7

close tikv#14038, close pingcap/tidb#40114 Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>

ti-chi-bot mentioned this pull request Jan 13, 2023

txn: Do constraint check when handling repeated acqurie_pessimsitic_lock request (#14037) #14048

Open

ti-chi-bot pushed a commit to ti-chi-bot/tikv that referenced this pull request Jan 13, 2023

This is an automated cherry-pick of tikv#14037

970f459

close tikv#14038, close pingcap/tidb#40114 Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>

ti-chi-bot mentioned this pull request Jan 13, 2023

txn: Do constraint check when handling repeated acqurie_pessimsitic_lock request (#14037) #14049

Merged

ti-chi-bot pushed a commit to ti-chi-bot/tikv that referenced this pull request Jan 13, 2023

This is an automated cherry-pick of tikv#14037

f8a6ed0

close tikv#14038, close pingcap/tidb#40114 Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>

ti-chi-bot mentioned this pull request Jan 13, 2023

txn: Do constraint check when handling repeated acqurie_pessimsitic_lock request (#14037) #14050

Merged

MyonKeminta deleted the m/pessimistic-lock-idempotency-check-existence branch January 13, 2023 03:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

txn: Do constraint check when handling repeated acqurie_pessimsitic_lock request #14037

txn: Do constraint check when handling repeated acqurie_pessimsitic_lock request #14037

MyonKeminta commented Jan 11, 2023 •

edited

ti-chi-bot commented Jan 11, 2023 •

edited

MyonKeminta commented Jan 11, 2023

sre-bot commented Jan 11, 2023

sticnarf Jan 12, 2023

cfzjywxk Jan 12, 2023

MyonKeminta Jan 12, 2023 •

edited

MyonKeminta Jan 12, 2023

cfzjywxk Jan 12, 2023

MyonKeminta Jan 12, 2023

cfzjywxk Jan 12, 2023

sticnarf left a comment

MyonKeminta commented Jan 12, 2023

ti-chi-bot commented Jan 12, 2023

MyonKeminta commented Jan 13, 2023

ti-chi-bot commented Jan 13, 2023

ti-chi-bot commented Jan 13, 2023

ti-chi-bot commented Jan 13, 2023

ti-chi-bot commented Jan 13, 2023

ti-chi-bot commented Jan 13, 2023

ti-chi-bot commented Jan 13, 2023

ti-chi-bot commented Jan 13, 2023

txn: Do constraint check when handling repeated acqurie_pessimsitic_lock request #14037

txn: Do constraint check when handling repeated acqurie_pessimsitic_lock request #14037

Conversation

MyonKeminta commented Jan 11, 2023 • edited

What is changed and how it works?

Related changes

Check List

Release note

ti-chi-bot commented Jan 11, 2023 • edited

MyonKeminta commented Jan 11, 2023

sre-bot commented Jan 11, 2023

sticnarf Jan 12, 2023

Choose a reason for hiding this comment

cfzjywxk Jan 12, 2023

Choose a reason for hiding this comment

MyonKeminta Jan 12, 2023 • edited

Choose a reason for hiding this comment

MyonKeminta Jan 12, 2023

Choose a reason for hiding this comment

cfzjywxk Jan 12, 2023

Choose a reason for hiding this comment

MyonKeminta Jan 12, 2023

Choose a reason for hiding this comment

cfzjywxk Jan 12, 2023

Choose a reason for hiding this comment

sticnarf left a comment

Choose a reason for hiding this comment

MyonKeminta commented Jan 12, 2023

ti-chi-bot commented Jan 12, 2023

MyonKeminta commented Jan 13, 2023

ti-chi-bot commented Jan 13, 2023

ti-chi-bot commented Jan 13, 2023

ti-chi-bot commented Jan 13, 2023

ti-chi-bot commented Jan 13, 2023

ti-chi-bot commented Jan 13, 2023

ti-chi-bot commented Jan 13, 2023

ti-chi-bot commented Jan 13, 2023

MyonKeminta commented Jan 11, 2023 •

edited

ti-chi-bot commented Jan 11, 2023 •

edited

MyonKeminta Jan 12, 2023 •

edited