Closed NickLaMuro closed 2 years ago
So I spent the better part of today looking into how we could fix the schema (mostly trying to understand the ActiveRecord
internals, and what how we have implemented the migrations around metrics
. In short, I have a POC branch for how we could start to approach this:
https://github.com/ManageIQ/manageiq-schema/compare/master...NickLaMuro:fix-db-schema
As mentioned in the commit, this doesn't really create a valid schema, just allows the generated schema to run with rake db:schema:load
. That said, this is a pattern lifted from projects that we probably could leverage (or at least use a inspiration):
(Note: The latter currently isn't heavily maintained at the moment)
But this leaves needing support:
metric*
table "id" column CONSTRAINT
s (ALTER TABLE
calls currently)
metric*
table inheritance (ALTER TABLE
calls currently)
metric*
TRIGGER
s (hair_trigger
might be able to support these)
metrics
DB views (scenic
should be able to support these)
I don't think it is worth me investing to much more time into this idea without some input and buy in from others, but scenic
seems to have a solid pattern for this, where they prepend
the overrides to the ActiveRecord::SchemaDumper
:
to also include their create_view
definitions:
https://github.com/scenic-views/scenic/blob/main/lib/scenic/schema_dumper.rb#L6-L20 https://github.com/scenic-views/scenic/blob/main/lib/scenic/view.rb#L43-L52
Which is pretty much what I followed for my branch for the dumper. For the others, some additions probably will be need to the adapter like is done in scenic
for create_view
:
(and friends: drop_view
, etc.)
When we collapse migrations:
This works because in the older version of the code, the migrations are not collapsed yet.
Do we have a version of code that is from long time ago that we feel is reasonable to ask to upgrade to an interim version first
As for the invalid schema, I think we just use the code that did the migration in the first place. this is non trivial in code base like ours.
agree: Sure wish schema:load worked If I remember correctly, we also have issues because the schema is alphabetical, and the child tables are before the parent table
If I remember correctly, we also have issues because the schema is alphabetical, and the child tables are before the parent table
No, I think it is more that we do a lot of custom stuff in these two migrations (one being the "collapse" one):
https://github.com/ManageIQ/manageiq-schema/blob/62d91b62/db/migrate/20130923182042_collapsed_initial_migration.rb#L963-L970 https://github.com/ManageIQ/manageiq-schema/blob/62d91b62/db/migrate/20190122213042_use_views_for_metrics.rb
And both of those lean heavily on the custom additions and helper here:
https://github.com/ManageIQ/manageiq-schema/blob/62d91b62/lib/migration_helper.rb
So when the db/schema.rb
is written, it doesn't know how to support those, so those triggers/views/alterations are left on the floor.
Update/Additional info:
When we collapse migrations:
Just to clarify: I realized this after digging heavily into the ActiveRecord
codebase, but db/schema.rb
is also a "collapsed migration", and the ActiveRecord::Schema
actually inherits from ActiveRecord::Migration
(it is a little more complicated then that, but it is mostly the case).
So we already have a baked in way in rails for collapsing migrations, we just don't/can't use it because of lib/migration_helper.rb
(triggers, views, etc.).
There are technical hurdles to consolidating the various migrations. And our result will be similar to our current schema.rb
I think we also need to take into account the user, developer, and support person's experience when upgrading to more recent versions of the product.
The developers and users will need to run this code many times more. (which again is why consolidation is important to do, but also why it is important to not make it too hard for us, users, and support to upgrade versions.
migration | version |
---|---|
1 | a |
2 | a |
3 | b |
4 | c |
5 | c |
6 | d |
7 | e |
8 | f |
versions can have none or many migrations in them. The migration number is actually a date stamp.
migration | version |
---|---|
1 | a |
2 | a |
migration | version |
---|---|
1 | a |
2 | a |
3 | b |
4 | c |
5 | c |
It collapsed migrations from version a, b, and c
migration | version |
---|---|
1'{1-5} | f |
5'{guard} | f |
6 | d |
7 | e |
8 | f |
1' and 5' have the same exact version number as before but are rewritten. 1' contains the original 1-5 2,3,4 no longer exist 5' contains code that checks that the original 4 has already been run. if it has been run then nothing happens, but if it has not then the user is told to first upgrade to c,d, or e before upgrading to this version.
This is setup this way because users perviously running a and b, won't know how to run half of the consolidated migration.
The good news is that versions c,d, and e have the non-consolidated migrations (1-5), and can get the user past the consolidated migration and at a point that the future versions know how to migrate.
We can support all users, but we do inconvenience users running very old versions of the code, namely versions that have been consolidated.
While we do want to minimize the situation, the request to go through an interim versions is reasonable. We have done this in the past as well as other products in the wild.
I had started a collapse initial migration but it had to wait until jansa was released, so I could get a hard commit on which it could be done. It's actually relatively easy this time around.
@NickLaMuro I would say hold off on any specific fixes, because a collapse saves a huge amount of time.
@Fryguy because I apparently like to ignore you, I did throw together a POC PR for handling fixing the db/schema.rb
☝️ 👇 :
https://github.com/ManageIQ/manageiq-schema/pull/504
If nothing else, it can help generate a collapse migration and prevent errors.
Do we have a version where we want the cutoff?
I would really like the old metrics code out of here. Not sure if that meshes with our business goals
Merged #504 - I also think we should do #506
I also think we should do #506
Throwing my hat into the ring as well to say I agree. My proposal above makes it seem like the two approaches above are mutually exclusive, which they are not, so I am +1 for doing both.
@NickLaMuro Is this basically done? We also got https://github.com/ManageIQ/manageiq/pull/21259 and https://github.com/ManageIQ/pg-logical_replication/pull/6 since this was opened which improved performance a lot.
@Fryguy well, we could also update ManageIQ/manageiq
to make use of rake db:schema:load
, which would be even faster. That would really only fix dev setups, where the generated db/schema.rb
already exist.
However, we could also generate the schema.rb
in this repo, then update/enhance
test:vmdb:setup
to use that, and apply a db:migrate
after to catch anything else. We would probably need a process in place for handling the schema.rb
, and probably one of the two would work:
db/schema.rb
in this repo so that it is up to date, so developers don't have to worry about this step.Each have their pros and cons, though I would argue the first is simpler and the devs on the team can just deal with a minor conflict here and there that is easily resolved by just doing rm -rf db/schema.rb
and re-running db:migrate
.
That said, do #504 and #506 #584 now speed up rake test:vmdb:setup
? Yes.
The benchmarks above show it could be quicker, but up to the group on if we want to invest a bit more time getting a bit more of a speed up in CI/dev.
Migrations take a while to run (see benchmarks), and probably could be collapsed to save time at this point.
We have 7ish years of migrations built up that have to run on every build that a
test:vmdb:setup
orevm:db:reset
task is run. This takes a lot of time for devs and build time on CI with no real value in doing that beyond them having them run semi-often in this repo.Benchmarks
Note: Times below don't include
db:seed
, which could also be improved, but not the focus this RFE.rake db:migrate
(before)rake db:shema:load
(after)(Note:
metrics_rollups_*
style tables are commented out for these to properly run, but the performance impact of those extra would be minimal, as most of the 14sec above is booting the Rails env)Proposals
One of the following probably could be done to solve this:
rake db:schema:load
(in other words, fix thedb/schema.rb
file) so it can be run (hard)_collapse_initial_migration.rb
that is up to date (hard?)The advantage to the first is that we can execute the "faster" code pretty much any time after a
bin/update
has been done. So we can get to a state of re:seeding much quicker, not just after a "collapse" has happened. That said, updating how the schema is generated to support this would be probably a significant bit of monkey patching to accomplish, and not a quick turn around.That said, the "collapse" migration is basically a schema, so figuring out how collapse would probably lead to how we could generate the schema. It also might be worth "archiving" migrations after every release so that migrations are "versioned" to some degree.
Testing is also something we probably would want to consider with either approach. The advantage again with the first is that we don't have to be concerned using it in production, since it is only a dev speed up. Making sure we get the collapse correct would probably require some testing interface that runs the full collection of migrations from the beginning, and then tests them from a snapshot onward (if something like this doesn't already exist).
Links