Add change_column_safely migration helper#107
Add change_column_safely migration helper#107fatkodima wants to merge 3 commits intoankane:masterfrom
Conversation
jturkel
left a comment
jturkel
left a comment
There was a problem hiding this comment.
We've rolled similar infrastructure into our internal application development kit so it's so awesome to see this get pulled into strong_migrations! I'm not a maintainer on this gem but I added some comments on things we're doing slightly differently in our homegrown infrastructure.
| reversible do |dir| | ||
| dir.up do | ||
| transaction do | ||
| add_column(table_name, column_name, type, default: nil, **options) |
There was a problem hiding this comment.
Should this only add the column if it doesn't already exist to properly handle retries since the entire transaction isn't wrapped in a transaction? We've hit issues with longer running migrations so we generally try to make them idempotent.
| change_column_default(table_name, column_name, default) | ||
| end | ||
|
|
||
| default_after_type_cast = connection.type_cast(default) |
There was a problem hiding this comment.
How hard would it be to support default expression e.g. uuid_generate_v4()? We've needed to do that pretty often as we've migrated away from bigint identifiers (at least for what we externally expose).
There was a problem hiding this comment.
It is not that hard, but it definitely will introduce some ugly code changes in backfill_column_safely method. Let's wait for more demand and implement it in separate PR, if needed.
|
|
||
| table = Arel::Table.new(table_name) | ||
| count_arel = table.project(Arel.star.count.as("count")) | ||
| total = connection.exec_query(count_arel.to_sql).first["count"] |
There was a problem hiding this comment.
We've seen count(*) on big tables take a long time. Should we avoid this count query and just handle the start_arel returning a null result?
|
|
||
| primary_key = connection.primary_key(table_name) | ||
|
|
||
| start_arel = table |
There was a problem hiding this comment.
Following up on my previous comment about restartability of migrations, should we skip rows that already have a non-null value for column_name? It's not strictly required but can be a really helpful for long running migrations that get restarted due to a network blip, Rails process dying, etc.
|
|
||
| finish_result = connection.exec_query(finish_arel.to_sql).first | ||
|
|
||
| update_arel = Arel::UpdateManager.new |
There was a problem hiding this comment.
Our app sometimes updates multiple rows which makes migrations like this deadlock prone. We always force locks to be acquired in primary key order to avoid this. Not sure how common that practice is and whether or not it's worth folding into the gem to avoid deadlocks.
There was a problem hiding this comment.
Didn't get it. Can you provide examples and elaborate a little more?
|
Decided helper methods are better as a separate gem for now - see #111 |
3 participants
This pr depends on (and consists of) changes from previous 2 prs, so should be merged after them.