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

[lukaspj]

Fixes #164 .

ensureVersionTable seems to be the only SQL operation in the postgres driver which handles locking of database itself, meaning it seems to break the convention of having the caller responsible for locking and not the callee.

This removes the locking logic from ensureVersionTable and makes sure to properly lock the database before calling ensureVersionTable from the WithInstance method.

[coveralls]

Pull Request Test Coverage Report for Build 305

• 0 of 0 changed or added relevant lines in 0 files are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage remained the same at 59.188%

---

Totals
Change from base Build 298:	0.0%
Covered Lines:	773
Relevant Lines:	1306

---

💛 - Coveralls

[coveralls]

Pull Request Test Coverage Report for Build 305

• 0 of 0 changed or added relevant lines in 0 files are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage remained the same at 59.188%

---

Totals
Change from base Build 298:	0.0%
Covered Lines:	773
Relevant Lines:	1306

---

💛 - Coveralls

[coveralls]

Pull Request Test Coverage Report for Build 344

• 1 of 1 (100.0%) changed or added relevant line in 1 file are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage increased (+0.4%) to 59.571%

---

Totals
Change from base Build 319:	0.4%
Covered Lines:	778
Relevant Lines:	1306

---

💛 - Coveralls

[dhui]

Thanks for your investigation.

I think a cleaner approach would be to not create the schema version table in Drop() and instead rely on WithInstance() to create it. I don't think many users would need an empty schema version table after a successful DROP.

[dhui]

Add a test case for this to prevent a regression.

[lukaspj]

@dhui looking at the MySQL implementation as reference, it simply doesn't lock the database when calling ensureVersionTable from either location.

I think the behaviour should be similar in the two, should I simply remove the locking logic around ensureVersionTable ? Thinking of it, it doesn't seem like it'd be necessary to lock it for that operation.

Edit: otherwise, if we want to remove ensureVersionTable from Drop, I think we should do so in other database implementations as well for consistency.

[dhui]

I think the behaviour should be similar in the two, should I simply remove the locking logic around ensureVersionTable ? Thinking of it, it doesn't seem like it'd be necessary to lock it for that operation.

ensureVersionTable() should be atomic. A recent fix was made to fix an issue with multiple instances of migrate running, which incidentally created the issue you're trying to fix.

Edit: otherwise, if we want to remove ensureVersionTable from Drop, I think we should do so in other database implementations as well for consistency.

This work would be greatly appreciated! MySQL isn't the only driver that uses ensureVersionTable():

$ git grep -l ensureVersionTable
database/cassandra/cassandra.go
database/clickhouse/clickhouse.go
database/cockroachdb/cockroachdb.go
database/mysql/mysql.go
database/mysql/mysql_test.go
database/postgres/postgres.go
database/ql/ql.go
database/redshift/redshift.go
database/spanner/spanner.go
database/sqlite3/sqlite3.go
$

So for consistency, we should lock in ensureVersionTable() and remove the call to ensureVersionTable() from Drop()

[lukaspj]

Alright, I will update the PR tomorrow.
Another suggestion, should it be renamed to lockAndEnsureVersionTable to make the break from the caller locks convention explicit?
Might avoid future confusion

[dhui]

Another suggestion, should it be renamed to lockAndEnsureVersionTable to make the break from the caller locks convention explicit?
Might avoid future confusion

I'd leave the name as is since it's sufficiently descriptive. After all, adding every bit of functionality to the function's name would result in really long function names.
The docs around db driver lock usage are on the the Driver interface in database/driver.go which should be enough to prevent future confusion. But if you have suggestions for improving the docs, I'm all ears.

[lukaspj]

@dhui I added a test-case, but only for Postgresql, as I don't know enough about most of the other databases.
See: https://github.com/golang-migrate/migrate/pull/173/files#diff-5cc52c39a5031732bc6d3821ffc558e6

Also, please be aware of this change:
https://github.com/golang-migrate/migrate/pull/173/files#diff-9870930c9960513ad7d4eae22e4692d5

The change in behaviour in Drop (namely that it leaves an empty database), breaks some logic in these test-cases (and possibly in user-code as well).

Finally, it's worth mentioning that the order of ensureLock / ensureVersion has been swapped in CockroachDB, in order to facilitate locking in ensureVersion

[lukaspj]

Btw, it looks like coverage is only measured for MongoDB and not all databases:
https://coveralls.io/builds/21705329

[dhui]

Thanks for making the changes to all of the DB drivers for consistency and for the review guidance!

The change in behaviour in Drop (namely that it leaves an empty database), breaks some logic in these test-cases (and possibly in user-code as well).

I'll note this in the release notes. People should not have been relying on this undocumented behavior so I don't consider this a breaking change.

Btw, it looks like coverage is only measured for MongoDB and not all databases:
https://coveralls.io/builds/21705329

I'm not sure why this is happening... This issue is being tracked here: #2

[dhui]

This test should be using migrate's Drop() implementation instead of the driver's to test that the lock isn't being twice.

[lukaspj]

Hmm... The migrate is a parent-package which the database package doesn't know, so it wouldn't make sense to use migrate's Drop() here. Then the test should be moved to the migrate_test.go or another test file in the root package but that doesn't test the specific database implementations.

I guess we would need a suite that does integration-tests on migrate and all the different database implementation which would be a rather significant addition.

I could add such a suite, but it would only cover the Drop() method, as I don't have the time to write migrations for each database.

[dhui]

Yeah, I wanna avoid scope creep, but also want to prevent a regression. We don't need to test the other methods. e.g. only testing Drop() is sufficient. The test in it's current form doesn't cover any different cases than the existing TestDrop() in database/testing/testing.go.

The quickest way (albeit less clean way) to add the proper regression test would be to import github.com/golang-migrate/migrate/v4 in database/testing/testing.go, create a migrate.Migrate instance using NewWithDatabaseInstance() in Test(), and test Drop() using that instance.

I'm all for adding a db driver <-> migrate integration test suite, but I'm not sure about the approach you have for this. The scope for such an integration test suite may also be quite large, so for now, let's add the regression test using the quick and dirty way mentioned above. Then we can add a db driver integration test suite and clean the code in a separate PR.

Thanks again for your work!

[lukaspj]

I went a bit rogue and made some changes.
First of all I cleaned up the git commit history a bit with a force-push.

I found that a change to the Driver interface was warranted. With the change to the functionality in Drop() there was no way to re-establish the version table after a Drop(). I found this to be quite tedious, at it meant you had to store a copy of the database URL for re-connecting if your code would do a Drop().
To this end I added the Initialize() method to the Driver interface:
f111488#diff-ea51b70c3e62fa1445d70670eceaafe6

Logic for choosing the default MigrationsTable, was moved to this Initialize() method, to ensure that a call to Initialize() always had a sensible MigrationsTable.

For the regression test, found here:
f111488#diff-8571f16a46121ca4b6bd807b05b6ed75

I wanted to keep it completely separate from the existing tests, so I had to duplicate some code in order to call the TestMigrate method.
Example:
f111488#diff-30f12fe639ae221117c9c518485c1930

I just duplicated the whole test-block when dktesting.ParallelTest was used. When ParallelTest wasn't used, I did a Drop() and Initialize() before invoking TestMigrate()

Example:
f111488#diff-da587f4c74d6e6dca490ec5a1807c0f5R43

@dhui what do you think of these changes? Does the change to the Driver interface make sense to you?

[dhui]

I went a bit rogue and made some changes.
First of all I cleaned up the git commit history a bit with a force-push.

That's fine since it's already done. In the future, I'd prefer that we squash commits after PR approval but before merging to cleanup the history to make reviewing easier. However your comments summarizing your changes have been quite useful!

I found that a change to the Driver interface was warranted. With the change to the functionality in Drop() there was no way to re-establish the version table after a Drop(). I found this to be quite tedious, at it meant you had to store a copy of the database URL for re-connecting if your code would do a Drop().
To this end I added the Initialize() method to the Driver interface:

I'd avoid making any changes to the database.Driver interface. As you mentioned, you could achieve the same goal in testing by re-connecting. Although such a change is more tedious, it's less intrusive. Any changes to the database.Driver interface should be discussed here first.
I don't think we should add an Initialize() method to database.Driver since it makes driver instantiation more clumsy and more error prone. e.g. it's better keep the interface exposed to users simple

[lukaspj]

Alright, reverted the changes and instead added a warning to the Drop() method documentation about it being breaking.
I left some of the changes to when establishment of the DefaultMigrationTable, as it makes it a bit more uniform. Previously it would check the MigrationTable at both Open() and WithInstance(), now it only does so for WithInstance()

[dhui]

Since you've already gone through the trouble of creating a separate integration test, can you also use a separate entrypoint from the each driver test? e.g. call dt.TestMigrate() from a new TestMigrate() test method instead of Test() in each driver test.

The following db drivers need an integration test (e.g. call dt.TestMigrate()):

• Cassandra
• Clickhouse

I'm surprised that the integration tests don't add to the total test runtime... I guess it's b/c they're running in parallel w/ the existing tests.

[lukaspj]

Sorry for abusing your test-server btw and testing-by-pushing, but I can't run Docker locally. I don't have enough RAM in this PC.

[lukaspj]

Alright, in order to create a TestMigrate I changed the tests so that they now read the migrations in the examples folder for that test run.

I aligned the folder structure in the database implementations, such that they all have the migration examples in examples/migrations/.

[dhui]

Sorry for the delayed review. I've been quite busy lately.

Thanks for making the example file structure consistent.
I like how all of the examples are executed now!

The TravisCI runs take longer now, but we can deal w/ this issue later. Maybe sprinkle t.Parallel() around...

It looks like there was an issue backing out your Initialize() changes. Namely, setting default config values was removed. I've marked the places that need to be fixed.

[dhui]

Thanks for the PR!