Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
memdb: prevent iterator invalidation #1563
base: master
Are you sure you want to change the base?
memdb: prevent iterator invalidation #1563
Changes from all commits
1a6d100
c46d490
4c16a14
e1a3b5a
29fc98e
e906cd5
e672ef4
941d94b
de43ee4
b15c57a
9b45230
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The
WriteSeqNo
seems not to have the concurrency invariance likeSnapshotSeqNo
below, should atomic variable be used for it?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The purpose of the sequence number is to check interleaving read and write operations, but not concurrent access. Even if we change it to an atomic variable, concurrent read and write can still corrupt the iterator.
If there exists data race on this variable, there must be a bug on the caller side.
I've updated the comment to explain the rationale.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Trying to sort out the current issues:
Implementation level:
lastTraversedNode
,hitCount
, andmissCount
, several internal states of ART, are implemented as atomic variables. These states are modified by write operations, and the WriteSeqNo below will also be modified by write operations, requiring the upper layer to handle concurrency correctly.This part of the code implementation is confusing, should we make the internal states of ART to be single-threaded, while allowing concurrent operations and protection to be handled at the upper layer of
MemDB
? So there is no need to consider data races or introduce atomic variables in the ART code./cc @you06
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One difference between
WriteSeqNo
andlastTraversedNode
is that the latter one is updated during read operations. Concurrent read operations are allowed (by design?). So even if we acquire an Rlock of memdb for every read operationlastTraversedNode
still needs to be implemented by an atomic variable.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the traverse in snapshot-get do not cache the last-traversed node(maybe it's not worth cache for a snapshot-get), I think the
art.lastTraversedNode
can also be a non-atomic variable, buthitCount
andmissCount
should be atomic.client-go/internal/unionstore/art/art_snapshot.go
Line 79 in 456c3af