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: proper prepared statement support in transactions #15606

Closed
dtromb opened this issue May 8, 2016 · 6 comments
Closed

database/sql: proper prepared statement support in transactions #15606

dtromb opened this issue May 8, 2016 · 6 comments
Assignees
Milestone

Comments

@dtromb
Copy link

@dtromb dtromb commented May 8, 2016

A very common use case is to create a bunch of prepared statements in a connection manager concern, when a new connection is needed and created (say, when a component requests a connection from a connection pool and there is not an idle one at the moment), associate the user context with a transaction on the returned connection (say when a web service begins to execute an http service request with a connection obtained from a connection pool), and then execute the prepared statements in it.

It seems, however, that Go database/sql requires the recompilation of every prepared statement each time it executes:

/* database/sql.go:1063, Go 1.6 */
        func (tx *Tx) Stmt(stmt *Stmt) *Stmt {
        // TODO(bradfitz): optimize this. Currently this re-prepares
        // each time.  This is fine for now to illustrate the API but
        // we should really cache already-prepared statements
        // per-Conn. See also the big comment in Tx.Prepare.

This is not fine, I think - it's worse than useless. In particular, it's strictly less performant (and /quietly/ so!) than not using statements where there are transactions in use.

So, we have a situation, it seems, where there is no de facto prepared statement support. Is this really the case? What needs to be done to implement support?

It appears as though Stmt /already/ knows how to do the right thing wrt transactions:

    /* database/sql.go:741 */
        // In a transaction, we always use the connection that the
        // transaction was created on.
        if s.tx != nil {
                s.mu.Unlock()
                ci, err = s.tx.grabConn() // blocks, waiting for the connection.
                if err != nil {
                        return
                }
                releaseConn = func(error) { s.tx.releaseConn() }
                return ci, releaseConn, s.txsi, nil
        }

Can we just create

func (tx *Tx) ExecStmt(s *Stmt, args ...interface{}) ...

and the like, which shallow-clone the Stmt (ie. reuse the same driver statement object), and associate a transaction with the new copy, before calling its Exec()?

@bradfitz bradfitz changed the title Proper SQL prepared statement support in transactions database/sql: proper prepared statement support in transactions May 9, 2016
@bradfitz bradfitz added this to the Go1.8 milestone May 9, 2016
@johto
Copy link
Contributor

@johto johto commented Jun 28, 2016

I was working on making this work here, but I never finished; and I never will so if you're interested, you could try picking it up.

@quentinmit quentinmit added the NeedsFix label Oct 6, 2016
@rsc rsc modified the milestones: Go1.9, Go1.8 Nov 11, 2016
@kardianos kardianos self-assigned this Nov 28, 2016
@quentinmit
Copy link
Contributor

@quentinmit quentinmit commented Jan 17, 2017

FWIW I just ran into this in production, and I only figured it out by logging raw packets. This is a huge gotcha.

@kardianos
Copy link
Contributor

@kardianos kardianos commented Jan 17, 2017

@quentinmit please drop by https://groups.google.com/forum/#!forum/golang-sql if you get a chance. I hope to tackle it soon(tm).

@gopherbot
Copy link

@gopherbot gopherbot commented Jan 18, 2017

CL https://golang.org/cl/35266 mentions this issue.

gopherbot pushed a commit to golang/perf that referenced this issue Jan 18, 2017
The MySQL protocol requires 1-3 synchronous round-trips for every
INSERT statement; to reduce the overhead, we now batch up 900 label
INSERT statments at a time. This makes a massive difference;
TestQuery previously ran in 108s; with this change, it now runs in 5s.

We were also affected by golang/go#15606; since we now generate a new INSERT
statement for every record, we are sidestepping that issue.

Change-Id: Id7a56c18c0978470542135894a2f2bcf6f7c9dd1
Reviewed-on: https://go-review.googlesource.com/35266
Reviewed-by: Russ Cox <rsc@golang.org>
@gopherbot
Copy link

@gopherbot gopherbot commented Jan 24, 2017

CL https://golang.org/cl/35476 mentions this issue.

@gopherbot gopherbot closed this in 82e1732 Mar 8, 2017
@golang golang locked and limited conversation to collaborators Mar 8, 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
8 participants
You can’t perform that action at this time.