[Dbix-class] DBIx::Safe. DBI wrapper.

[Brandon Black]

On 5/9/07, Oleg Pronin <syber.rus at gmail.com> wrote:
> Greeting! I looked at the Storage::DBI's new features in the current branch
> related to database connection losses troubles.
>
> There are still caveats there:
>
> 1) There are still race conditions
> Look at these strings of code (in sub _execute):
>
> $rv = eval { ..... }
> $self->throw_exception("Error executing '$sql': ".($@ || $sth->errstr)) if
> $self->connected;
> $self->_populate_dbh;
>
> Image the SQL syntax (or whatever) error happened in
> $rv = eval { ..... };
> # and suddenly right here connection to database has been lost
> # and the following code will not throw exception because database is not
> connected:
> $self->throw_exception("Error executing '$sql': ".($@ || $sth->errstr)) if
> $self->connected;
>
> This code will restart incorrect query again after it reconnects!
>

Yes, but it will fail again, resulting eventually in the right behavior.

> 2) Transactions.
>    a)
>       in txn_do:
>       if txn_begin (line 561) fails (not because of disconnection)
>       then current code will do txn_rollback (line 578) which will always
> fail.

The txn_rollback is in an eval too.  So perhaps they get a nested
rollback exception instead of a "failed to start transaction"
exception is your issue?  Either way, they do get an exception.

>    b) Similar to "1)" :
>        Imagine that "$coderef->(@_)" (lines 563, 566) or txn_commit fails
> (ordinary SQL error).
>        And at the next moment connection to database was lost.
>        Perl will not go through "if" at line 577. Incorrect transaction will
> be re-executed.

Right, but in that odd race-case, it will retry the transaction with
all the same protections.  It should fail again the second time as
appropriate.

>    c) If lost connection while txn_rollback - then rollback is not failed
> (just need to ignore, because database will do rollback by itself).
>

Again, this is already ignored?

> 3) This code will try to re-execute statements only once (immediately)!
> Administrators or developers will have no chance to restart database server
> without having troubles in applications running.
>

We have to throw an exception fairly quickly.  Do you really think the
correct behavior is to hang the caller's request for enough time for a
human to restart a database server?

> 4) Perfomance losses in txn_do, dbh_do etc because of permanent executing
> code related to connection losses protection.
>

It's better than the previous alternative, which was $dbh->ping +
fork/thread-handling magic before every statement.  The txn_do/dbh_do
isn't very heavy stuff, even though it looks complicated.  Any
solution that actually protects from disconnects, handles forks,
handles threads, and handles regular sql exceptions will be at least
this cumbersome.

> 5)  ::Storage always sets RaiseError to 1 and this is not good. Somebody
> might want sql error not to cause exceptions. If ::Storage doesn't set
> RaiseError then all protection code will not work.
>

If we don't enforce RaiseError, how do we catch errors?  If someone
wants to execute sql that they expect will cause an error, they should
wrap the call in eval {}, that's standard Perl.

> 6) $dbh->prepare is not protected (always cause exception in case of any
> error). Some databases can store prepared statements at server-side.

It is as far as I can see.  _execute calls $self->sth in an eval,
which does the prepare.  Are you saying that some databases will cache
the preparation when they throw an exception, and then not throw an
exception again when you re-use the cached entry?  That sounds like a
bug.

> Any non-100% solution is not a solution when working with database.
>
> DBIx::Safe solves all of this problems (this module is not yet released).
> So how could i put it to your svn if you dont mind.
>

I'd love to see your code.  I'm not saying the Storage::DBI code is
perfect, but I have spent a lot of time thinking through the fallouts
of these failure scenarios, and I don't think it's as flawed as you
think it is.  More code sharing == improvements for all.

-- Brandon