Treat dial error as driver.ErrBadConn

[tjenkinson]

Description

Currently if an error happens from the Dial() call, there are no retries. The go sql driver will only retry on a driver.ErrBadConn error.

The following is the doc for this error:

// ErrBadConn should be returned by a driver to signal to the sql
// package that a driver.Conn is in a bad state (such as the server
// having earlier closed the connection) and the sql package should
// retry on a new connection.
//
// To prevent duplicate operations, ErrBadConn should NOT be returned
// if there's a possibility that the database server might have
// performed the operation. Even if the server sends back an error,
// you shouldn't return ErrBadConn.

I noticed that other functions in MySQLDriver.Open() already return this error, so I wonder if this was an oversight?

One concern with this change is that users might be expecting and handling a net.Error from dial(), which with this PR would only be logged now.

It would also be possible to handle this without changes to the lib, by using RegisterDial(). If we are not concerned that users might be expecting net.Error, I feel that driver.ErrBadConn is a more appropriate error and users should't be required to use RegisterDial() if they want retries.

Checklist

• Code compiles correctly
• Created tests which fail without the change (if possible)
• All tests passing
• Extended the README / documentation, if necessary
• Added myself / the copyright holder to the AUTHORS file

[methane]

In case of Open(), all error means "driver.Conn is in bad state".
And since no query is sent in Open(), it's safe to retry in new connection.

[methane]

Then, we don't need to return ErrBadConn always in Open().

When we think database/sql should retry Open() again, we should return ErrBadConn.
Otherwise, returning ErrBadConn is not worth enough.

For example, if context is canceled, we should not return ErrBadConn.
On the other hand, if nerr, ok := err.(net.Error); ok && nerr.Temporary(), it may be worth to return ErrBadConn.

[tjenkinson]

👍 @methane I added a commit to only return driver.ErrBadConn on a temporary error or timeout

[methane]

Don't || nerr.Timeout().

nerr.Temporary() returns True when retry is worthful.
For example, lookup timeout is temporary.

func (e *DNSError) Temporary() bool { return e.IsTimeout || e.IsTemporary }

[tjenkinson]

👍

the docs say

IsTimeout   bool   // if true, timed out; not all timeouts set this
IsTemporary bool   // if true, error is temporary; not all errors set this; added in Go 1.6

so I thought it would be better to check both given IsTemporary might not be supported

[tjenkinson]

@methane is this good to be merged?

[methane]

I still dought this is worth enough to add more code to this project.
Is it really needed for your project?

[methane]

After reading database/sql again, I think this change is reasonable even though
I don't have experience I want this. Thanks.