proposal: database/sql: make free connection strategy configurable

[kishoresenji]

Proposal Details

The current strategy of lookup of a free connection is based on "most recently used" and this helps in closing connections as soon as they become idle. But this strategy creates traffic imbalance between multiple replicas of a cluster (behind haproxy) as it prefers recently used connections, some read replicas in a cluster get much higher qps than other replicas.

Proposal is to add a configurable option to DB type using which the free connection strategy will be using "least recently used" and picks connections from the beginning of the freeConn slice. Although this does not let idle connections to be cleaned up as connections won't be idle compared to the current strategy, this new strategy can be coupled with SetConnMaxLifetime to ensure that after ConnMaxLifetime, the optimal resources are being used.

[ianlancetaylor]

CC @kardianos

[kardianos]

@kishoresenji I'm skeptical. However, if you provide a proof of concept or sketch that shows this could be reasonably extracted from the current code base, I'd be game to re-evaluate. If the connection use strategy was able to be extracted to some internal interface, and the type is used cleanly throughout, then perhaps this could clean up existing code.

However, if any implementation resulted in more complicated or harder to understand logic surrounding this, that would be an instant no.

[kishoresenji]

@kardianos As the freeConn primitive in both strategies (most recently used or least recently used) is the same (i.e both strategies will order from oldest to newest on returedAt), we can keep it very simple and not add a new interface. The only place that needs a change is in the conn method on how the free connection is picked. There should not be any change anywhere else in the code.

We can have a new enum which models which strategy to be used.

type cachedConnPickStrategy uint8
const (
	mostRecentlyUsedStrategy cachedConnPickStrategy = iota
	leastRecentlyUsedStrategy
)

and LeastRecentlyUsed strategy can be enabled with an option on the DB. By default, it will be MostRecentlyUsed strategy.

func (db *DB) SetLeastRecentlyUsedCachedConnectionStrategy() {
	db.mu.Lock()
	defer db.mu.Unlock()
	db.cachedConnPickStrategy = leastRecentlyUsedStrategy
}

The conn method would change like this:

	// Prefer a free connection, if possible.
	if strategy == cachedOrNewConn {
		if conn := db.getCachedConnDBLocked(); conn != nil {
			conn.inUse = true
			if conn.expired(lifetime) {
				db.maxLifetimeClosed++
				db.mu.Unlock()
				conn.Close()
				return nil, driver.ErrBadConn
			}
			db.mu.Unlock()

			// Reset the session if required.
			if err := conn.resetSession(ctx); errors.Is(err, driver.ErrBadConn) {
				conn.Close()
				return nil, err
			}

			return conn, nil
		}
	}

where getCachedConnDBLocked is like the following:

func (db *DB) getCachedConnDBLocked() *driverConn {
	var conn *driverConn
	if len(db.freeConn) == 0 {
		return conn
	}
	switch db.cachedConnPickStrategy {
	case leastRecentlyUsedStrategy:
		conn = db.freeConn[0]
		db.freeConn = db.freeConn[1:]
	default:
		last := len(db.freeConn)-1
		conn = db.freeConn[last]
		db.freeConn = db.freeConn[:last]
	}
	return conn
}

[kishoresenji]

I have put the above code in a commit in my fork: 7f5dd3c?diff=split&w=1

[kishoresenji]

@kardianos gentle ping to know if there is a chance on getting this proposal accepted or I should look at other alternatives. We are seeing imbalance in our clusters and that is why I started this proposal. Another approach is to have pooling outside of database/sql by setting maxIdleConns to -1 and supplying a Connector, which does the pooling, to OpenDB function. The connector will return a Connection which just returns the Connection back to the pool on Close from database/sql package.