Add sql.Pinger implementation. #675
Conversation
Executes a query against the configured database.
|
The failed tests here just likely need to be re-ran, it looks like googlesource didn't respond for one of the test runs. Any comment on the PR? Thanks. |
|
I restarted the failing test job for you. The code looks good to me, but I'm not an active maintainer so I'll let someone else review and merge. |
|
Is it possible or reasonable to send a Flush message instead of a query? |
|
@cbandy I'm not sure, I did the initial research on how others we're accomplishing this and found a few notes from other libraries that we're simply accomplishing it with SELECTs, see the #533 ticket for any discussion points. That's not to say there isn't a better method, I'm just not sure. I did notice on a few newsletter posts there seemed to be a fair amount of pushback at Postgres-dev level on implementing any kind of pg_ping() style command you could execute to validate the database was up. @uhoh-itsmaciek Thank you for running the tests again. |
| @@ -76,6 +76,19 @@ func (cn *conn) BeginTx(ctx context.Context, opts driver.TxOptions) (driver.Tx, | |||
| return tx, nil | |||
| } | |||
|
|
|||
| func (cn *conn) Ping(ctx context.Context) error { | |||
There was a problem hiding this comment.
The godoc for the Ping function (https://golang.org/pkg/database/sql/driver/#Pinger) says it should return driver.ErrBadConn. I would like that error to be explicitly returned instead of returning whatever it is simpleQuery and rows.Close return (which I think is indeed driver.ErrBadConn from a quick look). We should hard code it here because that's what the API requires.
There was a problem hiding this comment.
Will update this tonight. Thanks.
There was a problem hiding this comment.
This should be solved with latest commit.
|
|
||
| cancel() | ||
|
|
||
| if err := db.PingContext(ctx); err == nil { |
There was a problem hiding this comment.
Can we come up with more tests that don't use context cancellation? I'd like a test where the conn has actually died and the query itself fails.
There was a problem hiding this comment.
Is there a method in Postgres via Travis that you're aware of to temporarily prevent access?
Alternatively, could grab one of the test SQL drivers that returns a good conn, and then mock the Ping call to return an error. It looks though like the driver is trying to stay using actual Postgres as much as possible.
There was a problem hiding this comment.
Here's a potential solution, what do you think..
Start a transaction, execute a query to grab the pid that postgres has given that open connection.
Grab a new connection, and execute SELECT pg_terminate_backend(pid) from the in-progress transaction to kill the backing process. This should cause the connection to die, and the commit should fail.
Pointed out that the Ping caller checks for the sentinel error driver.ErrBadConn, so we should explicitly return that if there is an error executing the simple SELECT.
|
Any updates on this issue? |
|
Just for reference to those who get here. #737 is the preferred PR to solve the problem. |
Executes a query
SELECT 'lib/pq ping test'against the configured database.Closes #533.