Fix DeleteAll for CacheKV/MultiVersionKV so that all keys are properly deleted #464
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
udpatil
reviewed
Mar 19, 2024
| keyStrs := map[string]struct{}{} | ||
| for _, pk := range v.parent.GetAllKeyStrsInRange(start, end) { | ||
| keyStrs[pk] = struct{}{} | ||
| } |
There was a problem hiding this comment.
we need to also consider the multiVersionStore when getting all the keys in range, since earlier txs potentially may have written within the range or deleted keys within the range, both of which should affect the result of this function. As such, we would also need to update readsets here, right? we should add all of these keys read both from parent OR from multiversion store to the readset so that the DeleteAll is properly backed by an updated readset representing the accesses of those keys / values
There was a problem hiding this comment.
actually, we should do something like iterateset in this case, because the keys that are accessed within the range is important here
There was a problem hiding this comment.
I think the easiest solution in this case (perhaps not the most optimal but should be correct) would be to create an Iterator within deleteAll, iterate over the whole iterator collecting keys, and THEN call delete on each one. Then you get the iteration safety out of the box
udpatil
requested changes
Mar 19, 2024
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## seiv2 #464 +/- ##
==========================================
- Coverage 55.36% 55.34% -0.02%
==========================================
Files 629 629
Lines 53878 53943 +65
==========================================
+ Hits 29827 29855 +28
- Misses 21917 21955 +38
+ Partials 2134 2133 -1
|
udpatil
reviewed
Mar 21, 2024
| vis.Set([]byte("key5"), []byte("value7")) | ||
| vis.Delete([]byte("key6")) | ||
| require.Equal(t, []string{"key1", "key2", "key4", "key5"}, vis.GetAllKeyStrsInRange(nil, nil)) | ||
| } |
There was a problem hiding this comment.
can we add an additional unit test that does the following:
has some keys in parent store, some written by tx1, and tx3 has some in its writeset
then tx3 does a delete all across the range
then tx2 writes its writeset (including a key within the target range)
Then we do validate transaction state which should be false due to that key added by tx2 that wasnt caught into deleteAll
udpatil
approved these changes
Mar 21, 2024
udpatil
pushed a commit
that referenced
this pull request
Mar 26, 2024
…y deleted (#464) ## Describe your changes and provide context If we have a cacheKV that has a dirty cache of 1->a and a parent with an entry 2->b, the previous implementation of `DeleteAll` would only call cacheKV.Delete on key 1 but not affecting 2 at all (i.e. if someone queries key 2 on the cacheKV they would still get `b`, but the expectation is for 2 to be deleted as appearing on the cacheKV level). This PR fixes that by first getting all the keys to be (marked as) deleted recursively and then delete all those keys one-by-one; note that this recursion should be fairly efficient since it's not subject to the exponential latency that merge iterators have. This PR also fixed DeleteAll for `mvkv` so that it doesn't actually touch parent store's value but only change the writeset (as a single `Delete` would have done). ## Testing performed to validate your change unit test
udpatil
pushed a commit
that referenced
this pull request
Mar 26, 2024
…y deleted (#464) ## Describe your changes and provide context If we have a cacheKV that has a dirty cache of 1->a and a parent with an entry 2->b, the previous implementation of `DeleteAll` would only call cacheKV.Delete on key 1 but not affecting 2 at all (i.e. if someone queries key 2 on the cacheKV they would still get `b`, but the expectation is for 2 to be deleted as appearing on the cacheKV level). This PR fixes that by first getting all the keys to be (marked as) deleted recursively and then delete all those keys one-by-one; note that this recursion should be fairly efficient since it's not subject to the exponential latency that merge iterators have. This PR also fixed DeleteAll for `mvkv` so that it doesn't actually touch parent store's value but only change the writeset (as a single `Delete` would have done). ## Testing performed to validate your change unit test
udpatil
pushed a commit
that referenced
this pull request
Mar 27, 2024
…y deleted (#464) ## Describe your changes and provide context If we have a cacheKV that has a dirty cache of 1->a and a parent with an entry 2->b, the previous implementation of `DeleteAll` would only call cacheKV.Delete on key 1 but not affecting 2 at all (i.e. if someone queries key 2 on the cacheKV they would still get `b`, but the expectation is for 2 to be deleted as appearing on the cacheKV level). This PR fixes that by first getting all the keys to be (marked as) deleted recursively and then delete all those keys one-by-one; note that this recursion should be fairly efficient since it's not subject to the exponential latency that merge iterators have. This PR also fixed DeleteAll for `mvkv` so that it doesn't actually touch parent store's value but only change the writeset (as a single `Delete` would have done). ## Testing performed to validate your change unit test
udpatil
pushed a commit
that referenced
this pull request
Apr 16, 2024
…y deleted (#464) ## Describe your changes and provide context If we have a cacheKV that has a dirty cache of 1->a and a parent with an entry 2->b, the previous implementation of `DeleteAll` would only call cacheKV.Delete on key 1 but not affecting 2 at all (i.e. if someone queries key 2 on the cacheKV they would still get `b`, but the expectation is for 2 to be deleted as appearing on the cacheKV level). This PR fixes that by first getting all the keys to be (marked as) deleted recursively and then delete all those keys one-by-one; note that this recursion should be fairly efficient since it's not subject to the exponential latency that merge iterators have. This PR also fixed DeleteAll for `mvkv` so that it doesn't actually touch parent store's value but only change the writeset (as a single `Delete` would have done). ## Testing performed to validate your change unit test
udpatil
pushed a commit
that referenced
this pull request
Apr 19, 2024
…y deleted (#464) ## Describe your changes and provide context If we have a cacheKV that has a dirty cache of 1->a and a parent with an entry 2->b, the previous implementation of `DeleteAll` would only call cacheKV.Delete on key 1 but not affecting 2 at all (i.e. if someone queries key 2 on the cacheKV they would still get `b`, but the expectation is for 2 to be deleted as appearing on the cacheKV level). This PR fixes that by first getting all the keys to be (marked as) deleted recursively and then delete all those keys one-by-one; note that this recursion should be fairly efficient since it's not subject to the exponential latency that merge iterators have. This PR also fixed DeleteAll for `mvkv` so that it doesn't actually touch parent store's value but only change the writeset (as a single `Delete` would have done). ## Testing performed to validate your change unit test
udpatil
pushed a commit
that referenced
this pull request
Apr 19, 2024
…y deleted (#464) ## Describe your changes and provide context If we have a cacheKV that has a dirty cache of 1->a and a parent with an entry 2->b, the previous implementation of `DeleteAll` would only call cacheKV.Delete on key 1 but not affecting 2 at all (i.e. if someone queries key 2 on the cacheKV they would still get `b`, but the expectation is for 2 to be deleted as appearing on the cacheKV level). This PR fixes that by first getting all the keys to be (marked as) deleted recursively and then delete all those keys one-by-one; note that this recursion should be fairly efficient since it's not subject to the exponential latency that merge iterators have. This PR also fixed DeleteAll for `mvkv` so that it doesn't actually touch parent store's value but only change the writeset (as a single `Delete` would have done). ## Testing performed to validate your change unit test
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Describe your changes and provide context
If we have a cacheKV that has a dirty cache of 1->a and a parent with an entry 2->b, the previous implementation of
DeleteAllwould only call cacheKV.Delete on key 1 but not affecting 2 at all (i.e. if someone queries key 2 on the cacheKV they would still getb, but the expectation is for 2 to be deleted as appearing on the cacheKV level). This PR fixes that by first getting all the keys to be (marked as) deleted recursively and then delete all those keys one-by-one; note that this recursion should be fairly efficient since it's not subject to the exponential latency that merge iterators have.This PR also fixed DeleteAll for
mvkvso that it doesn't actually touch parent store's value but only change the writeset (as a singleDeletewould have done).Testing performed to validate your change
unit test