sql.DB.SetMaxOpenConns(1)

[marko]

Hi list,

I would like to allow my program to only use one database connection through a database/sql instance, but the program deadlocks when attempting to run the second query (line 32 in the attached example).  The culprit seems to be this code in sql.go, function DB.conn():

    // If db.maxOpen > 0 and the number of open connections is over the limit
    // or there are no free connection, then make a request and wait.
    if db.maxOpen > 0 && (db.numOpen >= db.maxOpen || db.freeConn.Len() == 0) {

But I'm confused; why is the parenthesized condition an or?  If there's a free connection available, why do we not just pop it off the front of the list immediately?


.marko

[Andrew Hochhaus]

Hello,

We have been experiencing the same problem except with maxOpen > 1 and a large number of concurrently executed queries.

http://golang.org/src/pkg/database/sql/sql.go#L624

I am not that familiar with the code, but it seems to me that if we have a freeConn, it should always be used. If so, maybe we should:

  * first, check the freeConns

  * next, if we are over the max make a request and wait

  * finally, make the new driverConn.

This allows for simplification of the buggy if condition. To be concrete:

diff -r 87dea3f5ebe7 src/pkg/database/sql/sql.go

--- a/src/pkg/database/sql/sql.go       Fri Nov 29 08:32:31 2013 +1100

+++ b/src/pkg/database/sql/sql.go       Tue Dec 10 20:00:45 2013 -0500

@@ -619,9 +619,18 @@

                return nil, errDBClosed

        }

 

-       // If db.maxOpen > 0 and the number of open connections is over the limit

-       // or there are no free connection, then make a request and wait.

-       if db.maxOpen > 0 && (db.numOpen >= db.maxOpen || db.freeConn.Len() == 0) {

+       if f := db.freeConn.Front(); f != nil {

+               conn := f.Value.(*driverConn)

+               conn.listElem = nil

+               db.freeConn.Remove(f)

+               conn.inUse = true

+               db.mu.Unlock()

+               return conn, nil

+       }

+

+       // If db.maxOpen > 0 and the number of open connections is over the limit,

+       // then make a request and wait.

+       if db.maxOpen > 0 && db.numOpen >= db.maxOpen {

                // Make the connRequest channel. It's buffered so that the

                // connectionOpener doesn't block while waiting for the req to be read.

                ch := make(chan interface{}, 1)

@@ -643,15 +652,6 @@

                }

        }

 

-       if f := db.freeConn.Front(); f != nil {

-               conn := f.Value.(*driverConn)

-               conn.listElem = nil

-               db.freeConn.Remove(f)

-               conn.inUse = true

-               db.mu.Unlock()

-               return conn, nil

-       }

-

        db.mu.Unlock()

        ci, err := db.driver.Open(db.dsn)

        if err != nil {

Does this seem reasonable to those with more experience? If so, I can send a change if helpful.

Thanks,

-Andy

[Marko Tiikkaja]

On 12/11/13, 2:04 AM, Andrew Hochhaus wrote:
> I am not that familiar with the code, but it seems to me that if we have a
> freeConn, it should always be used. If so, maybe we should:
>
> * first, check the freeConns
> * next, if we are over the max make a request and wait
> * finally, make the new driverConn.
>
> This allows for simplification of the buggy if condition. To be concrete:

I've opened a CL with a slightly different approach:
https://codereview.appspot.com/40410043/

Do you see a problem with this? Is your approach better in some way?


.marko

[Hochhaus, Andrew]

Marko,

Thanks. Your code looks correct according to my (very limited) understanding.

My approach isn't better, it only let's you avoid the
"db.freeConn.Len() == 0" check because it has already been done.

However, you change is less invasive (only touching two lines) so that
has benefit too.

--
Andy Hochhaus
President
SameGoal, L.L.C.
https://samegoal.com/
Phone: 888-SAMEGOAL x902
888-726-3462 x902