Skip to content

store/tikv: make snapshot thread safe (#17593)#18145

Merged
jebter merged 11 commits intopingcap:release-4.0from
tiancaiamao:cherry-17593
Jun 30, 2020
Merged

store/tikv: make snapshot thread safe (#17593)#18145
jebter merged 11 commits intopingcap:release-4.0from
tiancaiamao:cherry-17593

Conversation

@tiancaiamao
Copy link
Contributor

@tiancaiamao tiancaiamao commented Jun 19, 2020

What problem does this PR solve?

Cherry pick #17593 to release-4.0

Problem Summary:

A correct fix need to cherry pick a series of commits:

#16056
#17555
#17593
#17678
#17911

That maybe too much for 4.0

What is changed and how it works?

make snapshot thread safe

Check List

Tests

  • Unit test

Release note

  • No release note

@tiancaiamao
Copy link
Contributor Author

tiancaiamao commented Jun 19, 2020

PTAL @coocood @lysu

@github-actions github-actions bot added the sig/execution SIG execution label Jun 19, 2020
@ti-srebot
Copy link
Contributor

ti-srebot commented Jun 28, 2020

@coocood,Thanks for you review.

@coocood
Copy link
Member

coocood commented Jun 28, 2020

@tiancaiamao
Just move code from Next to Open doesn't completely fix the bug.
The PointGet executor maybe built many times during execution. for example, in IndexJoin.

@tiancaiamao
Copy link
Contributor Author

tiancaiamao commented Jun 29, 2020 •
edited
Loading

@tiancaiamao
Just move code from Next to Open doesn't completely fix the bug.
The PointGet executor maybe built many times during execution. for example, in IndexJoin.

Move code from Next to Open so the snapshot is not captured everytime and reduce conflict (the transaction is modified and snapshot may be nil)

This code is not safe, as txn maybe set to nil after Valid() and before GetSnapshot.

if txn.Valid() {
     snapshot = txn.GetSnapshot()
}

I override GetSnapshot to provide the snapshot.
Not in a atomic way, but it should work , as a pointer is in a machine word size (assumption on hardware), either nil or a kv.Snapshot is OK.

@tiancaiamao
Copy link
Contributor Author

tiancaiamao commented Jun 29, 2020

Link issue #16817

@tiancaiamao
Copy link
Contributor Author

tiancaiamao commented Jun 29, 2020

/run-all-tests

@tiancaiamao
Copy link
Contributor Author

tiancaiamao commented Jun 29, 2020

github is unavailable

@tiancaiamao tiancaiamao reopened this Jun 29, 2020
@coocood
Copy link
Member

coocood commented Jun 29, 2020

LGTM

@ti-srebot
Copy link
Contributor

ti-srebot commented Jun 29, 2020

@coocood, Thanks for your review, however we are sorry that your vote won't be count. You already give a LGTM to this PR

@coocood
Copy link
Member

coocood commented Jun 29, 2020

@bobotu PTAL

bobotu
bobotu previously approved these changes Jun 29, 2020
View reviewed changes
Copy link
Contributor

@bobotu bobotu left a comment

Choose a reason for hiding this comment

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

Not in a atomic way, but it should work , as a pointer is in a machine word size (assumption on hardware), either nil or a kv.Snapshot is OK.

This may have a problem with ARMv7?

In particular, the model was originally non-multicopy-atomic: writes could become visible to some other threads before becoming visible to all

Anyway, it isn't a blocker for this PR.

LGTM

@ti-srebot
Copy link
Contributor

ti-srebot commented Jun 29, 2020

@bobotu, Thanks for your review, however we are sorry that your vote won't be count. You are not a reviewer or committer or co-leader or leader for the related sigs:execution(slack).

coocood
coocood reviewed Jun 30, 2020
View reviewed changes
coocood
coocood reviewed Jun 30, 2020
View reviewed changes
@coocood
Copy link
Member

coocood commented Jun 30, 2020

LGTM

@ti-srebot
Copy link
Contributor

ti-srebot commented Jun 30, 2020

@coocood, Thanks for your review, however we are sorry that your vote won't be count. You already give a LGTM to this PR

@ti-srebot
Copy link
Contributor

ti-srebot commented Jun 30, 2020

@SunRunAway,Thanks for you review.

@jebter
Copy link

jebter commented Jun 30, 2020

/merge

2 similar comments
@jebter
Copy link

jebter commented Jun 30, 2020

/merge

@zhouqiang-cl
Copy link
Contributor

zhouqiang-cl commented Jun 30, 2020

/merge

@ti-srebot ti-srebot added the status/can-merge Indicates a PR has been approved by a committer. label Jun 30, 2020
@ti-srebot
Copy link
Contributor

ti-srebot commented Jun 30, 2020

/run-all-tests

@ti-srebot
Copy link
Contributor

ti-srebot commented Jun 30, 2020

@tiancaiamao merge failed.

@jebter
Copy link

jebter commented Jun 30, 2020

/run-common-test

@jebter
Copy link

jebter commented Jun 30, 2020

/run-check_dev

@jebter jebter merged commit 64f64fc into pingcap:release-4.0 Jun 30, 2020
@ichn-hu ichn-hu mentioned this pull request Sep 2, 2020
Closed
@tiancaiamao tiancaiamao deleted the cherry-17593 branch September 7, 2020 07:59
@tiancaiamao tiancaiamao mentioned this pull request Sep 7, 2020
Merged
@tiancaiamao tiancaiamao mentioned this pull request Jan 20, 2021
Merged
@AilinKid AilinKid mentioned this pull request Jan 20, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@coocood coocood coocood left review comments

+2 more reviewers

@SunRunAway SunRunAway SunRunAway approved these changes

@bobotu bobotu bobotu left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

sig/execution SIG execution status/can-merge Indicates a PR has been approved by a committer. type/4.0-cherry-pick

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@tiancaiamao @ti-srebot @coocood @jebter @zhouqiang-cl @SunRunAway @bobotu