refinery

refinery copied to clipboard

Hi,

It's pretty common to use time based version numbers for migration files (e.g. migrations of Active Record or Diesel). The current implementation of refinery is only using the column type INT4 for the version column, which is too small to hold the current time based on a YEAR-MONTH-DAY-HOUR-MINUTE basis.

Is there any special reason not to use INT8 for the version column? The rust code seems to use a i32, so a switch to i64 should be possible.

(I'm using a postgres database).

Hi, basically historic reasons, it started with INTEGER, and because of https://github.com/rust-db/refinery/pull/50 was converted to INT4. If INT8 is sql standard and all drivers support converting INT8 rows to rust i64 I don't think it would pose a problem cc @akhilles

So, I looked into it and using INT8 in the version column and upgrading version fields to i64 could be done, though it will break for users with the refinery_schema_history already created with version as INT4. i found a couple alternatives:

• adding CAST(version as INT8) on GET_APPLIED_MIGRATIONS query, this seems the simplest solution, but doesn't work for mysql
• GET_APPLIED_MIGRATIONS could be moved into the migrate trait, return GET_APPLIED_MIGRATIONS by default, and be customized for mysql and mysql_async
• handle both situations with code like this
• handle both situations and deprecate INT4 tables with a message and develop a migration step on refinery_cli

apart from the first option (which doesn't work), all the others seem clunky to me, so if there aren't any more alternatives i feel like skipping it

A hacky workaround is to pre-process the migration file names. Sort the names by date and prefix them with integer IDs before feeding them into refinery.

EDIT: Actually this might not work since the assigned IDs aren't deterministic

@jxs That shounds like the refinery_schema_history is holy and can't be changed. Not upgrading to a more feature and future proof int8 seems like a big oversight.

There should be a way for refinery to handle such schema updates.

I think the change to int8 should be done in a major version upgrade and the user should be forced to do a migration either explicitly using the cli or implicitly by the library itself.

Hey, I was just wondering if there has been any additional thought on this? I have just recently begun working on an application with rusqlite that I would love to use refinery on it, but not being able to use date based versions is a bit of a deal breaker for me. I would be more than happy to (at least try to) do the work on this if there is an agreed upon path.

Hi @ankhers , I looked at this again today, but the arguments referred on https://github.com/rust-db/refinery/issues/83#issuecomment-620781813 persist, do you have an alternative?

With refinery still being pre 1.0, would it be such a big deal to just have users create a migration (or have the CLI generate it for them) to be run? I know you already said it felt clunky though. So I won't push it if you are not interested.

yeah, I have said that previously but this feature has had a significant demand. I agree, having the cli generate the migration needed for each db seems the cleanest solution, would you like to tackle that?

Sure, I can take a crack at it.

Before I continue with my PR. I just realized that if I have two migrations, 1 and 2, that if migration 2 gets applied BEFORE 1, then 1 will never be applied.

Would you be open to having refinery keep track of all applied migrations and apply any migrations that have not yet been applied? Or would you like to keep the current functionality?

Before I continue with my PR. I just realized that if I have two migrations, 1 and 2, that if migration 2 gets applied BEFORE 1, then 1 will never be applied.

yeah that's expected, that's also why there's the abort_missing which is enabled by default, so it should return an Erroron that case

Is this the functionality that you would like to continue with? One of the benefits of date based timestamps is that you can (almost) guarantee that two migrations will never have the same version number. And as long as you apply a series of migrations in numerical order, it will still work out.

Just in case I was not overly clear on my statement above, I will give a quick example.

I create the initial database, I use V1 and create a users table (just using smaller numbers so it is clear the order they need to be applied.

On branch A, I create migration 3 and 4 to create the posts and comments table.

On branch B, I create migration 5 and 6 to create two other tables.

Now, if I am on branch A, I will have access to migrations 1, 3 and 4. On branch B, I will have access to migrations 1, 5 and 6.

With the current functionality, everything will work properly if I merge branch A before B. However, if I merge B first, I will need to rename/reorganize my migrations because the new migrations on branch A would not be run.

With other migration tools I have used (mostly Elixir's Ecto and Ruby's ActiveRecord), you can apply the migrations from any branch in numerical order and everything just works™ because you create the migrations in a dependent order. So in my example, the new migrations on branches A and B are not dependent on each other, so it really doesn't matter if you would apply branch A before branch B.

Or is this use case already covered in some way that I missed?

I see, yeah maybe that makes sense to implement after this, we can discuss it on another issue if you want. thanks

Those who have a burning desire to use timestamped migrations may wish to check out #330. Although make note of the limitations documented in the README around lack of support for retrofitting into an existing database.