False negative for `index_exists?` when `remove_index` runs

[evsasse]

Falling into this scenario when I try to revert an add_index definition that is not "normalized" to what Postgres ends up generating on their side. https://github.com/fatkodima/online_migrations/blob/d632efd04fd8390a587a2e73d3fa098f0dcaecfb/lib/online_migrations/schema_statements.rb#L751

On Postgres, when you add_index with something like "CAST(payments.amount AS INT), (CREATED_AT::date)", when you query back the created index, it has been normalized to something like "((amount)::integer), ((created_at)::date)".

At the linked file above index_exists? is used, that doesn't take that into account. So when you revert such a migration, online_migrations skips it, and returns that such index already doesn't exist. _Maybe we could argue that we also have a bug on the original index_exists? implementation on Rails?_.

When you revert this migration using the original remove_index implementation from Rails, it seems to work fine 🤔...

Here is an example I've built based on the bug-reporting template from Rails. You can save it as bug.rg, and run it with ruby bug.rb.

bug.rb

```ruby # frozen_string_literal: true require "bundler/inline" gemfile(true) do source "https://rubygems.org" gem "rails" # If you want to test against edge Rails replace the previous line with this: # gem "rails", github: "rails/rails", branch: "main" gem "pg" end require "active_record" require "minitest/autorun" require "logger" # This connection will do for database-independent bug reports. ActiveRecord::Base.establish_connection( adapter: "postgresql", host: "db", username: "postgres", password: "postgres" ) ActiveRecord::Base.logger = Logger.new(STDOUT) ActiveRecord::Schema.define do drop_table :payments create_table :payments, force: true do |t| t.decimal :amount, precision: 10, scale: 0, default: 0, null: false t.timestamps end end class Payment < ActiveRecord::Base end NON_NORMALIZED_INDEX = "CAST(payments.amount AS INT), (CREATED_AT::date)" NORMALIZED_INDEX = "((amount)::integer), ((created_at)::date)" class AddComplexIndexToPayments < ActiveRecord::Migration::Current # or use a specific version via `Migration[number]` def change add_index( :payments, NON_NORMALIZED_INDEX, name: :my_complex_index, if_not_exists: true ) end end class BugTest < Minitest::Test i_suck_and_my_tests_are_order_dependent! def test_1_migration_up_adds_normalized_index AddComplexIndexToPayments.migrate(:up) assert_equal( [NORMALIZED_INDEX], Payment.connection.indexes(:payments).map(&:columns), ) end def test_2_migration_index_exists_without_name assert_equal( true, Payment.connection.index_exists?( :payments, NORMALIZED_INDEX, ), ) assert_equal( true, Payment.connection.index_exists?( :payments, NON_NORMALIZED_INDEX, ), ) end def test_3_migration_index_exists_with_name assert_equal( true, Payment.connection.index_exists?( :payments, NORMALIZED_INDEX, name: :my_complex_index ), ) assert_equal( true, Payment.connection.index_exists?( :payments, NON_NORMALIZED_INDEX, name: :my_complex_index ), ) end def test_4_migration_down_removes_index AddComplexIndexToPayments.migrate(:down) assert_equal( [], Payment.connection.indexes(:payments).map(&:columns), ) end end ```

And you should see some output similar to this:

Output

``` Run options: --seed 3607 # Running: == AddComplexIndexToPayments: migrating ====================================== -- add_index(:payments, "CAST(payments.amount AS INT), (CREATED_AT::date)", {:name=>:my_complex_index, :if_not_exists=>true}) D, [2024-08-07T14:15:57.678633 #2069] DEBUG -- : (4.1ms) CREATE INDEX IF NOT EXISTS "my_complex_index" ON "payments" (CAST(payments.amount AS INT), (CREATED_AT::date)) -> 0.0043s == AddComplexIndexToPayments: migrated (0.0044s) ============================= .F Failure: BugTest#test_2_migration_index_exists_without_name [the_bug.rb:75]: Expected: true Actual: false bin/rails test the_bug.rb:66 F Failure: BugTest#test_3_migration_index_exists_with_name [the_bug.rb:94]: Expected: true Actual: false bin/rails test the_bug.rb:84 == AddComplexIndexToPayments: reverting ====================================== -- remove_index(:payments, "CAST(payments.amount AS INT), (CREATED_AT::date)", {:name=>:my_complex_index, :if_not_exists=>true}) D, [2024-08-07T14:15:57.689626 #2069] DEBUG -- : (2.0ms) DROP INDEX "my_complex_index" -> 0.0030s == AddComplexIndexToPayments: reverted (0.0051s) ============================= . Finished in 0.017403s, 229.8423 runs/s, 344.7634 assertions/s. 4 runs, 6 assertions, 2 failures, 0 errors, 0 skips ```

That shows these 2 ideas:

• That index_exists? is returning false for the non-normalized index definition. Which causes the skip here on online_migrations.
• But because the original remove_index implementation on Rails doesnt check for that, it removes the index using its name fine.

---

For the time being I am creating my index definitions as close as possible to the "normalized" version, to allow the index_exists? check to work as expected.

But would love to possibly get an improvement here on the remove_index for online_migrations, or on the index_exists? for Rails, to make this easier... WDYT?

[fatkodima]

Thank you for a very detailed bug report!

Can you confirm that the merged changes fix the problem and so I will release a new version?

[evsasse]

Yep! It worked here ⚡ Thanks for jumping into this so quickly 🙇.

[fatkodima]

Thanks, released a new version.