-
Notifications
You must be signed in to change notification settings - Fork 38
Update LMDB version to latest from master #23
Conversation
This will fail on older versions of LMDB on Windows that doesn't have the ITS#3824 fix
This includes important improvements such as ITS#8324 which enables sparse file allocation on Windows
Hi @repi, thanks for the PR! And sorry for the delay responding. I'm not sure why ITS#8324 hasn't made it to the release branch either. I know that LMDB developers are working on a new 1.0 version, and it's possible they're restricting 0.9 changes to the most critical fixes. But that's purely speculation. In any case, I'd like to keep the released version of this crate in sync with the most recent release version of LMDB. However, I'd be happy to add a branch to this repo that tracks the master branch of LMDB, which you and others can use directly (bypassing crates.io) via Cargo's support for git dependencies. Would that satisfy your use case, and would you be willing to maintain such a branch by submitting periodic PRs that merge the latest changes on the LMDB master branch? |
@@ -28,7 +28,7 @@ members = [ | |||
[dependencies] | |||
bitflags = "1" | |||
libc = "0.2" | |||
lmdb-sys = "0.8.0" | |||
lmdb-sys = { path = "lmdb-sys" } |
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.
Sadly, crates.io doesn't support publishing a crate with a path dependency like this, and that doesn't seem likely to change, per rust-lang/cargo#1565. So we'll have to publish the new version to crates.io (renaming it in the process to avoid the naming conflict with this version, which is controlled and updated more slowly by the upstream repo danburkert/lmdb-rs).
However, while I haven't tested it, a path dependency like this probably does work if you're depending on this crate via a Git repo reference rather than a crates.io reference. So this is probably fine for a branch where we track LMDB's upstream master branch but don't publish the crate on crates.io, and you depend on the Git repo directly.
Note: Azure Pipelines builds failed because I hadn't configured the project to check out submodules (recursively). I've done so now, and future builds should succeed. Here's a manual build: https://dev.azure.com/mykzilla/lmdb-rkv/_build/results?buildId=112 I don't see a way to trigger a manual build that updates the status of Azure Pipelines for this PR. But pushing a change to this branch (even an |
To test this strategy, I merged your changes into another branch in this repo, https://github.com/mozilla/lmdb-rs/tree/lmdb-master, then created a branch in my rkv repo that depends on that branch via a direct Git dependency: [dependencies]
lmdb-rkv = { git="https://github.com/mozilla/lmdb-rs", branch="lmdb-master" } Building and testing that branch then succeeds, as Cargo correctly pulls the dependency directly from the Git repo. Perhaps this is a reasonable solution for consumers like you that need to track the LMDB master branch? (I'm agnostic about what we call the branch in this repo that tracks the LMDB master branch. We could even make it the master branch and then track the LMDB release branch via a release or release-0.9 branch or the like.) |
Alternately, I've now forked https://github.com/LMDB/lmdb to https://github.com/mozilla/lmdb in order to backport fixes to the release branch. So far I've backported one, and I'm happy to consider others. So an alternative approach would be to backport ITS#8324 to the https://github.com/mozilla/lmdb/tree/mdb.RE/0.9-moz branch. Note this comment about why the fix wasn't backported in the first place. However, there's a more recent commit that may address the problem on the list of commits containing the string "ITS#8324." |
We're doing this for the latest tagged version (0.9.24) in #56. Based on #23 (comment) I don't think we're eager to update to LMDB master, or backport that specific fix for the time being, however I'll reiterate that we're quite eager to update to 1.0 once it hits. |
Ran into issue on Windows with that the LMDB database files were created up front at the full map_size, instead of incrementally and sparsely. This was fixed in LMDB on master quite a while ago in ITS#8324 and realized this crate was using a 2 year old version of LMDB which I here have updated to the latest available on master.
Unfortunately this fix is not available on the LMDB release branch (
mdb.RE/0.9
) or the recent release from it 0.9.23 two weeks ago. Not fully sure why.But feel the ITS#8324 fix is critical for many large-scale LMDB database use cases, including our own at Embark, so filing this PR. But would understand if this crate would prefer to be on the the main release line of LMDB instead, but in that case it should at least update to 0.9.23.
Also added a test that verifies if the database map file was created sparsely or not.