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

Do not include column limit in schema.rb if it matches the default #20815

Merged

Conversation

byroot
Copy link
Member

@byroot byroot commented Jul 8, 2015

When working on engines that supports multiple databases, it's
very annoying to have a different schema.rb output based on which
database you use. MySQL being the primary offender.

This patch should reduce the disparities a bit.

Next on the list could be using: :btree for mysql indexes.

cc @rafaelfranca

When working on engines that supports multiple databases, it's
very annoying to have a different schema.rb output based on which
database you use. MySQL being the primary offender.

This patch should reduce the disparities a bit.
@chancancode
Copy link
Member

I sympathize with your pain but this is giving me an uneasy feeling when viewed in the context of applications :/

@byroot
Copy link
Member Author

byroot commented Jul 8, 2015

@chancancode What do you mean? Are you afraid this will cause issues to users?

@chancancode
Copy link
Member

Yeah; in theory, schema.rb is the canonical reference for your database setup, and that's true even across environments, so that you could look there and know that your production set up has the same DB layout (at least for things that "matter", in practice it seems like the columns ordering is always wrong, but that usually doesn't matter much), so I don't know if removing information from it is a good idea.

But I also don't have a very concrete concern/objection other than the vague uneasy feeling, so if others feel that it is fine, maybe I am just over-thinking it (shrug)

@matthewd too?

@byroot
Copy link
Member Author

byroot commented Jul 9, 2015

I actually just remembered I totally forgot to mention the main pain point:

When dumping schema with MySQL the boolean will end up as:

t.boolean 'foo', limit: 1

Because in the end MySQL's booleans are tinyint(1). And then if you try to load that schema into postgres it will fail with a syntax error.

So if this PR is a no go, then I think we should at least ignore the length property for column types that doesn't require it.

@matthewd
Copy link
Member

matthewd commented Jul 9, 2015

I think I'm tentatively in favour of this, on the basis that we'll have a proper migration API versioning mechanism Real Soon Now, which will better address the "but what if the default changes" concern.

The current behaviour isn't an accident, though: 2c76793

@matthewd matthewd merged commit 835617b into rails:master Dec 18, 2015
matthewd added a commit that referenced this pull request Dec 18, 2015
…it-is-default

Do not include column limit in schema.rb if it matches the default
kamipo added a commit to kamipo/rails that referenced this pull request May 31, 2016
Follow up of rails#20815.

```ruby
class CreatePeople < ActiveRecord::Migration[5.0]
  def change
    create_table :people do |t|
      t.integer :int
      t.bigint :bint
      t.text :txt
      t.binary :bin
    end
  end
end
```

Result.

In postgresql and sqlite3 adapters:

```ruby
ActiveRecord::Schema.define(version: 20160531141018) do

  create_table "people", force: :cascade do |t|
    t.integer "int"
    t.bigint  "bint"
    t.text    "txt"
    t.binary  "bin"
  end

end
```

In mysql2 adapter:

```ruby
ActiveRecord::Schema.define(version: 20160531141018) do

  create_table "people", force: :cascade, options: "ENGINE=InnoDB DEFAULT CHARSET=utf8mb4" do |t|
    t.integer "int"
    t.bigint  "bint"
    t.text    "txt",  limit: 65535
    t.binary  "bin",  limit: 65535
  end

end
```

After this patch:

```ruby
ActiveRecord::Schema.define(version: 20160531141018) do

  create_table "people", force: :cascade, options: "ENGINE=InnoDB DEFAULT CHARSET=utf8mb4" do |t|
    t.integer "int"
    t.bigint  "bint"
    t.text    "txt"
    t.binary  "bin"
  end

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

Successfully merging this pull request may close these issues.

None yet

5 participants