Database driver closes the connection pool

[Seb-C]

Describe the Bug

The Close method in the database drivers also closes the injected connection pool (*sql.DB) object.

Expected Behavior
The method returns it's internal connections to the pool, but does not close the pool itself.

Migrate Version
e.g. v4.14.1

Additional context
This is the way we are using this package. As the connection pool is injected, closing it is not the responsibility of this part of the code. The process should end-up with a database pool that is in the same state than in the beginning (pool opened, but all connections are free and not locked by the migration process).

Since the pool is not created by the database driver itself, it is not it's responsibility to close it.

func MigrateSomeDatabase(alreadyOpenedDatabase *sql.DB) error {
	driver, err := mysql.WithInstance(alreadyOpenedDatabase, &mysql.Config{})
	if err != nil {
		return err
	}

	m, err := migrate.NewWithDatabaseInstance("file:///migrations/...", "mysql", driver)
	if err != nil {
		return err
	}

	err = m.Up()
	if err != nil && err != migrate.ErrNoChange {
		return err
	}

	return driver.Close()
}

[dhui]

Thanks for the report!

If I understand correctly, you'd like Close() to call m.conn.Close() but not m.db.Close():

func (m *Mysql) Close() error {
	connErr := m.conn.Close()
	dbErr := m.db.Close()
	if connErr != nil || dbErr != nil {
		return fmt.Errorf("conn: %v, db: %v", connErr, dbErr)
	}
	return nil
}

The issue is that an instance of the DB driver can be created via Open() as well as WithInstance() and Close() needs to properly clean up both cases. Do you have an suggestions for a fix? I'd like to avoid polluting the interface by adding additional methods and parameters. We could track how the db driver was created and handle Close() differently but that could get messy too... Maybe we only set Config.db in Open() and only close non-nil m.db?

[Seb-C]

If I understand correctly, you'd like Close() to call m.conn.Close() but not m.db.Close():

Since I am the one injecting m.db in this case, yes. I don't expect it to be closed.

I'd like to avoid polluting the interface by adding additional methods and parameters.

Yes, I understand and agree with this concern.

Maybe we only set Config.db in Open() and only close non-nil m.db?

That would be a quick and easy solution that would help, but I'm wondering that this behaviour could get misunderstood, forgotten and get broken in the future 🤔 .

Alternatively, since this class only ever uses m.conn and not m.db, would it be acceptable to introduce a WithConnectionInstance(conn *sql.Conn, config ...) constructor? This way, there would be no confusing behaviour since it becomes clear that I am responsible for opening and closing the injected object.

[dhui]

Oh yeah, it'd still be the driver's responsibility to call m.conn.Close() if WithInstance() is used. I'm open to adding a WithConnection(conn *sql.Conn, config *Config) method.

[Seb-C]

Okay, nice. I think I can probably find time around next week to implement that.

[Fontinalis]

Closing this since it was implemented and resolved in #583