Will database/sql leak memory if I don't call Close() and remove references to it?

[Francis Chuang]

I want to use Vault to manage and rotate the usernames and passwords to my databases.

If I open a connection: db, err := sql.Open("mysql", "...."), and then later on, open a new connection due to the username/password being rotated: db, err = sql.Open("mysql", "...") //no new variables created,

will the original sql instance leak? I want to avoid having to force all current queries and executions on the old db to terminate using Close(), as I would need to have some mutex to act as a gatekeeper for all interactions with the database, which would be quite complex.

Ideally, I would prefer the following:

1. New username/password pair received and sql.Open() is called.

2. Old db instance is still being used by a few goroutines as they still hold references to the old sql instance.

3. Goroutines finishes and no longer hold references to the old sql instance. The old sql instance is GC'd. Is this possible without calling Close()?

4. New goroutines are launched and they use the new sql instance.

Cheers,

Francis

[Daniel Theophanes]

If you open DB with db.Open, yes, you will need to call db.Close(). Without that it will keep alive a pool of connections to your remote database. -Daniel

[Kiki Sugiaman]

database/sql does not specify what gets freed before calling Close(). If
the driver or the database server happen to release resources on their
own, that is incidental and should not be relied upon unless you have
deep knowledge of the driver and the database behavior.

You can schedule Close() after a certain amount of time (say an hour, if
you change the password once a day), after which you can assume that
99.9% of existing connections will have finished interacting with the
database.

After scheduling Close(), the old db instance should not open new
connections. Yes, there is a time interval when your program holds
references to more than one db instances, but it doesn't cost a lot. And
the Close() guarantee is there. Make sure to configure the database
server to allow more connections than usual.

# of new conn ~= # of old conn + expected # of lingering conn

[Daniel Theophanes]

I think there is a larger issue at play of how old connections are cached and handled. In particular there is no way to flush cached connections or update credentials in the API.

Could you file an issue with how to best update credentials on an existing connection pool?

[Tamás Gulácsi]

As *database/sql.DB is a connection pool, maybe you have to go deeper, and implement a driver by wrapping a mtsql driver.
This is not hard, as a driver.Driver is a narrow interface.
This way you can do as you wish, and close old connections (you can account for them), or just do the password changing as needed, for the new connections.
Or just return a specific error for old connections, to force database/sql to open a new connection.

[Chris Hines]

db.SetMaxIdleConns(-1) will flush cached connections. That leaves the problem that open connections still in use will not get closed until returned to the pool, and only if caching idle connections is still disabled at that time.

Changing credentials on an already open *sql.DB isn't directly supported. It seems possible to implement with a wrapper of driver.Driver as Tamás Gulácsi suggested in another response. 

[Francis Chuang]

Hey all,

Thanks for the discussion. I've opened https://github.com/golang/go/issues/17727 to see if it might be possible for database/sql to support rotating of username and passwords. As these are currently passed in through the DSN for all drivers, it might be generalized to support changing/updating the DSN for a connection pool.

Francis

[Francis Chuang]

Just some further questions. I think the easiest way would be to combine ksug and Chris' suggestions. I would prefer not having to maintain a wrapper for each database as we talk to quite a few different types of databases using database/sql.

Does this some bullet proof?

1. Rotate usernames/passwords and open a new connection and get a new db object. Keep the old db object around.

2. Wait for 1 hour or some other predetermined amount of time.

3. Call SetMaxConnections(-1) on the old db object. Does this flush and close cached connections immediately?

4. Call Close() on the old db object.

[Kiki Sugiaman]

Bullet proof means a few different things and will depend on other
details. It's best to run your own simulation of the edge cases. One I
can think of is: what happens if the old pool / db instance opens a new
connection during the split second after the credential change at the
backend but before the new instance has been created.

The initially proposed solution in the OP also didn't address this so
the new code should do no worse.

[Chris Hines]

Slight improvement:

1. Rotate usernames/passwords and call sql.Open(...) to get a new connection pool for future queries. (Save old pool in oldDB.)

2. Call oldDB.SetMaxIdleConns(-1), which will immediately close any unused connections in the old pool and cause those in use to be closed as soon as they are returned to the pool.

3. Start a goroutine that will poll oldDB.Stats().OpenConnections at an appropriate frequency, when it returns zero (or too much time has passed) call oldDB.Close() and exit the goroutine.

Chris