[Dbix-class] ROLLBACK seems to be skipped on 0.08

[Matt S Trout]

On Mon, Oct 22, 2007 at 02:08:17PM -0500, Brandon Black wrote:
> On 10/22/07, Jesper Krogh <jesper at krogh.cc> wrote:
> > Brandon Black wrote:
> > > So I guess what I'm saying is that forcing AutoCommit=>0 users to use
> > > explicit txns everywhere isn't compatible with the idea of generic
> > > DBIC modules/plugins and the idea of allowing AutoCommit=>1 users to
> > > not wrap everything in txns.
> > >
> > > I'm still thinking...
> >
> > Would it be an option to clearly mark the $storage->txn_begin/commit
> > stuff as deprecated and make it work "as broken" as in 0.07?
> >
> 
> Yes, I think, but it's more than just manual txn_begin/commit that are
> broken and should be deprecated...
> 
> The 0.07 behavior under AutoCommit=>0 in general is broken too, and we
> shouldn't be aiming to go back to it.  We should be looking for a
> long-term solution first, and then once we've got a workable long-term
> solution, we can examine what the best interim approach is to get us
> there that doesn't conflict and doesn't cause existing code more grief
> than necessary.
> 
> There's never been a version that's gotten the behavior right for all
> cases, because it's logically impossible.
> 
> If you require all DBI statements to made from within the context of
> an explicit DBIC transaction, various code (including code inside of
> DBIx::Class itself) will fail to work properly, because it's not a
> requirement in the general case that we wrap every database access in
> every module in a txn (and it shouldn't be).
> 
> If you allow statements to be made in implicit multi-statement
> transaction contexts, you really can't track transaction depth at all,
> which again breaks the ability of modules to abstract things (and also
> prevents us from properly supporting sub-transactions at the sql
> level).
> 
> My opinion at this point is that we really shouldn't be supporting
> AutoCommit=>0 at all, as it's too limiting and painful.  The only good
> argument for it I've seen is "sharing $dbh with other legacy code that
> expects AutoCommit=>0", but I'm not really buying that.  The code
> supporting this txn model doesn't even work right by itself, so
> there's no way you're not going to get your txns screwed up even worse
> when flip-flopping between it and a separate codebase doing manual
> transactions over a shared handle.  I say those people should just
> have two connections to the database, problem solved.
> 
> In order to avoid bad accidents for AutoCommit=>0 people when we
> switch to the above, the connect() logic should probably avoid trying
> to stuff AutoCommit manually.  Instead, after the connection has been
> established it should check $dbh->{AutoCommit} and die with a big
> error message if isn't on, and the rest of the code can assume that
> condition.  This will handle oddball cases like a DBD driver
> defaulting it off too (AFAIK, all the major ones default it on).

We should provide a "don't die but accept anything that goes wrong is your
fault" option too.
 
> We could do the above in 0.09 with a late deprecation cycle in 0.08.
> For 0.08, I can restore things to basically what they were in 0.07 for
> AutoCommit=>0 users (that is, explicit transactions work, implicit
> ones cause subtle and sometimes silent grief), and we can add some big
> warnings at connect() time about this mode being broken, deprecated,
> and unsupported in the next major release (plus appropriate docs
> updates).
> 
> Given how broken the 0.08 code is in the face of what worked for
> AutoCommit=>0 in 0.07, I can't imagine we have many users of this kind
> anyways.
> 
> I know it sounds abrupt and unfriendly, but I think this is the way
> forward.  I suspect some people will disagree :)  Any dissenting
> opinions out there with a better plan (Matt? @dbic-devel?) before I
> start putting a plan for this together?

I've always considered AutoCommit 0 behaviour to be basically undefined; my
gut feeling was that it was impossible to make sane, so I'm not desperately
upset that an in-depth examination proves this to be so.

Unless somebody comes up with a better plan, I say go for it.

-- 
      Matt S Trout       Need help with your Catalyst or DBIx::Class project?
   Technical Director                    http://www.shadowcat.co.uk/catalyst/
 Shadowcat Systems Ltd.  Want a managed development or deployment platform?
http://chainsawblues.vox.com/            http://www.shadowcat.co.uk/servers/