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: document what to do on DB.Ping error #23738

Closed
pjebs opened this issue Feb 8, 2018 · 4 comments

Comments

@pjebs
Copy link

@pjebs pjebs commented Feb 8, 2018

The documentation doesn't make clear what to do if we get an error "bad connection".

When Ping returns an error, is it advisable to close the DB object and create a new DB object to replace the old one?

After all, if Ping returns an error, something is clearly wrong.
Is it only the connection that is bad or the pool.

This is how a lot of people are implementing it: https://github.com/pjebs/go-skeleton/blob/master/app/providers/database.go#L61

@bradfitz bradfitz changed the title closing DB after Ping returns an error database/sql: document what to do on DB.Ping error Feb 8, 2018
@bradfitz bradfitz added this to the Go1.11 milestone Feb 8, 2018
@bradfitz bradfitz added the NeedsFix label Feb 8, 2018
@kardianos

This comment has been minimized.

Copy link
Contributor

@kardianos kardianos commented Feb 8, 2018

I'm not exactly sure what you mean by a "bad pool". How would you tell the difference between an incorrect DSN and a temporary network problem? I think retrying or closing the pool could both be correct.

Glancing at the code you linked concerns me the author is confusing a DB pool with a single connection.

I'm not convinced the docs need clarification, but I can look into it. Maybe we can discuss the pros and cons of each approch, maybe provide a recommendation. There is a direct tie here with how an application comes online (like needing a healthz endpoint or windows service).

@AlekSi

This comment has been minimized.

Copy link
Contributor

@AlekSi AlekSi commented Feb 20, 2018

I'm not convinced the docs need clarification

As an author of one Go SQL driver, author of one ORM-ish package, and contributor to numerous other SQL-related Go package, I can say that database/sql and database/sql/driver documentation can definitely be improved. Specifically, it would be very helpful if documentation for more or all methods and interfaces in database/sql/driver mention how returned errors are handled by database/sql internals. I don't think that would completely eliminate a need to dig into the code, but at least that documentation can be an explicit contract between the library and third-party drivers.

Sorry if it is not very specific. I will create separete specific issues and/or CL when I encounter those issues again.

@kardianos

This comment has been minimized.

Copy link
Contributor

@kardianos kardianos commented Mar 16, 2018

@AlekSi From what I can see, database/sql/driver does a good job explaining what special errors can be returned by interfaces.

However, until go1.10 with the SessionResetter interface, there was no good way to expose normal database errors to users without poisoning the connection pool. Now the story is simple: return the normal custom or wrapped network database error to the user. Then when the ResetSession on the connection is called, return driver.ErrBadConn.

I would love to have a conversation on another issue or https://groups.google.com/forum/#!forum/golang-sql about ways we can improve documentation in specific ways (or find an existing issue). One thing that would probably be helpful is to provide a bare-bones "real" database driver for others to inspect.

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Mar 16, 2018

Change https://golang.org/cl/101216 mentions this issue: database/sql: add more complete examples for opening a DB

@ianlancetaylor ianlancetaylor modified the milestones: Go1.11, Go1.12 Jun 29, 2018
@gopherbot gopherbot closed this in a33cebf Nov 16, 2018
bradfitz pushed a commit that referenced this issue Nov 21, 2018
Show two larger application examples. One example that
could be used in a CLI, the other in a long running
service. These demonstarates different strategies for
handling DB.Ping errors in context.

Fixes #23738

Change-Id: Id01213caf1f47917239a7506b01d30e37db74d31
Reviewed-on: https://go-review.googlesource.com/c/101216
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
@golang golang locked and limited conversation to collaborators Nov 16, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
6 participants
You can’t perform that action at this time.