Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Added a method to create a mysql database from a connection object #583

Merged
merged 4 commits into from
Jul 6, 2021
Merged

Added a method to create a mysql database from a connection object #583

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
f98ad3a
Added a method to create a mysql database from a connection object
Jun 17, 2021
394279a
Calling WithConnection from WithInstance to de-duplicate code
Jun 29, 2021
73081e2
Adding context and ping to mysql.WithConnection
Jul 6, 2021
4bfe192
Interface type check at compile time
Jul 6, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
48 changes: 36 additions & 12 deletions database/mysql/mysql.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,8 @@ import (
"github.com/golang-migrate/migrate/v4/database"
)

var _ database.Driver = (*Mysql)(nil) // explicit compile time type check

func init() {
database.Register("mysql", &Mysql{})
}
Expand Down Expand Up @@ -54,20 +56,26 @@ type Mysql struct {
config *Config
}

// instance must have `multiStatements` set to true
func WithInstance(instance *sql.DB, config *Config) (database.Driver, error) {
// connection instance must have `multiStatements` set to true
func WithConnection(ctx context.Context, conn *sql.Conn, config *Config) (*Mysql, error) {
if config == nil {
return nil, ErrNilConfig
}

if err := instance.Ping(); err != nil {
if err := conn.PingContext(ctx); err != nil {
return nil, err
}

mx := &Mysql{
conn: conn,
db: nil,
config: config,
}

if config.DatabaseName == "" {
query := `SELECT DATABASE()`
var databaseName sql.NullString
if err := instance.QueryRow(query).Scan(&databaseName); err != nil {
if err := conn.QueryRowContext(ctx, query).Scan(&databaseName); err != nil {
return nil, &database.Error{OrigErr: err, Query: []byte(query)}
}

Expand All @@ -82,21 +90,33 @@ func WithInstance(instance *sql.DB, config *Config) (database.Driver, error) {
config.MigrationsTable = DefaultMigrationsTable
}

conn, err := instance.Conn(context.Background())
if err != nil {
if err := mx.ensureVersionTable(); err != nil {
return nil, err
}

mx := &Mysql{
conn: conn,
db: instance,
config: config,
return mx, nil
}

// instance must have `multiStatements` set to true
func WithInstance(instance *sql.DB, config *Config) (database.Driver, error) {
ctx := context.Background()

if err := instance.Ping(); err != nil {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Move this to WithConnection() and use conn.PingContext(). DB.Ping() will create a connection to ping anyways

Copy link
Contributor Author

@Seb-C Seb-C Jun 30, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem I am trying to solve is that this class does unexpected stuff to my connection pool.
If I inject an active pool with n idle connections to this class, do my stuff and close it, I expect my pool to be returned to it's initial state, which means an active pool with n idle connections.
But currently I can only choose between ending-up with a closed pool (because Close not only returns the connection to the pool, but also closes the pool itself), or (if I don't call Close) a pool with n-1 idle connections and 1 allocated connection that will be stuck forever

So the last thing I want is for WithConnection to create new connections in a pool that I did not even inject in the first place.

If the connection I inject is not working, I expect the process to fail without side-effects on my pool.

If the connection is not working, doing conn.PingContext().DB.Ping() would create a new connection in the pool and ping the new connection, which does not test or fix the injected one anyway.

Copy link
Member

@dhui dhui Jun 30, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My understanding is that connections are pooled at the sql.DB instance level so conn.PingContext() shouldn't create a new connection e.g. connections won't create new connections

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I was misunderstanding your question.
I added the context to WithConnection, as well as calling PingContext.
I cannot add it as a parameter in WithInstance without breaking change, so it should be done along with #14.

return nil, err
}

if err := mx.ensureVersionTable(); err != nil {
conn, err := instance.Conn(ctx)
if err != nil {
return nil, err
}

mx, err := WithConnection(ctx, conn, config)
if err != nil {
return nil, err
}

mx.db = instance

return mx, nil
}

Expand Down Expand Up @@ -243,7 +263,11 @@ func (m *Mysql) Open(url string) (database.Driver, error) {

func (m *Mysql) Close() error {
connErr := m.conn.Close()
dbErr := m.db.Close()
var dbErr error
if m.db != nil {
dbErr = m.db.Close()
}

if connErr != nil || dbErr != nil {
return fmt.Errorf("conn: %v, db: %v", connErr, dbErr)
}
Expand Down