postgres driver should wait for lock

[Teddy-Schmitz]

It seems the postgres driver wasn't following convention as the Lock method will wait for the timeout before stopping the operation. But the driver was using postgres_try_advisory_lock which immediately errors if it cant acquire instead of waiting. This fixes that.

[Teddy-Schmitz]

For your previous comment about using a context instead, I would prefer that but the best solution would be to change the Driver interface to pass a context to lock and unlock. I'm happy to do that in another PR if you think this is a good approach.

[dhui]

For your previous comment about using a context instead, I would prefer that but the best solution would be to change the Driver interface to pass a context to lock and unlock. I'm happy to do that in another PR if you think this is a good approach.

I like the idea of requiring context in the Driver interface but this is a breaking change which shouldn't be made on a whim. Also, I'm wary about the behavior of the underlying DB. e.g. if a DB driver function call times out while running a migration, what's that mean?
Let's wait a bit before making any drastic or breaking changes. I've created an issue to discuss building a new Driver interface.

[dhui]

This change breaks the current behavior of Driver.Lock().The current behavior is that if the lock cannot be obtained, then an error should be returned.

This behavior is documented, which implies that Driver.Lock() should not block indefinitely.

[Teddy-Schmitz]

I would argue that the fact there is a LockTimeout
https://godoc.org/github.com/golang-migrate/migrate#Migrate

Says to me wait this long to try to get a lock, though you could say this is more for the database to be available. But the mysql driver change merged in #8 does the same thing with waiting for the lock. The only difference is they are setting their own lock timeout instead of using the configuration variable (which is even more confusing to me).

[dhui]

Thanks for bringing Migrate.LockTimeout to my attention, since I wasn't aware of this field.
It looks like Migrate is responsible for timing out DB driver locks, so with the current design, it's fine for the DB drivers to lock indefinitely.

However, I think there's a design flaw and that lock acquisition timeouts should be managed by the DB driver since the lock behavior is DB dependent.
Specifically, I'm worried about dangling locks like the ones mentioned in the postgres docs. This may happen if the lock is acquired in the DB after the Go code has timed out. In Postgres' and MySQL's case, the dangling locks will be release when the session ends e.g. when Migrate.Close() is called), but this may not be for a while depending on how Migrate is being used.
Based on a quick skim of the DB driver locks, only postgres, mysql, and cockroachdb use DB locks. All of the other DB drivers lock using a struct field in the DB driver instance. The cockroachdb driver implements locks using a table wrapped in a serializable transaction, so it doesn't have the dangling lock issue.

I'd like to avoid creating any dangling locks, but the impact doesn't seem catastrophic since the odds of creating them are low. From a DRY perspective, I'd much rather have the lock timeouts be managed by Migrate, so let's stick w/ the current design and allow the DB drivers to lock indefinitely.

This means that users should be aware that in rare cases, dangling locks may be created and other migrations for the same DB will be blocked from running until Migrate.Close() is called. We can try to clean all of this up when we write a new DB driver interface.

So I think your changes are fine and will merge them once tests pass.

[Teddy-Schmitz]

That all makes sense, I do think a "spring" cleaning might need to be done of the code to document odd issues like this. What you say makes perfect sense and I will be happy to help where I can in any cleanup efforts.

It seems the tests are failing for Cassandra and Mysql if I'm reading it right which is odd since this change doesn't touch their code. I'll try to see whats up.

[Teddy-Schmitz]

@dhui I cant seem to get the tests to rerun. I'm not sure how my postgres driver changes would cause mysql and cassandra tests to fail.

[dhui]

Are you able to reproduce the errors locally?
You can find the steps on how to run tests locally here: https://github.com/golang-migrate/migrate/blob/master/CONTRIBUTING.md
You'll need make and docker to run tests.

If you find the import rewrites too annoying, you can modify the project's location in your Go workspace accordingly.

[dhui]

@Teddy-Schmitz try rebasing your changes. The master branch now builds!

[Teddy-Schmitz]

I still am getting test failures for mysql locally. But I rebased and pushed it up lets see if CI fails.

[coveralls]

Pull Request Test Coverage Report for Build 81

• 0 of 0 changed or added relevant lines in 0 files are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage decreased (-2.4%) to 46.644%

---

Totals
Change from base Build 80:	-2.4%
Covered Lines:	681
Relevant Lines:	1460

---

💛 - Coveralls