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

Add index on STI type columns #190

Merged

Conversation

juliancheal
Copy link
Member

This follows on from pervious work in this PR ManageIQ/manageiq#9516

I created this as a new migration and not a cherry-pick to give us an updated timestamp.

/cc @Fryguy @kbrock

This follows on from pervious work in this PR ManageIQ/manageiq#9516
@juliancheal
Copy link
Member Author

@miq-bot add_label performance "sql migration" wip

@miq-bot
Copy link
Member

miq-bot commented Apr 25, 2018

@juliancheal Cannot apply the following label because they are not recognized: performance "sql migration" wip

@juliancheal
Copy link
Member Author

@miq-bot assign @Fryguy

@juliancheal juliancheal changed the title Add index on STI type columns [WIP] Add index on STI type columns Apr 25, 2018
@juliancheal
Copy link
Member Author

Looks like there are a few tables that don't exist anymore. Will update PR.

@juliancheal
Copy link
Member Author

@kbrock @Fryguy thoughts?

@Fryguy
Copy link
Member

Fryguy commented Apr 27, 2018

Seems ok to me. @kbrock Thoughts?

@juliancheal juliancheal changed the title [WIP] Add index on STI type columns Add index on STI type columns Apr 30, 2018
@miq-bot miq-bot removed the wip label Apr 30, 2018
@kbrock
Copy link
Member

kbrock commented May 1, 2018

I had thought we may want to make compound indexes out of these instead of just type.
But think merging it should be good enough

@juliancheal
Copy link
Member Author

@kbrock what's a compound index?

@juliancheal
Copy link
Member Author

Ping @Fryguy @kbrock

Copy link
Member

@kbrock kbrock left a comment

Choose a reason for hiding this comment

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

Thanks for circling back Julean

We have a lot of indexes we don't use.
And we have a lot of queries not using indexes

I think just get rid of the event_stream index and :shipit:

add_index :dialog_fields, :type
add_index :ems_clusters, :type
add_index :ems_folders, :type
add_index :event_streams, :type
Copy link
Member

Choose a reason for hiding this comment

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

Lets not add an index to event_streams unless we know it is needed for an index.
This table is BIG

Copy link
Member Author

Choose a reason for hiding this comment

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

Removed event_streams

@juliancheal juliancheal force-pushed the add_missing_indexes_to_type_colums branch from 6fa4519 to ff84d67 Compare June 1, 2018 09:53
@juliancheal
Copy link
Member Author

@Fryguy merge on 🍏 ?

@miq-bot
Copy link
Member

miq-bot commented Jun 1, 2018

Checked commits juliancheal/manageiq-schema@fc898fc~...ff84d67 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
1 file checked, 0 offenses detected
Everything looks fine. ⭐

Copy link
Member

@kbrock kbrock left a comment

Choose a reason for hiding this comment

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

LGTM

@Fryguy Fryguy merged commit 3bee538 into ManageIQ:master Jun 28, 2018
@Fryguy Fryguy added this to the Sprint 89 Ending Jul 2, 2018 milestone Jun 28, 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.

4 participants