Permalink
Browse files

Generate add_index by default when giving type belongs_to or references

  • Loading branch information...
spastorino committed Dec 16, 2010
1 parent f176b25 commit 3b9120fa52b76fb8591fe1d0db85d1a940e867d0
View
@@ -1,5 +1,27 @@
*Rails 3.1.0 (unreleased)*
+* When a model is generated add_index is added by default for belongs_to or references columns
+
+ rails g model post user:belongs_to will generate the following:
+
+ class CreatePosts < ActiveRecord::Migration
+ def up
+ create_table :posts do |t|
+ t.belongs_to :user
+
+ t.timestamps
+ end
+
+ add_index :posts, :user_id
+ end
+
+ def down
+ drop_table :posts
+ end
+ end
+
+ [Santiago Pastorino]
+
* Setting the id of a belongs_to object will update the reference to the
object. [#2989 state:resolved]
@@ -8,6 +8,10 @@ def up
t.timestamps
<% end -%>
end
+
+<% attributes.select {|attr| attr.reference? }.each do |attribute| -%>
+ add_index :<%= table_name %>, :<%= attribute.name %>_id
+<% end -%>
end
def down
@@ -811,6 +811,8 @@ class CreateComments < ActiveRecord::Migration
t.timestamps
end
+
+ add_index :comments, :post_id
end
def self.down
@@ -819,7 +821,7 @@ class CreateComments < ActiveRecord::Migration
end
</ruby>
-The +t.references+ line sets up a foreign key column for the association between the two models. Go ahead and run the migration:
+The +t.references+ line sets up a foreign key column for the association between the two models. And the +add_index+ line sets up an index for this association column. Go ahead and run the migration:
<shell>
$ rake db:migrate
@@ -3,7 +3,7 @@
class ScaffoldGeneratorTest < Rails::Generators::TestCase
include GeneratorsTestHelper
- arguments %w(product_line title:string price:integer)
+ arguments %w(product_line title:string product:belongs_to user:references)
setup :copy_routes
@@ -14,7 +14,10 @@ def test_scaffold_on_invoke
assert_file "app/models/product_line.rb", /class ProductLine < ActiveRecord::Base/
assert_file "test/unit/product_line_test.rb", /class ProductLineTest < ActiveSupport::TestCase/
assert_file "test/fixtures/product_lines.yml"
- assert_migration "db/migrate/create_product_lines.rb"
+ assert_migration "db/migrate/create_product_lines.rb", /belongs_to :product/
+ assert_migration "db/migrate/create_product_lines.rb", /add_index :product_lines, :product_id/
+ assert_migration "db/migrate/create_product_lines.rb", /references :user/
+ assert_migration "db/migrate/create_product_lines.rb", /add_index :product_lines, :user_id/
# Route
assert_file "config/routes.rb" do |route|

12 comments on commit 3b9120f

Mate, I was reviewing a legacy db today where some basic indexes were missing, and I wondered whether this exact feature should be added to Rails itself. Nice coincidence, thanks =).

Contributor

tilsammans replied Dec 17, 2010

Indexes are always great, but why not go full monty and generate actual foreign keys?

Cross-platform implementation here: https://github.com/matthuhiggins/foreigner

Wondering if the indexes in this commit will interfere with those kind of plugins...

Member

amatsuda replied Dec 17, 2010

Is there any benchmark basis for this?

AFAIK, I don't think this index on fk is useful.
While fk columns are used in a join statement, modern RDBMS automatically uses the matching table's pk.
Therefore, the index on fk has no effect, and is actually just a waste of DB resource.

AR will not generate any SQL statements that would use this fk.
The index on fk column will only be advantageous when you're directly executing SQL statements like
'select * from comments where post_id = 3;'
without joining the posts table, but IMO this should be discouraged when you are using an ORM like AR.

Therefore, I think the fk shouldn't be generated by default and this change should be reverted.

Contributor

mlomnicki replied Dec 17, 2010

@amatsuda: indices on FKs are definately useful. You probably mean that some RDBMS create index on foreign key by default. MySQL does it but Postgres and Oracle don't. When joining tables you usually need both index on PK and on FK.

But by default ActiveRecord doesn't use foreign keys at all so from database point of view t.references is just an integer and nothing more. It's not a foreign key which makes huge difference. You would use foreginer mentioned above or https://github.com/mlomnicki/automatic_foreign_key or sth similiar to have real foreign keys.

I'm not convicted to this patch for 2 reasons:

  • for small tables it's cheaper to do full table scan than use indices to perform join. So one would decide on his own when to add an index
  • when using MySQL and foreign keys it will fail. MySQL will create one index on FK and ActiveRecord will try to add another one

I think that kind of behaviour should be configured
active_record.add_index_on_references = true/false or something like that

Owner

fxn replied Dec 17, 2010

Not sure I follow, which query do you expect @post.comments to fire? Even eager loading will fire queries like those because long time ago it was refactored to fire a query of that type by included association generally speaking.

Contributor

josevalim replied Dec 17, 2010

@mlomnicki, about the second reason, MySQL will do the proper thing. You won't have two indexes for the FK.

Contributor

mlomnicki replied Dec 17, 2010

@josevalim
rails g model post user:belongs_to

create_table :posts do |t|
    t.references :user # let's say FK is created here. MySQL adds index by default
end
add_index :posts, :user_id # second index added for the same column 

So as far as I understand you will have 2 indices on user_id

Contributor

stephencelis replied Dec 17, 2010

@mlomnicki

Gems that add FK functionality to AR should probably override this behavior, and when a developer executes FK SQL statements themselves, they are opening their migrations anyway. I think this patch is helpful, personally, as I almost always add this index manually. The overhead an index costs small tables is a much smaller price to pay than forgetting the index in the first place.

Contributor

stevestmartin replied Dec 17, 2010

ActiveRecord makes 2 calls when including associations instead of a join, the second definitely uses the foreign_key field, we recently had an issue where an index was forgotten on a fk and it cause a 10 fold increase in query time.

This sort of thing is primarily about convention, by default adding an index is the right thing to do and migrations do not create actual foreign key contraints in the db, you will likely have to modify your migration file to begin with including the above mentioned plugin as the only RDBMS that would have an index by default would be MySQL and thats only if your using the InnoDB storage engine. You should still be setting an index on every fk in your migration anyways because auto_index is false by default, which brings us to doubling up indexes when using InnoDB, even the plugin happily creates its own index for the foreign key when these are set causing the same mentioned issue.

Contributor

mlomnicki replied Dec 17, 2010

@stephencelis I totally agree this patch introduces good convention. I just think that in some edge cases it could cause some serious problems. This is why I opt for making this feature configurabl so gems maintainers would simply get rid of that and replace it with own solutions.
For in instance in automatic_foreign_key we already assume that if you want FK you want index as well. Now it's rather hard to track whole migration and check if add_index statement is issued somewhere.

Contributor

josevalim replied Dec 17, 2010

The generator could easily have an option (using class_option) called --indexes that defaults to true. To turn it off, someone would only need to do: config.generators.active_record :indexes => false. Who is willing to try out a patch?

Contributor

mlomnicki replied Dec 17, 2010

@josevalim That's a very nice solution. I've issued a pull request #134

Please sign in to comment.