database/sql: Avoid re-preparing statements when all connections are busy

Previously, if all connections were busy, we would always
re-prepare the statement on the connection we were assigned from
the pool.  That meant that if all connections were busy most of the
time, the number of prepared statements for each connection would
keep increasing over time.

Instead, after getting a free connection, check to see if the
statement has already been prepared on it, and reuse the statement
handle if so.

LGTM=bradfitz
R=golang-codereviews, gobot, bradfitz
CC=golang-codereviews
https://golang.org/cl/116930043

diff --git a/src/pkg/database/sql/sql.go b/src/pkg/database/sql/sql.go
index 63c6dd68167bd48ce5a723b939c5be26e5cc3bdb..90f813d8230ae06bf5e6a6f92cf19b0d43442381 100644 (file)
--- a/src/pkg/database/sql/sql.go
+++ b/src/pkg/database/sql/sql.go
@@ -1326,15 +1326,12 @@ func (s *Stmt) connStmt() (ci *driverConn, releaseConn func(error), si driver.St
                return ci, releaseConn, s.txsi.si, nil
        }
 
-       var cs connStmt
-       match := false
        for i := 0; i < len(s.css); i++ {
                v := s.css[i]
                _, err := s.db.connIfFree(v.dc)
                if err == nil {
-                       match = true
-                       cs = v
-                       break
+                       s.mu.Unlock()
+                       return v.dc, v.dc.releaseConn, v.si, nil
                }
                if err == errConnClosed {
                        // Lazily remove dead conn from our freelist.
@@ -1346,28 +1343,41 @@ func (s *Stmt) connStmt() (ci *driverConn, releaseConn func(error), si driver.St
        }
        s.mu.Unlock()
 
-       // Make a new conn if all are busy.
-       // TODO(bradfitz): or wait for one? make configurable later?
-       if !match {
-               dc, err := s.db.conn()
-               if err != nil {
-                       return nil, nil, nil, err
-               }
-               dc.Lock()
-               si, err := dc.prepareLocked(s.query)
-               dc.Unlock()
-               if err != nil {
-                       s.db.putConn(dc, err)
-                       return nil, nil, nil, err
+       // If all connections are busy, either wait for one to become available (if
+       // we've already hit the maximum number of open connections) or create a
+       // new one.
+       //
+       // TODO(bradfitz): or always wait for one? make configurable later?
+       dc, err := s.db.conn()
+       if err != nil {
+               return nil, nil, nil, err
+       }
+
+       // Do another pass over the list to see whether this statement has
+       // already been prepared on the connection assigned to us.
+       s.mu.Lock()
+       for _, v := range s.css {
+               if v.dc == dc {
+                       s.mu.Unlock()
+                       return dc, dc.releaseConn, v.si, nil
                }
-               s.mu.Lock()
-               cs = connStmt{dc, si}
-               s.css = append(s.css, cs)
-               s.mu.Unlock()
        }
+       s.mu.Unlock()
+
+       // No luck; we need to prepare the statement on this connection
+       dc.Lock()
+       si, err = dc.prepareLocked(s.query)
+       dc.Unlock()
+       if err != nil {
+               s.db.putConn(dc, err)
+               return nil, nil, nil, err
+       }
+       s.mu.Lock()
+       cs := connStmt{dc, si}
+       s.css = append(s.css, cs)
+       s.mu.Unlock()
 
-       conn := cs.dc
-       return conn, conn.releaseConn, cs.si, nil
+       return dc, dc.releaseConn, si, nil
 }
 
 // Query executes a prepared query statement with the given arguments

diff --git a/src/pkg/database/sql/sql_test.go b/src/pkg/database/sql/sql_test.go
index 8849c81c4bc8603257b981f1648f14f4e62352af..12e5a6fd6fc04b305a47fe0888091d2c7f64bd91 100644 (file)
--- a/src/pkg/database/sql/sql_test.go
+++ b/src/pkg/database/sql/sql_test.go
@@ -1348,6 +1348,11 @@ func TestErrBadConnReconnect(t *testing.T) {
                return nil
        })
 
+       // Provide a way to force a re-prepare of a statement on next execution
+       forcePrepare := func(stmt *Stmt) {
+               stmt.css = nil
+       }
+
        // stmt.Exec
        stmt1, err := db.Prepare("INSERT|t1|name=?,age=?,dead=?")
        if err != nil {
@@ -1355,9 +1360,7 @@ func TestErrBadConnReconnect(t *testing.T) {
        }
        defer stmt1.Close()
        // make sure we must prepare the stmt first
-       for _, cs := range stmt1.css {
-               cs.dc.inUse = true
-       }
+       forcePrepare(stmt1)
 
        stmtExec := func() error {
                _, err := stmt1.Exec("Gopher", 3, false)
@@ -1373,9 +1376,7 @@ func TestErrBadConnReconnect(t *testing.T) {
        }
        defer stmt2.Close()
        // make sure we must prepare the stmt first
-       for _, cs := range stmt2.css {
-               cs.dc.inUse = true
-       }
+       forcePrepare(stmt2)
 
        stmtQuery := func() error {
                rows, err := stmt2.Query()