Alternatives to PG Advisory Locks?

Howdy all,

I’ve got a nearly-complete implementation for Cockroach in the migrate schema migration tool/library.

The only missing piece is implementing the Lock and Unlock functions, to acquire a lock for performing schema changes. In the postgres implementation, they take advantage of the pg_try_advisory_lock and pg_advisory_unlock function. These functions exist in the Cockroach grammar (presumably for compatibility), but merely return true and don’t do any locking.

So, I wanted to pick your brains to see if you had any thoughts on the best way to implement this.

I came up with two possible options

  1. Maintain a separate ‘advisory lock’ table that is a simple key-value (key = lock id, value = bool (locked/unlocked)). Read and set this value in a transaction to atomically acquire the lock, and release it at the end of the migrations.
  2. Perform the entire series of migrations in the context of a single transaction, which is effectively a lock and will work so long as the current schema version is checked within the transaction block.

The first option has the potential to be a bit messy in the event of an error during migration (possibly creating a lock that is never released), and the latter would involve some pretty large changes to the inner workings of the migrate library to maintain the transaction context across all migrations, and also then has the limitations of schema changes in a Cockroach transaction.

Any suggestions/ideas on alternate implementations?

I’ve also got a work-in-progress PR open on my fork in case anyone wants to look over the code and make any other suggestions before I make a PR to the mainline repo :slight_smile:

Hi @twrobel,

Very cool! Thanks for working on adding Cockroach to migrate.

As you noted, we only implemented these functions for syntax-level compatibility. We chose to leave them trivially implemented despite the obvious correctness issues because we observed that the tools that we knew of that were relying on these functions were doing so to prevent concurrent migrations run by users - something we judged was not too high of a risk.

We have an open issue to provide correct implementations of advisory locks here: https://github.com/cockroachdb/cockroach/issues/13546 - this isn’t on our official roadmap at the moment but hopefully we can get to it soon.

As a workaround for our missing support, your first suggestion sounds better to me because (as you allude to) CockroachDB doesn’t support true transactional schema changes.