-
Notifications
You must be signed in to change notification settings - Fork 92
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
Assume fast tombstone is true and fix bug in VersionMapImpl::follow_version_chain #366
Conversation
d727dfd
to
9a346bd
Compare
@@ -879,9 +875,13 @@ class VersionMapImpl { | |||
std::optional<AtomKey> first_key_to_tombstone = std::nullopt, | |||
std::shared_ptr<VersionMapEntry> entry = nullptr) { |
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.
I am starting to wonder if the first_key_to_tombstone
argument is safe before this change even @willdealtry
Calls using it basically assumes the version map has not changed without checking the timeout.
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.
I think in the case where we're writing version 5 with prune previous, we always want to tombstone from version 4 backwards, otherwise we might end up deleting the version we just wrote.
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.
Ah, yes, that's what it was for.
Would you agree that, ideally, we should check_reload
and then locate the version key in the VersionMapEntry
again? (I.e. not accepting the entry
argument.)
(Not that we should make that change in this PR
10876a7
to
34edd66
Compare
Remove all branches involving fast tombstone property assuming it's always true.
…isting symbol with no active versions. Now it throws only if the symbol does not exist.
the first tombstone_all key is reached. We may have tombstone_all key which deletes versions way down on the chain. Instead we should stop when the current key version is <= tombstone_all version.
34edd66
to
4abbc96
Compare
@@ -879,9 +875,13 @@ class VersionMapImpl { | |||
std::optional<AtomKey> first_key_to_tombstone = std::nullopt, | |||
std::shared_ptr<VersionMapEntry> entry = nullptr) { |
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.
I think in the case where we're writing version 5 with prune previous, we always want to tombstone from version 4 backwards, otherwise we might end up deleting the version we just wrote.
@@ -879,9 +875,13 @@ class VersionMapImpl { | |||
std::optional<AtomKey> first_key_to_tombstone = std::nullopt, | |||
std::shared_ptr<VersionMapEntry> entry = nullptr) { |
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.
Ah, yes, that's what it was for.
Would you agree that, ideally, we should check_reload
and then locate the version key in the VersionMapEntry
again? (I.e. not accepting the entry
argument.)
(Not that we should make that change in this PR
* Improve debug logging * Remove unit test
William gave verbal approval at the daily.
Resolves: #174
Changes
Bug description
Up to now the code assumes that there are no active versions after a tombstone_all key and stops following the version chain after encountering a
tombstone_all
key type. This is not true when we useprune_previous_versions
. Imagine the following calls:sym
sym
This would result in the following version chain:
(key_type: tombstone_all, version_id: 0)
(key_type: version, version_id: 0)
(key_type: index, version_id: 1)
(key_type: version, version_id: 1)
(key_type: index, version_id 0)
Where we have a live version (version 1) after the
tombstone_all
key.The bug does not appear when using
write + prune
because in that case we first add thetombstone_all
key and after that we write the new version.Solution
In case there is
tombstone_all
key, inVersionMapImpl::follow_version_chain
we need to keep track of the oldest index which was read and stop reading only iftombstone_all_->version_id()
is greater or equal to theversion_id
of the oldest index.