migrate/postgres: skip advisory lock for cockroachdb

[waynr]

This PR skips the use of pg_advisory_lock in impl Migrate for PgConnection if the string returned by SELECT version() includes the substring "CockroachDB".

This is necessary because the folks at CockroachDB haven't implemented it yet.

I actually don't know if this PR is the right approach (seems like it would be better for CockroachDB to do its best to implement support). Though for that matter, reading the postgres docs on advisory locks leaves me feeling a little confused because of the following:

PostgreSQL provides a means for creating locks that have application-defined meanings. These are called advisory locks, because the system does not enforce their use — it is up to the application to use them correctly.

Does this mean that locks held by the sqlx migration may not hold meaning or be honored by application(s) using the DB?

Anyway, it seems like this PR or something like it would be necessary for sqlx to run migrations against cockroach db instances older than whatever version finally supports pg_advisory_lock.

Also, for what it's worth this is the error I get without this PR:

Error: SQLXMigrateError(Execute(Database(PgDatabaseError { severity: Error, code: "42883", message: "unknown function: pg_advisory_lock()", detail: None, hint: None, position: None, where: None, schema: None, table: None, column: None, data_type: None, constraint: None, file: Some("name_resolution.go"), line: Some(205), routine: Some("ResolveFunction") })))

[waynr]

Oh yeah, I forgot to include in the description -- there is precedence in other projects for taking this approach: prisma/prisma-engines#2635

Which doesn't necessarily mean it's the right approach here of course.

[abonander]

@mehcode wrote most of the migration machinery so I'd like him to chime in when it's convenient.

But from what I understand, the point of taking a lock is so that multiple instances of the application starting at the same time (for example, when deploying a new image to a Kubernetes cluster with >1 replicas) don't try to execute the same migrations at the same time, which could cause errors or data loss.

Advisory locks are one way to do this, but we could also just acquire an exclusive lock on the _sqlx_migrations table itself: https://www.postgresql.org/docs/current/explicit-locking.html#LOCKING-TABLES

lock table _sqlx_migrations in exclusive mode

exclusive mode blocks everything but concurrent reads.

I think a little bit of refactoring would be necessary as we'd have to create the _sqlx_migrations table outside of the exclusive lock in order to acquire one on it, but that's easy enough with create table if not exists.

[mehcode]

@abonander is correct.

I'd be opposed to dropping locks for cockroachdb and would rather switch to an exclusive table lock for cockroachdb. That should maintain the same invariant.

@abonander Do you think there is any purpose in keeping the advisory lock here and downgrading to table lock or just doing only table lock?

[abonander]

Welp, unfortunately it looks like the LOCK statement isn't supported either, as it's not listed here: https://www.cockroachlabs.com/docs/stable/sql-statements.html

However, I think instead of trying to special-case for CockroachDB, we should maybe just have a no_lock flag on Migrator so it can work for other databases that talk the Postgres protocol but don't support manual locking.

[waynr]

But from what I understand, the point of taking a lock is so that multiple instances of the application starting at the same time (for example, when deploying a new image to a Kubernetes cluster with >1 replicas) don't try to execute the same migrations at the same time, which could cause errors or data loss.

Okay, this makes sense. I hadn't been thinking of the >1 replica situation and agree with you that it's probably not a good idea to enable that for CockroachDB until they implement some kind of appropriate locking mechanism. Would a transaction around the migration operations on _sqlx_migrations be sufficient?

Anyway, I hadn't thought to try it yesterday but today I see that sqlx migrate run works fine -- it's just the migrate! macro that doesn't work. So I actually don't need this PR or anything else fixed.

However, I think instead of trying to special-case for CockroachDB, we should maybe just have a no_lock flag on Migrator so it can work for other databases that talk the Postgres protocol but don't support manual locking.

I like the default being to keep the locking behavior and to opt in to disabling it -- if migrate! hadn't failed for me the way it did then i might have ended up dealing with some kind of funky data race during some random future migration. I do worry though, that allowing users to opt in to disabling it could be problematic for people who don't really understand the implications. I started hacking on support for this in the Migrator and added a comment to describe why it should be considered unsafe.

[abonander]

I started hacking on support for this in the Migrator and added a comment to describe why it should be considered unsafe.

@waynr were you going to add that here or in a new PR?

[abonander]

Closing due to inactivity. If you want to reopen, please follow up on my previous comment.

[waynr]

Hey @abonander my intuition is that it's best not to make it possible for migrations to be run in application without the advisory lock or something similar so I decided not to open a PR with my changes.