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: check constraint for "Delete-Your-Writes" records when txn commit #14968

Merged
merged 10 commits into from
Mar 6, 2020
Merged

store: check constraint for "Delete-Your-Writes" records when txn commit #14968

merged 10 commits into from
Mar 6, 2020

Conversation

lysu
Copy link
Contributor

@lysu lysu commented Feb 27, 2020

What problem does this PR solve?

in #9127, we push constraint check down to tikv layer when prewrite phase.

but it miss the situation that txn add record a then delete record a, we also need check record is real duplicate when commit.

so just as TestDeferConstraintCheckForDelete, it will lead to "delete wrong data"(primary key) or "inconsist index data"(unique key)

What is changed and how it works?

we can do a addition batch get check for those key before commit just like pre-#9127 logic
but maybe better to follow #9127's idea just check it with prewrite mutation flag.

Check List

Tests

  • Unit test
  • Integration test

Code changes

  • impl change
  • kvproto
  • tikv/unistore

Side effects

  • n/a

Related changes

  • Need to cherry-pick to the release branch 3.0/3.1

Release note

  • fix missing check constraint for "Delete-Your-Writes" records.

This change is Reviewable

@lysu lysu added type/bugfix This PR fixes a bug. sig/transaction SIG:Transaction needs-cherry-pick-3.0 labels Feb 27, 2020
@lysu
Copy link
Contributor Author

lysu commented Feb 27, 2020

need merge pingcap/kvproto#565 first to let CI happy

and tikv will be handle by tikv/tikv#6715

@lysu
Copy link
Contributor Author

lysu commented Feb 28, 2020

/run-all-tests tikv=pr/6715

@lysu
Copy link
Contributor Author

lysu commented Feb 28, 2020

check_dev_2 fail due to it only run with tikv master by design, it should be green after kv pr merged.

Copy link
Contributor

@youjiali1995 youjiali1995 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

@coocood
Copy link
Member

coocood commented Mar 2, 2020

LGTM

@codecov
Copy link

codecov bot commented Mar 3, 2020

Codecov Report

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

@@             Coverage Diff             @@
##             master     #14968   +/-   ##
===========================================
  Coverage   80.4098%   80.4098%           
===========================================
  Files           503        503           
  Lines        133011     133011           
===========================================
  Hits         106954     106954           
  Misses        17679      17679           
  Partials       8378       8378

@lysu
Copy link
Contributor Author

lysu commented Mar 3, 2020

/rebuild

@lysu
Copy link
Contributor Author

lysu commented Mar 6, 2020

/run-all-tests

@lysu
Copy link
Contributor Author

lysu commented Mar 6, 2020

/run-unit-test

@youjiali1995 youjiali1995 added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Mar 6, 2020
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
Copy link
Member

jackysp commented Mar 6, 2020

/merge

@sre-bot sre-bot added the status/can-merge Indicates a PR has been approved by a committer. label Mar 6, 2020
@sre-bot
Copy link
Contributor

sre-bot commented Mar 6, 2020

Your auto merge job has been accepted, waiting for 14816

@sre-bot
Copy link
Contributor

sre-bot commented Mar 6, 2020

/run-all-tests

@sre-bot
Copy link
Contributor

sre-bot commented Mar 6, 2020

@lysu merge failed.

@lysu
Copy link
Contributor Author

lysu commented Mar 6, 2020

/merge

@sre-bot
Copy link
Contributor

sre-bot commented Mar 6, 2020

Your auto merge job has been accepted, waiting for 15172

@sre-bot
Copy link
Contributor

sre-bot commented Mar 6, 2020

/run-all-tests

@sre-bot
Copy link
Contributor

sre-bot commented Mar 6, 2020

@lysu merge failed.

@lysu
Copy link
Contributor Author

lysu commented Mar 6, 2020

/merge

@sre-bot
Copy link
Contributor

sre-bot commented Mar 6, 2020

/run-all-tests

@sre-bot sre-bot merged commit a37a0ff into pingcap:master Mar 6, 2020
sre-bot pushed a commit to sre-bot/tidb that referenced this pull request Mar 6, 2020
Signed-off-by: sre-bot <sre-bot@pingcap.com>
@sre-bot
Copy link
Contributor

sre-bot commented Mar 6, 2020

cherry pick to release-3.0 in PR #15176

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
sig/transaction SIG:Transaction status/can-merge Indicates a PR has been approved by a committer. status/LGT2 Indicates that a PR has LGTM 2. type/bugfix This PR fixes a bug.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants