search

jackc / tern

The SQL Fan's Migrator
MIT License
925 stars 68 forks source link

session-level only advisory lock #31

Closed gruzovator closed 3 years ago

gruzovator commented 3 years ago

Hi

tern uses session level advisory lock that may be a problem for systems with pgbouncer, cause pgbouncer reuses session.
It would be great to have ability to use transaction level advisory locks.

I've forked the project and tried to change advisory lock type, but it can't be done without serious changes of the project structure. It may be worth to do locking at higher level (not inside MigrateTo & ensureSchemaVersionTableExists), to allow user to select correct locking strategy for their infrastructure.

Thanks.

jackc commented 3 years ago

It never occurred to me to try to do migrations through a pooler. As you discovered, it would probably be rather difficult to support that use case. Each migration runs in its own transaction. The only way to make transaction-level locks work would be to separately lock and unlock for every migration. This would make preventing race conditions / concurrent execution of migrations much more complicated. Also, tern supports disabling transactions. I don't see transaction-level locking as being feasible. The lock really needs to be something acquired once at the beginning of the migration and released at the end.

Session-level locks have the advantage that if something goes wrong the lock is guaranteed to be released. The only other option would be a permanent lock such as writing a row. That risks getting permanently locked. Dealing with that could greatly increase complexity.

The only option that would not be super invasive to the structure of tern would be to allow arbitrary SQL to be used to lock and unlock. That would offload all the complexity to the user / application. I suppose I could be open to that... but to be honest I think that trying to do migrations through a pooler is fundamentally a bad idea. I would recommend not doing that.

gruzovator commented 3 years ago

Thank you for reply.

Well, in my experience, pgbouncer is a kind of standard "add-on" for postgres and it's not easy to access postgres directly. May be i'm wrong.

Session-leve lock is a handy feature, but it doesn't work correctly with pgbouncer. And the problem is that tern users don't have any workaround.

What do you think about option to disable advisory lock to allow users to select locking strategy outside?

(A bit more about advisory locks and pooling in rails project: https://github.com/rails/rails/issues/31190)

jackc commented 3 years ago

I agree it is common to use pgbouncer in front of application servers, but I don't see why the migration runner shouldn't have direct access to the database. In fact, it probably should be preferred because the migration runner generally needs to connect as a superuser or database owner and the application doesn't.

Regardless, I suppose an option to disable tern's locking would be acceptable.

gruzovator commented 3 years ago

migration runner generally needs to connect as a superuser or database owner

Hmm, may be i'm using migration tooling incorrectly, but in my microservices projects there is no such role as migration runner. Migrations are applied at every app instance start (tern is embedded), So, that's why i'm concerned about locking problems.

Anyway, thank you.
Should I close the issue?

jackc commented 3 years ago

migration runner generally needs to connect as a superuser or database owner

Hmm, may be i'm using migration tooling incorrectly, but in my microservices projects there is no such role as migration runner. Migrations are applied at every app instance start (tern is embedded), So, that's why i'm concerned about locking problems.

I wouldn't go so far as to say it is incorrect, but decoupling migrations from a specific application and from the application deployment event has a number of advantages -- but at the expense of a little more operations work.

But my point of view is primarily from working with integration databases, not the 1 DB per app microservice model.

Should I close the issue?

If you want to make a PR disabling tern's locking that would be fine, but I don't plan on implementing it myself.