Supporting Migration from old-school DBI

[bluefeet]

I tend to work in environments with a large legacy code base. These environments tend to have funky setups for their DBI connections such as AutoCommit=>0 and RaiseError=>0 (and a lot worse such as custom timeout handling with alarm() and such...). I'd like to use DBIx::Connector in these environments and incrementally port the code to use it.

But, the problem with that is DBIx::Connector is pretty heavy handed. In particular what is causing me issues right now is:

• txn() and svp() do not work if AutoCommit is off.
• fixup doesn't work if RaiseError is off.

So, the following DBI attributes just won't work with DBIx::Connector even tho my legacy code depends on it:

my $connector = Foo->new( $dsn, $user, $pass, { RaiseError=>0, AutoCommit=>0 });

I tried coming up with a solution that uses local() to encapsulate the setting of RaiseError and AutoCommit so that they are set what DBIx::Connector wants when it needs them that way but reverts to their original value when the DBI handle is being used directly:

package Foo;

use base 'DBIx::Connector';

sub _exec {
    my $self = shift;
    my $dbh = shift;

    local $dbh->{RaiseError} = 1;

    return $self->SUPER::_exec( $dbh, @_ );
}

sub in_txn {
    my $self = shift;
    return $self->{_in_txn} ? 1 : 0;
}

sub txn {
    my $self = shift;
    local $self->{_in_txn} = 1;
    return $self->SUPER::txn( @_ );
}

sub svp {
    my $self = shift;
    local $self->{_in_txn} = 1;
    return $self->SUPER::svp( @_ );
}

1;

And got pretty much stuck at that point as I couldn't figure out a good way to inject AutoCommit=>1 at the points it needs it like I am able to to with RaiseError=>1. And, I'm aware that RaiseError=>1 needs to be set in more places than for _exec(), such as when $driver->ping() is being called.

Now, I could just copy and paste the functions (_txn_run, _txn_fixup_run, etc) where I need to local()ize these attributes but then my code is going to break when DBIx::Connector changes something internal. Bad.

I see two valid solutions:

• Abstract out various bits of the logic in DBIx::Connector so that a subclass can wrap around more fine-grained parts and.
• Change DBIx::Connector to not be so heavy handed and just build-in the local()ization of RaiseError and AutoCommit.

Thoughts? Thanks! :)

[bluefeet]

Oh, and this is in a web environment so I can't just use DBIx::Connector for new code and DBI for old code - that would cause double-DBI connections which would make the DBAs oh-so-not-happy.

[theory]

On Nov 7, 2012, at 3:23 PM, Aran Deltac notifications@github.com wrote:

• txn() and svp() do not work if AutoCommit is off.

Well, if AutocCommit is off, then a transaction is already running. So neither txn nor svp would be scoped in the way you expect.

• fixup doesn't work unless RaiseError is off.

Er, what? That sounds sound right. I always have RaiseError (or HandleError) enabled in my apps. If fixup is not working for you when RaiseError is on, can you provide a test case? It's something that should be fixed.

So, the following DBI attributes just won't work with DBIx::Connector even tho my legacy code depends on it:

my $connector = Foo->new( $dsn, $user, $pass, { RaiseError=>0, AutoCommit=>0 });

Oh, you mean fixup doesn't work if RaiseError is off? If so, that would be a bug.

As for AutoCommit=>0, boy, wow, I just strongly recommend against that.

I tried coming up with a solution that uses local() to encapsulate the setting of RaiseError and AutoCommit so that they are set what DBIx::Connector wants when it needs them that way but reverts to their original value when the DBI handle is being used directly:

package Foo;

use base 'DBIx::Connector';

sub _exec { my $self = shift; my $dbh = shift;

local $dbh->{RaiseError} = 1;

return $self->SUPER::_exec( $dbh, @_ );

}

That doesn't work? Huh.

sub in_txn { my $self = shift; return $self->{_in_txn} ? 1 : 0; }

sub txn { my $self = shift; local $self->{_intxn} = 1; return $self->SUPER::txn( @ ); }

Yeah, that won't work, because DBIx::Connector checks the value of AutoCommit, not in_txn().

And got pretty much stuck at that point as I couldn't figure out a good way to inject AutoCommit=>1 at the points it needs it like I am able to to with RaiseError=>1.

Well, note that setting AutoCommit to 1 is the same thing as calling $dbh->commit.

And, I'm aware that RaiseError=>1 needs to be set in more places than for _exec(), such as when $driver->ping() is being called.

Yeah, that's likely. But DBIx::Connector should really be modified to set those where it checks for exceptions itself. IOW, it's a bug.

Now, I could just copy and paste the functions (_txn_run, _txn_fixup_run, etc) where I need to local()ize these attributes but then my code is going to break when DBIx::Connector changes something internal. Bad.

Yeah. And there is no getting around the AutoCommit issue.

I see two valid solutions:

• Abstract out various bits of the logic in DBIx::Connector so that a subclass can wrap around more fine-grained parts and. • Change DBIx::Connector to not be so heavy handed and just build-in the local()ization of RaiseError and AutoCommit.

I definitely think it should do a better job with RaiseError, but unless I misunderstand the way AutoCommit works, you can't really use txn or svp to get the scoping you expect with AutoCommit off. It just doesn't work.

Thoughts? Thanks! :)

Well, I guess, for AutoCommit, we could change DBIx::Connector so that it does its own transaction checking rather than rely on AutoCommit. It used to do that, but was removed here:

https://github.com/theory/dbix-connector/commit/45bb09f374708d090207d63d54f57480102a84a8

Because it felt redundant. But if it was made that way again, then your subclass calls to txn() and svp() would work. Well, sort of. They would execute the code, but they would not actually be scoping a transaction as documented. Which was the other reason I removed _in_txn.

Your other alternative is to just use run until you update your apps to set AutoCommit to true.

HTH,

David

[matthewlenz]

David, along the same line. You disable mysql_auto_reconnect. DBD::mysql already ignores this setting when inside a transaction (won't reconnect). I assume your code knows if it's inside a run() block, correct? Is there a reason why reconnect couldn't be enabled by default and then disabled (and then re-enabled) by your run() block? This would allow reconnects to happen automatically when dbix::connector methods are not explicitly being utilized to supervise db interaction.

[theory]

Yes, it can tell when it's in a transaction. No, I don't know the proper way to handle mysql_auto_reconnect; changes were mostly in response to various bug reports.