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

Revert "Added missed indexes." #709

Merged
merged 1 commit into from
Mar 21, 2017
Merged

Conversation

fabn
Copy link
Contributor

@fabn fabn commented Dec 20, 2015

Reverts #682

I'm investigating some performance issues and I found out this PR while looking into this repo.

I think this PR should be reverted because it creates some useless indexes and the other created indexes will likely never be used and only increases tag creation times.

By going into details

add_index :taggings, :tag_id

This is already provided by this line as leftmost prefix of an existing index

add_index :taggings, :taggable_id

This is redundant because is a left prefix of the index below and also already provided by this index

add_index :taggings, :taggable_type
add_index :taggings, :context

These two index as is may provide some benefits if one is doing some sort of stats (e.g. tags vs keywords, etc) but I don't think they should be added by default.

add_index :taggings, :tagger_id 

Left prefix of below index

add_index :taggings, [:tagger_id, :tagger_type]

This is the only truly missing index and can be useful if one wants to show user tagged elements.

add_index :taggings, [:taggable_id, :taggable_type, :tagger_id, :context], name: 'taggings_idy'

This is already provided by this migration, it only add additional columns. If this is going to be added then the other should be removed in favor of this.

In conclusion in my opinion #682 as is should be reverted. Maybe a good replacement would only include
a commented index for taggable_type and context explaining that it can be useful only to do some stats and a commented version of add_index :taggings, [:tagger_id, :tagger_type] that explain its usage.

In this way upgrading users will not add them blindly only because it's description says "Add missing indexes".

What do you think?

@CSammy
Copy link

CSammy commented Aug 20, 2016

I have been working on quite large databases for a while now and I agree with @fabn on every point he made. The only thing I'm undecided on is his suggestion to provide deactivated indices with comments on when to activate them.

@denschub
Copy link

/cc @mbleigh @seuros

We (as in the diaspora* proejct) use quite a heavy tag base and these indexes are causing nothing but additional database load by adding indexes that won't get used. What's the state of this PR?

@razum2um
Copy link

Doubled everyone above, besides indexes on single columns like taggable_type and context have low selectivity anyway and only slow things down

@seuros seuros merged commit 5163a75 into mbleigh:master Mar 21, 2017
@seuros
Copy link
Collaborator

seuros commented Mar 21, 2017

Let get a new release out this week.

@denschub
Copy link

Thank you.

SuperTux88 added a commit to SuperTux88/acts-as-taggable-on that referenced this pull request Aug 2, 2017
This is the same as in mbleigh#709, but the file was not removed when merging
this, because it was renamed in the meantime. So remove it again.
tekniklr pushed a commit to tekniklr/acts-as-taggable-on that referenced this pull request Mar 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants