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.

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

Fix reverse iterator in RocksDB #2398

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Fix reverse iterator in RocksDB #2398

wants to merge 3 commits into from

Conversation

AurelienFT
Copy link
Contributor

@AurelienFT AurelienFT commented Oct 25, 2024

Linked Issues/PRs

Closes #2044

Description

Problem analysis

Our database columns use the prefix extractor configuration which allow rocksdb to make optimizations on storage based on prefix. However, this breaks the lexicographic order between prefixs and we need to pass a special option to the read iterator to bypass the "prefix sharding" (source: https://github.com/facebook/rocksdb/wiki/Prefix-Seek/3a5e28faf6c2d0ec1bdb99763043e1e3322007e9#how-to-ignore-prefix-bloom-filters-in-read).

Current solution

I took the same approach as before and started iteration from the next prefix using the bypass read option argument.

However I change the behavior to make only iterator to simplify and optimize the solution. But now, this iterator can include elements that have the next prefix in their key and so I added a combinator skip_while to skip them.

Better solution for the future

I think that breaking the "prefix sharding" from RocksDB to iterate can make the iterator way more costly.

There is a way to avoid it because we can iterate in reverse order inside a prefix (source: https://github.com/facebook/rocksdb/wiki/SeekForPrev) but we need to get the maximum value for a key in a given prefix to use it as a starting point of our iteration which is not possible to get in the current code because we don't have any information about the key.

Happy to here your thoughts @FuelLabs/client :)

Checklist

  • Breaking changes are clearly marked as such in the PR description and changelog
  • New behavior is reflected in tests
  • The specification matches the implemented behavior (link update PR if changes are needed)

Before requesting review

  • I have reviewed the code myself
  • I have created follow-up issues caused by this PR and linked them here

@AurelienFT AurelienFT changed the title Add first working version but not sure of the correctness. Fix reverse iterator in RocksDB Oct 25, 2024
Copy link
Collaborator

@xgreenx xgreenx left a comment

Choose a reason for hiding this comment

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

It would be nice to have tests for reverse iteration with prefix. If can use examples from the mainnet, would be nice

Comment on lines +459 to +463
if let Ok(item) = item {
T::starts_with(item, next_prefix.as_slice())
} else {
false
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmm, it looks like it will not work properly in the next case:

Database has keys with following prefix [1, 3]. If you want to iterate over prefix 1, your code will use 2 as a next prefix and return 3 as next near entry. It leads to returning 3 and 1 after, while we expect only 1.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No from what I tested it doesn't work like that as the rocksDB function use by our iterator is SeekForPrev we will not go after 2 and so if two doesn't exists it will take the key that is the nearest but lower than 2. In your case it's 1. I will add tests and you will see it from tests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants