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

Assume fast_tombstone_all is set when deleting data #174

Closed
GemDot-neubla opened this issue Mar 29, 2023 · 1 comment · Fixed by #366
Closed

Assume fast_tombstone_all is set when deleting data #174

GemDot-neubla opened this issue Mar 29, 2023 · 1 comment · Fixed by #366
Assignees
@vasil-pashov
Labels
enhancement New feature or request

Comments

@GemDot-neubla
Copy link
Contributor

GemDot-neubla commented Mar 29, 2023 •
edited by mehertz
Loading

When deleting data, either a tombstone or a TOMBSTONE_ALL key will be written.

Whilst we need to respect either key type when reading data (for backwards compatibility reasons), there is no longer any reason to not write TOMBSTONE_ALL keys.

This change will require simplifying the flow for deleting data to remove any code branches that would execute should fast_tombstone_all be off.
@GemDot-neubla GemDot-neubla self-assigned this Mar 29, 2023
@mehertz mehertz changed the title Remove non-fast tombstones flow. We only want to use fast tombstones for all uses. It includes to remove corresponding code in the write code Assume fast_tombstone_all is set when deleting data Apr 2, 2023
@mehertz mehertz added the enhancement New feature or request label Apr 2, 2023
@vasil-pashov vasil-pashov mentioned this issue Jun 8, 2023
Merged
@vasil-pashov
Copy link
Collaborator

PR: #366

@mehertz mehertz linked a pull request Jun 9, 2023 that will close this issue
Assume fast tombstone is true and fix bug in VersionMapImpl::follow_version_chain #366
Merged
vasil-pashov added a commit that referenced this issue Jun 9, 2023
@vasil-pashov
Assume fast tombstone is true and fix bug in VersionMapImpl::follow_v…
d454f0e 
…ersion_chain (#366)

Resolves #174 

**Changes**

* Remove all branches involving fast tombstone property assuming it's
always true.
* Fix a bug in follow version chain functionality.

**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 use
`prune_previous_versions`. Imagine the following calls:
1. Add version 0 for a symbol `sym`
2. Add version 1 for the symbol `sym`
3. Prune previous versions

This would result in the following version chain:

0. `(key_type: tombstone_all, version_id: 0)`
1. `(key_type: version, version_id: 0)`
2. `(key_type: index, version_id: 1)`
3. `(key_type: version, version_id: 1)`
5. `(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 the `tombstone_all` key and after that we write the new
version.

**Solution**
In case there is `tombstone_all` key, in
`VersionMapImpl::follow_version_chain` we need to keep track of the
oldest index which was read and stop reading only if
`tombstone_all_->version_id()` is greater or equal to the `version_id`
of the oldest index.

---------

Co-authored-by: Vasil Pashov <Vasil.Pashov@man.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@vasil-pashov vasil-pashov

Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Assume fast tombstone is true and fix bug in VersionMapImpl::follow_version_chain man-group/ArcticDB
3 participants
@mehertz @vasil-pashov @GemDot-neubla