Multiple DB migrations priority #41664
Conversation
| check_target_version | ||
|
|
||
| scope = ENV["SCOPE"] | ||
| verbose_was, Migration.verbose = Migration.verbose, verbose? | ||
|
|
||
| Base.connection.migration_context.migrate(target_version) do |migration| | ||
| Base.connection.migration_context.migrate(target_version || version) do |migration| |
There was a problem hiding this comment.
tests are failing and I believe this is because this line.
Maybe it's possible to patch interval (to grab all the versions before the ENV['version']
@eileencodes maybe share your idea so after that it will be easier to solve the issues and provide the tests if needed :)
There was a problem hiding this comment.
Hey sorry for the delay! This is what I was thinking in terms of an approach for fixing this issue. https://github.com/rails/rails/compare/main...eileencodes:fix-migration-ordering-bug?expand=1 It's similar to yours but avoids the complexity of connection_intervals. Let me know what you think. If you don't have time to pick this back up and write tests I can take this and make sure you get credit on the final PR.
There was a problem hiding this comment.
Yeah, that looks good, we can go both ways I can try to patch tests on my end but if you feel like that will be easier to do on your side I am not against it :)
My main goal is to fix migration order in the end
There was a problem hiding this comment.
I updated this request with your commit, will see how it goes with the tests
There was a problem hiding this comment.
The only question I have is ActiveRecord::Base.establish_connection(db_config) we call this for each migration
if we have a situation like (A and B here are databases):
mig1 from A, mig2 from A, mig3 from A, mig4 from A, mig1 from B, mig5 from A
do we really need to re-establish connections for the first 4 ?
I understand that this is just migrations and the optimization won't be that big but the easy check can be done if it's needed e.g.:
sorted_version_configs = db_config_by_version.sort
prev_config = sorted_version_configs[0][1]
sorted_version_configs.each do |version, db_config|
ActiveRecord::Base.establish_connection(db_config) unless prev_config == db_config
ActiveRecord::Tasks::DatabaseTasks.migrate(version)
prev_config = db_config
end
What do you think @eileencodes ?
| end | ||
|
|
||
| def pending_migration_versions # :nodoc: | ||
| migrations.collect(&:version) - get_all_versions | ||
| migrations.map(&:version) - get_all_versions |
There was a problem hiding this comment.
in the other places in this file map used
| @@ -1140,11 +1140,15 @@ def current_version # :nodoc: | |||
| end | |||
|
|
|||
| def needs_migration? # :nodoc: | |||
| pending_migration_versions.size > 0 | |||
| !pending_migration_versions.empty? | |||
There was a problem hiding this comment.
this may be reverted if needed
| end | ||
|
|
||
| def pending_migrations # :nodoc: | ||
| migrations.select { |migration| pending_migration_versions.include?(migration.version) } |
There was a problem hiding this comment.
to get the full migration struct including scope (we do need it)
Or we can raise for multi dbs if VERSION or SCOPE passed, don't know what's better
I tried to allow VERSION with up direction here... but down direction won't work for some dummy versions like 0 (few tests failed with that)
So I guess we can support only UP direction here, but maybe raise for VERSION passed is ok as well
There was a problem hiding this comment.
@eileencodes this is the most interesting part, what do you think ?
Tests are green now, I didn't add new ones, but will do once we choose a strategy here
There was a problem hiding this comment.
we can also use Set(O(1) checks) here but I don't think if this optimization is needed, can migrations number be that high ?
| db_configs = ActiveRecord::Base.configurations.configs_for(env_name: ActiveRecord::Tasks::DatabaseTasks.env) | ||
|
|
||
| # no additional job if it's only single db app | ||
| if db_configs.size == 1 |
There was a problem hiding this comment.
fast check to avoid anything related to multi dbs for single db app
|
Hey @dsounded I finally got back around to this. @seejohnrun and I decided to pare back your PR a bit since it included some refactoring. I also added a test in #42604 and gave you credit. |
Previously if there were 2 migrations in one db and 1 migration in the other db all the migrations for db one would run and then all migrations for db two would run. If a migration in one database depended on a migration in another database then it could fail. This is probably pretty rare, however in a multi-db application that's moving tables from one db to another, running them out of order could result in a migration error. In this this change we collect all the versions for each migration and the corresponding db_config so we can run them in the order they are created rather than per-db. Closes rails#41664 Related rails#41538 Co-authored-by: John Crepezzi <john.crepezzi@gmail.com> Co-authored-by: Kiril Dokh <dsounded@gmail.com>
Thank you for addressing this 👍 |
Summary
Basically sometimes we need to specify the order for multiple database migrations, we can do that only with consecutive calls e.g.
rake db:migrate:secondary rake db:migrate:primary ... rake db:migrate:nth(or by position in YAML file)But this is not always handy.
This request is going from discussion #41538
and tries to change migrations behavior like they are all in one folder (
flat_map) (execution by timestamp aka version)