-
Notifications
You must be signed in to change notification settings - Fork 70
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
[Question] pg_advisory_lock not supported by CockroachDB #35
Comments
The advisory lock is to protect against multiple migrations being run at the same time. tern would run fine without it, but concurrently running migrations would have unforeseen consequences. #31 is somewhat related. The solution I proposed there was to allow user provided SQL for lock and unlock. With that in place you could use some other means of locking such as creating a table and checking its existence. |
I am new to cockroach, but I believe they will not support pg_advisory_lock anytime soon. So this solution would likely work for roach. From what I can tell, there are no locks in cockroach. Also, I am not able to find any recommendations with respect to concurrently applying schema migrations. It looks like it runs raft and is CP, so I think they can just be applied concurrently without much worry. Tern was mentioned here btw |
What I mean is that two migration processes are run at the same time. You never would want that to happen. e.g. migration 1 is running twice at the same time. Now it is extremely likely that one would fail because adding or dropping a table or column can only be done once. But if it was a data migration the user might not be so lucky. Practically speaking though, it is a very unlikely case that is being protected against. An optional flag to disable locking might be reasonable. |
Makes sense.
I think a feature flag here makes a lot of sense. Do you want to take a wack at it or should I? |
It seems like there ought to be a flag called something like The Migrator could be lock aware in the same way that it is transaction aware. That is, change the MigratorOptions struct to
And then change MigrateTo to
|
I added the flag here. Although, there is still a problem with multiple transactions trying to run on the same connection. It works if I set
Example Tern conf
If you'd like a roach stack to play with base-infra.yml:
immediately after running |
That error indicates that transactions are still being used somewhere and an error occurs inside it. |
Yes I figured that out thank you. Another thing that could be done is using the strategy that golang-migrate uses. If the url has postgresql:// as the protocol, then an advisory lock is applied like in this repo. If cockroach:// is the protocol, then a lock table is created
|
Yeah, that might work. Though I'm a little reluctant to use lock tables. The nice thing about advisory locks is they automatically get cleaned up if the migration connection dies unexpectedly. Whereas the strategy golang-migrate uses can require manual cleanup in an error case. See https://github.com/golang-migrate/migrate/blob/aef2747761d2a59d87e36fa942d9ece5339a2c41/database/cockroachdb/cockroachdb.go#L199. |
I'd love to move from |
The only potential solutions that have come up on the tern side is an option to disable or customize the locking logic. I would lean to customizing the lock logic as that would also allow disabling it if desiring. But I'm not currently working on this and I don't know anyone who is. |
Would you be open to a change along these lines? Any idea what kind of customisation would you want to allow? Would allowing users to opt into the go migrate style be acceptable? |
@tmcnicol Which approach? Several have been mentioned on this thread. |
Sorry I was referring to the go-migrate option, of a lock table that people could opt into. Was thinking that we could get around the unlocking when the connection unexpectedly dies by using a heartbeat mechanism. |
@tmcnicol Yes, that seems reasonable. |
@ryanluu12345 Not that I know of. |
Time necessitated that I moved to a different library for my application. I
don’t think it would be a hard change but didn’t have the capacity sorry.
…On Sat, 11 May 2024 at 08:22, Jack Christensen ***@***.***> wrote:
@ryanluu12345 <https://github.com/ryanluu12345> Not that I know of.
—
Reply to this email directly, view it on GitHub
<#35 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABI5YE7NSF4FJIV3RE3AESDZBVCCBAVCNFSM5CKW6Y22U5DIOJSWCZC7NNSXTN2JONZXKZKDN5WW2ZLOOQ5TEMJQGUZTGOJUGQ2Q>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
Greetings,
This is my favorite migration tool. However, I just started a new project with CockroachDB, and on migration I get a warning
It looks like there are a couple places where the function is used in migrate.go. Is it your opinion that this lock function is critical to the library? It seems like cockroach always uses
SERIALIZABLE isolation
.The text was updated successfully, but these errors were encountered: