code review 5785043: database/sql{,driver}: add ErrBadConn

database/sql{,driver}: add ErrBadConn

[bradfitz, 2012-03-07 22:05:12]

Hello golang-dev@googlegroups.com,

I'd like you to review this change to
https://go.googlecode.com/hg

[rsc, 2012-03-07 22:06:06]

What if it doesn't implement this?

[bradfitz, 2012-03-07 22:07:48]

On Wed, Mar 7, 2012 at 2:06 PM, Russ Cox <rsc@golang.org> wrote:

> What if it doesn't implement this?
>

Assumed healthy, as before.

Will update docs.

[bradfitz, 2012-03-07 23:29:34]

Hello golang-dev@googlegroups.com, rsc@golang.org (cc:
golang-dev@googlegroups.com),

Please take another look.

[rsc, 2012-03-08 15:26:13]

LGTM

http://codereview.appspot.com/5785043/diff/6001/src/pkg/database/sql/driver/d...
File src/pkg/database/sql/driver/driver.go (right):

http://codereview.appspot.com/5785043/diff/6001/src/pkg/database/sql/driver/d...
src/pkg/database/sql/driver/driver.go:46: // ErrBadConn should be returned by
drivers to signal to the sql
s/drivers/a driver/

http://codereview.appspot.com/5785043/diff/6001/src/pkg/database/sql/driver/d...
src/pkg/database/sql/driver/driver.go:47: // package that a driver's Conn is in
a bad state (such as the server
s/driver's Conn/driver.Conn/

http://codereview.appspot.com/5785043/diff/6001/src/pkg/database/sql/driver/d...
src/pkg/database/sql/driver/driver.go:53: // successfully performed the
operation.
s/successfully //

Even if the server sends back an error, you shouldn't return ErrBadConn.

http://codereview.appspot.com/5785043/diff/6001/src/pkg/database/sql/sql.go
File src/pkg/database/sql/sql.go (right):

http://codereview.appspot.com/5785043/diff/6001/src/pkg/database/sql/sql.go#n...
src/pkg/database/sql/sql.go:254: for n, conn := range db.freeConn {
n is a strange iteration variable.  s/n/i/

http://codereview.appspot.com/5785043/diff/6001/src/pkg/database/sql/sql.go#n...
src/pkg/database/sql/sql.go:290: defer func() {
Cute but maybe a little subtle and also hard to limit.

maybe s/prepare/prepare1/ and then do

func (db *DB) prepare(query string) (stmt *Stmt, err error) {
    for i := 0; i < 10; i++ {
        stmt, err = db.prepare1(query)
        if err != driver.ErrBadConn {
            break
        }
    }
    return
}

http://codereview.appspot.com/5785043/diff/6001/src/pkg/database/sql/sql.go#n...
src/pkg/database/sql/sql.go:754: for {
for i := 0;; i++ {

http://codereview.appspot.com/5785043/diff/6001/src/pkg/database/sql/sql.go#n...
src/pkg/database/sql/sql.go:760: if err == driver.ErrBadConn {
&& i < 10

[bradfitz, 2012-03-08 18:07:23]

http://codereview.appspot.com/5785043/diff/6001/src/pkg/database/sql/driver/d...
File src/pkg/database/sql/driver/driver.go (right):

http://codereview.appspot.com/5785043/diff/6001/src/pkg/database/sql/driver/d...
src/pkg/database/sql/driver/driver.go:46: // ErrBadConn should be returned by
drivers to signal to the sql
On 2012/03/08 15:26:14, rsc wrote:
> s/drivers/a driver/

Done.

http://codereview.appspot.com/5785043/diff/6001/src/pkg/database/sql/driver/d...
src/pkg/database/sql/driver/driver.go:47: // package that a driver's Conn is in
a bad state (such as the server
On 2012/03/08 15:26:14, rsc wrote:
> s/driver's Conn/driver.Conn/
> 

Done.

http://codereview.appspot.com/5785043/diff/6001/src/pkg/database/sql/driver/d...
src/pkg/database/sql/driver/driver.go:53: // successfully performed the
operation.
On 2012/03/08 15:26:14, rsc wrote:
> s/successfully //
> 
> Even if the server sends back an error, you shouldn't return ErrBadConn.

Done.

http://codereview.appspot.com/5785043/diff/6001/src/pkg/database/sql/sql.go
File src/pkg/database/sql/sql.go (right):

http://codereview.appspot.com/5785043/diff/6001/src/pkg/database/sql/sql.go#n...
src/pkg/database/sql/sql.go:254: for n, conn := range db.freeConn {
On 2012/03/08 15:26:14, rsc wrote:
> n is a strange iteration variable.  s/n/i/

Done.

http://codereview.appspot.com/5785043/diff/6001/src/pkg/database/sql/sql.go#n...
src/pkg/database/sql/sql.go:290: defer func() {
On 2012/03/08 15:26:14, rsc wrote:
> Cute but maybe a little subtle and also hard to limit.
> 
> maybe s/prepare/prepare1/ and then do
> 
> func (db *DB) prepare(query string) (stmt *Stmt, err error) {
>     for i := 0; i < 10; i++ {
>         stmt, err = db.prepare1(query)
>         if err != driver.ErrBadConn {
>             break
>         }
>     }
>     return
> }

Done.

http://codereview.appspot.com/5785043/diff/6001/src/pkg/database/sql/sql.go#n...
src/pkg/database/sql/sql.go:754: for {
On 2012/03/08 15:26:14, rsc wrote:
> for i := 0;; i++ {

Done.

http://codereview.appspot.com/5785043/diff/6001/src/pkg/database/sql/sql.go#n...
src/pkg/database/sql/sql.go:760: if err == driver.ErrBadConn {
On 2012/03/08 15:26:14, rsc wrote:
> && i < 10

Done.

[bradfitz, 2012-03-08 18:09:51]

*** Submitted as http://code.google.com/p/go/source/detail?r=7a8c58d0b7b8 ***

database/sql{,driver}: add ErrBadConn

R=golang-dev, rsc
CC=golang-dev
http://codereview.appspot.com/5785043

[blake.mizerany, 2012-03-08 21:50:28]

I see a problem with this:

If the database returns a FATAL error to me, which it can have many of, I can't
get the message associated with it, to the user this way.

The `HealthyConn`, or `FatalConn` interface approach, you suggested the other
day, really appealed to me because it allows any error value to be returned.

i.e.

type FatalError interface {
   FatalError()
}

if _, ok := err.(driver.FatalError); ok {
    // don't put back in idle pool
    return err
}
...

On 2012/03/08 18:09:51, bradfitz wrote:
> *** Submitted as http://code.google.com/p/go/source/detail?r=7a8c58d0b7b8 ***
> 
> database/sql{,driver}: add ErrBadConn
> 
> R=golang-dev, rsc
> CC=golang-dev
> http://codereview.appspot.com/5785043

[rsc, 2012-03-08 21:57:44]

I don't understand.

If you want the driver to try a new connection, return ErrBadConn.
If you want the driver to return the error to the user, return some
other error.

What other options are there?

Russ

[blake.mizerany, 2012-03-08 22:10:42]

I'm saying I want to have my cake and eat it.

I need to get an error message back to the user, and I also do not
want that connection to get put into the idle pool.

On Thu, Mar 8, 2012 at 1:57 PM, Russ Cox <rsc@golang.org> wrote:
> I don't understand.
>
> If you want the driver to try a new connection, return ErrBadConn.
> If you want the driver to return the error to the user, return some
> other error.
>
> What other options are there?
>
> Russ

[rsc, 2012-03-08 22:14:51]

On Thu, Mar 8, 2012 at 17:10, Blake Mizerany <blake.mizerany@gmail.com> wrote:
> I need to get an error message back to the user, and I also do not
> want that connection to get put into the idle pool.

You could return the real error now and set a bit
so that the next call on that Conn returns ErrBadConn.

Russ

[bradfitz, 2012-03-08 22:15:16]

Get this "FATAL" thing from Postgres, set a "broken" bit on your Conn type,
return ErrWhatever to the user, let it be returned to the pool, and then
next time it's reused, check your broken bit, return ErrBadConn, and a new
conn will be used.

On Thu, Mar 8, 2012 at 2:10 PM, Blake Mizerany <blake.mizerany@gmail.com>wrote:

> I'm saying I want to have my cake and eat it.
>
> I need to get an error message back to the user, and I also do not
> want that connection to get put into the idle pool.
>
> On Thu, Mar 8, 2012 at 1:57 PM, Russ Cox <rsc@golang.org> wrote:
> > I don't understand.
> >
> > If you want the driver to try a new connection, return ErrBadConn.
> > If you want the driver to return the error to the user, return some
> > other error.
> >
> > What other options are there?
> >
> > Russ
>

[kr, 2012-03-08 23:25:05]

I suggest letting the conn expose a flag
to indicate whether it's ok to continue
using it. Every time sql wants to return a
conn to the pool, it can check this flag.

The question of whether or not the conn is
usable isn't really a property of the error;
it's a property of the conn.

This process of returning a descriptive error
to the user, setting a bit, then returning
ErrBadConn would seem to force the end user
to handle two errors unnecessarily.

[bradfitz, 2012-03-08 23:42:34]

ErrBadConn never makes it to the user.

On Fri, Mar 9, 2012 at 10:25 AM, <kr@xph.us> wrote:

> I suggest letting the conn expose a flag
> to indicate whether it's ok to continue
> using it. Every time sql wants to return a
> conn to the pool, it can check this flag.
>
> The question of whether or not the conn is
> usable isn't really a property of the error;
> it's a property of the conn.
>
> This process of returning a descriptive error
> to the user, setting a bit, then returning
> ErrBadConn would seem to force the end user
> to handle two errors unnecessarily.
>
>
http://codereview.appspot.com/**5785043/<http://codereview.appspot.com/5785043/>
>

[blake.mizerany, 2012-03-08 23:51:02]

Ah. I see that now. I apologize.

On Thu, Mar 8, 2012 at 3:42 PM, Brad Fitzpatrick <bradfitz@golang.org> wrote:
> ErrBadConn never makes it to the user.
>
>
> On Fri, Mar 9, 2012 at 10:25 AM, <kr@xph.us> wrote:
>>
>> I suggest letting the conn expose a flag
>> to indicate whether it's ok to continue
>> using it. Every time sql wants to return a
>> conn to the pool, it can check this flag.
>>
>> The question of whether or not the conn is
>> usable isn't really a property of the error;
>> it's a property of the conn.
>>
>> This process of returning a descriptive error
>> to the user, setting a bit, then returning
>> ErrBadConn would seem to force the end user
>> to handle two errors unnecessarily.
>>
>> http://codereview.appspot.com/5785043/
>
>

[kr, 2012-03-09 00:23:24]

Is the flexibility of having a special-case
error value worth the extra complexity?
Do we know that that flexibility will ever
be needed?

Also, I suspect the ErrBadConn strategy will
be more error-prone. Rerunning queries 10
times in a loop is correct only if every driver
author returns ErrBadConn under the right
circumstances without bugs. A driver bug
could cause a query to run several times,
subtly and quietly corrupting database
records. By contrast, a flag for "is-this-conn-
usable" is more forgiving of driver bugs,
since it would fail noisily, sending many
errors to the user.

kr

[rsc, 2012-03-09 03:26:48]

> Also, I suspect the ErrBadConn strategy will
> be more error-prone. Rerunning queries 10
> times in a loop is correct only if every driver
> author returns ErrBadConn under the right
> circumstances without bugs. A driver bug
> could cause a query to run several times,
> subtly and quietly corrupting database
> records. By contrast, a flag for "is-this-conn-
> usable" is more forgiving of driver bugs,
> since it would fail noisily, sending many
> errors to the user.

There's an inherent race in asking the conn
"are you usable right now?" and then later
(even microseconds later) asking it to do work.
The ErrBadConn solution, like the x,ok form of
the channel receive, avoids the race by letting
the operation itself return the ok value.

No matter what we do, it is true that if drivers are
buggy, bad things will happen.

Russ

[kr, 2012-03-09 04:57:02]

On Thu, Mar 8, 2012 at 7:26 PM, Russ Cox <rsc@golang.org> wrote:
> There's an inherent race in asking the conn
> "are you usable right now?" and then later
> (even microseconds later) asking it to do work.

ErrBadConn has the same race.

Case 1: sql issues a command, and the driver
returns something other than ErrBadConn, so sql
assumes the conn is usable until the next command.

Case 2: sql calls conn.IsUsable() and the driver
returns true, so sql assumes the conn is usable
until the next command.

Either way, if the conn becomes unusable in that
interval, the requested action will fail. That's ok.

> No matter what we do, it is true that if drivers are
> buggy, bad things will happen.

Some bad things are worse than others. I maintain
that running a command in a loop in the sql package
is riskier than running it once.

Aside from that, the code to deal with ErrBadConn
is more complicated for both sql and the drivers.

kr

[rsc, 2012-03-09 05:05:09]

On Thu, Mar 8, 2012 at 23:56, Keith Rarick <kr@xph.us> wrote:
> ErrBadConn has the same race.
>
> Case 1: sql issues a command, and the driver
> returns something other than ErrBadConn, so sql
> assumes the conn is usable until the next command.
>
> Case 2: sql calls conn.IsUsable() and the driver
> returns true, so sql assumes the conn is usable
> until the next command.

It only has this race if you use it the way that you
asked for (and Brad and I both suggested keeping
a failure bit), where you are trying to expose the
FATAL error to the user.

If you have another situation, where there is such a thing
as a retryable error, then using ErrBadConn can handle
that situation race-free, while IsUsable cannot.

Russ

[kr, 2012-03-09 05:55:15]

On Thu, Mar 8, 2012 at 9:05 PM, Russ Cox <rsc@golang.org> wrote:
> If you have another situation, where there is such a thing
> as a retryable error, then using ErrBadConn can handle
> that situation race-free, while IsUsable cannot.

Yes, a subset of errors make the conn unusable, and
a subset of those errors can be safely retried.
ErrBadConn has an advantage if it's desirable to
hide that smaller subset from the user. Is that
a goal? It might be preferable for the user to be
able to see those errors. And, even if we want to
hide those errors, is that smaller subset large
enough to be worth the cost?

(I'm not asking rhetorically; I honestly don't know.)

kr

[rsc, 2012-03-09 19:11:02]

On Fri, Mar 9, 2012 at 00:55, Keith Rarick <kr@xph.us> wrote:
> Yes, a subset of errors make the conn unusable, and
> a subset of those errors can be safely retried.
> ErrBadConn has an advantage if it's desirable to
> hide that smaller subset from the user. Is that
> a goal? It might be preferable for the user to be
> able to see those errors. And, even if we want to
> hide those errors, is that smaller subset large
> enough to be worth the cost?
>
> (I'm not asking rhetorically; I honestly don't know.)

I don't know either.  However, the possible benefit seems
large - if this case comes up, we can handle it - and the
cost seems miniscule: people writing drivers have to write a
few more lines of code.  How many drivers will there ever be?
10?  50?

FWIW, I do think that the case comes up: if you have a
long-standing TCP connection to a database server and
an idle connection drops between calls, you can return ErrBadConn
from the next call and avoid showing that connection drop
to the user.

Russ