LDB and DRS patch review
Andrew Bartlett
abartlet at samba.org
Sun Mar 17 04:20:28 MDT 2013
Matthieu,
I know we have both been busy on other things, such as upgradeprovision
and the DRS fixes, but have you had a chance to address any of the
private review feedback I gave on your ldb branch ealier?
In particular, as I mention below, I tried to push some of the tests or
other parts of the branch, but there seem to be some dependencies on
later patches.
(I don't mind bumping the ABI twice if we have to, given how long it
seems to be taking to move on this).
I would very much like to make progress on this, particularly to get the
ldb_unpack and checking the scope based on the index DN into master.
Andrew Bartlett
On Fri, 2013-02-08 at 22:34 +1100, Andrew Bartlett wrote:
> Matthieu,
>
> Here is my review of the patches I didn't push today from your ldb_perf,
> drs-fix and wait_reviews branches.
>
> Hopefully we can again find a series of patches with tests and changes
> that we can agree on, and so slim down the remaining changeset.
>
> > commit 55c8b808d5761b695a319bf2955ef6bef68b3a62
> > Author: Andrew Bartlett <abartlet at samba.org>
> > Date: Fri Feb 8 13:13:16 2013 +1100
> >
> > ldb: bump ldb ABI version due to new private functions
> >
> > commit ec72d93227edb593b6ca7529f9505893db58d64b
> > Author: Matthieu Patou <mat at matws.net>
> > Date: Thu Dec 27 22:48:07 2012 -0800
> >
> > ldb-tdb: construct the list of attributes when parsing the search tree
> >
> > If the list is complete it will be used via ltdb_search_dn1_withlist in
> > order to limit the number of attributes that are actually returned.
>
> I would like the FIXME fixed here, particularly for LDB_OP_PRESENT. It
> shouldn't be too hard to extract the attribute names involved here.
>
> > commit 1f918c03286d2a38575a50c26574b94d9c293580
> > Author: Matthieu Patou <mat at matws.net>
> > Date: Thu Dec 27 22:44:53 2012 -0800
> >
> > ldb-tdb: use ltdb_search_dn1_withlist via ltdb_search_dn1_withlist
>
> I think you mean something else here. Can you reword?
>
> > This new function allow to specify a list of attributes to be unpacked
> > instead of unpacking all attributes. ltdb_search_dn1 is changed to call
> > the new function but with an empty list which will cause the full list
> > of attributes to be returned
>
> > commit bd9e18534241de4f72d95ef4732d5becc1581141
> > Author: Matthieu Patou <mat at matws.net>
> > Date: Thu Dec 27 21:38:29 2012 -0800
> >
> > ldb: introduce ldb_unpack_data_withlist to unpack partial list of attributes
> >
> > When provided with non NULL list ldb_unpack_data_withlist will only
> > unpack attributes that are specified in the list (+ distinguished name)
> > ldb_unpack_data is changed to call ldb_unpack_data_withlist behind the
> > scene.
>
> I've tried to merge just these parts, but they depend on other things.
> Is there any chance you could get me a branch with just tests and the
> above changes? That way, we can deal with the index changes and
> casefold stuff in a distinct review.
>
> > commit 19ec24bcc76612d802bda3e08b20fb4a53cc9c14
> > Author: Matthieu Patou <mat at matws.net>
> > Date: Sat Dec 22 18:40:24 2012 -0800
> >
> > ldb-tdb: Use talloc pool for the DNs used while doing indexed search
>
> How did you choose the size of the talloc pool here? How did you show that this was faster?
>
> > commit f8486d4333e1770766f8a7c6a03a94b5851654f1
> > Author: Matthieu Patou <mat at matws.net>
> > Date: Fri Dec 14 01:02:09 2012 -0800
> >
> > ldb-tdb: If the index has case folded DNs mark those DNs as casefolded
> >
> > This will save a lot of time as casefolding DNs is very expensive
> >
> > commit 57458fa0d5bbfd6a345c0f08597c9a43d9d66c10
> > Author: Matthieu Patou <mat at matws.net>
> > Date: Sat Dec 22 16:00:36 2012 -0800
> >
> > ldb-tdb: Store DNs in index entries already casefolded
> >
> > Performance analysis have shown that casefolding index entries when
> > doing record lookup takes an important part of a ldb lookup, by storing
> > DNs already casefolded then can be used as is (almost) and so reduce
> > time and cpu spent in searches.
> >
> +/*
> + * This version is not really used to store a new index but is the result from
> + * intersecting LTDB_INDEXTYPE_LEGACY_MIXED and LTDB_INDEXTYPE_CASEFOLDED indexes
> + */
> +#define LTDB_INDEXTYPE_LEGACY_MIXED 3
> +#define LTDB_INDEXTYPE_CASEFOLDED 4
>
> I'm not sure you really cope with the idea of a mixed index. I don't
> see any code to change the core list intersection code, which is
> strncmp() based in dn_list_cmp().
>
> > commit 588da1c918688d1f3c833bce552208df6edae2f1
> > Author: Matthieu Patou <mat at matws.net>
> > Date: Tue Jan 8 22:13:10 2013 -0800
> >
> > ldb-tdb: load index version and store the correct version of the index
> >
> > Don't assume that every index stored is necesserly stored
> > with the highest version, on newly created index use the latest
> > version possible.
> >
> > commit 6aa36a8ad14b1b2006bf1c796806186e74894150
> > Author: Matthieu Patou <mat at matws.net>
> > Date: Wed Dec 12 20:47:05 2012 -0800
> >
> > ldb-tdb: split ldb_match_msg_error by 2 seprate calls to ldb_match_scope + ldb_match_message
> >
> > Calling ldb_match_scope before the ldb_msg is actually loaded from the
> > database allow to skip the loading of entries that didn't match the
> > base DN of the request.
> > Depending on the number of records that will actually be skiped before
> > loading and unpacking rather than just after.
> >
> > commit f5f583c549cb2e3502cf6a3b01d8da2bf8a2e673
> > Author: Matthieu Patou <mat at matws.net>
> > Date: Sat Dec 22 16:06:49 2012 -0800
> >
> > ldb-tdb: Use IDXMASTERVERSION for determining current indexing version in the ldb
> >
> > Fallback on IDXVERSION that is used by Samba'DSDB to version rules for
> > casefolding attributes
>
> I'm still not convinced by the index version stuff here. I've already
> excluded the dsdb-schema and provision patches, and I guess what I don't
> like is that you need to 'opt in' to the new code on a per-ldb basis,
> despite this being a backwards-compatible change.
>
> The difficultly is of course that while a fully converted database is
> backwards compatible, you must not have a partially-converted database,
> because the list intersection stuff won't work. I realise you don't
> want to make the first edit do a re-index, but I still think this is the
> best option. I don't like how this is mixed in with the @ATTRIBUTES
> stuff however - I know the dsdb version is in there, but it still
> doesn't feel light.
>
> > commit 3b0d8a8793a8a0aaaa7707fba050e4666900022b
> > Author: Matthieu Patou <mat at matws.net>
> > Date: Tue Jan 8 22:03:25 2013 -0800
> >
> > ldb-tdb: keep the current indexing version in the ltdb->cache
> >
> > commit f36f6c86a0e4c5ad4329a80827203adb779a4699
> > Author: Matthieu Patou <mat at matws.net>
> > Date: Fri Dec 14 01:02:00 2012 -0800
> >
> > ldb: Add ldb_dn_set_casefolded so that a DN can marked as already casefolded
> >
> > This is intended for internal use in the indexing code when index
> > entries are DN stored already case folded.
> > This function will skip all the costly case folding calls and just mark
> > the DN as casefolded. The cf_name and cf_value of each componenent will
> > point to their counter part (name and value).
> >
> > commit 7a2e81fe95fe5b949f9223aca833d9897399de2c
> > Author: Matthieu Patou <mat at matws.net>
> > Date: Wed Feb 6 23:44:10 2013 -0800
> >
> > ldb: increase version due to new public function
> >
> > Added function:
> > ldb_dn_compare_base_one
> >
> > Made public so that they can be used by ldb_tdb
> > ldb_match_message
> > ldb_match_scope
>
> Let's combine this with the other version change at the top.
>
> > commit 52d925f91616848fde803a23a58d9e96a34e8676
> > Author: Matthieu Patou <mat at matws.net>
> > Date: Wed Dec 12 21:15:57 2012 -0800
> >
> > ldb: Make ldb_match_scope and ldb_match_message available to other ldb parts
> >
> > This allow the indexing code to use them.
> >
> > commit aeb350bfad3202857978741b0fa14fc83d93cc32
> > Author: Matthieu Patou <mat at matws.net>
> > Date: Thu Dec 13 22:37:30 2012 -0800
> >
> > ldb: Use ldb_dn_compare_base_one in ldb_match_scope
> >
> > In order to return the number of element in a DN, it has to be exploded
> > if it's not already the case which is costly, in most case where this
> > function is called DN are still in non exploded form
> > Also the ldb_compare_base_one has quick test for DN that are linearized but
> > are not case folded.
> >
> > commit 97af7f5a416273c812731f922ab35802d6f98bcf
> > Author: Matthieu Patou <mat at matws.net>
> > Date: Sat Dec 29 21:47:29 2012 -0800
> >
> > ldb: Add ldb_dn_compare_base_one for checking if a dn is just bellow the base
>
> It would be really good it it were possible to merge the patches without
> this one, and then show this is an optimisation. I say this because I
> really need to look into this more carefully.
>
> > commit 89ad85e0d9da8942d63d843f1570e814bf1397ea
> > Author: Matthieu Patou <mat at matws.net>
> > Date: Tue Jan 8 00:09:16 2013 -0800
> >
> > ldb: Fix broken tests in api.py, extend ldb_dn_compare ones
>
> Somewhere in these tests things fail, when I try to just apply the
> tests. Are they dependent on other things that should have been
> included first?
>
> > commit 324f752bfe510d3762d51644d1cd7d6eae4bfd79
> > Author: Matthieu Patou <mat at matws.net>
> > Date: Wed Jan 2 15:52:23 2013 -0800
> >
> > ldb_tdb: Do not return -1 when there is 0 elements in the ldb_message
> >
> > talloc_realloc returns -1 if count is 0 as it's treated
> > like a talloc_free, so if the number of elements in the message is 0,
> > before or after the filtering the realloc will return -1, which is then
> > handled as a failure by indexing functions causing a fullscan to be
> > done.
> > Also if the message already has 0 elements we skip the filtering (can
> > occur in some base search with no search criteria).
>
> I tried to push this, but on it's own it gives:
>
> UNEXPECTED(failure): api.SimpleLdb.test_modify_delete
> REASON: _StringException: Traceback (most recent call last):
> File "/memdisk/abartlet/a/b921669/samba/lib/ldb/tests/python/api.py", line 224, in test_modify_delete
> self.assertEquals(1, len(rm))
> AssertionError: 1 != 0
>
> > commit 5ca8461e8aafa6e4cc35fc78f0587d732be95a42
> > Author: Matthieu Patou <mat at matws.net>
> > Date: Fri Jan 11 20:05:39 2013 -0800
> >
> > drsuapi: Debug more clearly why NC is bad in updateRefs
>
> This needs to be a level 1 or 2, not a level 0.
>
> I hope this gives you some feedback you can work with. I'm sorry it has
> taken so long.
>
> Andrew Bartlett
>
--
Andrew Bartlett http://samba.org/~abartlet/
Authentication Developer, Samba Team http://samba.org
More information about the samba-technical
mailing list