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

[WIP] add index on STI type columns #9516

Closed

Conversation

durandom
Copy link
Member

this adds indexes to all tables that have a type column which is used for STI

because I'm too lazy to do this by hand I created a small script: https://gist.github.com/durandom/b23d7e12722b2ea4f6aee20178ee9cb2

  • checks if there is an index on type already (only miq_cim_instances and miq_workers)
  • greps the codebase for classes inheriting from the base class
  • greps the base class if NewWithTypeSti mixin got mixed in
  • otherwise gives a note

Unfortunately neither the rails api docs nor guides suggest adding an index for that column. And I havent found any recent stuff on the internet. But that does not mean that it does not make sense 😄

@miq-bot add_label performance, sql migration

@kbrock because of performance
@carbonin
@Fryguy because you suggested it :)

@kbrock
Copy link
Member

kbrock commented Jun 29, 2016

just kicking this build. if it fails again, then please rebase. thnx

@durandom durandom force-pushed the add_missing_indexes_to_type_columns branch 2 times, most recently from 2e2ce95 to 3fd6d94 Compare June 30, 2016 09:02
@durandom
Copy link
Member Author

hmm, I dont get, why this fails on travis. Locally its green

@durandom
Copy link
Member Author

with echo 1 > REGION it also fails locally. But I still don't get it :/

@durandom durandom force-pushed the add_missing_indexes_to_type_columns branch from 3fd6d94 to c4846a5 Compare July 4, 2016 09:22
@miq-bot
Copy link
Member

miq-bot commented Jul 4, 2016

Checked commit durandom@c4846a5 with ruby 2.2.4, rubocop 0.37.2, and haml-lint 0.16.1
1 file checked, 6 offenses detected

db/migrate/20160628142706_add_index_on_all_type_columns.rb

@durandom
Copy link
Member Author

durandom commented Jul 5, 2016

making this WIP until I figure out whats causing CI failure

@miq-bot add_label wip

@durandom durandom changed the title add index on STI type columns [wip] add index on STI type columns Jul 5, 2016
@miq-bot miq-bot added the wip label Jul 5, 2016
@kbrock
Copy link
Member

kbrock commented Jul 5, 2016

Are the indexes unique or something dumb like that?

@durandom
Copy link
Member Author

durandom commented Jul 6, 2016

Are the indexes unique or something dumb like that?

nope, btree indexes

@chessbyte chessbyte changed the title [wip] add index on STI type columns [WIP] add index on STI type columns Jul 14, 2016
@Fryguy
Copy link
Member

Fryguy commented Jun 22, 2017

Database migrations have now been moved to the https://github.com/ManageIQ/manageiq-schema repo. Please see http://talk.manageiq.org/t/new-split-repo-manageiq-schema/2478 for instructions on how to transfer your database migrations. If this PR contains only migrations, I will leave it open for a short time during the transition, after which I will close this if it has not been moved over.

@kbrock
Copy link
Member

kbrock commented Jun 29, 2017

wondering if some of these indexes should include type or some other columns?

@juliancheal
Copy link
Member

@Fryguy is this something we still want to work on?

@Fryguy
Copy link
Member

Fryguy commented Nov 27, 2017

@juliancheal yes, I think this is still a good idea, but it needs to be moved to manageiq-schema.

@juliancheal
Copy link
Member

@Fryguy ok cool

juliancheal pushed a commit to juliancheal/manageiq-schema that referenced this pull request Apr 25, 2018
This follows on from pervious work in this PR ManageIQ/manageiq#9516
@juliancheal
Copy link
Member

This should be closed in favour of ManageIQ/manageiq-schema#190

@kbrock kbrock closed this Jun 11, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants