Bug #47005 for DBIx-Class: txn_do should provide a way to disable retry

Stalling until Tim finds some time to get this going.

On Fri Oct 09 04:51:42 2009, Tim.Bunce@pobox.com wrote: Well we can't both be right :) Grep Connector.pm[1] for the string: '# Not connected. Try again.' [1] http://cpansearch.perl.org/src/DWHEELER/DBIx-Connector-0.12/lib/DBIx/Connector.pm

On Fri Oct 09 12:27:27 2009, Tim.Bunce@pobox.com wrote: I've been thinking about this. I'm not sure how to proceed, really, because if the retry is disabled, there is no point to do(). Best, David

On Fri Oct 09 12:32:46 2009, DWHEELER wrote: This is not entirely true. _seems_connected() is virtually free - you can always call it before do/txn_do. This way do() without retry makes sense in forked environments - i.e. you reconnect before even calling the coderef. In fact we did something similar in DBIC with _get_dbh (this is in fact the reason _seems_connected was factored out). For txn_do() the benefits are even broader, as you are most likely going to use it for the nesting benefits, thus a single execution cycle can in fact be much desired. I side with Tim here - if it is not too late, making the auto-retry non-default would be sound design. In DBIC we would still turn it on by default but that's for backcompat reasons, which you (yet) don't have.

On Fri Oct 09 15:54:39 2009, RIBASUSHI wrote: ^^^^^^^^^^^^^^^^^^^^^^ In fact if you look at the e.g. Oracle special handling of ping() you'll see that even this is not the case :)

On Oct 9, 2009, at 12:53 PM, Peter Rabbitson via RT wrote: It seems to me that if do() (and txn_do()) aren't going to retry, then they have to ping(), no? I'm on the fence. David

On Fri Oct 09 18:48:12 2009, DWHEELER wrote: Ok,then more explanation is in order. Sorry I didn't bring this up earlier, leaky memory :( In fact I now realize that the API has a very serious deficiency, which I will try to highlight below. Also I am not trying to mock your module needlessly, I am just sharing all the thoughts that came up once I opened this can of worms. Still I think all of us can get out of this rather happier: What does D::C provide? - a service that allows the user not to care how did he get a specific $dbh. The two "atomic parts" that D::C uses are: *) Is said $dbh apparently usable (let's call this AU). It checks: - is there a $dbh in the first place - is it still in the same fork/thread - has it been closed by the *user* ({ACTIVE}) *) Is said $dbh *guaranteed* to be usable, at least for the SQL statement that immediately follows (let's call this GU). Check is either via ping() or with some equivalent kludge. Now, what potential users might expect: 1) You want to run some non-persistent code. You want to connect only if/when necessary, and want this to happen transparently, even if you fork somewhere along the way. You do not expect transactions nor any code-retries: This can be achieved without using a coderef, but with writing code directly around $conn. Nothing to see here. 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. 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. 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. 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. 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 :) To sum it up: *) Passing a bare $dbh to the coderefs is a pessimization - the coderef may never need it. *) do() without retries is not very useful, but can be added for claity *) do_with_retry(), txn_do(), and txn_do_with_retry() each has its very legitimate uses. Sorry for the long write up :)

2) Based on [this report](http://github.com/theory/dbix-connector/issues/#issue/4), I plan to change `dbh()` so that it does not do the `ping()` when called from within a `do*()` block. I think that this will address your concern. For shits and giggles, I'm also going to stick `$dbh` in `$_`, so that the blocks feel even more blockish. I'm going to work on these changes today. 3) The above-described change will help with this scenario too, as long as the user calls `$conn->dbh` for the situation you describe. But I still want to pass it in and assign it to `$_` for the simple cases, which I expect will be the most common. Does this make sense? Anyone who `fork`s within a transaction sub is asking for trouble, though. It's just insane. If the code ref never needs it, why would one call the blocks? They're specifically for accessing the database. What's the point otherwise? Yes, the lack of utility was what I didn't like. Agreed. I'm going to make some changes now and think on it. More later. David

On Fri Oct 09 15:50:09 2009, Tim.Bunce@pobox.com wrote: Thanks Tim, I changed it to your wording. —Theory

Reading this more carefully: On Sat Oct 10 06:08:32 2009, RIBASUSHI wrote: 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. With a commit I made today, if you only use `$conn->dbh` inside your blocks, you get that behavior. It's a choice. 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. How is this different from 2)? Yeah, not really interested in that, at least not until we get all the other kinks worked out. 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. Agreed. 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

On Mon Oct 12 20:23:12 2009, DWHEELER wrote: Now leaning towards run runup txn_run txn_runup svp_run The "runup" versions will assume that the connection is up, unless there's a failure, and then it will check the connection and re-up it if necessary. And yes, I think I'll pass the connection object (and put it into $_) rather than the $dbh. Maybe. Thoughts? —Theory

On Tue Oct 13 01:44:28 2009, DWHEELER wrote: I don't think I will, because otherwise how can you start the transaction in `txn_run()` or `txn_runup()` without first getting the database handle and issuing a `BEGIN`? You can't. Best, David

On Tue Oct 13 01:44:28 2009, DWHEELER wrote: Now I'm thinking run() rerun() txn_run() txn_rerun() svp_run() It has the same issue as do/redo, but it seems a little more emphatic, more casual to me, like a television rerun. It also makes a bit more sense than runup. Thoughts? David

So I think that this issue is resolved. But should a bug be opened against DBD::Oracle so that there's a chance for them to fix their ping() issue? Based on Peter's explanation, I have this comment in DBIx::Connector::Driver::Oracle: # Note from https://rt.cpan.org/Ticket/Display.html?id=47005: # DBD::Oracle has some shutdown state in which it will return 1 on ping as # long as the socket is still open. This however did not guarantee the server # is any longer in a state to execute queries. So what happened was: # # 1) the weird state is reached # 2) a txn_do takes place and fails on the first sql command # 3) the code calls ping() and gets a connected reply # 4) the txn_do is not retried # 5) ... # 6) users lose profit Peter, is that right? If so, should it not be reported? Otherwise, I think that this issue is resolved, at least as far as DBIx::Connector is concerned. Thanks, David