This project is archived and is in readonly mode.

#3452 ✓ committed
Matt Jones

[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

    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

    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

    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

    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

    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é

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

  • Matt Jones

    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é

    É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

    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

    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

    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

    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 »

Tickets have moved to Github

The new ticket tracker is available at https://github.com/rails/rails/issues

Shared Ticket Bins

Referenced by

Pages