-
Notifications
You must be signed in to change notification settings - Fork 19
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
Add a configuration option for always disabling ddl transactions #96
base: master
Are you sure you want to change the base?
Add a configuration option for always disabling ddl transactions #96
Conversation
@rkrage Any thoughts on this? Would be spiffy to get it in if possible. |
@ChrisVoxland can we try to come up with a better name for the config? If I'm understanding this correctly, this setting only affects the behavior of |
@rkrage not attached to that name by any means. How about Also if there is a way to do this without code changes that I'm missing, I'd be happy to put up a PR updating the docs to describe that more specifically instead. I tried a few different variations of things with config/initializers, but wasn't actually able to get the default behavior back, but I very well may be missing something obvious. |
@ChrisVoxland I think
I thought there was already a global config for toggling this behavior in Rails but I don't think that actually exists and it can only be turned off on a per migration basis. So it appears the only way to change the default behavior is by monkey patching the migration class, which is what we're doing here 😄 So yeah, this PR is definitely a welcome change |
@rkrage Thanks!, I updated the pr to use the new name, lmk if there is anything else I can do to get this through. |
@ChrisVoxland I don't have any issues philosophically with adding this kind of configuration, but I do think we have one thing we need to address to be able to merge: some DDL (e.g., I think the simplest solution would be to:
|
@jcoleman I'd be happy to update the docs, but I don't think any other code changes should be necessary here. And Also happy to dig in and investigate if there are other methods of concern here. |
I think I agree with @ChrisVoxland on this one.
Is it really worth adding extra validation to avoid one extra call to Postgres (that will fail immediately with a pretty descriptive error)? In both cases, whether we raise the error ourselves or let it bubble up from Postgres, the result will be the same: a rolled back transaction. @jcoleman what do you think? |
Hi @ChrisVoxland! I took a pass at this and reworked a bunch of documentation on my branch There are a few concerns:
For more on that last point see this manual demo in psql: Session 1:
Session 2:
Thoughts? |
I believe this was just a problem with our test setup. See #101 |
But back to the original conversation. I'm starting to think that we shouldn't allow this at all...
But if there are blocking transactions, that part of So here's a concrete example of one of the risks @jcoleman called out in the docs:
class MyTransactionalMigration < ActiveRecord::Migration[7.1]
self.disable_ddl_transaction = false
def up
safe_make_column_nullable :foo, :column
safe_make_column_nullable :bar, :column
end
end We might successfully take out the lock on |
This adds a configuration option:
always_disable_ddl_transactions
to allow folks to easily disable the default behavior of making all migrations run without a DDL transaction.The reasoning for that as the default makes sense, but I found it violated the principle of least surprise when I was first experimenting with this gem, and it was simple enough to make it configurable.
The existing warnings for concurrent migrations in a DDL transaction seem safe enough to me, and the tradeoff of potential locking with multiple DDL statements in a single transaction seems worth it for those who want to opt into that behavior.