[SSSD] [PATCH] Perform fewer transactions during RFC2307bis initgroups
Jakub Hrozek
jhrozek at redhat.com
Mon Oct 17 15:57:59 UTC 2011
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 at lists.fedorahosted.org
> https://fedorahosted.org/mailman/listinfo/sssd-devel
More information about the sssd-devel
mailing list