Automatically create indexes for references/belongs_to statements in migrations. #5262

joshuap · 2012-03-03T22:26:10Z

My attempt at solving an annoyance mentioned a couple weeks ago by @tenderlove, where using "references" or "belongs_to" when creating/changing a table in a migration does not create a corresponding index, even though in most cases it is needed.

One consideration is that most developers will have already specified indexes for these columns in their existing migrations... I'm not sure if that will be a problem or not moving forward. I'm happy to make additional changes if need be.

jeremy · 2012-03-03T22:32:50Z

I think it's healthy to explicitly add indexes, but I think this should be the default. Nice patch; needs a changelog bump and some guide/docs updates!

joshuap · 2012-03-04T00:54:50Z

Thanks! I'll get right on that :)

josevalim · 2012-03-04T09:36:26Z

activerecord/lib/active_record/connection_adapters/abstract/schema_definitions.rb

        args.each do |col|
          column("#{col}_id", :integer, options)
          column("#{col}_type", :string, polymorphic.is_a?(Hash) ? polymorphic : options) unless polymorphic.nil?
+          index("#{col}_id", index_options.is_a?(Hash) ? index_options : nil) unless index_options == false


Maybe we should add an index including both #{col}_id and #{col}_type? I know It would work fine in MySQL because it can uses just part of the index, unsure how it would play with other databases.

I thought about that - I can try it out this week and see how it works. You're talking about something like this?

index(polymorphic ? %w(id type).map { |t| "#{col}_#{t}" } : "#{col}_id", index_options.is_a?(Hash) ? index_options : nil)

This seems to work. I tested mysql, mysql2, postgres and sqlite as those are what I have. I would imagine that this shouldn't introduce any major problems with the others since it's using existing rails behavior to add the indexes. Can anyone confirm?

josevalim · 2012-03-04T09:41:03Z

Nice patch! There are a couple things we need to have in mind though:

If we are going to make this approach the default, it needs to be turned on via a flag, otherwise it will break backwards compatibility;
When you use a generator to generate a migration, we already include indexes for each belongs_to/references. So if we are merging this patch, we need to reevaluate our current migration generator.

My $0.02: this patch adds a simpler syntax to add indexes which is welcome but I wouldn't make this the default. I would simply improve the current generators.

/cc @jeremy @tenderlove

joshuap · 2012-03-05T19:16:53Z

I'll hold off on updating the docs and changelog until we decide which option to go with. I tend to agree with @josevalim; the generator could be modified to add t.references :user, :index => true rather than including the extra add_index statement. I guess some may still end up forgetting to use :index => true when manually using references; that might be fine, or it could give a warning when adding an association without the :index option explicitly defined.

tenderlove · 2012-03-12T18:39:11Z

@joshuap I think explicitly updating the generators and emitting a warning if there is no index is the right way to go.

We may also want a flag to shut off the warning? I'm not sure. I don't know why anyone would want a references and not have an index. But I suppose the agile thing to do would be to cross that bridge when we get there.

josevalim · 2012-03-12T18:47:04Z

Unfortunately we cannot add a warning as this would mean we would print a warning for all old migrations. Unless we create a new method.

tenderlove · 2012-03-12T18:58:07Z

@josevalim good point. Nevermind about the warning then. Although.... Migrations should have a timestamp, we could only display the warning if the migration is new enough. ;-)

josevalim · 2012-03-12T19:01:45Z

Deny. I could upgrade my app one year after Rails 4 was released :P

tenderlove · 2012-03-12T19:02:35Z

@josevalim sure, just make test for one year plus one day.

joshuap · 2012-03-12T19:02:41Z

I was thinking that to shut off the warning, you could add :index => false to explicitly state that you don't want it; that doesn't solve the legacy migrations issue, however. Sounds like this might be a moot point :)

tenderlove · 2012-03-12T19:03:38Z

Ya, I guess we should do "update generators, but no warning".

joshuap · 2012-03-12T19:04:55Z

Great, I'll try to get that knocked out later this week.

joshuap · 2012-03-21T19:28:40Z

Indexes must now be explicitly defined using the :index option, model/scaffold generators now use the new syntax, and I've updated the guides/doc/changelog. Tests are all passing for MySQL, Sqlite, and Postgres.

joshuap · 2012-04-14T15:06:40Z

@josevalim @tenderlove ready for merge (or feedback)

jeremy · 2012-04-14T19:31:45Z

activerecord/lib/active_record/connection_adapters/abstract/schema_definitions.rb

      #
      #   create_table :taggings do |t|
      #     t.integer :tag_id, :tagger_id, :taggable_id
      #     t.string  :tagger_type
      #     t.string  :taggable_type, :default => 'Photo'
      #   end
+      #   add_index :taggings, :tag_id, :name => 'index_taggings_by_tag_id'


index_taggings_on_tag_id

Yeah, my original logic behind that was I wanted to show that it was setting something other than the default, but I wasn't nearly explicit enough. Updated.

jeremy · 2012-04-14T20:31:39Z

Looks good! Could you rebase against master?

…migrations.

joshuap · 2012-04-14T21:29:11Z

@jeremy done!

Automatically create indexes for references/belongs_to statements in migrations.

vijaydev · 2012-04-15T16:14:06Z

railties/lib/rails/generators/generated_attribute.rb

@@ -21,6 +21,10 @@ def parse(column_definition)
          has_index, type = type, nil if INDEX_OPTIONS.include?(type)

          type, attr_options = *parse_type_and_options(type)
+
+          references_index = (type.in?(%w(references belongs_to)) and UNIQ_INDEX_OPTIONS.include?(has_index) ? {:unique => true} : true)


@joshuap We don't use and . Can you change it please?

Changed in 3b0ffb1

Thanks - sorry I didn't get to this in time, busy with client work :)

vijay1189 · 2015-05-19T12:02:02Z

can we use below syntax for indexing?
column_name, index: true

josevalim reviewed Mar 4, 2012
View reviewed changes

jeremy reviewed Apr 14, 2012
View reviewed changes

Automatically create indexes for references/belongs_to statements in …

ca0af82

…migrations.

jeremy added a commit that referenced this pull request Apr 14, 2012

Merge pull request #5262 from joshuap/references_index

6a054b0

Automatically create indexes for references/belongs_to statements in migrations.

jeremy merged commit 6a054b0 into rails:master Apr 14, 2012

vijaydev reviewed Apr 15, 2012
View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Automatically create indexes for references/belongs_to statements in migrations. #5262

Automatically create indexes for references/belongs_to statements in migrations. #5262

joshuap commented Mar 3, 2012

jeremy commented Mar 3, 2012

joshuap commented Mar 4, 2012

josevalim Mar 4, 2012

joshuap Mar 5, 2012

joshuap Mar 6, 2012

josevalim commented Mar 4, 2012

joshuap commented Mar 5, 2012

tenderlove commented Mar 12, 2012

josevalim commented Mar 12, 2012

tenderlove commented Mar 12, 2012

josevalim commented Mar 12, 2012

tenderlove commented Mar 12, 2012

joshuap commented Mar 12, 2012

tenderlove commented Mar 12, 2012

joshuap commented Mar 12, 2012

joshuap commented Mar 21, 2012

joshuap commented Apr 14, 2012

jeremy Apr 14, 2012

joshuap Apr 14, 2012

jeremy commented Apr 14, 2012

joshuap commented Apr 14, 2012

vijaydev Apr 15, 2012

vijaydev Apr 17, 2012

joshuap Apr 17, 2012

vijay1189 commented May 19, 2015

Automatically create indexes for references/belongs_to statements in migrations. #5262

Automatically create indexes for references/belongs_to statements in migrations. #5262

Conversation

joshuap commented Mar 3, 2012

jeremy commented Mar 3, 2012

joshuap commented Mar 4, 2012

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

josevalim commented Mar 4, 2012

joshuap commented Mar 5, 2012

tenderlove commented Mar 12, 2012

josevalim commented Mar 12, 2012

tenderlove commented Mar 12, 2012

josevalim commented Mar 12, 2012

tenderlove commented Mar 12, 2012

joshuap commented Mar 12, 2012

tenderlove commented Mar 12, 2012

joshuap commented Mar 12, 2012

joshuap commented Mar 21, 2012

joshuap commented Apr 14, 2012

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeremy commented Apr 14, 2012

joshuap commented Apr 14, 2012

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vijay1189 commented May 19, 2015