-
Notifications
You must be signed in to change notification settings - Fork 99
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Provide option to lock migration table during migration to prevent concurrent migrations #38
Comments
Hi @mdales, and thank for opening this issue. Gormigrate was initially created to be a minimalistic migration tool, and meant to be used when a single executable is been ran. This is why we haven't bothered to implement stuff like locking, etc. That said, I think we could do that if easy enough. But yes, it should ideally work on all supported databases (but it's probably fine to be no-op on SQLite). Ideally we need to look for what other migration tools do to solve this. And in the meantime, if your app use something like Redis, you could use some distributed lock with Redis to solve your problem for now. |
Thanks for the feedback @andreynering. I've not forgotten about this, but I will say that it is harder that I initially (naively :) assumed. My deployment is with Postgres, and the initial thought that I'd just lock the migration table doesn't work (as the README for gormigrate hints at, you can't do DDL things within a lock in Postgres). It may end up being more appropriate to do this at the app level, in which case this will perhaps just end up being a PR to update the documentation to make the situation explicit, but I'll have a read around and see if I can find some best practice from other migration tools here. |
@mdales Thanks. I just updated the README to mention the need to have manual locks. |
I've had a little look here at the options others have used, and what I've ended up doing in the product I'm working on is use an advisory lock. This works fine in Postgres, and my understanding from research is that it should work okay with both SQL Server and MySQL using application locks. This can be implemented outside of gormigrate readily, as it does not lock an specific migration related structures. I think a compelling case could be made to integrate this optionally into gormigrate also as it's not that much code. The one thing that makes me a little nervous would be less the code but more the testing - currently there's no concurrency in the testing suite, and adding this will make tests more complex for a use case that I assume not many others worry about? If that test complexity is undesirable, then perhaps we just update the documentation yet again to suggest that application/advisory locks are one simple way to protect your gormigrate migrations? I have no strong preference either way right now, as I have a way forward for my application. If you'd like to integrate it I'm happy to do so. |
FWIW, the testing in my application for the lock wasn't too bad. I was worried I'd need to augment gormigrate to let me synchronise things properly to ensure testing clashing migrations, but by using a couple of Mutexs in a migration itself I was able to write suitable testing. |
@mdales I'm curious on what approach you used to have locks using the database itself. If you don't mind opening a PR, it'd be nice. We could also discuss the testing approach on the PR, if you are unsure about it. |
Just use a distribute lock , such as a redis lock. |
I started trying to solve this in gormigrate, but before I get too stuck into this, I thought I'd seen what, if any, appetite there is for solving this here.
The problem I have is that in a production environment where I have many nodes talking to a single DB backend the migrations may race. To solve this I'd like the migrate function to have the option to lock the migrations table for the duration of migrations.
However given how different the syntax is between Postgres, MySQL and SQLServer, this will require a number of DB specific implementations (I wouldn't plan to support this feature on SQLite).
Is this worth doing here? Logically it's the right place to put the logic I think, but looking at the existing code there's no real DB specific code in there yet, so I don't want to write all the code and have it rejected as no something the gormigrate community want.
The text was updated successfully, but these errors were encountered: