Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

database/sql: Open() doesn't actually open a connection #4804

Closed
julienschmidt opened this issue Feb 14, 2013 · 9 comments
Closed

database/sql: Open() doesn't actually open a connection #4804

julienschmidt opened this issue Feb 14, 2013 · 9 comments
Assignees

Comments

@julienschmidt
Copy link
Contributor

@julienschmidt julienschmidt commented Feb 14, 2013

It is very unintuitive that Open() does not directly open a connection to the database.
Instead the first connection is opened when the database is actually used the first time
(first query).

Often people try to catch errors on Open() (wrong password, server not running etc) but
this doesn't work.

The database/sql package should either actually open a connection on Open() or at least
it should be documented that it does not.
@bradfitz
Copy link
Contributor

@bradfitz bradfitz commented Feb 14, 2013

Comment 1:

Good point.  I've ended up implementing that myself a number of times.
Alternatively/additionally, I'd like to see a *DB and optional Driver way to do:
// Ping returns whether the database is available.
func (*DB) Ping() error
But ignoring Ping for now, I think Open getting (& maybe returning or returning in n
seconds) a connection is a good default policy.
But I'd like drivers to be able to opt-out of it.  I'd make a new optional interface on
Driver like:
// OpenInitialer is an optional interface that may be implemented by drivers which
// have a preference on whether an initial connection is created on sql.Open to
// determine the health of the database.  OpenInitial should behave exactly like
// Driver.Open, but the ErrSkip value may be returned to indicate that an initial
// connection should not be opened.
type OpenInitialer interface {
    OpenInitial(name string) (Conn, error)
}
Want to do this?

Labels changed: removed priority-triage.

Status changed to Accepted.

@julienschmidt
Copy link
Contributor Author

@julienschmidt julienschmidt commented Feb 14, 2013

Comment 2:

Mhm, Ping() sounds useful. Time to undeprecate
https://github.com/Go-SQL-Driver/MySQL/blob/master/connection.go#L319
The optional opt-out seems fine to me.
Right now I have no time to implement this since it's finals time (fun fact: next one is
on ... surprise: databases) so don't hesitate to start working on this.
@julienschmidt
Copy link
Contributor Author

@julienschmidt julienschmidt commented Feb 26, 2013

Comment 3:

The finals are mastered, so I start working on this now.
Whats your opinion on yet another optional interface which allows to "clone"
connections? So the driver doesn't need to parse the DSN again. Maybe the driver also
queried some server vars which could be taken over instead of querying them again or
there are some other checks / operations a driver might skip.
So the first connection is opened with Driver.OpenInitial(dsn) and for all further
connections opened internally by the sql package just Conn.Clone().
@rsc
Copy link
Contributor

@rsc rsc commented Mar 12, 2013

Comment 4:

[The time for maybe has passed.]

Labels changed: removed go1.1maybe.

@bradfitz
Copy link
Contributor

@bradfitz bradfitz commented Mar 14, 2013

Comment 5:

Sent out https://golang.org/cl/7819043

Owner changed to @bradfitz.

Status changed to Started.

@bradfitz
Copy link
Contributor

@bradfitz bradfitz commented Mar 14, 2013

Comment 6:

This issue was closed by revision a4a8651.

Status changed to Fixed.

@bradfitz
Copy link
Contributor

@bradfitz bradfitz commented Mar 14, 2013

Comment 7:

To be clear, this was "Fixed" by documenting the status quo instead, and adding a Ping
method.
@julienschmidt
Copy link
Contributor Author

@julienschmidt julienschmidt commented Dec 5, 2013

Comment 8:

Can you please reopen the issue?
We have a workaround now, but it is not as 'fixed' as it should be in the long term.
@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Dec 6, 2013

Comment 9:

It's not clear to me that changing the status quo is appropriate.  It appears that it's
straightforward to get the functionality you want.
I'll leave it for Brad to decide whether we should change Open.
@julienschmidt julienschmidt added the fixed label Dec 6, 2013
@golang golang locked and limited conversation to collaborators Jun 24, 2016
This issue was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
5 participants
You can’t perform that action at this time.