Drop invalid index in safe_add_index

[dgsuarez]

Hi! Saw this TODO in the code, we've been running something similar in our codebase, so better contribute it back :)

Allow safe_add_index to be seamlessly retried.

When the add_index...concurrently fails, the index is left in Postgres with INVALID status.

The new code drops the index if it's found and invalid. With this, if the migration fails because of a lock timeout for example, on retry it can work. Without it the retries always fail until you manually drop the index.

[dgsuarez]

Small update to fix the errors in pre-6.1 rails versions

[ermolaev]

@ankane please show tis PR

when enable Lock Timeout Retries, possible situation:

-- add_index(:users, :email_last_open_at, {:algorithm=>:concurrently, :using=>:brin})
-- Lock timeout. Retrying in 10 seconds...
-- add_index(:users, :email_last_open_at, {:algorithm=>:concurrently, :using=>:brin})
rake aborted!
StandardError: An error has occurred, all later migrations canceled:
 
PG::DuplicateTable: ERROR:  relation "index_users_on_email_last_open_at" already exists

SELECT * FROM pg_class, pg_index WHERE pg_index.indisvalid = false AND pg_index.indexrelid = pg_class.oid;

oid	7411974
relname	index_users_on_email_last_open_at

We must run index deletion before re-creation

remove_index :users, :email_last_open_at, if_exists: true

[chaadow]

@ankane For my case, this pull request is helpful because when indices are INVALID rails does not add the CREATE UNIQUE INDEX inside db/structure.sql

which in CI would not generate a proper database and make the tests fail ( and in a team of many developers, this creates issues where structure.sql differ a lot from one developer ( who does not have an invaild index ) to another who has

the solution is to manually remove the index from psql or in some other way, but it would be great to have this PR merged to automatically handle the case

Thanks!

[ankane]

Hi @dgsuarez, thanks for the PR, and sorry for the delay. Merged a version of this in the commit above (requires Active Record 7.1+, but may decide to support earlier versions before the release).

[ankane]

Spent some more time on this and think it's better to call REINDEX INDEX CONCURRENTLY (when possible), since it only requires acquiring a lock once.

[ankane]

It looks like REINDEX INDEX CONCURRENTLY leaves behind another invalid index if it fails, so reverted back to DROP INDEX CONCURRENTLY.

Also, this seems like a good option to have independent of safe_by_default, so added experimental support for:

StrongMigrations.remove_invalid_indexes = true