PG: migration failure when re-attempting concurrent index addition - ArgumentError: unknown keyword: :algorithm

[owst]

Summary

schema_plus_indexes raises ArgumentError: unknown keyword: :algorithm when encountering an invalid index from a previous (concurrent) index creation attempt, rather than raising the underlying PG::DuplicateTable error (or a similar error).

Background

Postgres indexes can be added concurrently to prevent reads/writes from being blocked while the index is created.

If the index can't be added, for example if it is a unique index and the table data is not unique, then the index is left in an "invalid" state that must be cleaned up before re-attempting the index creation.

Reproduction

The following tests exhibits the issue:

diff --git a/spec/migration_spec.rb b/spec/migration_spec.rb
index c51a6a6..eecfb27 100644
--- a/spec/migration_spec.rb
+++ b/spec/migration_spec.rb
@@ -124,6 +124,32 @@ describe ActiveRecord::Migration do
     end
   end
 
+  context "when a PostgreSQL concurrent index creation fails", postgresql: :only do
+    before(:each) do
+      @model = Comment
+      # Create rows so that user_id: 1 is not unique...
+      @model.create!(user_id: 1)
+      @model.create!(user_id: 1)
+    end
+
+    def add_unique_index
+      change_table(@model) do |t|
+        t.index :user_id, unique: true, algorithm: :concurrently
+      end
+    end
+
+    it "raises if an invalid index already exists" do
+      # Simulate a migration that attempts to (concurrently) add a unique index. The first time it
+      # runs, the index add fails because the user_id is not unique. The second time, the migration
+      # fails because the previous migration attempt left an index that is in an invalid state and
+      # thus requires manual attention to clean up (to delete the index)
+      expect { add_unique_index }.to raise_error(ActiveRecord::RecordNotUnique)
+
+      # Try again... we expect that we should be alerted to the already-present (but invalid) index
+      expect { add_unique_index }.to raise_error(/already exists/)
+    end
+  end
+
   context "when column is added", :sqlite3 => :skip do
 
     before(:each) do

The issue seems to stem from the functionality to not raise duplicate index errors (I can't tell why this functionality is desirable - surely a duplicate index indicates some kind of logic error and is thus unexpected. Perhaps it would be sensible to deprecate/remove it):

schema_plus_indexes/lib/schema_plus/indexes/middleware/migration.rb

Lines 63 to 68 in f095d39

	attempted = ::ActiveRecord::ConnectionAdapters::IndexDefinition.new(
	env.table_name, name, env.options[:unique], env.column_names,
	**env.options.except(:unique)
	)
	raise if attempted != existing
	::ActiveRecord::Base.logger.warn "[schema_plus_indexes] Index name #{name.inspect}' on table #{env.table_name.inspect} already exists. Skipping."

The ArgumentError is raised since ::ActiveRecord::ConnectionAdapters::IndexDefinition does not have an algorithm: keyword. I think the class in question is meant to represent an index that has already been created and thus the algorithm that was used to create that index is irrelevant.

Related

This Rails PR looks useful, but is not (as far as I can tell) available in a released version of Rails: rails/rails#45160