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

Keep Cache and Env alive with Rc #497

Merged
merged 6 commits into from Apr 18, 2021
Merged

Conversation

acrrd
Copy link
Contributor

@acrrd acrrd commented Feb 18, 2021

Fix #462.

  • Add a lifetime to Checkpoint to ensure that it does not outlive the DB
  • Wrap Cache and Env in an Rc and keep a copy of that in the database to ensure that they are not dropped before the database is.

This add two auxiliary structs where to keep Cache and Env that are set using set_row_cache in Options or set_block_cache, set_block_compressed in BlockBasedOptions. When a DB is open this structs are cloned and kept in the database to ensure the lifetime requirements are met.

This approach allow us to have an unchanged api (with the exception of Checkpoint life time) but the compiler will not help us if we forget to keep track of something. Implementing this with lifetime will require to have four lifetimes parameters in (at least), Options, BlockBasedOptioins, DB, and ColumnFamilyDescriptor.

Do we need to keep track of Cache and Env that are in the Options inside ColumnFamilyDescriptor when we open a database through them?

Copy link
Member

@stanislav-tkach stanislav-tkach left a comment

Choose a reason for hiding this comment

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

Looking good to me! 👍

@stanislav-tkach
Copy link
Member

Do we need to keep track of Cache and Env that are in the Options inside ColumnFamilyDescriptor when we open a database through them?

I'm not one hundred percent sure, but I'm afraid that yes. I need to check the RocksDB code more thoroughly, though.

}

#[derive(Default)]
pub struct BlockBasedOptionsMustOutliveDB {
Copy link
Member

Choose a reason for hiding this comment

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

Does it really need to be public rather than pub(crate) ? If so then I'd rename it to BlockBasedCache or something because it seems weird name as for public API.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It could be private!

@acrrd
Copy link
Contributor Author

acrrd commented Apr 4, 2021

I'm saving also the Options from a ColumnFAmilyDescriptor, we can remove this later if we see that is not needed.
We can have two databases crated with the same Env and Cache and send them to two different thread, so I changed Rc to Arc. I would like to add the test is_sync<OptionsMustOutliveDb> and is_send<OptionsMustOutliveDb> but at the moment Env and Cache are not marker Send/Sync. For Env a comment says that is safe to implemented them, I think would be ok to implement them also for Cache since it can be used from different database in different thread.

@aleksuss
Copy link
Member

aleksuss commented Apr 7, 2021

@acrrd resolve please conflicts. Thanks.

@acrrd
Copy link
Contributor Author

acrrd commented Apr 8, 2021

Done,thanks.

Copy link
Member

@aleksuss aleksuss left a comment

Choose a reason for hiding this comment

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

LGTM

@aleksuss aleksuss merged commit b7af394 into rust-rocksdb:master Apr 18, 2021
@acrrd acrrd deleted the fix_lifetimes branch April 18, 2021 20:56
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.

Issues with Cache
3 participants