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

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

Merged
merged 1 commit into from Apr 14, 2012

Conversation

Projects
None yet
6 participants
@joshuap
Copy link
Contributor

joshuap commented Mar 3, 2012

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

This comment has been minimized.

Copy link
Member

jeremy commented Mar 3, 2012

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

This comment has been minimized.

Copy link
Contributor

joshuap commented Mar 4, 2012

Thanks! I'll get right on that :)

@josevalim

View changes

activerecord/lib/active_record/connection_adapters/abstract/schema_definitions.rb Outdated
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

This comment has been minimized.

@josevalim

josevalim Mar 4, 2012

Contributor

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.

This comment has been minimized.

@joshuap

joshuap Mar 5, 2012

Contributor

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 comment has been minimized.

@joshuap

joshuap Mar 6, 2012

Contributor

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

This comment has been minimized.

Copy link
Contributor

josevalim commented Mar 4, 2012

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

  1. 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;

  2. 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

This comment has been minimized.

Copy link
Contributor

joshuap commented Mar 5, 2012

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

This comment has been minimized.

Copy link
Member

tenderlove commented Mar 12, 2012

@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

This comment has been minimized.

Copy link
Contributor

josevalim commented Mar 12, 2012

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

This comment has been minimized.

Copy link
Member

tenderlove commented Mar 12, 2012

@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

This comment has been minimized.

Copy link
Contributor

josevalim commented Mar 12, 2012

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

@tenderlove

This comment has been minimized.

Copy link
Member

tenderlove commented Mar 12, 2012

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

@joshuap

This comment has been minimized.

Copy link
Contributor

joshuap commented Mar 12, 2012

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

This comment has been minimized.

Copy link
Member

tenderlove commented Mar 12, 2012

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

@joshuap

This comment has been minimized.

Copy link
Contributor

joshuap commented Mar 12, 2012

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

@joshuap

This comment has been minimized.

Copy link
Contributor

joshuap commented Mar 21, 2012

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

This comment has been minimized.

Copy link
Contributor

joshuap commented Apr 14, 2012

@josevalim @tenderlove ready for merge (or feedback)

@jeremy

View changes

activerecord/lib/active_record/connection_adapters/abstract/schema_definitions.rb Outdated
#
# 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'

This comment has been minimized.

@jeremy

jeremy Apr 14, 2012

Member

index_taggings_on_tag_id

This comment has been minimized.

@joshuap

joshuap Apr 14, 2012

Contributor

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

This comment has been minimized.

Copy link
Member

jeremy commented Apr 14, 2012

Looks good! Could you rebase against master?

@joshuap

This comment has been minimized.

Copy link
Contributor

joshuap commented Apr 14, 2012

@jeremy done!

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

Merge pull request #5262 from joshuap/references_index
Automatically create indexes for references/belongs_to statements in migrations.

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

@@ -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)

This comment has been minimized.

@vijaydev

vijaydev Apr 15, 2012

Member

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

This comment has been minimized.

@vijaydev

vijaydev Apr 17, 2012

Member

Changed in 3b0ffb1

This comment has been minimized.

@joshuap

joshuap Apr 17, 2012

Contributor

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

@vijay1189

This comment has been minimized.

Copy link

vijay1189 commented May 19, 2015

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment