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: Drivers cannot deprecate Queryer and Execer #21663

Closed
vmg opened this issue Aug 28, 2017 · 3 comments
Closed

database/sql: Drivers cannot deprecate Queryer and Execer #21663

vmg opened this issue Aug 28, 2017 · 3 comments
Assignees
Milestone

Comments

@vmg
Copy link

@vmg vmg commented Aug 28, 2017

What version of Go are you using (go version)?

~ $ go version
go version go1.9 linux/amd64

Does this issue reproduce with the latest release?

Yes, all releases since 1.8

What operating system and processor architecture are you using (go env)?

~ $ go env
GOARCH="amd64"
GOBIN=""
GOEXE=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOOS="linux"
GOPATH="/home/vmg/src/gopath"
GORACE=""
GOROOT="/usr/local/go"
GOTOOLDIR="/usr/local/go/pkg/tool/linux_amd64"
GCCGO="gccgo"
CC="gcc"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build317274576=/tmp/go-build -gno-record-gcc-switches"
CXX="g++"
CGO_ENABLED="1"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"

What did you do?

According to the docs in database/sql/driver, the Queryer interface for SQL drivers is deprecated and QueryContext should be implemented instead:

Queryer is an optional interface that may be implemented by a Conn.
If a Conn does not implement Queryer, the sql package's DB.Query will first prepare a query, execute the statement, and then close the statement.
Query may return ErrSkip.
Deprecated: Drivers should implement QueryerContext instead (or additionally).

I created a dummy SQL driver that implements QueryerContext, but not Queryer. I also implemented an identical dummy driver that implements both Queryer and QueryerContext.

I then ran a simple SELECT query using both drivers.

Go Playground link

What did you expect to see?

I expected the database/sql library to call QueryContext() on both my dummy drivers, and to print Called QueryContext with 'SELECT 'foobar'' twice.

What did you see instead?

The dummy driver that implements Queryer and QueryerContext works just fine, and calls QueryContext() to resolve the query.

The dummy driver that only implements QueryerContext does not work. It panics because database/sql doesn't attempt to use QueryContext(), and instead falls back to preparing a statement and discarding it immediately.

Further notes

I came across this bug when attempting to write a "wrapper" driver to instrument SQL queries in an app. The very first version showed a 100% slowdown when performing simple queries with a wrapped MySQL driver.

After some investigation, I noticed that the implementation for both Query and Exec in database/sql does not match the provided documentation.

The docs state that the Queryer and Execer interfaces are now deprecated, and new drivers should implement their Context-aware counterparts "instead". But this is not the case: if the original deprecated interfaces are not implemented, the following type checks will fail and the library will never call the new interfaces:

go/src/database/sql/sql.go

Lines 1311 to 1312 in 5d39af9

func (db *DB) queryDC(ctx, txctx context.Context, dc *driverConn, releaseConn func(error), query string, args []interface{}) (*Rows, error) {
if queryer, ok := dc.ci.(driver.Queryer); ok {

go/src/database/sql/sql.go

Lines 1241 to 1245 in 5d39af9

func (db *DB) execDC(ctx context.Context, dc *driverConn, release func(error), query string, args []interface{}) (res Result, err error) {
defer func() {
release(err)
}()
if execer, ok := dc.ci.(driver.Execer); ok {

The end result is that any new SQL driver that implements only the new QueryContext() and ExecContext() methods will not really work: these methods will never be called, and database/sql will fallback to the slow path of preparing and discarding a statement for the query.

The same situation will happen if any of the existing SQL drivers decide to truly deprecate Query and Exec in any future release by removing the two APIs altogether.

@kardianos kardianos self-assigned this Sep 7, 2017
@kardianos
Copy link
Contributor

@kardianos kardianos commented Sep 7, 2017

Related #21084

@kardianos
Copy link
Contributor

@kardianos kardianos commented Sep 9, 2017

I agree that this should be done and will target go1.10. I haven't looked at the code in detail with this in mind, but I think it would be reasonable for anyone to do this.

@kardianos kardianos added this to the Go1.10 milestone Sep 9, 2017
@gopherbot
Copy link

@gopherbot gopherbot commented Sep 24, 2017

Change https://golang.org/cl/65733 mentions this issue: database/sql: allow drivers to only implement Context varients

@gopherbot gopherbot closed this in 5327148 Oct 24, 2017
@golang golang locked and limited conversation to collaborators Oct 24, 2018
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
3 participants
You can’t perform that action at this time.