database/sql: expose DB.MaxIdleConns and add DB.SetMaxIdleConns

src/pkg/database/sql/sql.go

 // Copyright 2011 The Go Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style
 // license that can be found in the LICENSE file.
 
 // Package sql provides a generic interface around SQL (or SQL-like)
 // databases.
 package sql
 
 import (
         "database/sql/driver"
@@ skipping 171 matching lines @@
 // pool of idle connections. If observing session state is required,
 // either do not share a *DB between multiple concurrent goroutines or
 // create and observe all state only within a transaction. Once
 // DB.Open is called, the returned Tx is bound to a single isolated
 // connection. Once Tx.Commit or Tx.Rollback is called, that
 // connection is returned to DB's idle connection pool.
 type DB struct {
         driver driver.Driver
         dsn    string
 
-        mu       sync.Mutex // protects freeConn and closed
-        freeConn []driver.Conn
-        closed   bool
+        mu           sync.Mutex // protects freeConn, closed and maxIdleConns
+        freeConn     []driver.Conn
+        closed       bool
+        maxIdleConns int
 }
 
 // Open opens a database specified by its database driver name and a
 // driver-specific data source name, usually consisting of at least a
 // database name and connection information.
 //
 // Most users will open a database via a driver-specific connection
 // helper function that returns a *DB.
 func Open(driverName, dataSourceName string) (*DB, error) {
         driver, ok := drivers[driverName]
         if !ok {
                 return nil, fmt.Errorf("sql: unknown driver %q (forgotten import?)", driverName)
         }
-        return &DB{driver: driver, dsn: dataSourceName}, nil
+        return &DB{driver: driver, dsn: dataSourceName, maxIdleConns: 2}, nil
 }
 
 // Close closes the database, releasing any open resources.
 func (db *DB) Close() error {
         db.mu.Lock()
         defer db.mu.Unlock()
         var err error
         for _, c := range db.freeConn {
                 err1 := c.Close()
                 if err1 != nil {
                         err = err1
                 }
         }
         db.freeConn = nil
         db.closed = true
         return err
 }
 
-func (db *DB) maxIdleConns() int {
-        const defaultMaxIdleConns = 2
+// Adjust the maximum number of permanently retained database connections. This

[bradfitz, 2013/03/08 22:40:08]

See next comment.

// SetMaxIdleConns sets ...

+// defaults to 2 for historical reasons. N.B. This is not the maximum number of

[bradfitz, 2013/03/08 22:40:08]

I think we should instead make an exported constant for 2 (DefaultMaxIdleConns)
and reference that.  No need to say "for historical reasons".  Drop the "N.B.",
as good as the note is.  We avoid e.g. i.e. n.b.

+// connections that can be open, or the minimum. It is the maximum number that
+// are retained between uses.
+func (db *DB) SetMaxIdleConns(v int) int {
+        db.mu.Lock()
+        defer db.mu.Unlock()
+        db.maxIdleConns = v
+        return v
+}
+
+// The current maximum number of idle database connections allowed. See

[bradfitz, 2013/03/08 22:40:08]

Go comment style is to start sentences with the name of the thing being
described. They have to stand alone.

// MaxIdleConns returns the ....

+// SetMaxIdleConns for more information.
+func (db *DB) MaxIdleConns() int {
         // TODO(bradfitz): ask driver, if supported, for its default preference

[bradfitz, 2013/03/08 22:40:08]

probably not here anymore.  Probably only once the driver is opened (where you
have the 2 now).

you should probably have a func like:

func initialMaxIdleConns(db *DB) int {
   if di, ok := db.driver.(driver.MaxIdleConner); ok {
        if v, ok := di.MaxIdleConns(db.dsn); ok {
            return v
        }
   }
   return DefaultMaxIdleConns
}

and then call that from Open.

But rather than a proliferation of small preferred-config interfaces like that
in the driver package, perhaps we should just have an alternate Open interface
that returns that config from the dsn once.

Imagine, in the driver package:

type DriverConfig struct [
   MaxIdleConns int

   // future stuff

   NewConn func() (Conn, Error)
}

And then we add:

package driver
type DriverConfig interface {
    OpenConfig(dsn string) (*DriverConfig, error)
}

And we prefer to use that if it exists, else falling back to driver.Driver's
Open instead.

That seems more extensible.

Thoughts?

[julienschmidt, 2013/03/09 02:10:03]

I like the idea of a config struct ( like I did here
https://codereview.appspot.com/7365049/ ;) ) but I don't think we need 3 Open
functions. It might be a good idea to combine this with
http://code.google.com/p/go/issues/detail?id=4804 .
The "NewConn" function would also go well together with the "clone" function
(driver side) I suggested in the last comment.

If you agree, I try to sent out a CL for the new Open function ASAP.

On 2013/03/08 22:40:08, bradfitz wrote:

-        // TODO(bradfitz): let users override?
-        return defaultMaxIdleConns
+        db.mu.Lock()
+        defer db.mu.Unlock()
+        return db.maxIdleConns
 }
 
 // conn returns a newly-opened or cached driver.Conn
 func (db *DB) conn() (driver.Conn, error) {
         db.mu.Lock()
         if db.closed {
                 db.mu.Unlock()
                 return nil, errors.New("sql: database is closed")
         }
         if n := len(db.freeConn); n > 0 {
@@ skipping 23 matching lines @@
 // putConnHook is a hook for testing.
 var putConnHook func(*DB, driver.Conn)
 
 // putConn adds a connection to the db's free pool.
 // err is optionally the last error that occured on this connection.
 func (db *DB) putConn(c driver.Conn, err error) {
         if err == driver.ErrBadConn {
                 // Don't reuse bad connections.
                 return
         }
+        // maxIdle is fetched before we open the lock. Ideally move to an RWLock
+        // instead, and retain it, which may reduce the time over which inconsistent
+        // numbers of connections may be retained. The value also could just be
+        // modified using atomic.
+        maxIdle := db.MaxIdleConns()

[bradfitz, 2013/03/08 22:40:08]

this is grabbing a lock and unlocking it, even though you grab the same lock on
the next line.

You can just use db.maxIdleConns directly once you're holding the lock.

         db.mu.Lock()
         if putConnHook != nil {
                 putConnHook(db, c)
         }
-        if n := len(db.freeConn); !db.closed && n < db.maxIdleConns() {
+        if n := len(db.freeConn); !db.closed && n < maxIdle {
                 db.freeConn = append(db.freeConn, c)
                 db.mu.Unlock()
                 return
         }
         // TODO: check to see if we need this Conn for any prepared
         // statements which are still active?
         db.mu.Unlock()
         c.Close()
 }
 
@@ skipping 745 matching lines @@
 
 // A Result summarizes an executed SQL command.
 type Result interface {
         LastInsertId() (int64, error)
         RowsAffected() (int64, error)
 }
 
 type result struct {
         driver.Result
 }