Change cursor from bigint to string

[sambostock]

Working on the upcoming support for tasks backed by custom enumerators surfaced the need for non-numeric cursors.

For instance, one might have a task that iterates over resources backed by some API providing its own opaque string cursor, which would be impossible to serialize to a bigint column.

As such, this changes the maintenance_tasks_runs.cursor column to be a string. For this to work, we must deserialize the cursor back to an integer in the Array and CSV collection cases, while the ActiveRecord case is handled transparently.

⚠️ Before Merging

• Ensure we've added any relevant documentation or migration guide
  • What happens if the DB is migrated while tasks are running using the old version of the gem?
    • Should we publish an intermediate forward compatible version of the gem including only the .to_i change, so this is a non-issue? (Ensure cursor is converted into integer as needed #340)
  • Should consumers consider using an LHM version of the migration?

[etiennebarrie]

We have to investigate a bit more what it means to update the column. We may need a more involved migration with an additional column depending on how it goes.
We've started looking quickly, and with mysql, it seems like a task may be able to continue running just fine through the migration, but not be paused/resumed, and if it's paused before, it can't be resumed. (There may be an issue with timeouts during the lock for the schema change, I haven't investigated that well, but if the query timeout is longer than the duration of the schema change it should be ok.)
We need to build a full understanding of how it will behave in production so that we can re-evaluate the approach or simply document it well (maybe they need to have no running tasks for example).

To test you need to run the production environment, and see how it behaves to interruptions/pauses with the old code (i.e. old schema loaded in ActiveRecord) and between old and new code.

Also the migration should be mostly optional as long as you don't use string cursors, it could be delayed after they ship the new version of the gem.

[adrianna-chang-shopify]

Sorry for the delay on this Sam! We managed to tophat this pretty thoroughly with MySQL 5.7, MySQL 8.0, and PostgresQL, and confirmed that the only scenario that poses a problem is if the migration is deployed while a Task is paused. @etiennebarrie and I were thinking that a comment directly in the migration might help:

class ChangeCursorToString < ActiveRecord::Migration[6.1]
  # This migration will clear all existing data in the cursor column for 
  # both MySQL and PostgresQL. Ensure no Tasks are paused when this migration
  # is deployed, or they will be resumed from the start.
  # Running tasks are able to gracefully handle this change, even if
  # interrupted.
  def up
    change_table(:maintenance_tasks_runs) do |t|
      t.change(:cursor, :string)
    end
  end

  def down
    change_table(:maintenance_tasks_runs) do |t|
      t.change(:cursor, :bigint)
    end
  end

We will also make it clear in the release notes that this is a situation to watch out for.

Let's close #340 , since actively running Tasks can actually gracefully handle the changes even if this gets deployed, and the problematic scenario with paused Tasks won't get solved by the intermediary PR anyways.

If you could get this rebased and just add in the comment to the migration (feel free to tweak it or offer your own feedback on it), we'll get this 🚢 !

[etiennebarrie]

I think that's great, we have to mention in the changelog that tasks should not be paused but that's it I think.

[sj26]

Very keen for this change. We have models with uuid primary keys which don't function entirely as expected with bigint cursors. 🙏

[sj26]

Is there anything we could do to help land this PR?

[etiennebarrie]

I just pushed a rebase, are you willing to test this branch or would you rather wait until a new version is released? Our tests showed that it should be fine.

The cursor is pulled from the database, then while Active Record thinks it's still an integer, it turns it into a Ruby Integer, and then it's also serialized and stored as an Integer in the job queue whenever it's interrupted, so jobs can continue running while the migration runs.

The next time Rails boots and figures out the column is no longer an Integer, Active Record will stop turning it into a Ruby integer, but the code changes here will properly coerce those for the CSV/Array enumerators. But for your case with ActiveRecord::Relation, that will allow cursors that are UUID to be passed from the record as a string down to Active Record which deals with it.

[sj26]

Amazing, thank you!

Yes, I'll point our Gemfile at this branch and give it a go, then report back. We're in the middle of some hairy long-running migrations and this will make it a lot easier. 🙏

[sj26]

We updated out rails app to use this branch in production today. Works a treat. We have a backfill on a table with a uuid primary key running right now.

We made sure there were no active tasks during deployment. But we had paused tasks etc and all cursors were preserved after the migration. We’re using Postgres.

Thanks so much!

[etiennebarrie]

We made sure there were no active tasks during deployment. But we had paused tasks etc and all cursors were preserved after the migration. We’re using Postgres.

Great to know, I'll update the migration comment. I don't think active tasks would have been an issue, like I mentioned the cursor is kept in the job queue/job instance variables in those situations.

Paused tasks is an issue for MySQL with that simple migration because the values aren't kept during the ALTER TABLE.

[adrianna-chang-shopify]

Let's make sure we emphasize the risks of paused tasks for apps using MySQL in the release notes, but other than that this looks good to ship on my end. I'll merge if you have no further feedback @etiennebarrie

[etiennebarrie]

Yes sorry I forgot about this. Let's merge this and ship a new version.