Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Should Granite have a built in Migrator? #182

Closed
eliasjpr opened this issue Apr 19, 2018 · 19 comments
Closed

Should Granite have a built in Migrator? #182

eliasjpr opened this issue Apr 19, 2018 · 19 comments

Comments

@eliasjpr
Copy link
Contributor

eliasjpr commented Apr 19, 2018

Given this PR #172 and #168 which as basic support for table migration and bulk import, I have been wondering if Granite should fully support migrations?

class SomeMigration < Granite::Migration
  def up
      User.migrator.create
      User.execute("Custom SQL here")

      User.each do |record|
          record.status = active
          record.save!
      end
  end

 def down
      User.migrator.drop
      User.execute <<-SQL
         Custom SQL here
      SQL
  end
end

I think is time :)

Open implementation detail discussion

@drujensen
Copy link
Member

drujensen commented Apr 19, 2018

@eliasjpr Granite actually did have this but it was impossible to know the intent of the developer so we removed in favor of micrate.

An example is renaming a column in the db. You can create a new column but the old one should not be removed until the data is migrated.

The technique I used when I built this was that the migrations were always additive unless you included a flag to perform a cleanup to remove old columns. This sorta worked but required much more care and handling than just supporting migration scripts.

@faustinoaq
Copy link
Contributor

Well, I think a migration system using crystal is a bit overloaded since crystal is compiled and not an scripting language, so every time you need to run a migration action you would need to compile it first 😅

Currently, I just edit the SQL and run bin/amber db migrate without requiring crystal compiler

So, by example, If I want to execute migrations in Heroku, and I want to change something for the migration itself, (some type, bugfix, etc), I can't because I would need to compile the migrations first.

Another example, if you try to use Lucky record you will notice that lucky db.migrate isn't instantaneous because their migrations are made in crystal and need to be compiled first.

@robacarp
Copy link
Member

I'm not opposed to granite having a migrator library, but it seems like there are a lot more places which need time and energy at the moment. Micrate isn't a pretty, DSLy solution, but it is simple, functional and doesn't seem like it's going to be a maintenance problem.

@Pepan
Copy link

Pepan commented Jan 18, 2019

Ruby starts some time too, and running migration is like start Ruby and then run script. So I thing speed will be very similar when you compile migration and then run it. Crystal already can do something like crystal run src/my/main.cr ...

Also if there is more than one migration it will start Ruby once and run all migrations. Migration in Crystal can do the same.

@igbanam
Copy link

igbanam commented Jan 30, 2019

The question which kept coming to mind while considering this is "Is it faster in Crystal or in SQL?"

In deliberating on ☝️ with myself, I realized that the Migrator for Crystal would only be sugar on SQL. This would mean that over time, the Migrator may begin to struggle to do what SQL does best. Also, leaving the migrations in SQL forces the developer to think about their data from a data perspective — this is one of the few things which attracted me to Amber, as opposed to Rails where everything is Rails-y

I believe having migrations in Crystal may constrain what developers can do with their databases.

@drujensen
Copy link
Member

@igbanam I agree with you! But we are in the minority.

I have heard several complaints from self proclaimed "lazy" rails developers that don't want to use SQL DDL (Data Definition) or DML (Data Manipulation) and prefer the framework handle it for you.

We have already introduced where, has_many, etc for the DML. This is just continuing down that path (rabbit hole) to create the DDL as well.

@igbanam
Copy link

igbanam commented Jan 30, 2019

@drujensen …but this is Amber, not Rails 🙂

@kevinelliott
Copy link

kevinelliott commented Jan 30, 2019

@igbanam Food for thought: What about allowing multiple types of migrations, detected either by a hint at the top of the migration file or by extension (i.e. 201901300020302782_add_twitter_to_users.sql for SQL migration and 201901300020302782_add_twitter_to_users.cr for Crystal-based migration). This would obviously introduce migration order complexity, but this could be mitigated in the migration planner perhaps.

Thus, it leaves it up to the developer to choose which type of migrations preferred, and could consider using both as necessary interchangeably.

Though take with a grain of salt because I'm not exactly authoritative on this and I have not put much thought into the deeper implementation details here.

@Pepan
Copy link

Pepan commented Jan 31, 2019

Hi. I thing you miss one benefit of DSL migrations and this is switching between databases ... You develop on Mysql or SQLITE and have production in PostgreSql ... I using SQL migration you can't do that ...
Second one you miss a speed . Writing User.find_by email: 'guest@gmail.com' is definitely shorter than SELECT * FROM "users" WHERE "users"."email" LIKE "%guest@gmail.com%";

Customers are paying for short time dev. and reliable app and you are doing definitely more mistakes in plain SQL then in AR - so it takes more time to make something working. So you are more expensive and less able to compete. I don't see any advantage in that for business. ;)

@drujensen
Copy link
Member

@Pepan Yes, very aware of the benefits of a DSL but I think you may not be aware of the downside.

The biggest one is that the developer doesn't know what is going on. They write the code completely unaware the SQL is even being used. I have seen too many rails projects considered slow because the developer didn't know how to add an index or properly avoid N+1. They then decide that the problem is the database so they go to a NoSQL solution. :face_palm:

What I think we need is a balance. We need to provide the ease of find_by and where but make it clear what SQL is being generated. I think if the DSL is more SQL like, you will have an easier time converting in your head the SQL being generated.

I've been playing around with a DSL that directly mimics the SQL. Something like:

query = User.select(:all).where(:name, :like, 'John*').join(:comments).on(user_id).order_by(:last_updated)
puts query.to_sql
User.all(query.to_sql)

This provides a clean separation of the SQL query being generated and the ORM mapping.

Obviously, we could do the same for migrations or DDL.

@Pepan
Copy link

Pepan commented Jan 31, 2019

@drujensen We know about N+1 problem ale if it is in case we optimise it for this problem, but if it is no bottle neck, we don't care (f.e. if it is on small table little often used). Also devs have University education so It is expectable that they know about SQL :)
I thing that N+1 is also no problem for migrations ... in other hand you can use some optimising, caching if you use AR which is not available with SQL where you must use JOINS and so on to handle N+1 and can't use things like includes which handles it for you with AR more optimal way in most situations. Also using .includes is much faster to code than write complicated JOINS ... ;)

AR can in this days do same thing like
Model.where.not(a: nil).or_not(b: nil)
Model.where.not(a: nil).or(Model.where.not(b: nil))

@drujensen
Copy link
Member

@Pepan Right, you would think these are easily solvable problems. ;-) I'm just stating my anecdotal observations here in the U.S. Maybe my concerns are not founded.

@eliasjpr
Copy link
Contributor Author

eliasjpr commented Mar 7, 2019

I'm split on this. The reason I think it would be good to have a migrator comes up mostly because of the Migrate shard. Don't get me wrong I think is a great project but it still has its limitations. One of them being executing multiple queries.+

@kvirani
Copy link

kvirani commented Apr 2, 2019

Migrations written in the app language (eg: Ruby) and with the framework (eg: Rails) loaded up, do offer a lot of convenience:

  1. We're able to forget DDL syntax, which imho is the first part of SQL syntax that non-DBAs forget, and that's largely an okay thing. I do agree that it's important for the syntax to remind us that we're dealing with tables, indices, etc. I think AR's migration syntax for example does a decent job of this, no?
  2. It's easier to write DML logic where data needs to be migrated
  3. More specifically, when schema changes require app code/logic (such as service objects, model methods, etc), SQL-only migrations I assume will be a challenge / roadblock, whereas ActiveRecord (AR) migrations let us write regular code.

I love the idea and philosophy behind what @drujensen is saying. When I first saw SQL migrations in Granite (while learning Amber) my thought was "Nice, great idea. SQL Migrations should be the default.". Not just because of performance but also because of simplicity.

But then I also thought about all those times in Rails where I wrote migrations that needed to access my AR models or other objects (like services) in order to migrate data. I wondered what I would do in situations like that with Amber & Granite. Perhaps there is a clear workaround for this that I haven't considered @drujensen ?

Anyway, my current +1 is on supporting both .cr and .sql migrations, due to benefit number 3 above. But that this feature should be part of Micrate not Granite, right? Why does it need to be built in to Granite?

PS: It's also a bit inconsistent having a seeds file which is .cr (allowing us to use Crystal and our Amber models, service objects, etc if we want to boot up / require the app) but our migrations are in .sql only.

@robacarp
Copy link
Member

robacarp commented Apr 4, 2019

To my experience, one of the best features of SQL migrations is that your models and app code aren't available. Data migration logic should live elsewhere, not in with the schema migrations.

@m-o-e
Copy link

m-o-e commented Jan 15, 2020

You develop on Mysql or SQLITE and have production in PostgreSql ... I using SQL migration you can't do that ...

Why would you develop for a different database than what you have in production?
That's a recipe for disaster.

Second one you miss a speed . Writing [DSL] is definitely shorter than [SQL]

Shorter != faster.
The latter can be read, written and understood by anyone who knows SQL.

It can easily be tested by pasting it into a database console.
It naturally supports every database specific feature (scripting,
SQL dialects) that the author wishes to use.
It can easily be generated from other tools.

None of that is true for a custom migration DSL.

+1 for keeping Amber migrations in plain SQL rather than
inventing an awkward DSL like Rails did.

@Pepan
Copy link

Pepan commented Jan 16, 2020

Hey

  1. User.first is definitely shorter than SQL equivalent
  2. It is much error prone than SQL equivalent
  3. If customer is asking to change DB for example from MySql to PostgreSql you need to do absolutely nothing.
  4. DSL enables you do various caching and preloading and produce more readable code
    User.enabled.females.in_productive_age.joins(:husband) compared to SQL equivalent which will probably take several lines of cryptic code.
    This is super for developer team - reading someones elses complex SQL is real pain ... and do some change into it is double pain

So developer using DSL is faster, happier, more productive and doing less errors against otherone using pure SQL.
And code will be probably faster too, because in 90% of time you don't need something special. In other hand creators of DSL knows SQL well and also platform and can use features developer does not know about or usage is so complicated that he refuses to learn it ...

For customer is developer using DSL much more interesting, because deliver safer product in shorter time and that is what is counting ...

@m-o-e
Copy link

m-o-e commented Jan 16, 2020

I don't know why you keep using query examples, this ticket is about migrations (DDL).
DB portability is a red herring. It is very rarely needed and it always involves manual work.
ORMs tend to make that process harder rather than easier because they obscure the view
on the generated SQL.

You seem to be focussed on developers who are unfamiliar with SQL.
I would argue Granite already does a great job at helping them query the db
but they should not be the ones who write your database migrations.

@eliasjpr
Copy link
Contributor Author

Closing this ticket since Granite already has a way to migrate the database via Micrate and adding a new DSL is not desired at this time.

Benefits of Micrate

  • Plain old SQL allows for clarity, flexibility and freedom to write SQL in native SQL language without tapping into another DSL to learn

Drawbacks of Micrate

  • Not actively maintained
  • Running Migration within crystal classes requires compilation of those classes slowing development.
  • Missing some useful features such as custom migration sequence strategy for migration files, run multple sql statements easily

Desires

  • Community to contribute on Micrate more actively.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

9 participants