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

[Brandon Black]

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 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?

-- Brandon