Cockroach migration locking workaround

[kclonts]

Adds a new locking semantic with a dedicated conn/tx locking on the row with value -1

updates the bootstrap schema_version table to include this row and also enables partitions to prevent contention with negative and positive values

added flag --cockroachdb to enable

replace NewMigratorEx with simply NewMigrator

Allow disable of row locks during a migration, particularly necessary on cockroach with functions as they're a schema change

guards lock from recursive calls

abstract locking/releasing from the lock type

[jackc]

I've made some specific comments inline, but I have two overall related concerns.

I'd be happy for tern to support CockroachDB, but I don't want to have a specific CockroachDB codepath. I'd much rather have something like --lock-type=row instead of --cockroachdb. This leaves room for multiple lock types.

Second, this change touches a bunch of different parts of the code. I wonder if there is a simpler approach and/or if this can be done in multiple steps.

For the simpler approach, I suppose there may not be any other way to do this with CRDB, but I'm not excited about needing two connections. migrate.NewMigrator never managed connections previously. Now it does. That's a definite increase in complexity.

For multiple steps, I wonder if a first step would be abstracting out the concept of the lock type or strategy and implementing a disable strategy. It would be a smaller step that would actually allow CRDB to work at the expense of not having any locking.

---

I also still wonder about the idea I mentioned in another issue to have locking be a SQL command passed as an argument to the migrator. That would make it completely flexible for whatever needs pseudo-PostgreSQL databases would need.

---

Also, I wonder if something really simple like setting and checking application_name would be a sufficient lock. Apparently, CRDB supports that.

[kclonts]

Regarding --lock-type=row instead of --cockroachdb it could be both. Any db specific options just use some specific combo of features. There are a few quirks that I've encountered with cockroach compatibility to postgres and it's likely there could be some other flags for implementation specifics.

so --lock-type=row is available if you simply and only want to use this feature. But if you just generally want to support cockroach, use the --cockroachdb flag on it's own to enable any changes added in the future

I can change it to whatever you ultimately decide there

Second, for sure there could be a simpler way. This was my quick & dirty get it done for my own purpose. I think it's possible to use a single connection but I'm just not positive that'll work 100% of the time (and don't have the time to verify)

Disable would work easily, removing any connection management from migrate struct. But a huge feature of migration systems is locking out other processes especially if you're running in CI. Any existing use cases have no changes to complexity or code paths, the additional feature here is not negatively affecting current projects.

I also still wonder about the idea I mentioned in another issue to have locking be a SQL command passed as an argument to the migrator. That would make it completely flexible for whatever needs pseudo-PostgreSQL databases would need. This would require using the same connection for lock/unlock and I'm not sure it's 100% supported. In addition you would still need to turn it on/off in cockroach when making certain changes, like adding functions or altering data types with existing data. Ex

alter table nba_games
  alter column away_id set data type uuid using away_id::UUID;

Also, I wonder if something really simple like setting and checking application_name would be a sufficient lock. Apparently, CRDB supports that. The problem here is how locks work in cockroach. If you lock where application_name = '' you also lock on any rows that are read, it's why my initial versions used partitions on n >= 0 and n < 0 so we can search a partition and just lock on the negative partition. But partitions are not supported on global tables of which my serverless application was running and I ran into the limitation there quickly so we're using a separate table with a single row.

Now since Cockroach by default offers serializable isolation we might be able to just read for application_name and quit if found without locking but I haven't tested, and we're talking about multiple rows instead of managing just one. I also think that naturally lends to implementation details of how cockroach manages that table and the ordering of inserting the rows between all servers / clients. Now for a migration system it might be overkill to worry about this too much but at the same time you're managing schema for a database in production.

All that said, maybe it's a change better left as the start of the next version or at the very least as a beta branch with some more testing from the community before it's brought to master.

However, it does not change functionality of existing applications and enabling the --cockroachdb flag should come with a warning/message that we're still testing or changing this

[jackc]

Regarding the cockroachdb flag, I may have misstated what I meant above. I'm happy for tern to work with CRDB, but I'm reluctant to have a specific mode for it, because that implies support such that if it doesn't work it is a bug. And personally, it's not something I would want to spend time maintaining. But maybe I'm just splitting hairs. If the docs say use this combination of flags for CRDB, then we'll still get bug reports even if there is no CRDB specific code. 🤷

So I guess I could acquiesce on the flag, but I still want to push back on the multiple connection lock strategy. I'd like to avoid that complexity and I suspect one of the ideas mentioned above would work.