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

implement driver.Pinger interface #737

Merged
merged 1 commit into from Mar 21, 2019
Merged

implement driver.Pinger interface #737

merged 1 commit into from Mar 21, 2019

Conversation

ghouscht
Copy link
Contributor

@ghouscht ghouscht commented Apr 2, 2018

This PR implements the driver.Pinger interface. I had some trouble in an internal project because I relayed on database/sql's Ping Method to check the DB connection. Unfortunately this interface is currently not implemented (nor documented -> resulting in a inaccurate default behaviour) by lib/pq and I lost quite a bit of time finding out why.

Credits to @itsjamie who created the original PR #675 (code is mostly copy/paste) except for the tests (anyway idea for the test also came from @itsjamie).

I hope to get some attention with this PR so this (or PR #675) can be merged to master.

@ghouscht
Copy link
Contributor Author

ghouscht commented Apr 2, 2018

Unfortunately the db.Conn method was not exported in go1.8, thus those tests failed. Will work around this tomorrow.

@danilobuerger
Copy link

Struggled with the same problem. Hope this gets merged soon.

@danilobuerger
Copy link

ping @mjibson

@itsjamie
Copy link

itsjamie commented Sep 1, 2018

Some handling of the Ping interface would be good to have in the driver to prevent further confusion. Please consider this to be merged in the next release for v1.1.0 of lib/pq.

Closing #675 in favor of this as it is a superset of what I had written, with the test as @ghouscht mentioned and @mjibson requested in the original review.

conn_go18.go Outdated
if err != nil {
return driver.ErrBadConn // https://golang.org/pkg/database/sql/driver/#Pinger
}
defer rows.Close()
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no need for the defer. just close it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, changed that and squashed all the commits together. So this can hopefully be merged soon.

@andradei
Copy link

andradei commented Mar 21, 2019

When can this be merged? Looks like a good solution for the Pinger implementation and would close #533 which is a limitation.

@kardianos
Copy link
Contributor

cc @mjibson This PR LGTM

@maddyblue maddyblue merged commit 5ccc985 into lib:master Mar 21, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants