#3452 [PATCH] Improve DB index handling - Ruby on Rails

Type	To find
responsible:me	tickets assigned to you
tagged:"@high"	tickets tagged @high
milestone:next	tickets in the upcoming milestone
state:invalid	tickets with the state invalid
created:"last week"	tickets created last week
sort:number, importance, updated	tickets sorted by #, importance or updated
Combine keywords for powerful searching.
Use advanced searching »

This project is archived and is in readonly mode.

#3452 ✓committed

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

better-index-support.patch 10.2 KB

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”
[bulk edit]
You flagged this item as spam.
É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 override execute 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 for index_exists? during these tests.

Only tested against mysql, sqlite3 and pg and for 2.3.6.
- better-indexes.patch 12.8 KB
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).
You flagged this item as spam.
É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.
- better-indexes.patch 13.2 KB
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...
You flagged this item as spam.
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.
You flagged this item as spam.
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