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.

Sign up for GitHub

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

Jump to bottom

txn: make kv pairs converted from locks invisible #42409

Merged
merged 3 commits into from
Mar 21, 2023
Merged

txn: make kv pairs converted from locks invisible #42409

merged 3 commits into from
Mar 21, 2023

Conversation

zyguan
Copy link
Contributor

@zyguan zyguan commented Mar 20, 2023

What problem does this PR solve?

Issue Number: ref #28011

Problem Summary: as described in #28011 , kv pairs converted from locks is visible to union-scan, which leads to duplicated rows (one from snapshot and the other from mem-index-reader).

What is changed and how it works?

Mark keys converted from lock as invisible and bypass those keys on read. Since this PR is only a hotfix for #28011 , it's better not to introduce extra complexity. Thus, instead of adding a new flag to client-go, an invisible key set is added to tikvTxn for tracking key visibility.

For v6.5.0 and above, tikv/tikv#13694 is a better solution.

Check List

Tests

  • Unit test
  • Integration test
  • Manual test (add detailed scripts or steps below)
  • No code

Side effects

  • Performance regression: Consumes more CPU
  • Performance regression: Consumes more Memory
  • Breaking backward compatibility

Documentation

  • Affects user behaviors
  • Contains syntax changes
  • Contains variable changes
  • Contains experimental features
  • Changes MySQL compatibility

Release note

Please refer to Release Notes Language Style Guide to write a quality release note.

None

@zyguan zyguan requested a review from cfzjywxk March 20, 2023 17:08
@ti-chi-bot
Copy link
Member

ti-chi-bot commented Mar 20, 2023 •
edited
Loading

[REVIEW NOTIFICATION]

This pull request has been approved by:

  • cfzjywxk
  • ekexium
  • you06

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.

@ti-chi-bot ti-chi-bot added release-note-none Denotes a PR that doesn't merit a release note. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Mar 20, 2023
@zyguan
Copy link
Contributor Author

zyguan commented Mar 21, 2023

/test unit-test

cfzjywxk
cfzjywxk previously approved these changes Mar 21, 2023
View reviewed changes
@ti-chi-bot ti-chi-bot added the status/LGT1 Indicates that a PR has LGTM 1. label Mar 21, 2023
@cfzjywxk cfzjywxk added the require-LGT3 Indicates that the PR requires three LGTM. label Mar 21, 2023
ekexium
ekexium previously approved these changes Mar 21, 2023
View reviewed changes
Copy link
Contributor

@ekexium ekexium left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The rest LGTM.

store/driver/txn/unionstore_driver.go Outdated
)

type visibilityChecker interface {
isVisible(kv.Key) bool
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The name is a bit vague. The status is 3-valued: (1)exist + visible, (2)exist + invisible, (3)absent(unknown).
It returns true iff (1) || (3).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about to rename it to invisible?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good. Or something like isVisibleOrUnknown(?)

@ti-chi-bot ti-chi-bot added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Mar 21, 2023
you06
you06 previously approved these changes Mar 21, 2023
View reviewed changes
@ti-chi-bot ti-chi-bot added status/LGT3 The PR has already had 3 LGTM. and removed status/LGT2 Indicates that a PR has LGTM 2. labels Mar 21, 2023
@zyguan
address the comment
b789231 
Signed-off-by: zyguan <[email protected]>
@zyguan zyguan dismissed stale reviews from you06, ekexium, and cfzjywxk via b789231 March 21, 2023 03:32
@purelind
Copy link
Contributor

/test mysql-test

cfzjywxk
cfzjywxk previously approved these changes Mar 21, 2023
View reviewed changes
@cfzjywxk
Copy link
Contributor

/merge

@ti-chi-bot
Copy link
Member

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

Commit hash: b789231

@ti-chi-bot ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Mar 21, 2023
@zyguan
fix check_dev
d8483a8 
Signed-off-by: zyguan <[email protected]>
@zyguan zyguan requested a review from a team as a code owner March 21, 2023 10:50
@ti-chi-bot ti-chi-bot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed status/can-merge Indicates a PR has been approved by a committer. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Mar 21, 2023
@zyguan
Copy link
Contributor Author

zyguan commented Mar 21, 2023

/merge

@ti-chi-bot
Copy link
Member

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

Commit hash: d8483a8

@ti-chi-bot ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Mar 21, 2023
@zyguan
Copy link
Contributor Author

zyguan commented Mar 21, 2023

/retest

1 similar comment
@zyguan
Copy link
Contributor Author

zyguan commented Mar 21, 2023

/retest

@zyguan
Copy link
Contributor Author

zyguan commented Mar 21, 2023

/test unit-test

@zyguan
Copy link
Contributor Author

zyguan commented Mar 21, 2023

/test check-dev

@ti-chi-bot ti-chi-bot merged commit 3ff66ac into pingcap:release-6.1-20230221-v6.1.1 Mar 21, 2023
This was referenced Mar 23, 2023
Closed
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ekexium ekexium ekexium left review comments

@MyonKeminta MyonKeminta MyonKeminta approved these changes

@cfzjywxk cfzjywxk cfzjywxk left review comments

@you06 you06 you06 left review comments

Assignees
No one assigned
Labels
release-note-none Denotes a PR that doesn't merit a release note. require-LGT3 Indicates that the PR requires three LGTM. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. status/can-merge Indicates a PR has been approved by a committer. status/LGT3 The PR has already had 3 LGTM.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@zyguan @ti-chi-bot @purelind @cfzjywxk @you06 @MyonKeminta @ekexium