Skip to content
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

Fixes #30783 - Squash database migrations #7961

Closed
wants to merge 2 commits into from

Conversation

ekohl
Copy link
Member

@ekohl ekohl commented Sep 6, 2020

Squashing migrations saves time when migrations need to run. This happens on every fresh installation and every CI run.

This was generated using squasher version 0.6.2 with 2019 -m 6.0 as arguments.

On my system, before this patch:

$ dropdb foreman && time bundle exec rake db:create db:migrate > /dev/null
real	0m20,328s
user	0m12,373s
sys	0m0,591s

After this patch

$ dropdb foreman && time bundle exec rake db:create db:migrate > /dev/null
real	0m10,341s
user	0m6,731s
sys	0m0,372s

Perhaps there is a better moment than just 2019. For example, all migrations included in Foreman 2.0.

This PR does verification of the approach in general.

I first verified the output of:

$ dropdb foreman && bundle exec rake db:create db:migrate db:dump

Other than the created_at and updated_at columns there was no difference.

I have not done any testing on existing databases, nor any testing with plugins.

@theforeman-bot
Copy link
Member

Issues: #30783

@@ -0,0 +1,13 @@
class SquasherClean < ActiveRecord::Migration[6.0]
class SchemaMigration < ActiveRecord::Base

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rails/ApplicationRecord: Models should subclass ApplicationRecord.

Copy link
Member

@tbrisker tbrisker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

perhaps 2.0 does indeed provide a better point to start from, but not crucial.

def up
migrations = Dir.glob(File.join(File.dirname(__FILE__), '*.rb'))
versions = migrations.map { |file| File.basename(file)[/\A\d+/] }
SchemaMigration.where("version NOT IN (?)", versions).delete_all
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will remove migrations that came from plugins, causing them to be executed again.
TBH, I'm not sure we have to clean up the squashed migrations from the schema_migrations table - rails is perfectly fine with having migrations listed in the table that have been removed - see https://edgeguides.rubyonrails.org/active_record_migrations.html#old-migrations.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I also had some issues with that. It's generated from https://github.com/jalkoby/squasher/blob/master/lib/squasher/templates/squasher_clean.rb.erb. I also thought about an explicit list of ones that we know have been removed.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thinking more about it, it may be good to keep a record of that. If something ever shows up, you at least know which migration path it took.

Squashing migrations saves time when migrations need to run. This
happens on every fresh installation and every CI run.

This was generated using squasher version 0.6.2 with `2019 -m 6.0` as
arguments.

On my system, before this patch:

$ dropdb foreman && time bundle exec rake db:create db:migrate > /dev/null
real	0m20,328s
user	0m12,373s
sys	0m0,591s

After this patch

$ dropdb foreman && time bundle exec rake db:create db:migrate > /dev/null
real	0m10,341s
user	0m6,731s
sys	0m0,372s
@ekohl
Copy link
Member Author

ekohl commented Sep 6, 2020

Looks like an upgrade fails:

PG::DuplicateTable: ERROR:  relation "architectures" already exists

I suspect that our upgrade test is using a version older than 2019 and that's causing it. We can show a better error in that case.

@tbrisker
Copy link
Member

tbrisker commented Sep 6, 2020

Looks like an upgrade fails:

PG::DuplicateTable: ERROR:  relation "architectures" already exists

I suspect that our upgrade test is using a version older than 2019 and that's causing it. We can show a better error in that case.

This is actually always going to fail on upgrade - the new migration will execute, trying to create the already existing tables. Either we need each table to only be created if it exists, or we need to make the migration into noop if all the previous migrations have executed already (probably better option).

@ekohl
Copy link
Member Author

ekohl commented Sep 6, 2020

No, looks like migrations with a version older than the current are not executed. I tried this locally.

@ekohl
Copy link
Member Author

ekohl commented Dec 2, 2020

@tbrisker I won't have time to pick this up and follow through till the end. I'd like to ask you for help in getting this done before Foreman 3.0. Then we can adopt a policy where you must be on 2.0 to upgrade to 3.0.

@ekohl
Copy link
Member Author

ekohl commented Aug 31, 2021

This needs some serious work and we won't get to this for 3.0 anymore. I'm closing this for now but I'd still love to see this at some point.

@ekohl ekohl closed this Aug 31, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants