[PATCH] LMDB full patch set

Stefan Metzmacher metze at samba.org
Thu Apr 12 15:48:03 UTC 2018


Hi Andrew,

> On Wed, 2018-04-11 at 14:15 +0200, Stefan Metzmacher via samba-
> technical wrote:
>> I'd like to have a look, but that might take a few days.
> 
> Can you please be specific?  Last week you said you would have feedback
> for me in a day, and it has been over a week.

I looked at it today.

>> And as you seem to still find bugs, I don't understand why this needs
>> to get to master as soon as possible. 
> 
> I've not found any bugs in the LMDB patch set.  It has not changed
> since the re-order you requested last week. 
> 
> What I have done is address yet another serious locking bug.  Indeed
> rare but existing locking bugs have taken up most of the time for this
> project!

As always:-)

> Perhaps it happens more often with this patch, or perhaps it is just
> like the branch point before Samba 4.8 where flapping tests came out of
> nowhere.  Either way, what I've done is find and fix it.
> 
> (Gary did his due testing the entire time he was testing, and when he
> first proposed it, and they didn't fail then). 
> 
>> The branch point for 4.9.0rc1
>> won't happen in near future.
> 
> I really, really don't want to leave this any longer.  I've got a set
> of patches that pass the Gitlab CI.  They also show good results on
> Catalyst Cloud autobuilds, results being:

I reordered the patchset a bit further:

The patches in
https://git.samba.org/?p=metze/samba/wip.git;a=shortlog;h=refs/heads/master4-lmdb-pre1
should be fine for master if they pass autobuild.

https://git.samba.org/?p=metze/samba/wip.git;a=shortlog;h=refs/heads/master4-lmdb-pre2

Contains the test_transaction_start_across_fork and
test_lock_read_across_fork tests, but it tries to run them
also with tdb.

I think it's very important to have the same external behavior between
tdb and lmdb. We should also protect against a fork between
transaction_start and transaction_[prepare]_commit. Maybe even more
situations.

This should then pass a build without having lmdb.h on the system.

https://git.samba.org/?p=metze/samba/wip.git;a=shortlog;h=refs/heads/master4-lmdb-full

Contains the rest with some build fixes, whitespace cleanups
and some more TODO markers in commit messages.

I think the ldb_mdb/* related fixes should be squashed all into the
initial commit (maybe with an expection for tests, which can follow)
otherwise it's really confusing to see the work in progress history.

Is it correct that we'll use lmdb as default for the sam.ldb and
sam.ldb.d/* databases? Or will all ldb files use it?
From reading the changes it's not obvious where the switch to "ldb://"
is done for the creation process of databases.

I was also not able to understand if the getpid() based detection for
the fork case correct. What is the correct way for lmdb to cleanup
after fork? Is close(fd) in lmdb_pvt_destructor() really the only
thing? Doesn't lmdb has other state, which we will leak?

Thanks for your patience, but there's still a bit of work required,
sorry! But given that will be the core of our AD database careful review
is required.

metze

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: OpenPGP digital signature
URL: <http://lists.samba.org/pipermail/samba-technical/attachments/20180412/0a7cd81d/signature.sig>


More information about the samba-technical mailing list