Removing the global transaction from ldbmodify (was Re: [SCM] Samba Shared Repository - branch master updated - release-4-0-0alpha6-90-g3942e53)

[simo]

On Mon, 2009-02-02 at 11:04 +1100, Andrew Bartlett wrote:
> On Fri, 2009-01-23 at 14:18 -0500, simo wrote:
> > On Fri, 2009-01-23 at 19:44 +0100, Stefan (metze) Metzmacher wrote:
> > > Hi Simo,
> > > 
> > > >     Do not start a transaction this way.
> > > >     Because we still want to commit any changes that successfully apply we
> > > >     never want to do a global cancel, and because of how transaction nesting
> > > >     works that means we never cancel any "transaction" at the single modify
> > > >     operation level.
> > > 
> > > Who says that? I think it's quite useful to only apply all operations!
> 
> Indeed!
> 
> > > We should at least have a command line option to control this.
> > > And also have all ldb tools in sync with this behavior.
> > > 
> > > Could you please revert that for now.
> > 
> > Nope, the previous behavior leaves the database in an inconsistent state
> > if an operation that should modify multiple entries based on a
> > triggering modify fails.
> > The reason is that if a module returns an error the transaction is not
> > canceled. (Tested with a module I am writing)
> 
> Then this is what should be fixed!

This is exactly what I fixed.

> > The intended behavior of ldbmodify was to apply all mods that could be
> > successfully applied, but it did not do that correctly.
> > My change fixes a real problem.
> > 
> > If you want to modify the intended behavior I am ok, but the code right
> > now works correctly. Reverting will make it wrong.
> > 
> > >  Then we could readd this to all
> > > tools with an option. Where I don't care what default behavior we choose.
> > 
> > Unless my change is causing problems I think a revert would be worng,
> > you can still add an option to have a real global transaction without
> > any revert.
> 
> We should add an option to apply the partial set, but given that we have
> transactions, we really should use them globally by default - and the
> admin would expect that they be on the global/whole file level.

Well that's not what happens with ldapmodify, and I think ldbmodify was
modeled after ldapmodify. We can certainly have a different behavior, I
just changed the original behavior to work instead of being broken.

> The other issue here is that this was added as a speedup - I understand
> from tridge that the global transaction is faster, because of the single
> 'write & sync'.

Sure, the let's add a proper global transaction that checks for any
failure and actually cancel *all* the changes.

No problem in you adding that feature.

Simo.

-- 
Simo Sorce
Samba Team GPL Compliance Officer <simo at samba.org>
Principal Software Engineer at Red Hat, Inc. <simo at redhat.com>