Switch id fields to default to BIGINT

[bradleybeddoes]

This is an initial cut at implementing BIGINT as the default for primary/foreign keys within Ecto for consideration (especially given this is my first proposed PR for this project).

There are some outstanding issues which I need to document as a second commit within this PR but I wasn't sure on the best place. They are:

PostgreSQL

PostgreSQL will silently ignore differences between foreign key column types and keep functioning, until it doesn’t (This would be right around the id 2147483647 as best I can tell).

Options:

1. Update the existing Types to be compatible with Ecto (recommended):

   1. Alter the type of the primary key in existing tables to be bigserial
   2. Alter the type of foreign key in existing tables to be bigint

2. Alter the type option provided to the references function when creating any new foreign key to be :integer

MySQL

MySQL will not ignore differences between foreign_key column types and instead provides the following error:

ERROR 1005 (HY000): Can't create table database.table_name (errno: 150 "Foreign key constraint is incorrectly formed")

Options:

1. Update the existing Types to be compatible with Ecto:

   1. Alter the type of the primary key in existing tables to be bigint unsigned not null auto_increment
   2. Alter the type of foreign key in existing tables to be bigint unsigned

2. Alter the type option provided to the references function when creating any new foreign key to be :integer

[elixir-bot]

Hello, @bradleybeddoes! This is your first Pull Request that will be reviewed by Ebert, an automatic Code Review service. It will leave comments on this diff with potential issues and style violations found in the code as you push new commits. You can also see all the issues found on this Pull Request on its review page. Please check our documentation for more information.

[josevalim]

Thank you @bradleybeddoes. I don't think we should rename actual types, such as :serial to mean something else. The best would be to introduce :bigserial and translate it accordingly in the adapters.

Also, I don't think we should translate :serial to "not null autoincrementing" on the MySQL side. We already get those properties when we declare the field as primary key.

[bradleybeddoes]

Thanks @josevalim, happy to make a change to define bigserial in the adaptors as you've suggested.

To your second point the translation on the MySQL side attempted to keep the existing semantics of serial intact per http://dev.mysql.com/doc/refman/5.7/en/numeric-type-overview.html which was already being used as BIGINT UNSIGNED NOT NULL AUTO_INCREMENT. I guess we could knock out the NOT NULL but the AUTO_INCREMENT component would need to remain at a minimum as I understand it.

[josevalim]

I guess we could knock out the NOT NULL but the AUTO_INCREMENT component would need to remain at a minimum as I understand it.

I see. Let's keep it NOT NULL AUTO_INCREMENT then except it should be INT and we will use BIGINT for :bigserial.

[bradleybeddoes]

Thanks @josevalim - I've finished up the implementation for bigserial.

Note that serial within the MySQL adaptor still utilises BIGINT as this is what they've defined and what Ecto was already doing, seems like less of a surprise to not change to INT imho.

[josevalim]

❤️ 💚 💙 💛 💜

[bradleybeddoes]

Hi @josevalim - Sorry to drag out a really old PR here but I was just looking through master and oddly I can't find these changes having ever landed, or they got backed out somehow, even though this was merged in.

Perhaps the proposed changes didn't work out for some reason. Is there still interest in having id fields default to BIGINT?

[josevalim]

Unfortunately it was a backwards incompatible change, so we reverted it, which means we can't redo it.

[bradleybeddoes]

Thanks @josevalim, shame it caused backwards compatibility issues. There are real world examples of this causing issues, rails changed this default some years ago rails/rails#26266.

[chulkilee]

Isn't it just moved to ecto_sql?

https://github.com/elixir-ecto/ecto_sql/blob/d5257f2b9a9054381e82e54b48c95cfd51a1e65b/lib/ecto/migration.ex#L345

[josevalim]

Oh, indeed! I think we changed it back again i a major version!

On Sat, Aug 31, 2019 at 23:46 Chulki Lee ***@***.***> wrote: Isn't it just moved to ecto_sql? https://github.com/elixir-ecto/ecto_sql/blob/d5257f2b9a9054381e82e54b48c95cfd51a1e65b/lib/ecto/migration.ex#L345 — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1880?email_source=notifications&email_token=AAACK3RF2GXO2KYTUXOFOBDQHNJKLA5CNFSM4C3AHVYKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD5T3BYI#issuecomment-526889185>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAACK3XRWXBC4FJFAIX64P3QHNJKLANCNFSM4C3AHVYA> .

-- *José Valimwww.plataformatec.com.br <http://www.plataformatec.com.br/>Founder and Director of R&D*

[bradleybeddoes]

Thanks @chulkilee and @josevalim!! Great to see this landed in some form.