Reading this more carefully:
On Sat Oct 10 06:08:32 2009, RIBASUSHI wrote:
Show quoted text> 2) You want to do the same as 1), but your code will run for a while,
> and there is a pretty good chance that the connection will timeout
> somewhere along the way. This is where you need a wrapper around the
> coderef - once an exception is thrown, it will run the GU code once, and
> retry only if the result is negative. However we now brokesome behavior
> of 1) - if the coderef never needs the $dbh, we just wasted cycles
> connecting. This would work if instead of $dbh, do() would pass $conn as
> its first attribute, or if you just used the coderef as a closure - e.g.:
>
> my $res = $conn->do_with_retry (sub { my $dbh = $conn->dbh; ... } );
>
> This subtle difference allows the coderef to do whatever it wants,
> including never calling ->dbh(). Which is a substantial saving.
I do not view this as the purpose of DBIx::Connector. If you have long-running code that
doesn't touch the database, do *not* run it inside a `txn_do()` or `do()` block. Run it
somewhere else.
Now, you might have long-running code that *does* touch the datbase that you want to run
inside a transaction block. Perhaps you want to have an entire HTTP request run inside a
transaction. But really, you should only do this if the work you're doing is fast -- or if it's
mostly database-intensive (and then only for updates). Otherwise, from a design POV, you
should do your expensive computation before you enter the transaction. This will minimize
the chance of deadlocks, as well.
Show quoted text> Additionally every call to ->dbh() will run the AU code, and will
> re-connect if the last cached $dbh is deemed useless (i.e. a fork within
> the do_with_retry() coderef). Once an exception is caught a single GU
> check taks place and the code is retried if the results are negative.
With a commit I made today, if you only use `$conn->dbh` inside your blocks, you get that
behavior. It's a choice.
Show quoted text> 3) You want to do the same as 1), but want to write things in a
> transaction. No retries expected. Here unlike with 1) we need a wrapper
> to issue the begin/commit statements. The concept is currently buggy in
> both DBIC and D::C. Suppose your coderef forks. At this point nothing
> protects the $dbh given to the user - he will just keep calling it,
> across processes. If we in turn switch to dbh() as a method call, we
> will have the benefit of die()ing right then an there, when it is
> detected that the process which started the transaction and the current
> one do in fact differ.
This is assuming no retry?
Otherwise this too is dealt with in the commit I made today. Basically, if `dbh()` is called
from within a `do()` or `txn_do()` block, it won't `ping()`, but will check for `fork`ing,
threading, and the DBI `Active` flag.
Show quoted text> 4) You want to txn_do_with_retry() - nothing special - again you run the
> AU code on every call to dbh(), an run the GU code as an indicator on
> whether to retry. Any time AU detects a process change a non-retriable
> exception is thrown, as the outermost transaction will no longer be atomic.
How is this different from 2)?
Show quoted text> 5) (bonus) I can't find the ML post, but there was once a user who
> wanted to do N retries per txn_do, not just 1. Granted it was a horrific
> hack, but a txn_do_with_retry argument would make this possible :)
Yeah, not really interested in that, at least not until we get all the other kinks worked out.
Show quoted text> To sum it up:
>
> *) Passing a bare $dbh to the coderefs is a pessimization - the coderef
> may never need it.
I reject this assertion. If it doesn't need it, don't use `txn_do()`. If you think it may not need
it becaus an exception will be thrown before it gets to using the database handle, well, in
the vast majority of environments the database handle will be used sooner or later --
usually sooner. Maybe you don't need it on the current request, but you'll need it on the
next request. So I don't view this as much of a loss.
Show quoted text> *) do() without retries is not very useful, but can be added for claity
Agreed.
Show quoted text> *) do_with_retry(), txn_do(), and txn_do_with_retry() each has its very
> legitimate uses.
Yes, I can see that.
So in addition to the changes I've already made today, I'm thinking of renaming `do()` and
`txn_do()` to `do_with_retry()` and `txn_with_retry()` (or something better, if I can think of
it), and then add `do()` and `txn_do()` with no retries. This should address the primary
issue, yes?
Best,
David
RT for rt.cpan.org