underlying *sql.DB not being Closed

[Kay-Zee]

Describe the Bug
While running some tests, I've noticed that even when making sure to defer migrate.Migrate.Close() I still run into pq: sorry, too many clients already

I've taken the liberty of digging a little bit and noticed that the *sql.DB, after it is used to create the sql.Conn, it is never closed

Steps to Reproduce
Steps to reproduce the behavior:

1. Choose any migration with both up and down
2. Create a migrate.Migrate with migrate.New(...)
3. Migrate up and down (Optional)
4. Call migrate.Migrate.Close()
5. Repeat based on max number of connections. (for me it was postgres default 100 connections)

Expected Behavior
Expect Close() to close both the sql.Conn as well as the sql.DB

Migrate Version
v3.2.0

Loaded Source Drivers
file

Loaded Database Drivers
postgres

Stacktrace
Please provide if available

Additional context
Not sure if theres any reason to not close the *sql.DB connection but since it is created from within the Open function and never stored, theres no external way to close this db.

I'll be fixing it on a personal fork, so if theres no objections to closing the *sql.DB connection when migrate.Migrate.Close() is called, I'd be happy to open a PR.

[dhui]

You're correct. The *sql.DB also needs to be tracked in the Postgres struct and closed (along with the *sql.Conn) when Postgres.Close() is called.

Good catch and thanks for reporting the issue!
A PR to fix this would be appreciated.

[dhui]

fixed in https://github.com/golang-migrate/migrate/releases/tag/v3.5.2

[twalwyn]

This breaks clients that expect to manage the DB lifetime themselves.

[dhui]

@twalwyn The *sql.DB is managed by the driver, which is managed by migrate.

A DB driver's underlying *sql.DB shouldn't be exposed (most DB drivers adhere to this). If you're using WithInstance(), then you should give up lifecycle management of the provided *sql.DB to the DB driver.

[andremarianiello]

@dhui Given that a common pattern for resource handling is that whoever creates the resource is responsible for cleaning it up, some people might not know that the migrate/driver takes responsibility for instances that are passed in. I think it should be documented somewhere. It also whittles down the usefulness of WithInstance. You can't safely share instances if the migrate/driver owns the lifecycle.

[dhui]

Unfortunately, that's the issue you hit when using WithInstance() methods.
There doesn't seem to be a clean way to support opening via both URIs and instances.

@andremarianiello Feel free to open a PR that improves the docs

[kernle32dll]

Just FYI, I just hit this problem, when I used db.SetMaxOpenConns(1) for debugging purposes, and my application was suddenly unable to to serve any responses. Turns out, migrate holds the connection in question open, and thus my application was unable to create any new connections.

Of course, using the close method on either migrate or the driver does not only close this connection, but the db too (which, from what I read here, is intended behavior). As @andremarianiello already pointed out, this is neither intuitive nor obvious.

As it stands, to work around this issue I have to create one connection to the DB just for migrating, drop that db connection, and then create a second one afterwards for the application to actually use. Is that right?

[andremarianiello]

@kernle32dll That is what I ended up doing.

[dhui]

Yeah, it's not intuitive, but docs should help...

As it stands, to work around this issue I have to create one connection to the DB just for migrating, drop that db connection, and then create a second one afterwards for the application to actually use. Is that right?

Correct, the best approach for now is to use separate db connections for migrate (when using WithInstance()) and your queries.

[twalwyn]

I haven’t looked at the code, but can migrate not keep track of whether it opened the db or not, and then only close the db if it created it?

[dhui]

I haven’t looked at the code, but can migrate not keep track of whether it opened the db or not, and then only close the db if it created it?

This is possible but would require tracking extra state in the db instance which would require each db driver implementation to implement this state with the current db driver interface. Going down this path (without a db driver interface change) could result in inconsistencies and additional unexpected behaviors.