Named indicies too long #1993

Closed
wants to merge 1 commit into
from

Projects

None yet
@sethvargo

When creating a migration with a lot of primary keys (or one with a long table):

create_table :really_long_table_name do |t|
  t.integer :another_column_thats_long
  t.integer :this_is_a_column
end

add_index :really_long_table_name, [:another_column_thats_long, :this_is_a_column]

causes SQLite (and some other db packages) to take a fit because Index Names can't be > 64 characters in length. Could you just truncate the index names at 64 characters?

@chuyeow

You can pass a :name option to add_index

add_index :really_long_table_name, [:another_column_thats_long, :this_is_a_column], :name => 'foo_index'
@sethvargo

This is true, but I shouldn't have to manually shorten the index name...

index_name = (whatever method currently makes the method)[0..63]
@chuyeow

Well, I suspect the reason this wasn't done automatically is because different database engines have different length restrictions on index names.

@sethvargo

Yea, I guess so, but Rails already handles different databases for other things already :)

@dmathieu

I like the idea of this.
Do you want to work on it ? If so, please do !
Otherwise, I'd be willing to spend some time on it after the release of 3.1.

@sethvargo

I can try and work on it, but it has been a really long time since I've dug into the Rails source code :)

@bogdan

I think we should shorten only auto generated index name, because auto generating something invalid is not a good idea.

If name is set manually then it should be exception like it is right now.

@sethvargo

Okay. I'll update the patch later today. I definitely agree:

  • exceptions for user-named indices
  • truncation for auto-generated indicies
@sethvargo

Okay, I've fixed the truncation to only apply to auto-named indices and written tests (that pass)

@softwaregravy

I see this is moving along. I want to bring this up as a "real" bug though.

I have an anonymous index (didn't give it a name). I just found out that I'm unable to roll back due to:

== AddHourOfDayToImpressions: reverting ======================================
-- remove_column(:impressions, :hour_of_day)
rake aborted!
An error has occurred, this and all later migrations canceled:

Index name 'temp_index_altered_impressions_on_created_at_and_controller_and_action' on table 'altered_impressions' is too long; the limit is 64 characters

(See full trace by running task with --trace)

this is local on sqlite3

@sethvargo

@softwaregravy - that is exactly what this patch fixes... If you manually name an index add_index :foo, :bar, :name => 'really_long_index_that_is_over_the_limit_of_characters_allowed_by_the_database, it will now through an error. However, if you let Rails auto-assign that index (or allow the "anonymous" index as you've referred to it), this patch tells Rails to auto-truncate that index name. It never should have allowed an index of that length to begin with...

@earnold

I hope this patch gets in - I just ran into this same problem in rails 3.0.9 and Postgres, which has a 63 character limit on indexes

@Fryguy

I've hit a similar issue like this in the past, and truncation actually exacerbated the issue. For example, a migration like

add_index :really_long_table_name, [:another_column_thats_long, :this_is_a_column, :third_column, :fourth_column]
add_index :really_long_table_name, [:another_column_thats_long, :this_is_a_column, :third_column, :different_fourth_column]

produces two indexes with the exact same name when using truncation, which of course raises an exception. Internally, we just used the :name option to get around it, but one idea we came up with was to hash the column names (e.g. md5) to get a short name when the index length was higher than the max allowed length. Even so, that's more of an edge case, and truncation is a good start.

@sethvargo

Yeah, I think that is more of an edge case. md5 the column might be a good idea, but it's nice to have indicies that are named something meaningful.

@Fryguy

Yeah, I agree. That's actually why we went the :name route.

@vijaydev vijaydev commented on an outdated diff Oct 30, 2011
...ord/connection_adapters/abstract/schema_statements.rb
@@ -541,7 +541,9 @@ module ActiveRecord
end
if index_name.length > index_name_length
- raise ArgumentError, "Index name '#{index_name}' on table '#{table_name}' is too long; the limit is #{index_name_length} characters"
+ # automatically truncate index lengths based on the adapter, we should probably issue a warning as well...
@vijaydev
vijaydev Oct 30, 2011

You are issuing a warning. Comment should change a bit?

@vijaydev
Ruby on Rails member

@sethvargo Could you please squash the commits and rebase?

@sethvargo

squashed :). I also fixed the comment as you indicated

@vijaydev
Ruby on Rails member

Is this done? #1993 (comment).

Also, the tests aren't passing.

@sethvargo

Okay. I updated the code and fixed the tests to pass...

@vijaydev vijaydev and 2 others commented on an outdated diff Nov 2, 2011
...ord/connection_adapters/abstract/schema_statements.rb
- "index_#{table_name}_on_#{Array.wrap(options[:column]) * '_and_'}"
- elsif options[:name]
- options[:name]
- else
- raise ArgumentError, "You must specify the index name"
- end
- else
- index_name(table_name, :column => options)
+ def index_name(table_name, options = {}) #:nodoc:
+ return index_name(table_name, :column => options) unless Hash === options # legacy support
+
+ raise ArgumentError, "Index name '#{index_name}' on table '#{table_name}' is too long for the current adapter!" unless options[:column] || options[:name]
+ the_index_name = "index_#{table_name}_on_#{Array.wrap(options[:column]) * '_and_'}" if options[:column]
+ the_index_name = options[:name] if options[:name]
+
+ if the_index_name.length > index_name_length
@vijaydev
vijaydev Nov 2, 2011

this will fail when the_index_name is a symbol

@sethvargo
sethvargo Nov 3, 2011

why? :hello.length #=> 5

@sethvargo
sethvargo Nov 3, 2011

There's also already a test in migration_test#126 that tests for accepting symbols as names which passes.

@Fryguy
Fryguy Nov 3, 2011

Ruby 1.8.7:

> :hello.length
NoMethodError: undefined method 'length' for :hello:Symbol
from (irb):1

@sethvargo
sethvargo Nov 4, 2011

Okay sorry. I forgot about 1.8. I added a conditional test to convert the symbol to a string if RUBY_VERSION < '1.9' and squashed.

@al2o3cr

This is a REALLY bad idea - raising in index_name means the migration will fail, and that the DB will be left in a state requiring manual recovery on DBs that don't have transactional DDL (MySQL and Oracle, at least).

See the original ticket for the index length limit stuff for more details:
https://rails.lighthouseapp.com/projects/8994/tickets/3452

Code-wise, it's very difficult to follow exactly what's going on in there with all of the 'if options[:name]' etc. Perhaps it would be cleaner to split the logic for "check the given name" apart from "compute and then check the name"?

@softwaregravy

The current situation is not much different, and, I'd argue, probably worse. You currently can't remove, rename, or 'alter' impacted sequences -- it's a one way migration, and one you might not catch until it already goes to prod.

If you've already migrated a migration with this issue, the only way to fix it is to manually log into the database to manually undo the index then remove/change the migration(file) that added the 'too long index' (and then do you remove the migration from the rollback sequence?).

With the fix, theoretically, this should be raised in a development database migration without messing up prod.

@al2o3cr

Bleh - nevermind, there's already other code that blows up migrations and b0rks MySQL so some more won't hurt. :)

55d0d57

@sethvargo

It already throws exceptions... This just auto-truncates the names unless the user specifies one (in which case it does throw an exception)...

@sethvargo

What are the chances of getting this into v3.2?

@isaacsanders

First, @al2o3cr, I love that you are in this :D

Second, is this still an issue? Any thoughts on this?

@softwaregravy

I'd love to get this or something similar in. I'm not running on edge so I can't be sure, but to my knowledge, this has not been fixed.

@isaacsanders

@sethvargo Can you rebase off of master to make it easier to merge in if core decides to do so?

@sethvargo

Well, I'm really confused. I tried to rebase, but the test file has completely changed. Can someone from the rails core point me in the direction as to where index tests went? Things are very different now, and I can't seem to find where to put my tests. The diff between the files is like 90% change.

@carlosantoniodasilva
Ruby on Rails member

I'm a bit afraid of automatically truncating index names due to what @Fryguy has said: it makes things easier for two distinct indexes to conflict, and this will raise more confusion than just raising an exception when the name gets too long, being it automatically created or manually added. This will force the user to give the index a proper :name.

Given Rails already raises an exception for long indexes, I'd rather leave it as is.

@softwaregravy

@carlosantoniodasilva

this will raise more confusion than just raising an exception when the name gets too long

I disagree. As a user who hit this on unnamed indexes (auto-named), it was incredibly confusing because at the time I didn't know how Rails named indexes. Because of this we now always supply names to our indexes so we don't get in this mess again. Shame on me for having wordy table and column names.

This will force the user to give the index a proper :name.

the alternative is that Rails chooses a bad one that may make your life more difficult down the road

IMHO, the name (manual or auto-generated) should be validated as 5 characters less than the actual max. This will prevent rails from throwing up when certain migrations would cause them to go over the limit by prefixing "temp_".

Having an index which is 59-64 characters long is an error, it's just a matter of how and when you hit it. This argument put forth on this thread is that it is better to catch the error when trying to create the index in the first place than wait for it to blow up later.

@carlosantoniodasilva
Ruby on Rails member

I disagree. As a user who hit this on unnamed indexes (auto-named), it was incredibly confusing because at the time I didn't know how Rails named indexes.

Not sure I was clear enough, I believe that having two indexes being automatically truncated with the same name may be confusing for us developers when catching such issue.

This will prevent rails from throwing up when certain migrations would cause them to go over the limit by prefixing "temp_".

I don't see Rails prefixing things with temp_.

Having an index which is 59-64 characters long is an error, it's just a matter of how and when you hit it. This argument put forth on this thread is that it is better to catch the error when trying to create the index in the first place than wait for it to blow up later.

Exactly, the idea is that Rails will raise in case the index name gets longer than that, to avoid possible problems later. This should happen during migration time, not later on. So, when you add an index, and run your migration, it should blow up with an exception saying the name is too long, no matter if it was automatically generated or manually added. If it was manually, you just go there and change. If it was automatically, you have to add a manual :name. That's my thought, of course there may be other approaches, but I feel ok with that.

@softwaregravy

RE truncation
seems we're having 2 conversations: 1) is it really a problem? 2) is the fix what we should be doing?
I am trying to assert that it is really a problem, and that we shouldn't leave what's there. I wasn't specifically trying to defend the solution of truncation.

I don't see Rails prefixing things with temp_.

I'll have to go look. Maybe it's postgres / mysql itself that does this? Regardless, I hit this problem not when creating an index but much later when changing the table and the index got auto-renamed to temp_[previous name here]. So we need index names to be shorter than any auto-renaming done under the hood.

@carlosantoniodasilva
Ruby on Rails member

@softwaregravy agree with you. We definitely need to check if the current implementation is really a problem or not, and in my opinion the idea of raising when you run the migration is ok.

I'm not aware of an auto-rename feature in these databases, so if you want/can take a look it'd be great. Thanks!

@frodsan

@carlosantoniodasilva any news on this? Seems like a feature request.

@softwaregravy

@carlosantoniodasilva So, I'm sorry this took so long. Here's an example reproducing the error I was describing. https://github.com/softwaregravy/RailsIndexErrorExample

In my experience, this comes up when you do not specifically name your indices, but let rails name them. When you do a compound index over 2-3 columns, it gets much more likely to get to the 59 character limit.

@danielfone

I ran into this problem just days ago (and coincidentally, codetriage.com sent me to this issue).

I believe this boils down to the belief that rails shouldn't auto-generate index names that are longer than the allowed length for the adapter. Whether or not this belief is shared, I think the behaviour this pull request introduces is dangerous for the reason outlined above by @Fryguy and reiterated by @carlosantoniodasilva.

Perhaps we close this PR and alternative suggestions can be raised in a separate issue?

For reference, the current behaviour is:

vagrant@rails-dev-box:/vagrant/index_test$ bundle exec rails generate scaffold AReallyReallyReallyLongModelName another_long_model_name:references
      invoke  active_record
      create    db/migrate/20121122104817_create_a_really_really_really_long_model_names.rb
      [...]

vagrant@rails-dev-box:/vagrant/index_test$ bundle exec rake db:migrate
==  CreateAReallyReallyReallyLongModelNames: migrating ========================
-- create_table(:a_really_really_really_long_model_names)
rake aborted!
An error has occurred, this and all later migrations canceled:

Index name 'index_a_really_really_really_long_model_names_on_another_long_model_name_id' on table 'a_really_really_really_long_model_names' is too long; the limit is 64 characters
[...]
/vagrant/rails/activerecord/lib/active_record/railties/databases.rake:48:in `block (2 levels) in <top (required)>'
Tasks: TOP => db:migrate
(See full trace by running task with --trace)
@sethvargo sethvargo closed this Jan 22, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment