[PATCH] Improve DB index handling

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

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

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.

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

Ok so I read #3252 add_index on postgresql with long index names: migrations fail with Input str... and #3508 Limit MySQL index name length 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 Limit MySQL index name length 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.

(from [3809c80cd55ac2838f050346800889b6f8e041ef]) make add_index and remove_index more resilient; new rename_index method; track database limits

[#3452 [PATCH] Improve DB index handling state:committed]

Signed-off-by: Jeremy Kemper jeremy@bitsweat.net
http://github.com/rails/rails/commit/3809c80cd55ac2838f050346800889...

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.