Do not return invalid indexes in PostgreSQL

[fatkodima]

When creating indexes with CONCURRENTLY, it is possible to end up with an invalid index if PostgreSQL was unable to do so. Fo example, if we have duplicate records and try to create a unique index, the error will be raised, but also an invalid index in the db will be left.

This can be a problem. For example, someone may try to create a migration like this:

disable_ddl_transaction!

def change
  add_index ...., algorithm: :concurrently
  something_else_that_can_fail_the_migration
end

And to make it bulletproof, add a guard:

disable_ddl_transaction!

def change
  unless index_exists?(...)
    add_index ...., algorithm: :concurrently
  end
  something_else_that_can_fail_the_migration
end

But if that migration failed and needs to be rerun, index_exists? will silently succeed and an invalid index will be left in the db. With this PR, this check will return false and when adding an index - an expected error will be raised.

Another example: a gem active_record_doctor can make incorrect assumptions about healthiness of the db and models based on invalid indexes:

1. https://github.com/gregnavis/active_record_doctor/blob/cbb9183887f18587bffdaf32c247eb511482ffb8/lib/active_record_doctor/detectors/missing_unique_indexes.rb#L62-L68
2. https://github.com/gregnavis/active_record_doctor/blob/cbb9183887f18587bffdaf32c247eb511482ffb8/lib/active_record_doctor/detectors/extraneous_indexes.rb#L39-L52
3. https://github.com/gregnavis/active_record_doctor/blob/cbb9183887f18587bffdaf32c247eb511482ffb8/lib/active_record_doctor/detectors/unindexed_deleted_at.rb#L44-L48

[byroot]

Nice catch. To be honest the more it goes the more I believe migrations should be limited to a single idempotent operation, and that we should enforce that though some API. But that's a much larger discussion.

[ghiculescu]

@fatkodima @byroot can I offer a second opinion on this. IMO this change is not right. I think the contract for index_exists? is that if it returns false, Rails is confident creating an index will work. If we are adding more nuance to that, the docs should be updated.

With this PR, the test will fail if you add this:

diff --git a/activerecord/test/cases/adapters/postgresql/postgresql_adapter_test.rb b/activerecord/test/cases/adapters/postgresql/postgresql_adapter_test.rb
index db97ce8844..d3f9350af7 100644
--- a/activerecord/test/cases/adapters/postgresql/postgresql_adapter_test.rb
+++ b/activerecord/test/cases/adapters/postgresql/postgresql_adapter_test.rb
@@ -331,6 +331,8 @@ def test_invalid_index
           assert_match(/could not create unique index/, error.message)

           assert_not @connection.index_exists?(:ex, :number, name: :invalid_index)
+
+          @connection.add_index(:ex, :number, unique: false, algorithm: :concurrently, name: :invalid_index)
         end
       end

Error:
ActiveRecord::ConnectionAdapters::PostgreSQLAdapterTest#test_invalid_index:
ActiveRecord::StatementInvalid: PG::DuplicateTable: ERROR:  relation "invalid_index" already exists

To me it's confusing if index_exists? returns false, but add_index raises because the index exists.

(I think a new kwarg - index_exists ..., valid: true - could be an option here?)

[byroot]

@ghiculescu I think you have a point. I will revert for now.

@fatkodima can you open the same PR again and tag @ghiculescu so that we discuss this a bit more, see what we can do?