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,session: invalidate snapshot cache under pessimistic transaction #12147

Merged
merged 3 commits into from
Sep 11, 2019

Conversation

tiancaiamao
Copy link
Contributor

@tiancaiamao tiancaiamao commented Sep 11, 2019

What problem does this PR solve?

Fix a bug in the pessimistic transaction that the transaction snapshot doesn't invalidate its cache after the snapshotTS changes.

insert into conflict values (1, 2);
begin pessimistic;
insert ignore into conflict values (1, 2);

update conflict set c = c - 1     // in another terminal

select * from conflict where id = 1 for update
insert into conflict values (1, 999) on duplicate key update c = c + 2
commit

select * from conflict

Expect:

mysql> select * from conflict;
+----+------+
| id | c    |
+----+------+
|  1 |    3 |
+----+------+
1 row in set (0.00 sec)

Get:

mysql> select * from conflict;
+----+------+
| id | c    |
+----+------+
|  1 |    4 |
+----+------+
1 row in set (0.00 sec)

What is changed and how it works?

In a pessimistic transaction, the snapshotTS may change, so we should invalidate
the cache if the snapshotTS changes

Check List

Tests

  • Unit test

…action

In a pessimistic transaction, the snapshotTS may change, so we should invalidate
the cache if the snapshotTS changes
@tiancaiamao tiancaiamao added type/bugfix This PR fixes a bug. component/tikv labels Sep 11, 2019
@tiancaiamao
Copy link
Contributor Author

PTAL @coocood @jackysp

tiancaiamao added a commit to tiancaiamao/tidb that referenced this pull request Sep 11, 2019
@codecov
Copy link

codecov bot commented Sep 11, 2019

Codecov Report

Merging #12147 into master will not change coverage.
The diff coverage is n/a.

@@             Coverage Diff             @@
##             master     #12147   +/-   ##
===========================================
  Coverage   81.3581%   81.3581%           
===========================================
  Files           453        453           
  Lines         97356      97356           
===========================================
  Hits          79207      79207           
  Misses        12485      12485           
  Partials       5664       5664

@tiancaiamao
Copy link
Contributor Author

I've updated the test case @coocood

@coocood
Copy link
Member

coocood commented Sep 11, 2019

LGTM

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.

LGTM

@jackysp jackysp added the status/can-merge Indicates a PR has been approved by a committer. label Sep 11, 2019
@sre-bot
Copy link
Contributor

sre-bot commented Sep 11, 2019

/run-all-tests

@sre-bot sre-bot merged commit 57da569 into pingcap:master Sep 11, 2019
@tiancaiamao tiancaiamao deleted the fix-snapshot-cache branch May 26, 2020 09:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component/tikv status/can-merge Indicates a PR has been approved by a committer. type/bugfix This PR fixes a bug.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants