This project is archived and is in readonly mode.
[PATCH] Improve DB index handling
Reported by Matt Jones | November 2nd, 2009 @ 05:27 AM | in 2.3.6
This patch is a rollup of several related ideas first brought up on rails-core. Here's the details:
-
defines a set of accessors for the various limits of the database engine (identifier length, etc).
-
verifies that index names are within the limits; otherwise, a migration on a DB without transactional DDL can leave the system "stuck", unable to migrate forward or back.
-
hardens add_index and remove_index against duplicate/missing indexes
-
adds a new operation, rename_index. This is primarily a housekeeping operation, but necessary as the default name of an index includes the table name. Thus, renaming a table leaves it with incorrectly named indexes.
Only tested against MySQL and SQLite3; I'm still trying to get a working PG install up and running.
A few notes/questions on the implementation:
-
The indexes() method is commented out of AbstractAdapter; are there DBs that don't support this? There's a fair bit of effort in the code/tests devoted to working around this.
-
the changes made to add_index and remove_index only work if the specific adapter doesn't override them; is there a better way to handle this?
-
I'm not certain the calls to @logger.warn are appropriate; ideally, it would be possible to output the warnings back to the terminal running the migration.
-
Should there be an option to go back to the old behavior (maybe a :safe flag, or (reversed) :force)?
-
The default values in DatabaseLimits are best guesses; should they be different? Not all of them are currently used, but it seemed logical to avoid nagging adapter maintainers to add options one at a time.
Attached patch is against 2-3-stable; once it's in a final format, I'll prepare one against master.
Comments and changes to this ticket
-
Jeremy Kemper November 3rd, 2009 @ 02:40 AM
- State changed from new to open
- Assigned user set to Jeremy Kemper
Very nice!
-
Not sure about #indexes
-
Yes, the adapters should call super or they should have an implementation-specific method to override
-
logger.warn seems appropriate and in line with everything else, which logs
-
no need to add extra config since the old behavior's never needed
-
default database limits are good but should probably be modeled as Hash rather than as a mixin of instance methods
-
Matt Jones November 3rd, 2009 @ 04:06 PM
At least in the existing code, calling super wouldn't make sense for the adapters; the AbstractAdapter method would generate invalid SQL for Postgres and SQLite (they namespace indexes to the DB, not the table, and therefore don't support 'ON table_foo' in the DROP INDEX statement). Maybe it would make sense to abstract out the last line of add_index/remove_index (the actual SQL) so that adapters could override that while leaving the checks in place?
A Hash would make sense most of the time, but I based the current method-driven implementation on the one existing method (table_alias_length), which does actually execute code for some adapters (see the override in postgresql_adapter.rb, line 294-296, for instance).
-
Jeremy Kemper April 23rd, 2010 @ 06:03 PM
- Milestone set to 2.3.6
Hey Matt, interested in getting this on master and up-to-date for 2.3.6 release?
-
Matt Jones April 26th, 2010 @ 12:45 AM
Absolutely - we could really use this over in Hobo-land (generating too-long indexes automatically really confuses users). I've got some ideas for cleaning this up as well; add_index could logically be split into two parts:
-
add_index itself; this does all the length / existence checking
-
add_index!, which skips all the chrome and just does the SQL. This should simplify the changes to, for instance, the SQLite3 adapter; that adapter could keep the standard add_index from AbstractAdapter and merely override add_index!.
-
-
Rizwan Reza May 16th, 2010 @ 02:41 AM
- Tag changed from 2.3-stable, enhancement, index, patch to 2.3-stable, bugmash, enhancement, index, patch
-
Étienne Barrié May 16th, 2010 @ 06:00 PM
Brought the patch up to date to 2.3.6.
Added a test for the index length check.
Factored out SQL generation from remove_index into remove_index! that I actually needed in a schema related test.
Haven't had the need for an add_index!, I didn't see any Adapter overriding add_index. However, I had to do some ugly stuff to get the add_index tests to work since they overrideexecute
to test for SQL queries and we now need it to actually execute queries to get the existing indexes. I worked around this by always returning false forindex_exists?
during these tests.Only tested against mysql, sqlite3 and pg and for 2.3.6.
-
Matt Jones May 16th, 2010 @ 10:27 PM
Looks good - I was confused about the add_index! thing, as I was remembering the remove_index case as well as the Oracle adapter (which overrides add_index to shorten them, as Oracle's maximum identifier is only 30 chars).
-
Étienne Barrié May 18th, 2010 @ 06:10 PM
Ok so I read #3252 and #3508 which made me realize PostgreSQL's index names length limit is actually 63, not 64, so I went back and made my test better by using
index_name_length
(to make it work through all adapters) and by checking for both working and failing creations of index. MySQL's limit is 64 while PostgreSQL's is 63 so I added that to PostgreSQL's adapter. I tried to find a limit for sqlite3 and it started to fail around 2**22 (that's a long index name…). However, sqlite will use 64 as per the defaults.In #3508 the idea of truncating indexes is entertained and I'm still unsure if this isn't a better idea. With this patch, at least your migration won't error out but you could end up with a production deployment missing indexes.
Also, the patch doesn't apply to master but the resulting commit can be successfully cherry-picked from 2-3-stable.
Tested against sqlite3, mysql and postgresql on 2-3-stable and master.
-
Repository May 18th, 2010 @ 07:04 PM
- State changed from open to committed
(from [99bcce7ec1e7f59db3e6f1e1d3cd02e15eb59602]) make add_index and remove_index more resilient; new rename_index method; track database limits
[#3452 state:committed]
Signed-off-by: Jeremy Kemper jeremy@bitsweat.net
http://github.com/rails/rails/commit/99bcce7ec1e7f59db3e6f1e1d3cd02... -
Repository May 18th, 2010 @ 07:04 PM
(from [3809c80cd55ac2838f050346800889b6f8e041ef]) make add_index and remove_index more resilient; new rename_index method; track database limits
[#3452 state:committed]
Signed-off-by: Jeremy Kemper jeremy@bitsweat.net
http://github.com/rails/rails/commit/3809c80cd55ac2838f050346800889... -
Raimonds Simanovskis May 27th, 2010 @ 10:38 PM
Why index with too long name is just skipped without exception? I think it is bad as in this case it may be unnoticed during running migrations and missing indexes could cause big performance issues.
-
Paul Eipper February 4th, 2011 @ 08:06 PM
- Importance changed from to
Is this fixed? In which versions of Rails? I had this affect me when installing Redmine on Dreamhost, which uses Rails 3.0.3
Create your profile
Help contribute to this project by taking a few moments to create your personal profile. Create your profile »
<h2 style="font-size: 14px">Tickets have moved to Github</h2>
The new ticket tracker is available at <a href="https://github.com/rails/rails/issues">https://github.com/rails/rails/issues</a>
People watching this ticket
Attachments
Referenced by
- 3508 Limit MySQL index name length There is a patch in #3452 which was mentioned in #3252 li...
- 3252 add_index on postgresql with long index names: migrations fail with Input string is longer than NAMEDATALEN-1 (63) There's a more general issue where the AR adapters don't ...
- 3452 [PATCH] Improve DB index handling [#3452 state:committed]
- 3452 [PATCH] Improve DB index handling [#3452 state:committed]
- 4219 Add column and index query methods to ActiveRecord::Schema I've had to rename the index_exists? method from #3452 as...
- 4219 Add column and index query methods to ActiveRecord::Schema I've had to rename the index_exists? method from #3452 as...
- 6187 PostgreSQL and Rails 3.0.3 migrations fail with index name length > 64 chars I understand that PG cuts it off, but doesn't it make mor...