[PATCH] LMDB full patch set

[Stefan Metzmacher]

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.

>> 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?

>> 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.

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/6cb07bdd/signature.sig>