On Mon, Oct 17, 2011 at 04:46:55PM +0200, Jan Zelený wrote: > > On Mon, 2011-10-17 at 13:26 +0200, Jakub Hrozek wrote: > > > https://fedorahosted.org/sssd/ticket/1006 > > > > > > Instead of saving each group as we fetch it from LDAP during initgroups, > > > just build the membership diff in-memory and the perform three > > > > > > transactions: > > > * save the groups if not cached already > > > * save the intergroup memberships > > > * save the user membership to his direct parents > > > > > > [PATCH 1/3] Utility functions for LDAP nested schema initgroups > > > These will be used later on > > > > Nack. > > Just a note here, the structure sysdb_domain_info should be removed, because > it is no longer used in underlying functions. >
Good finding, but I'll remove dom in a cleanup patch because the change would touch unrelated parts of code now and make the backport to 1.5/1.6 harder > One nitpick here: > - tmp_ctx in build_membership_diff seems to be redundant I prefer to explicitly steal stuff I want to return to the caller. There's no knowing what else might functions that require a memory context allocate - they /should/ play nice, but sometimes they don't and allocate stuff blindly on state, so we end up carrying allocated memory for longer than needed. > > > > > If the sysdb_transaction_commit() fails, you need to at least try to > > call sysdb_transaction_cancel() (otherwise we could be left with a > > transaction lock that we never free). > > > > > [PATCH 2/3] Use fewer transactions during RFC2307bis initgroups > > > Save the groups to a hash table and save them in three transactions > > > later > > > > Nack. > > Two more nitpicks: > - in sdap_initgr_rfc2307bis_send: I'd recommend creating a fail: label and > goto there OK, but in a cleanup patch, it's not needed for fixing the initgroups. > - in sdap_initgr_rfc2307bis_state: ndirect is a bit confusing, I'd rename it > to direct_groups_count or just direct_count Done > > > > > Instead of using three separate transactions in > > sdap_initgr_rfc2307bis_done(), wrap the save_* calls in a new outer > > transaction. Then they'll only get written to disk once with all the > > data. > > > > So just call sysdb_transaction_start() before save_rfc2307bis_groups() > > and then call sysdb_transaction_commit() before tevent_req_done(); (And > > obviously handle errors and cancel the outer transaction as needed). > > > > > [PATCH 3/3] Use fewer transactions during IPA initgroups > > > No need for a hash table because we can grab all the groups in a single > > > request. Just save them in a way similar to RFC2307bis initgroups > > > > Same comment about transactions as patch 2. > > > > Otherwise, these look good. > > I didn't have a chance to through the rest of patches, I guess I'll trust > Stephen's judgement. > > > Thanks > Jan > _______________________________________________ > sssd-devel mailing list > sssd-devel@lists.fedorahosted.org > https://fedorahosted.org/mailman/listinfo/sssd-devel _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://fedorahosted.org/mailman/listinfo/sssd-devel
