fix: implement sql/driver.Pinger interface #57
Conversation
The `sql/driver.Pinger` interface is an optional interface to implement. For drivers that do implement the interface, the `Conn` type needs to try to call through.
| func (conn *Conn) Ping(ctx context.Context) error { | ||
| p, ok := conn.Conn.(driver.Pinger) | ||
| if !ok { | ||
| return driver.ErrSkip |
There was a problem hiding this comment.
ErrSkip is not documented as something supported by Ping. Reading the documentation of driver this error is only allowed to be returned when explicitly documented as supported for that function.
To be honest, to support Ping here properly is quite a bit of effort. However, its pretty bad that sqlhooks doesn't implement Ping since pretty much everyone should be supporting it.
So either we do the hard work of correctly supporting Ping, or we document a contract that Pinger has to be supported by whatever we wrap?
There was a problem hiding this comment.
ErrSkip is not documented as something supported by Ping. Reading the documentation of driver this error is only allowed to be returned when explicitly documented as supported for that function.
As per the docs:
// ErrSkip may be returned by some optional interfaces' methods to
// indicate at runtime that the fast path is unavailable and the sql
// package should continue as if the optional interface was not
// implemented.
What I understand is that the optional interface Ping implementation should return ErrSkip when not implemented by the underlying driver.
I'd love to understand better what you mean @keegancsmith .
Also, assuming most drivers already implement Ping, what would be the downside of keeping this change?
There was a problem hiding this comment.
From https://pkg.go.dev/database/sql/[email protected]#pkg-variables
ErrSkip may be returned by some optional interfaces' methods to indicate at runtime that the fast path is unavailable and the sql package should continue as if the optional interface was not implemented. ErrSkip is only supported where explicitly documented.
Note the mention of explicitly documented. Then look at the documentation of Pinger and it is not explicitly documented unlike some other optional interfaces.
The downside of keeping this change is the call sites of Ping inside of driver will fail rather than handling ErrSkip for drivers that do not support Pinger. I took a quick look at the code and think non-pinger supported drivers will be broken.
However, I think this project should support this interface given in practice I assume all drivers support it.
There was a problem hiding this comment.
@wburningham are you aware of any sql driver that does not implement the Pinger interface so that we can test the ErrSkip behavior and see how bad the damage will be?
There was a problem hiding this comment.
I'm not aware of any, but commenting out the Ping method of a sqllite driver should be sufficient for local testing.
There was a problem hiding this comment.
is that something you can try and share the outcome?
|
Out of interest at Sourcegraph we work around this problem by wrapping the driver again and skipping over sqlhooks. |
The
sql/driver.Pingerinterface is an optionalinterface to implement. For drivers that do
implement the interface, the
Conntype needs totry to call through.