Remove redundant index on :task_name

[adrianna-chang-shopify]

For: #342

Confirmed that the index on just :task_name is redundant given our composite index on [:task_name, :created_at]. We can remove it.

I think we need to remove the index from the table itself in a change_table block (instead of using https://api.rubyonrails.org/classes/ActiveRecord/ConnectionAdapters/SchemaStatements.html#method-i-remove_index) due to LHM constraints (as mentioned here, modifying indexes outside of table blocks seems to pose problems for LHMs).

[sambostock]

I suspect the reason #239 was able to get around LHM constraints was because it consolidated everything into a create_table block. I feel like change_table or add_index would both have the same problems for LHM (don't modify existing tables without using LHM). 🤔

[adrianna-chang-shopify]

Oof yeah good shout @sambostock, whether we use change_table or not likely won't solve the LHM issue because it's still modifying the table 🤦‍♀️ Didn't think that one through very hard lol.

Given the apps that use LHMs are a minority, we should just be able to ship this, and any app that requires use of LHMs can delete this migration and rewrite it as an LHM if need be (I think only Partners had an issue with this)?

[sambostock]

While I do agree it's probably fine to ship as is and let consumers rewrite migrations as LHM, Shopify's data stores best practices recommend everyone use LHM for all table changes, so I'm not sure how uncommon it is internally. That said, the only other thing we could do would be to try to detect LHM and generate an appropriate migration, but I don't know if that makes sense.

[etiennebarrie]

Any reason not to use remove_index and add_index directly?

[adrianna-chang-shopify]

The only reason I could see using this over remove_index and add_index directly is that it makes it easier to change it to an LHM:

class RemoveIndexOnTaskName < ActiveRecord::Migration[6.1]
  def up
    change_table(:maintenance_tasks_runs) do |t|
      t.remove_index(:task_name)
    end
  end

  def down
    change_table(:maintenance_tasks_runs) do |t|
      t.index(:task_name)
    end
  end
end

require 'lhm'

class RemoveIndexOnTaskName < ActiveRecord::Migration[6.1]
  def up
    Lhm.change_table :maintenance_tasks_runs do |t|
      t.remove_index(:task_name)
    end
  end

  def down
    Lhm.change_table :maintenance_tasks_runs do |t|
      t.index(:task_name)
    end
  end
end

Not sure if if this is something we care about that much though 😄

[etiennebarrie]

Yup it's really nice if it lets people adapt it more easily to LHM.

[adrianna-chang-shopify]

Yeah, it's a really good point @sambostock - it's tricky to find the balance between making this meet our internal best practices vs trying to keep things conventional for use externally. Looking at the dependency list in SDB, there are a number of gems using Shopify/lhm 😬 But I'm not sure it's worth the complexity of having to detect and generate new LHM migrations.. Maybe a good middle point would be to document rewriting one of our migrations as an LHM in the README?

Edit: After reviewing how LHMs can be used with AR migrations, if we keep the change_table syntax it's very simple to change the migration to an LHM, so I think we can rely on users to make the changes if they need an LHM.