[Dbix-class] Re: Transactions and AutoCommit

[Josef Karthauser]

On Fri, Jan 26, 2007 at 07:56:36PM +0000, Josef Karthauser wrote:
> Why does DBIx::Class involve itself with the status of the AutoCommit
> flag in its transaction code?  Shouldn't it just delegate this to
> the DBI and propagate any exceptions raised there?

Let me try again, but this time I'll not be so curt!

The background is that I'm working on adding support for proper nested
transactions, for mysql to start off with, but providing a framework
so that other databases can also be supported in a non-adhoc way.

Now this isn't too much work.  As things stand in -current it would
amount to basically overriding _dbh_txn_{begin,commit,rollback}
in the ...::DBI::database.pm files.

So, I've spent some time understanding the existing code, and I see
something which complicates matter a little bit, and I don't see why
some code should be there.  So I'm wondering whether I can remove it!

The transaction code was first committed (in revision 373, 2005-12-10
by ningu) and looked like this back then:

    sub tx_begin {
	$_[0]->dbh->begin_work if $TRANSACTION++ == 0
	    and _[0]->dbh->{AutoCommit};
    }

    sub tx_commit {
	$_[0]->dbh->commit if --$TRANSACTION == 0;
    }
     
    sub tx_rollback {
	--$TRANSACTION == 0 ? $_[0]->dbh->rollback : die $@;
    }

What I'm interested in discussing is the relationship with the DBI
variable 'AutoCommit'.  This conditional behaviour and operation
with respect to AutoCommit has endured since this first commit, and
been also added to tx_commit (now txn_commit) in more recent times.

Ok, so what do I think is wrong?  And why do I want to remove the
dependency on AutoCommit?  Let's look at the -current version of
txn_begin().

    sub _dbh_txn_begin {
      my ($self, $dbh) = @_;
      if ($dbh->{AutoCommit}) {
	$self->debugobj->txn_begin()
	  if ($self->debug);
	$dbh->begin_work;
      }
    }

    sub txn_begin {
      my $self = shift;
      $self->dbh_do($self->can('_dbh_txn_begin'))
	if $self->{transaction_depth}++ == 0;
    }

So, first see that _dbh_txn_begin only gets run the first time that
txn_begin is called.  Any subsequent times it will be suppressed because
they will be run at a higher transaction_depth.  So we can limit the
analysis to transaction_depth=0.  In that circumstance the call
to begin_work() will only happen if the database has AutoCommit switched
on.  If it is off then it ignores the txn_begin, and becomes a
null operation.

However looking at DBI's documentation about the AutoCommit variable in
relation to begin_work, it says:

       "begin_work"
             $rc  = $dbh->begin_work   or die $dbh->errstr;

           Enable transactions (by turning "AutoCommit" off) until the next
           call to "commit" or "rollback". After the next "commit" or "roll-
           back", "AutoCommit" will automatically be turned on again.  

           If "AutoCommit" is already off when "begin_work" is called then it
           does nothing except return an error. If the driver does not support
           transactions then when "begin_work" attempts to set "AutoCommit"
           off the driver will trigger a fatal error.  

The second paragraph is the relevant one.  It says that if begin_work
were to be called when AutoCommit happened to be off then DBI would
return an error.   So here we have an error case, something that should
never be made to happen, possibly fatal and which would DBI complain about,
but that DBIC masks from the caller from.  So if the user calls txn_begin()
in this circumstance (remember this test only happens the first time that
a 'begin transaction' is requested) instead of getting an error exception,
which is what they should get, they get to carry on as if nothing is
wrong.

So my question is basically this. Why does DBIC think it knows more about
transaction handling than DBI does?  It would be far clearer if it
did:

    sub _dbh_txn_begin {
      my ($self, $dbh) = @_;
      $self->debugobj->txn_begin() if ($self->debug);
      eval { $dbh->begin_work };
      $self->throw_exception($@) if ($@);
    }

and similar changes elsewhere.

So with this change in place all code which is successfully using
transaction handling will notice no changes in behaviour.  Code which
is also talking to the database directly, and is either issuing transaction
commands directly, or opens the database with AutoCommit = 0, will
feel pain when they call txn_begin, in as much as now they will get
a DBI error propagated through to them.  However I believe that such
code would be broken anyway.

The conclusion is that there is code complexity in DBIC for which
no valid user of the code benefits.

What I propose is removing the unnecessary complexity.

Joe
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 195 bytes
Desc: not available
Url : http://lists.scsys.co.uk/pipermail/dbix-class/attachments/20070127/b72efd8a/attachment.pgp