[PATCH] LMDB full patch set

[Andrew Bartlett]

On Thu, 2018-04-12 at 21:30 +0200, Stefan Metzmacher wrote:
> Am 12.04.2018 um 21:00 schrieb Andrew Bartlett via samba-technical:
> > Thanks Metze for getting back to me.
> > 
> > I'll look over this more after I get to the office, but some initial
> > responses to the easy questions:
> > 
> > On Thu, 2018-04-12 at 17:48 +0200, Stefan Metzmacher wrote:
> > 
> > > 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.
> > 
> > OK.  The reason these were left out was that they are significant
> > points, subtle things that seemed better to expand on than hide into a
> > 'too large to really review/follow' commit.  So it wasn't a mistake,
> > but of course it can be done. 
> 
> I think these important points could be better comments in the code.

OK

> > > 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.
> > 
> > Correct, the sam.ldb is still TDB, and will likely stay that way until
> > this is all combined into one environment.  This allows a number of
> > things, in particular it ensures we don't break LMDB databases
> > trivially by having the main Samba tool open them as TDB.  
> > 
> > The ldb:// thing is needed for the tests that poke into the backend,
> > and when setting up the backend databases in provision. 
> 
> So, there will be provision command line option to specifiy mdb for
> the sam.ldb.d/* partition databases?

There is (it already landed): --backend-store=[tdb|mdb]

> > > 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?
> > 
> > It seems so, and there isn't much we can do.  If you think about a
> > fork() based memory model then cleaning up the internal state will only
> > dirty memory anyway.  They don't provide a 'clean up after fork()'
> > function, but do strictly require that you not touch an environment
> > after a fork(). 
> 
> From reading the code of mdb_env_close() I don't see why we shouldn't
> be able to call it after fork. What we may need is a pthread_at_fork()
> handler to make sure we don't work in the middle of a transaction.

Given we are not going to bundle it, we were nervous about operating
outside their advice in this regard:

http://www.lmdb.tech/doc/

* Use an MDB_env* in the process which opened it, without fork()ing.

* Do not have open an LMDB database twice in the same process at the 
same time. Not even from a plain open() call - close()ing it breaks 
flock() advisory locking.

Indeed, perhaps it might be better not do do anything at all, even
'leaking' the FD: if a new LMDB is open then close() on the inherited
FD could destroy locks. 

I hate POSIX locking.

Andrew Bartlett

-- 
Andrew Bartlett                       http://samba.org/~abartlet/
Authentication Developer, Samba Team  http://samba.org
Samba Developer, Catalyst IT          http://catalyst.net.nz/services/samba