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: type Stmt struct reused bug #8376

Closed
gopherbot opened this issue Jul 16, 2014 · 5 comments
Closed

database/sql: type Stmt struct reused bug #8376

gopherbot opened this issue Jul 16, 2014 · 5 comments

Comments

@gopherbot
Copy link

@gopherbot gopherbot commented Jul 16, 2014

by beyondmj0.student@sina.com:

go version go1.3 darwin/amd64


when reuse stmt struct to do "Query", "Exec", "QueryRow"
seems has bug.

mysql:
show global status like '%stmt%';
| Prepared_stmt_count        | 7598   |
| Prepared_stmt_count        | 7600   |
| Prepared_stmt_count        | 7603   |
| Prepared_stmt_count        | 7607   |


can't reuse prepared statement, the prepared_stmt_count increment till exceed
max_prepare_stmt in mysql.


http://golang.org/src/pkg/database/sql/sql.go?s=32373:33023#L1338
if all connection busy, then 
http://golang.org/src/pkg/database/sql/sql.go?s=32373:33023#L1358
get a connection from pool, then call prepareLocked the prepare a new stmt .

but!!!!
if the connection in pool was just released by a another goroutine with a prepared stmt
stored in dc.openStmt,
then 
http://golang.org/src/pkg/database/sql/sql.go?s=32373:33023#L251
create a duplicate one....

codes:
http://play.golang.org/p/hxq1tyHS4e
@gopherbot
Copy link
Author

@gopherbot gopherbot commented Jul 16, 2014

Comment 1 by beyondmj0.student@sina.com:

http://golang.org/src/pkg/database/sql/sql.go:
  1336      var cs connStmt
  1337      match := false
  1338      for i := 0; i < len(s.css); i++ {
  1339          v := s.css[i]
  1340          _, err := s.db.connIfFree(v.dc)
  1341          if err == nil {
  1342              match = true
  1343              cs = v
  1344              break
  1345          }
  1346          if err == errConnClosed {
  1347              // Lazily remove dead conn from our freelist.
  1348              s.css[i] = s.css[len(s.css)-1]
  1349              s.css = s.css[:len(s.css)-1]
  1350              i--
  1351          }
  1352  
  1353      }
  1354      s.mu.Unlock()
  1355  
  1356      // Make a new conn if all are busy.
  1357      // TODO(bradfitz): or wait for one? make configurable later?
  1358      if !match {
  1359          dc, err := s.db.conn()
  1360          if err != nil {
  1361              return nil, nil, nil, err
  1362          }
  1363          dc.Lock()
  1364          si, err := dc.prepareLocked(s.query)
  1365          dc.Unlock()
  1366          if err != nil {
  1367              s.db.putConn(dc, err)
  1368              return nil, nil, nil, err
  1369          }
  1370          s.mu.Lock()
  1371          cs = connStmt{dc, si}
  1372          s.css = append(s.css, cs)
  1373          s.mu.Unlock()
  1374      }
@griesemer
Copy link
Contributor

@griesemer griesemer commented Oct 1, 2014

Comment 2:

Labels changed: added repo-main.

@gopherbot
Copy link
Author

@gopherbot gopherbot commented Oct 8, 2014

Comment 3 by marty.woodlee@mapmyfitnessinc.com:

We are experiencing similar problems and came to the same conclusion regarding the root
cause before finding this issue. Here's a proposal for a solution. I've compiled code
against this and been able to get rid of the excessive statement preparation and memory
leakage that was occurring otherwise:
@@ -1360,17 +1360,27 @@ func (s *Stmt) connStmt() (ci *driverConn, releaseConn
func(error), si driver.St
                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
+               alreadyPrepared := false
+               for _, v := range s.css {
+                       if v.dc == dc {
+                               alreadyPrepared = true
+                               cs = v
+                               break
+                       }
+               }
+               if alreadyPrepared == false {
+                       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()
                }
-               s.mu.Lock()
-               cs = connStmt{dc, si}
-               s.css = append(s.css, cs)
-               s.mu.Unlock()
        }
        conn := cs.dc
@gopherbot
Copy link
Author

@gopherbot gopherbot commented Oct 30, 2014

Comment 4 by marko@joh.to:

This should be fixed in Go 1.4 by fdb52a28028a.
@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Oct 30, 2014

Comment 5:

Thanks.

Status changed to Fixed.

@golang golang locked and limited conversation to collaborators Jun 25, 2016
This issue was closed.
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.