postgres: Move lock out of ensureVersionTable, for consistency with other SQL operations

database/cassandra/cassandra.go

@@ -12,6 +12,7 @@ import (
 
 	"github.com/gocql/gocql"
 	"github.com/golang-migrate/migrate/v4/database"
+	"github.com/hashicorp/go-multierror"
 )
 
 func init() {
@@ -240,13 +241,29 @@ func (c *Cassandra) Drop() error {
 			return err
 		}
 	}
-	// Re-create the version table
-	return c.ensureVersionTable()
+
+	return nil
 }
 
-// Ensure version table exists
-func (c *Cassandra) ensureVersionTable() error {
-	err := c.session.Query(fmt.Sprintf("CREATE TABLE IF NOT EXISTS %s (version bigint, dirty boolean, PRIMARY KEY(version))", c.config.MigrationsTable)).Exec()
+// ensureVersionTable checks if versions table exists and, if not, creates it.
+// Note that this function locks the database, which deviates from the usual
+// convention of "caller locks" in the Cassandra type.
+func (c *Cassandra) ensureVersionTable() (err error) {
+	if err = c.Lock(); err != nil {
+		return err
+	}
+
+	defer func() {
+		if e := c.Unlock(); e != nil {
+			if err == nil {
+				err = e
+			} else {
+				err = multierror.Append(err, e)
+			}
+		}
+	}()
+
+	err = c.session.Query(fmt.Sprintf("CREATE TABLE IF NOT EXISTS %s (version bigint, dirty boolean, PRIMARY KEY(version))", c.config.MigrationsTable)).Exec()
 	if err != nil {
 		return err
 	}

database/clickhouse/clickhouse.go

@@ -11,6 +11,7 @@ import (
 
 	"github.com/golang-migrate/migrate/v4"
 	"github.com/golang-migrate/migrate/v4/database"
+	"github.com/hashicorp/go-multierror"
 )
 
 var DefaultMigrationsTable = "schema_migrations"
@@ -159,7 +160,25 @@ func (ch *ClickHouse) SetVersion(version int, dirty bool) error {
 	return tx.Commit()
 }
 
-func (ch *ClickHouse) ensureVersionTable() error {
+
+// ensureVersionTable checks if versions table exists and, if not, creates it.
+// Note that this function locks the database, which deviates from the usual
+// convention of "caller locks" in the ClickHouse type.
+func (ch *ClickHouse) ensureVersionTable() (err error) {
+	if err = ch.Lock(); err != nil {
+		return err
+	}
+
+	defer func() {
+		if e := ch.Unlock(); e != nil {
+			if err == nil {
+				err = e
+			} else {
+				err = multierror.Append(err, e)
+			}
+		}
+	}()
+
 	var (
 		table string
 		query = "SHOW TABLES FROM " + ch.config.DatabaseName + " LIKE '" + ch.config.MigrationsTable + "'"
@@ -207,7 +226,7 @@ func (ch *ClickHouse) Drop() error {
 			return &database.Error{OrigErr: err, Query: []byte(query)}
 		}
 	}
-	return ch.ensureVersionTable()
+	return nil
 }
 
 func (ch *ClickHouse) Lock() error   { return nil }

database/cockroachdb/cockroachdb.go

@@ -13,6 +13,7 @@ import (
 
 import (
 	"github.com/cockroachdb/cockroach-go/crdb"
+	"github.com/hashicorp/go-multierror"
 	"github.com/lib/pq"
 )
 
@@ -85,11 +86,12 @@ func WithInstance(instance *sql.DB, config *Config) (database.Driver, error) {
 		config: config,
 	}
 
-	if err := px.ensureVersionTable(); err != nil {
+	// ensureVersionTable is a locking operation, so we need to ensureLockTable before we ensureVersionTable.
+	if err := px.ensureLockTable(); err != nil {
 		return nil, err
 	}
 
-	if err := px.ensureLockTable(); err != nil {
+	if err := px.ensureVersionTable(); err != nil {
 		return nil, err
 	}
 
@@ -294,15 +296,29 @@ func (c *CockroachDb) Drop() error {
 				return &database.Error{OrigErr: err, Query: []byte(query)}
 			}
 		}
-		if err := c.ensureVersionTable(); err != nil {
-			return err
-		}
 	}
 
 	return nil
 }
 
-func (c *CockroachDb) ensureVersionTable() error {
+// ensureVersionTable checks if versions table exists and, if not, creates it.
+// Note that this function locks the database, which deviates from the usual
+// convention of "caller locks" in the CockroachDb type.
+func (c *CockroachDb) ensureVersionTable() (err error) {
+	if err = c.Lock(); err != nil {
+		return err
+	}
+
+	defer func() {
+		if e := c.Unlock(); e != nil {
+			if err == nil {
+				err = e
+			} else {
+				err = multierror.Append(err, e)
+			}
+		}
+	}()
+
 	// check if migration table exists
 	var count int
 	query := `SELECT COUNT(1) FROM information_schema.tables WHERE table_name = $1 AND table_schema = (SELECT current_schema()) LIMIT 1`

database/cockroachdb/cockroachdb_test.go

@@ -90,6 +90,22 @@ func Test(t *testing.T) {
 		}
 		dt.Test(t, d, []byte("SELECT 1"))
 	})
+	dktesting.ParallelTest(t, specs, func(t *testing.T, ci dktest.ContainerInfo) {
+		createDB(t, ci)
+
+		ip, port, err := ci.Port(26257)
+		if err != nil {
+			t.Fatal(err)
+		}
+
+		addr := fmt.Sprintf("cockroach://root@%v:%v/migrate?sslmode=disable", ip, port)
+		c := &CockroachDb{}
+		d, err := c.Open(addr)
+		if err != nil {
+			t.Fatalf("%v", err)
+		}
+		dt.TestMigrate(t, d, []byte("SELECT 1"))
+	})
 }
 
 func TestMultiStatement(t *testing.T) {

database/driver.go

@@ -74,6 +74,8 @@ type Driver interface {
 	Version() (version int, dirty bool, err error)
 
 	// Drop deletes everything in the database.
+	// Note that this is a breaking action, a new call to Open() is necessary to
+	// ensure subsequent calls work as expected.
 	Drop() error
 }
 

database/mongodb/mongodb.go

@@ -59,6 +59,7 @@ func WithInstance(instance *mongo.Client, config *Config) (database.Driver, erro
 		db:     instance.Database(config.DatabaseName),
 		config: config,
 	}
+
 	return mc, nil
 }
 
@@ -77,9 +78,6 @@ func (m *Mongo) Open(dsn string) (database.Driver, error) {
 		return nil, err
 	}
 	migrationsCollection := purl.Query().Get("x-migrations-collection")
-	if len(migrationsCollection) == 0 {
-		migrationsCollection = DefaultMigrationsCollection
-	}
 
 	transactionMode, _ := strconv.ParseBool(purl.Query().Get("x-transaction-mode"))
 

database/mongodb/mongodb_test.go

@@ -81,6 +81,36 @@ func Test(t *testing.T) {
 		dt.TestSetVersion(t, d)
 		dt.TestDrop(t, d)
 	})
+	dktesting.ParallelTest(t, specs, func(t *testing.T, c dktest.ContainerInfo) {
+		ip, port, err := c.FirstPort()
+		if err != nil {
+			t.Fatal(err)
+		}
+
+		addr := mongoConnectionString(ip, port)
+		p := &Mongo{}
+		d, err := p.Open(addr)
+		if err != nil {
+			t.Fatalf("%v", err)
+		}
+		defer d.Close()
+		dt.TestNilVersion(t, d)
+		//TestLockAndUnlock(t, d) driver doesn't support lock on database level
+		dt.TestRun(t, d, bytes.NewReader([]byte(`[{"insert":"hello","documents":[{"wild":"world"}]}]`)))
+		dt.TestSetVersion(t, d)
+		dt.TestDrop(t, d)
+		// Reinitialize for new round of tests
+		err = d.Drop()
+		if err != nil {
+			t.Fatalf("%v", err)
+		}
+		d, err = p.Open(addr)
+		if err != nil {
+			t.Fatalf("%v", err)
+		}
+		defer d.Close()
+		dt.TestMigrate(t, d, []byte(`[{"insert":"hello","documents":[{"wild":"world"}]}]`))
+	})
 }
 
 func TestWithAuth(t *testing.T) {

database/mysql/mysql.go

@@ -17,6 +17,7 @@ import (
 
 import (
 	"github.com/go-sql-driver/mysql"
+	"github.com/hashicorp/go-multierror"
 )
 
 import (
@@ -75,15 +76,15 @@ func WithInstance(instance *sql.DB, config *Config) (database.Driver, error) {
 
 	config.DatabaseName = databaseName.String
 
-	if len(config.MigrationsTable) == 0 {
-		config.MigrationsTable = DefaultMigrationsTable
-	}
-
 	conn, err := instance.Conn(context.Background())
 	if err != nil {
 		return nil, err
 	}
 
+	if len(config.MigrationsTable) == 0 {
+		config.MigrationsTable = DefaultMigrationsTable
+	}
+
 	mx := &Mysql{
 		conn:   conn,
 		db:     instance,
@@ -127,9 +128,6 @@ func (m *Mysql) Open(url string) (database.Driver, error) {
 	purl.RawQuery = q.Encode()
 
 	migrationsTable := purl.Query().Get("x-migrations-table")
-	if len(migrationsTable) == 0 {
-		migrationsTable = DefaultMigrationsTable
-	}
 
 	// use custom TLS?
 	ctls := purl.Query().Get("tls")
@@ -342,15 +340,29 @@ func (m *Mysql) Drop() error {
 				return &database.Error{OrigErr: err, Query: []byte(query)}
 			}
 		}
-		if err := m.ensureVersionTable(); err != nil {
-			return err
-		}
 	}
 
 	return nil
 }
 
-func (m *Mysql) ensureVersionTable() error {
+// ensureVersionTable checks if versions table exists and, if not, creates it.
+// Note that this function locks the database, which deviates from the usual
+// convention of "caller locks" in the Mysql type.
+func (m *Mysql) ensureVersionTable() (err error) {
+	if err = m.Lock(); err != nil {
+		return err
+	}
+
+	defer func() {
+		if e := m.Unlock(); e != nil {
+			if err == nil {
+				err = e
+			} else {
+				err = multierror.Append(err, e)
+			}
+		}
+	}()
+
 	// check if migration table exists
 	var result string
 	query := `SHOW TABLES LIKE "` + m.config.MigrationsTable + `"`

database/mysql/mysql_test.go

@@ -76,6 +76,17 @@ func Test(t *testing.T) {
 		}
 		defer d.Close()
 		dt.Test(t, d, []byte("SELECT 1"))
+		// Reinitialize for new round of tests
+		err = d.Drop()
+		if err != nil {
+			t.Fatalf("%v", err)
+		}
+		d, err = p.Open(addr)
+		if err != nil {
+			t.Fatalf("%v", err)
+		}
+		defer d.Close()
+		dt.TestMigrate(t, d, []byte("SELECT 1"))
 
 		// check ensureVersionTable
 		if err := d.(*Mysql).ensureVersionTable(); err != nil {

database/postgres/postgres.go

@@ -92,13 +92,18 @@ func WithInstance(instance *sql.DB, config *Config) (database.Driver, error) {
 		return nil, err
 	}
 
+	if len(config.MigrationsTable) == 0 {
+		config.MigrationsTable = DefaultMigrationsTable
+	}
+
 	px := &Postgres{
 		conn:   conn,
 		db:     instance,
 		config: config,
 	}
 
-	if err := px.ensureVersionTable(); err != nil {
+	err = px.ensureVersionTable()
+	if err != nil {
 		return nil, err
 	}
 
@@ -117,14 +122,12 @@ func (p *Postgres) Open(url string) (database.Driver, error) {
 	}
 
 	migrationsTable := purl.Query().Get("x-migrations-table")
-	if len(migrationsTable) == 0 {
-		migrationsTable = DefaultMigrationsTable
-	}
 
 	px, err := WithInstance(db, &Config{
 		DatabaseName:    purl.Path,
 		MigrationsTable: migrationsTable,
 	})
+
 	if err != nil {
 		return nil, err
 	}
@@ -325,14 +328,14 @@ func (p *Postgres) Drop() error {
 				return &database.Error{OrigErr: err, Query: []byte(query)}
 			}
 		}
-		if err := p.ensureVersionTable(); err != nil {
-			return err
-		}
 	}
 
 	return nil
 }
 
+// ensureVersionTable checks if versions table exists and, if not, creates it.
+// Note that this function locks the database, which deviates from the usual
+// convention of "caller locks" in the Postgres type.
 func (p *Postgres) ensureVersionTable() (err error) {
 	if err = p.Lock(); err != nil {
 		return err