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 the Database implementation to address the documentation #144

Open
Kerollmops opened this issue Jan 5, 2023 · 4 comments
Open

Fix the Database implementation to address the documentation #144

Kerollmops opened this issue Jan 5, 2023 · 4 comments
Labels
bug Something isn't working
Milestone

Comments

@Kerollmops
Copy link
Member

The issue with the current heed library is that it is not ensuring either of those two points. Here is the documentation of mdb_dbi_open LMDB function:

  1. The database handle will be private to the current transaction until the transaction is successfully committed. If the transaction is aborted the handle will be closed automatically. After a successful commit the handle will reside in the shared environment, and may be used by other transactions.
  2. This function must not be called from multiple concurrent transactions in the same process. A transaction that uses this function must finish (either commit or abort) before any other transaction in the process may use this function.
@Kerollmops
Copy link
Member Author

Kerollmops commented Jan 15, 2023

After a small meeting with @dureuill, we thought about a possible solution to make it safe to use databases with heed and follow the limits of LMDB, even if it can be a little bit more restrictive.

When we open an environment for the first time, we list all existing databases in the environment and open them all. Note that LMDB stores the database names in the unnamed database. The maxdbs parameter could be increased to at least have the number of databases (which are mixed with user keys) in the unnamed one. The documentation specifies that a moderate number is cheap, but a high number is expensive.

This will fix issue 1. as we will use a read transaction to open and commit those databases to make them global to the process. It will then be possible for the user to Env::open_database as it will be stored in the heed Env map. Note that the database will be stored with a new UnknownType/YetToBeTyped variant as the user didn't specify the Poly/Database<KC, DC> type yet and will be defined on the first real call to Env::open_database.

However, if the database is not in the map, a read transaction will not call the mdb_dbi_open as it could break 2., only a write transaction will do. That is another small limitation. So, only a write transaction can Env::create_database and open a new database by using open_database and discover new databases i.e. created by another process.

The Poly/Database types must now have a new lifetime to handle issue 1. where the database must be private to the transaction. It will have the transaction's lifetime when used with a write one and 'static with a read one. I don't know if this is possible/easy to code yet.

@darnuria
Copy link
Contributor

darnuria commented Jul 5, 2023

Powerdns in lmdb-safe force short transaction (there also the not commited readtxn bug, will report it tomorrow, spotted it today) to open the database and have the DBI thus forcing publishing to the shared env the dbi on commit.

https://github.com/PowerDNS/pdns/blob/master/ext/lmdb-safe/lmdb-safe.cc#L202

Note: their api is transaction centered like redb one.

@Kerollmops
Copy link
Member Author

Powerdns in lmdb-safe force short transaction (there also the not commited readtxn bug, will report it tomorrow, spotted it today) to open the database and have the DBI thus forcing publishing to the shared env the dbi on commit.

So it means they are opening a short-lived transaction to open your database. What I don't like about this solution is that it tries to open a transaction without you knowing it. I would prefer we open all the databases when we open the environment or let the user open them (and be able to commit read txn) to make them globally available.

I don't know what you think about what I wrote above. What do you think about it?

@Kerollmops
Copy link
Member Author

I experimented a little bit on this playground. The basic idea is to add a lifetime to the Database<'txn> type which constrains it to only live for the transaction's lifetime. We can then make the database 'static by using the Committable::prepare_for_commit trait method, which wraps the type and can then be given to the RoTxn/RwTxn::commit_with_databases and get a static database that can be stored and lives forever.

@Kerollmops Kerollmops modified the milestones: v0.20.0, v0.21.0 Apr 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants