Change Default Primary Keys to BIGINT #26266

Merged
merged 2 commits into from Dec 5, 2016
@jmccartie
Contributor
jmccartie commented Aug 24, 2016 edited

Friends don't let friends use INT as a primary key.

— Schneems (@schneems) May 13, 2016

Summary

Per a conversation with @sgrif: changes default primary keys from Integer to BIGINT for both Postgresql and MySQL. Leaves behavior alone for SQLite since this database does not provide support for BIGINT primary keys.

Other Information

For obvious reasons, this also requires foreign keys to change from integer to bigints. As a result the test suite's schema.rb has been change in the necessary places.

I'll squash and add a CHANGELOG entry once the rest looks ok...

@kaspth kaspth was assigned by rails-bot Aug 24, 2016
@rails-bot

Thanks for the pull request, and welcome! The Rails team is excited to review your changes, and you should hear from @kaspth (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

This repository is being automatically checked for code quality issues using Code Climate. You can see results for this analysis in the PR status below. Newly introduced issues should be fixed before a Pull Request is considered ready to review.

Please see the contribution instructions for more information.

@sgrif sgrif assigned sgrif and unassigned kaspth Aug 24, 2016
@sgrif
Member
sgrif commented Aug 24, 2016

We'll need to make sure that migrations written against 5.0 and later don't have the type of their primary key changed.

@jmccartie
Contributor

@sgrif Can you recommend a best-practice for ensuring that? Have we done something like that in the past that I can learn from?

@jmccartie
Contributor

@sgrif Thanks.

@jmccartie
Contributor

@sgrif So override the necessary methods inside class V5_0 ?

@rafaelfranca
Member
rafaelfranca commented Aug 24, 2016 edited

This is a dup of #24962. While this implementation may be more complete it is really a bad OSS etiquette to finish other people PR without given them a chance to finish it or proper credits.

My recommendation:

  1. pull #24962 commits in this PR.
  2. Work on top of that commits
@jmccartie
Contributor

@rafaelfranca Ah - thanks for showing me that. I searched for open PR's, but that didn't turn up for me.

I work at Heroku with @rwz, so I'll cycle around with him and see if we can tag team this.

@rafaelfranca
Member

👍

@jmccartie jmccartie closed this Aug 24, 2016
@jmccartie jmccartie reopened this Aug 24, 2016
@jmccartie
Contributor

I've now entered into the 3rd layer of git rebase hell...

@rafaelfranca
Member

I've now entered into the 3rd layer of git rebase hell...

lol. This is really tough.

@kaspth
Member
kaspth commented Aug 24, 2016

r? @sgrif

@schneems
Member

Looks like tests are passing now

@sgrif sgrif and 1 other commented on an outdated diff Aug 25, 2016
...verecord/lib/active_record/migration/compatibility.rb
@@ -103,6 +103,16 @@ def index_name_for_remove(table_name, options = {})
end
class V5_0 < V5_1
+ def create_table(table_name, options = {})
+ # Since version 5.1 Postgres adapter uses bigserial type for primary
+ # keys by default. This compat layer makes old migrations utilize
+ # serial type instead, the way it used to work before 5.1
+ if connection.adapter_name == "PostgreSQL"
@sgrif
sgrif Aug 25, 2016 Member

This needs to handle MySQL as well. Is it possible for us to implement this in an adapter agnostic way?

@jmccartie
jmccartie Aug 25, 2016 Contributor

Yeah, I can make that change.

On Thu, Aug 25, 2016 at 10:13 AM, Sean Griffin notifications@github.com
wrote:

In activerecord/lib/active_record/migration/compatibility.rb
#26266 (comment):

@@ -103,6 +103,16 @@ def index_name_for_remove(table_name, options = {})
end

   class V5_0 < V5_1
  •    def create_table(table_name, options = {})
    
  •      # Since version 5.1 Postgres adapter uses bigserial type for primary
    
  •      # keys by default. This compat layer makes old migrations utilize
    
  •      # serial type instead, the way it used to work before 5.1
    
  •      if connection.adapter_name == "PostgreSQL"
    

This needs to handle MySQL as well. Is it possible for us to implement
this in an adapter agnostic way?


You are receiving this because you modified the open/close state.
Reply to this email directly, view it on GitHub
https://github.com/rails/rails/pull/26266/files/de485a4379a265218f1bbea900c08f45e39a93fd#r76285358,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAIJEYj45MhqMMLNBPIkwwMhoRSdHCBuks5qjc1DgaJpZM4JrgwZ
.

@jmccartie
Contributor

@sgrif Added a test for MySQL compatibility. The logic still queues off the adapter name, though. We could move that logic into a default on each adapter, but then we have some sort of "legacy_primary_key" method on the adapter itself, which I liked much less than the ternary.

Thoughts?

@sgrif
Member
sgrif commented Aug 26, 2016

Can we just make the PG adapter do the right thing when the PK type is integer?

@jmccartie
Contributor
jmccartie commented Aug 26, 2016 edited

@sgrif I'm not sure. Both adapters need the logic since we need to ensure MySQL uses integer (and not bigint) and Postgresql uses serial (and not bigserial)

@sgrif
Member
sgrif commented Aug 26, 2016

How about just :integer?

On Thu, Aug 25, 2016 at 11:04 PM Jon McCartie notifications@github.com
wrote:

@sgrif https://github.com/sgrif Something like this?

options[:id] ||= connection.class::LEGACY_PK_TYPE


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#26266 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/ABdWK2-R2Rj-aRqV77WLaJaf0AC5MeBYks5qjlfYgaJpZM4JrgwZ
.

@jmccartie
Contributor
jmccartie commented Aug 26, 2016 edited

@sgrif

Because Postgresql needs serial, which is an ...

image

.... oh, right ... an integer.

Done.

@matthewd
Member

Pulling over my concern from #24962 (comment):

Forcing explicit type declarations on FKs is not going to fly, though... and migration versioning is not enough to solve that one for people who are upgrading: they're going to end up with some tables using int4 and some using int8 PKs.

@jmccartie
Contributor

@matthewd I get that. Any idea on how to resolve that? Ditch it and go the rails:upgrade path like you suggested in #24962? Other?

@jmccartie jmccartie closed this Aug 28, 2016
@jmccartie jmccartie reopened this Aug 28, 2016
@rafaelfranca rafaelfranca and 1 other commented on an outdated diff Sep 12, 2016
..._record/connection_adapters/abstract_mysql_adapter.rb
@@ -40,7 +40,7 @@ def arel_visitor # :nodoc:
self.emulate_booleans = true
NATIVE_DATABASE_TYPES = {
- primary_key: "int auto_increment PRIMARY KEY",
+ primary_key: "BIGINT(8) UNSIGNED DEFAULT NULL auto_increment PRIMARY KEY",
@rafaelfranca
rafaelfranca Sep 12, 2016 Member

This should not have DEFAULT NULL. It is invalid in MySQL 5.7 #13247

@jmccartie
jmccartie Sep 12, 2016 Contributor

Removed

@rafaelfranca rafaelfranca referenced this pull request in Shopify/rails-bigint-pk Sep 12, 2016
Merged

Remove DEFAULT NULL to support MySQL 5.7 #5

@jmccartie
Contributor

Code Climate shows no issues: https://codeclimate.com/github/rails/rails/pull/26266

Passed — No new issues were found.

Anything else to do on this PR?

@sgrif
Member
sgrif commented Sep 21, 2016

I'll do a final review soon.

@jmccartie
Contributor

Thanks, Sean

On Wed, Sep 21, 2016 at 2:57 PM, Sean Griffin notifications@github.com
wrote:

I'll do a final review soon.


You are receiving this because you modified the open/close state.
Reply to this email directly, view it on GitHub
#26266 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAIJETBVd8uZe-NLNpAlROxED9iUK9Bgks5qsahEgaJpZM4JrgwZ
.

@schneems
Member
schneems commented Oct 3, 2016

What's the timeline on a review? I'm wondering if a complementary gem that errors or warns on keys used with serial instead of bigserial would be helpful. I'm not sure the best way to go about it though.

@sgrif
Member
sgrif commented Nov 1, 2016

It looks like we're the foreign key type mismatch error is only raised for MySQL. Are PostgreSQL and SQLite fine with the mismatched types? (I'm sure we've already discussed that but I'm having a hard time remembering). We should add a test to ensure that the error is correctly raised. Could the message give a specific recommendation as well?

@jmccartie
Contributor
jmccartie commented Nov 1, 2016 edited

@sgrif Thanks for having another look at this

It looks like we're the foreign key type mismatch error is only raised for MySQL. Are PostgreSQL and SQLite fine with the mismatched types?

It's been awhile, so I'll try to remember before jumping all the way back in. If I remember, MySQL was the only one that freaked out -- thus all the workaround code for it. PG is fine. SQLite isn't being touched at all.

Could the message give a specific recommendation as well?

I think we spent quite a bit of time going around on the error message and landed where we did. Happy to add more specificity if you have a suggestion?

@sgrif
Member
sgrif commented Nov 1, 2016

Something like

To fix this, change the type of the #{column_name} column on #{child_table} to be :integer. (For example t.integer #{foreign_key_name}

@sgrif
Member
sgrif commented Nov 1, 2016

Other than the additional test this looks good. Once you've added a test (and updated the error message if you agree with my suggestion) can you squash and rebase this so I can merge?

activerecord/CHANGELOG.md
@@ -1,3 +1,7 @@
+* PostgreSQL & MySQL: Use bigserial as primary key type for new tables
@sgrif
sgrif Nov 1, 2016 Member

We should say big integer since bigserial isn't a thing in MySQL and the serial aspect in postgres just means auto incrementing.

@jmccartie
Contributor

@sgrif

[ ] Once you've added a test (skipped -- existing)
[x] and updated the error message if you agree with my suggestion
[x] can you squash and rebase this so I can merge?

@@ -0,0 +1,49 @@
+require "cases/helper"
+
+class PostgreslqLegacyMigrationTest < ActiveRecord::PostgreSQLTestCase
@qrush
qrush Nov 2, 2016 Contributor

This class name is misspelled - maybe meant PostgresqlLegacyMigrationTest?

@jmccartie
jmccartie Nov 2, 2016 Contributor

Thanks @qrush -- i'll make that change

@sgrif
Member
sgrif commented Nov 2, 2016

The test failures appear to be real

@matthewd
Member
matthewd commented Nov 2, 2016

Are PostgreSQL and SQLite fine with the mismatched types?

We probably covered this in previous discussion too, but I'll note that there's "fine" and then there's 'fine"... is PostgreSQL casting the searched-for value once before hitting the index (good), or casting the comparison values individually (potentially not so good)?

activerecord/lib/active_record/errors.rb
+ # Raised when a foreign key constraint cannot be added because the column type does not match the referenced column type.
+ class MismatchedForeignKey < WrappedDatabaseException
+ def initialize(message)
+ parts = message.scan(/`(\w+)`[ $)]/).flatten
@matthewd
matthewd Nov 2, 2016 Member

I'm uncomfortable about teaching a generic-sounding exception (admittedly, currently only used by one in-tree adapter) how to parse a particular DBMS's error message... even before potential i18n gets involved.

@jmccartie
jmccartie Nov 2, 2016 Contributor

I agree this is brittle. The original implementation simply bubbled up the DBMS error, but this was added by request from Sean. In order to display the column and table names from the error -- and provide the specific message that was requested -- this was the best way I could come up with. I'm happy to back it out, but then need direction on the error message.

@jmccartie
jmccartie Nov 2, 2016 Contributor

@matthewd

Personally, I'd prefer just this:

  class MismatchedForeignKey < WrappedDatabaseException
    def initialize(message=nil)
      msg = "There is a mismatch between the foreign key and primary key column types.\n"
      msg << "Verify that the foreign key column type and the primary key of the associated table match types.\n"
      msg << message if message
      super(msg)
    end
  end

It gives plenty of context when message is passed in.

Current implementation:

Column `old_car_id` on table `engines` has a type of `bigint(20)`.
This does not match column `id` on `old_car`, which has type `int(11)`.
To resolve this issue, change the type of the `old_car_id` column on `engines` to be :integer. (For example `t.integer old_car_id`).

Mysql2::Error: Cannot add foreign key constraint: ALTER TABLE `engines` ADD CONSTRAINT `fk_rails_9f49f34f36`
FOREIGN KEY (`old_car_id`)
  REFERENCES `old_car` (`id`)

Proposed:

There is a mismatch between the foreign key and primary key column types.
Verify that the foreign key column type and the primary key of the associated table match types.

Mysql2::Error: Cannot add foreign key constraint: ALTER TABLE `engines` ADD CONSTRAINT `fk_rails_9f49f34f36`
FOREIGN KEY (`old_car_id`)
  REFERENCES `old_car` (`id`)
activerecord/lib/active_record/errors.rb
+
+ private
+ def column_type(table, column)
+ ActiveRecord::Base.connection.columns(table).detect { |c| c.name == column }.sql_type
@matthewd
matthewd Nov 2, 2016 Member

Assuming ActiveRecord::Base.connection seems unfortunate too

@jmccartie
jmccartie Nov 2, 2016 Contributor

Agreed. This came along with the need to display the specific column and table names. This can be removed if we can agree on a more generic error message.

@sgrif
sgrif Nov 3, 2016 Member

Can we just pass the connection into the constructor, or do this work in the connection that creates this object?

@jmccartie
jmccartie Nov 3, 2016 Contributor

Can do...

- assert_match %r{create_table "widgets", id: :bigint, force: :cascade}, schema
- end
+ if current_adapter?(:PostgreSQLAdapter, :Mysql2Adapter)
+ assert_equal 8, column_type.limit
@matthewd
matthewd Nov 2, 2016 Member

Do we not expect this to be true of other adapters too?

@jmccartie
jmccartie Nov 2, 2016 edited Contributor

This is at the exclusion of SQLite, which would be 4 in this case. Not necessary to test IMHO, but I can add it.

.gitignore
@@ -19,3 +19,4 @@ pkg
/railties/doc
/railties/tmp
/guides/output
+/*/.byebug_history
@matthewd
matthewd Nov 2, 2016 Member

@schneems convince me this belongs here?

@jmccartie
jmccartie Nov 2, 2016 Contributor

I can remove this if it's a big deal...

@jmccartie
Contributor

@matthewd

PostgreSQL casting the searched-for value once before hitting the index (good), or casting the comparison values individually (potentially not so good)?

I don't have the PostgreSQL expertise to verify this. When verifying if PG cared, this is as far as I went:

dev=# create table t1(id integer primary key);
CREATE TABLE
dev=# create table t2(t1_id bigint references t1);
CREATE TABLE
dev=# 

Working on busted tests next...

@jmccartie
Contributor
jmccartie commented Nov 2, 2016 edited

@matthewd

is PostgreSQL casting the searched-for value once before hitting the index (good), or casting the comparison values individually (potentially not so good)?

I checked in with the PG team at Heroku and got this answer:

It's casting it once, because int4 and int8 opclasses are members of the same opfamily. (That's the exact technical reason). Moreover, on little-endian systems, casting int4 to int8 is literally a no-op. But, as far as pg backwards compatibility goes, making things like int4 and int8 play nice together is something that has received a lot of thought...I don't foresee compatibility problems on the pg side.

Let me know if I need to get more info or if that's enough to suffice.

@jmccartie
Contributor

@matthewd I have a local commit ready that simplifies the error message in question. Tests are passing now as well.

Just need to know if that's the direction we want to go -- it's backwards from the specificity that @sgrif asked for a few weeks ago

+class PostgresqlLegacyMigrationTest < ActiveRecord::PostgreSQLTestCase
+ class GenerateTableWithoutBigserial < ActiveRecord::Migration::Compatibility::V5_0
+ def change
+ create_table :legacy_integer_pk do |table|
@kamipo
kamipo Nov 3, 2016 Member

Should test whether Compatibility::V5_0's primary key is auto increment (serial) or not.

  assert LegacyIntegerPk.create!
@schneems
schneems Nov 3, 2016 Member

I don't follow, why do we need to explicitly test the creation?

@jmccartie
jmccartie Nov 3, 2016 Contributor

@kamipo You want to test the creation? or ensure it's auto-increment?

@kamipo
kamipo Nov 3, 2016 Member

Primary key columns cannot be null. Therefore implicit default of primary key should be auto numbered I think. Currently looks like that Compatibility::V5_0's primary key default is not auto numbered.

@jmccartie
jmccartie Nov 3, 2016 Contributor

@kamipo I've modified the create_table method to ensure the ID columns auto-increment. Added tests for both adapters as well.

+ def test_create_table_uses_serial_as_pkey_by_default
+ col = column(:legacy_integer_pk, :id)
+ assert_equal "integer", sql_type_for(col)
+ assert_match(/nextval/, col.default_function)
+
+ def default_primary_key?(column)
+ schema_type(column) == :integer
+ end
@kamipo
kamipo Nov 4, 2016 Member

Looks like this module is unnecessary.

@jmccartie
jmccartie Nov 7, 2016 Contributor

I'd prefer not to remove code that is not directly related to this PR (postgres and mysql)

@kamipo
kamipo Nov 7, 2016 Member

This PR adds sqlite3/schema_dumper.rb even though does not change sqlite3 behavior (only change postgres and mysql). Why?

@jmccartie
jmccartie Nov 7, 2016 Contributor

@kamipo Ah -- forgot that was added (this PR has been going on for awhile 😄 ). The parent class has it set as bigint, so we're locking SQLite at integer.

@kamipo
kamipo Nov 7, 2016 edited Member

Oh, I see. Thanks. This (the parent class has it set as bigint) means that all third party adapters should choose whether implement default bigint pk (and implement Compatibility::V5_0) or override def default_primary_key?(column) schema_type(column) == :integer; end, right?

@jmccartie
jmccartie Nov 7, 2016 edited Contributor

@kamipo That's my understanding. Since the default has changed, other adapters can either choose to join us in bigint land (by doing nothing), or can choose to stay in int land by overriding.

@kamipo
kamipo Nov 9, 2016 Member

I see. Thanks.

@rafaelfranca
rafaelfranca Nov 17, 2016 Member

hmmm. Can we invert this? I think it is a breaking change to require other adapters to change their code to opt-out our default behavior.

@sgrif
sgrif Nov 17, 2016 Member

I'm not sure other adapters would actually need to change here -- SQLite is special cased because it has no bigint type, but it's unique in that regard. However, as long as https://github.com/rails/rails/pull/26266/files#diff-2a8be25f82da6b3935cc6a41300a1b01R112 is specific to those two adapters, I do agree that we should invert this.

@matthewd
matthewd Nov 18, 2016 Member

Yeah, I think the most compatible choice would be to default to pushing everyone to bigint. SQLite just happens to spell that 'integer'.

We've decided everyone's database should use bigint PKs, and that's not something that individual adapters should be revisiting -- their only interest should be if they need to do something special to represent bigint (again, as SQLite does).

(As Sean noted, though, this means the migration compatibility thing needs to change.)

@jmccartie
jmccartie Nov 18, 2016 Contributor

@matthewd @sgrif Trying to follow... what needs to change exactly? I'm reading conflicting opinions and want to get it right.

@sgrif
sgrif Nov 30, 2016 Member

Either https://github.com/rails/rails/pull/26266/files#diff-2a8be25f82da6b3935cc6a41300a1b01R112 needs to be changed to apply to everything except SQLite, or the default implementation of default_primary_key? needs to be reverted to the original implementation on the abstract adapter, and overridden on PostgreSQL and MySQL2. As this is written, any out of tree adapters are going to get incorrect behavior.

@jmccartie
jmccartie Nov 30, 2016 Contributor

Thanks @sgrif -- will do.

@jmccartie
Contributor
jmccartie commented Nov 8, 2016 edited

@sgrif I cannot reproduce the error from Travis locally. Last attempt: bundle exec rake db:mysql:rebuild && bundle exec rake mysql2:test

Passes in isolation also:

$ ARCONN=mysql2 be ruby -w -Itest test/cases/adapters/mysql2/mysql2_adapter_test.rb
Using mysql2
/Users/jmccartie/Code/rails/activerecord/lib/active_record/connection_adapters/mysql/database_statements.rb:90: warning: assigned but unused variable - result
Run options: --seed 3880

# Running:

..........

Finished in 0.100218s, 99.7825 runs/s, 119.7390 assertions/s.

10 runs, 12 assertions, 0 failures, 0 errors, 0 skips

Any ideas or tips?

@sgrif
Member
sgrif commented Nov 8, 2016

Let's take a look tomorrow

@jmccartie
Contributor
@schneems
Member

Ping

@jmccartie
Contributor

@sgrif Ping. I resolved the merge conflicts. Should be all set.

+
+ def default_primary_key?(column)
+ schema_type(column) == :integer
+ end
@rafaelfranca
rafaelfranca Nov 17, 2016 Member

hmmm. Can we invert this? I think it is a breaking change to require other adapters to change their code to opt-out our default behavior.

@jmccartie
Contributor

@matthewd @sgriff Ping. Left a comment here asking for clarification: #26266 (comment)

@jmccartie
Contributor
jmccartie commented Nov 30, 2016 edited

@matthewd @sgriff Ping. Left a comment here asking for clarification: #26266 (comment)

@sgrif
Member
sgrif commented Nov 30, 2016

Left a comment, but please don't repeatedly ping. We have a ton of notifications that we're going through, and this is already on our radar.

@sgrif
Member
sgrif commented Nov 30, 2016

Also you weren't even pinging the right user name. ;)

@jmccartie
Contributor

@sgrif Ack, sorry. 😞 Thanks for the reply.

@jmccartie
Contributor
jmccartie commented Nov 30, 2016 edited

@sgrif Flipped this to an else since the MySQL defaults seem to be good ones. https://github.com/rails/rails/pull/26266/files#diff-2a8be25f82da6b3935cc6a41300a1b01

If those aren't good defaults -- and there aren't good ones for an else statements -- I can revert that and go back to looking at changing default_primary_key behavior.

Let me know. Thanks!

.gitignore
@@ -18,4 +18,4 @@ pkg
/railties/test/initializer/root/log
/railties/doc
/railties/tmp
-/guides/output
+/guides/output
@sgrif
sgrif Nov 30, 2016 Member

git config --global core.autocrlf input

or just fix your editor ;P

@jmccartie
jmccartie Nov 30, 2016 Contributor

@sgrif Made local change, thanks. Need to remove this change (somehow)?

@sgrif
sgrif Nov 30, 2016 Member

Just git checkout master on this file.

@jmccartie
jmccartie Dec 1, 2016 Contributor

@sgrif You've made me facepalm twice today :) Done.

@@ -104,12 +104,30 @@ def index_name_for_remove(table_name, options = {})
class V5_0 < V5_1
def create_table(table_name, options = {})
+<<<<<<< HEAD
@sgrif
sgrif Nov 30, 2016 Member

I don't think this is valid Ruby

@jmccartie
jmccartie Nov 30, 2016 Contributor

@sgrif :facepalm:

activerecord/CHANGELOG.md
@@ -76,6 +80,9 @@
Fixes #24195.
+* Fix an Active Record DateTime field NoMethodError caused by incomplete
+ datetime. [Bug #24195](https://github.com/rails/rails/issues/24195)
+
@kamipo
kamipo Nov 30, 2016 Member

✂️

+ options[:id] = :serial
+ else
+ options[:id] ||= :integer
+ options[:primary_key] = :id
@kamipo
kamipo Dec 5, 2016 Member

This line overwites user provided primary key name. It is not needed.

+ else
+ options[:id] ||= :integer
+ options[:primary_key] = :id
+ options[:auto_increment] = true
@kamipo
kamipo Dec 5, 2016 Member

This else block is unexpected for sqlite3.

sqlite> create table t1(id INTEGER PRIMARY KEY AUTOINCREMENT NOT NULL); -- expected
sqlite> create table t2(id INTEGER PRIMARY KEY AUTO_INCREMENT NOT NULL); -- unexpected
Error: near "AUTO_INCREMENT": syntax error
@jmccartie
Contributor

@kamipo Thanks for the feedback. I'll get those fixed

@jmccartie
Contributor

@kamipo Done. Let me know if the recent changes resolve the issues you pointed out.

+ # keys by default and MySQL uses bigint. This compat layer makes old migrations utilize
+ # serial/int type instead -- the way it used to work before 5.1.
+ if options[:id].blank?
+ options[:id] = (connection_name == "PostgreSQL") ? :serial : :integer
@kamipo
kamipo Dec 5, 2016 Member

options[:id] should be :primary_key (should not be changed) for sqlite3.
:integer type is not auto numbered.

@matthewd
matthewd Dec 5, 2016 Member

Can we improve things by making this:

options[:id] = :integer
options[:auto_increment] = true

... and then leave the SQlite and PostgreSQL adapters to turn that back into whatever internal type they need?

@matthewd
matthewd Dec 5, 2016 Member

My theory here is that we'd be improving the underlying API, by better abstracting the DB-specific :serial etc., and then just using said newly-improved API for the compatibility layer.

@jmccartie
jmccartie Dec 5, 2016 Contributor

👍🏼

+ options[:auto_increment] = true if connection_name == "MySQL"
+ end
+
+ super table_name, options
@kamipo
kamipo Dec 5, 2016 Member

Simply super is enough.

@jmccartie
jmccartie Dec 5, 2016 Contributor

👍🏼

@jmccartie
Contributor

@kamipo @matthewd Updated. Not a huge fan of the case statement, but feels better to me than abstracting overrides into the adapters for this logic. I hope this fits everyone's needs.

@kamipo -- I also added a compatibility test for sqlite3 to make sure that it was covered here.

@jmccartie
Contributor

Codeclimate says failed, but looks to be passing...

image

+
+ def auto_increment?(table_name, column)
+ ActiveRecord::Base.connection.execute("PRAGMA table_info(#{table_name})").find {|col| col["name"] == column }["pk"] == 1
+ end
@kamipo
kamipo Dec 5, 2016 Member

Looks like that this method is primary_key?, not auto_increment?.

+ ActiveRecord::Base.connection.execute("PRAGMA table_info(#{table_name})").find {|col| col["name"] == column }["pk"] == 1
+ end
+
+
@kamipo
kamipo Dec 5, 2016 Member

Seems that it offences against Style/EmptyLines.
https://github.com/rails/rails/blob/master/.rubocop.yml#L27-L29

@jmccartie
Contributor

Updated. Thanks @kamipo

+class MysqlLegacyMigrationTest < ActiveRecord::Mysql2TestCase
+ self.use_transactional_tests = false
+
+ class GenerateTableWithoutBigint < ActiveRecord::Migration::Compatibility::V5_0
@matthewd
matthewd Dec 5, 2016 Member

ActiveRecord::Migration[5.0] (the real class is a hidden implementation detail, so best to stick to what users will do)

@jmccartie
jmccartie Dec 5, 2016 Contributor

@matthewd Good catch. Updated. Thanks.

+
+ # Since 5.1 Postgres adapter uses bigserial type for primary
+ # keys by default and MySQL uses bigint. This compat layer makes old migrations utilize
+ # serial/int type instead -- the way it used to work before 5.1.
@matthewd
matthewd Dec 5, 2016 Member

My worry here is just that enumerating the adapter types implies not-so-great things about the compatibility story for out-of-tree adapters.

cc @yahonda @metaskills

@jmccartie
jmccartie Dec 5, 2016 edited Contributor

@matthewd @yahonda @metaskills Is there a decent set of defaults that will work well?

In your previous comment, you suggested what I have in the else block here. Rather than moving PG and Sqlite logic to the adapters, they're here. While I'm not a massive fan of the case statement, I feel like moving it out is premature. Either way -- here or in the adapters -- we need a set of sane defaults, I think.

@matthewd
matthewd Dec 5, 2016 Member

That's where I was thinking we could define defaults that must work well, and teach our own adapters to support them.

I'm fine with forcing changes onto adapters* -- I just don't want them to have to reach outside their classes to patch this V5_0 class, for example.


* Ultimately we'd like to get to a point where we don't do that so much, but right now, it's par for the course. So while big-picture unfortunate, it's not a negative against a particular approach here & now.

@jmccartie
jmccartie Dec 5, 2016 Contributor

I understand. If you think that's the best way to go, I'll make the change. Can you help me by pointing me to a good spot to place this logic? I want to get it right the first time...

@sgrif
sgrif Dec 5, 2016 Member

I really do think that this is fine. I'm in favor of opting other adapters into the new behavior by default. I'm not aware of any adapter that won't behave the same as the MySQL case.

@jmccartie
jmccartie Dec 5, 2016 edited Contributor

@sgrif Thanks, Sean. Do you mean you're ok with the current case statement? Or want to move this into the adapters? If the latter, can you suggest a good place to put this logic?

Something like this?

self.connection.class::Compatibility::INTEGER_PRIMARY_KEY_OPTIONS
{:id=>:integer}
@sgrif
sgrif Dec 5, 2016 Member

The fact that this required code changes in every adapter makes me hesitate though. Ideally it would either "just work" for third party adapters or require opting in. We should avoid having the default behavior require code changes for them.

@jmccartie
jmccartie Dec 5, 2016 edited Contributor

@sgrif Agreed -- that's why I was hoping there was a good default. it sounds like the else block works here. So I'm suggesting something like this:

if options[:id].blank?
  if self.connection.class.const_defined?("Compatibility")
    options.merge(self.connection.class::Compatibility::INTEGER_PRIMARY_KEY_OPTIONS)
  else
    options[:id] = :integer
    options[:auto_increment] = true
  end
end
@jmccartie
jmccartie Dec 5, 2016 Contributor

note: #27272 made it possible to set the single set of defaults. Change pushed.

@yahonda
yahonda Dec 6, 2016 Contributor

Falling back to options[:id] = :integer looks ok for Oracle enhanced adapter.

jmccartie and others added some commits Aug 21, 2016
@jmccartie jmccartie Change MySQL and Postgresql to use Bigint primary keys b92ae61
@rwz @jmccartie rwz Make pg adapter use bigserial for pk by default
3e452b1
@sgrif sgrif merged commit d7f55e9 into rails:master Dec 5, 2016

2 checks passed

codeclimate Code Climate didn't find any new or fixed issues.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@yahonda yahonda added a commit to yahonda/oracle-enhanced that referenced this pull request Dec 6, 2016
@yahonda yahonda Fallback :bigint to :integer for primary keys
Since Oracle enhanced adapter already handles Rails primary keys as NUMBER(38) Oracle data type.
Here 38 means precision, not bytes to store.

Also Rails :bigint data type is handled as NUMBER(19) Oracle data type which has less precision
than Oracle enhanced adapter :integer. Then this pull request falls back :bigint for primary keys
to Rails :integer.

Refer rails/rails#26266
032fc2d
@yahonda yahonda referenced this pull request in rsim/oracle-enhanced Dec 6, 2016
Merged

Fallback :bigint to :integer for primary keys #1077

@metaskills
Contributor

Cool, thanks everyone. I'll make this happen in the SQL Server work.

@kamipo kamipo added a commit to kamipo/rails that referenced this pull request Dec 11, 2016
@kamipo kamipo `primary_key` and `references` columns should be identical type
Follow up to #26266.

The default type of `primary_key` and `references` were changed to
`bigint` since #26266. But legacy migration and sqlite3 adapter should
keep its previous behavior.
acd8658
@kamipo kamipo added a commit to kamipo/rails that referenced this pull request Dec 11, 2016
@kamipo kamipo `primary_key` and `references` columns should be identical type
Follow up to #26266.

The default type of `primary_key` and `references` were changed to
`bigint` since #26266. But legacy migration and sqlite3 adapter should
keep its previous behavior.
0ff6169
@eileencodes eileencodes added a commit to eileencodes/rails that referenced this pull request Dec 15, 2016
@eileencodes eileencodes Revert "Merge pull request #26266 from jmccartie/jm/bigint"
This reverts commit d7f55e9, reversing
changes made to 575212a.

Conflicts:
	activerecord/CHANGELOG.md
	activerecord/lib/active_record/connection_adapters/abstract_mysql_adapter.rb
	activerecord/lib/active_record/errors.rb
	activerecord/lib/active_record/migration/compatibility.rb
b8cd11b
@eileencodes eileencodes added a commit to eileencodes/rails that referenced this pull request Dec 16, 2016
@eileencodes eileencodes Revert "Merge pull request #26266 from jmccartie/jm/bigint"
This reverts commit d7f55e9, reversing
changes made to 575212a.

Conflicts:
	activerecord/CHANGELOG.md
	activerecord/lib/active_record/connection_adapters/abstract_mysql_adapter.rb
	activerecord/lib/active_record/errors.rb
	activerecord/lib/active_record/migration/compatibility.rb
f35cb4c
@kamipo kamipo added a commit to kamipo/rails that referenced this pull request Dec 17, 2016
@kamipo kamipo Fix MySQL PK dumping correctly
Follow up to #26266.

Currently master branch was lost 1fa6c9e (#22090) due to #26266
and #26266 has another regression #27374.

| primary key definition     | before #26266                     | after #26266                      |
|----------------------------|-----------------------------------|-----------------------------------|
| id: :primary_key (default) | int auto_increment PRIMARY KEY    | bigint auto_increment PRIMARY KEY |
| id: :integer               | int PRIMARY KEY                   | int PRIMARY KEY                   |
| id: :integer, default: nil | int PRIMARY KEY                   | int PRIMARY KEY                   |
| id: :bigint                | bigint auto_increment PRIMARY KEY | bigint PRIMARY KEY                |
| id: :bigint,  default: nil | bigint PRIMARY KEY                | bigint PRIMARY KEY                |

1fa6c9e (#22090) is to ease to create an auto incremental bigint primary
key and to dump the primary key correctly.

Since #26266, changed default primary key and 1fa6c9e (#22090) was lost.
So currently an auto incremental *int* primary key cannot dump (lose
auto increment).

To fix the issue, `integer` and `bigint` primary key has auto increment
as implicit default, and legacy migration still keep its previous
behavior as before #26266.

| primary key definition     | suggested fix                     |
|----------------------------|-----------------------------------|
| id: :primary_key (default) | bigint auto_increment PRIMARY KEY |
| id: :integer               | int auto_increment PRIMARY KEY    |
| id: :integer, default: nil | int PRIMARY KEY                   |
| id: :bigint                | bigint auto_increment PRIMARY KEY |
| id: :bigint,  default: nil | bigint PRIMARY KEY                |
1462844
@kamipo kamipo added a commit to kamipo/rails that referenced this pull request Dec 17, 2016
@kamipo kamipo Fix MySQL PK dumping correctly
Follow up to #26266.

Currently master branch was lost 1fa6c9e (#22090) and #18220 due
to #26266 and #26266 has another regression #27374.

| primary key definition     | before #26266                     | after #26266                      |
|----------------------------|-----------------------------------|-----------------------------------|
| id: :primary_key (default) | int auto_increment PRIMARY KEY    | bigint auto_increment PRIMARY KEY |
| id: :integer               | int PRIMARY KEY                   | int PRIMARY KEY                   |
| id: :integer, default: nil | int PRIMARY KEY                   | int PRIMARY KEY                   |
| id: :bigint                | bigint auto_increment PRIMARY KEY | bigint PRIMARY KEY                |
| id: :bigint,  default: nil | bigint PRIMARY KEY                | bigint PRIMARY KEY                |

1fa6c9e (#22090) and #18220 are to ease to create an auto incremental
(or not) bigint primary key and to dump the primary key correctly.

Since #26266, changed default primary key, and 1fa6c9e (#22090)
and #18220 were lost. And so currently an auto incremental *int* primary
key cannot dump (lose auto increment).

To fix the issue, `integer` and `bigint` primary key has auto increment
as implicit default, and legacy migration still keep its previous
behavior as before #26266.

| primary key definition     | suggested fix                     |
|----------------------------|-----------------------------------|
| id: :primary_key (default) | bigint auto_increment PRIMARY KEY |
| id: :integer               | int auto_increment PRIMARY KEY    |
| id: :integer, default: nil | int PRIMARY KEY                   |
| id: :bigint                | bigint auto_increment PRIMARY KEY |
| id: :bigint,  default: nil | bigint PRIMARY KEY                |
1a76eb9
@kamipo kamipo added a commit to kamipo/rails that referenced this pull request Dec 18, 2016
@kamipo kamipo Restore custom primary key tests lost at #26266
Some custom primary key tests (added at #17631, #17696, #18220, #18228)
were lost at #26266. Restore the tests to improve test coverage.
17fd5df
@kamipo kamipo added a commit to kamipo/rails that referenced this pull request Dec 24, 2016
@kamipo kamipo `primary_key` and `references` columns should be identical type
Follow up to #26266.

The default type of `primary_key` and `references` were changed to
`bigint` since #26266. But legacy migration and sqlite3 adapter should
keep its previous behavior.
5a0e9c0
@bradleybeddoes bradleybeddoes referenced this pull request in elixir-ecto/ecto Dec 28, 2016
Closed

Default primary keys to BIGINT #1879

@kamipo kamipo added a commit to kamipo/rails that referenced this pull request Dec 30, 2016
@kamipo kamipo `primary_key` and `references` columns should be identical type
Follow up to #26266.

The default type of `primary_key` and `references` were changed to
`bigint` since #26266. But legacy migration and sqlite3 adapter should
keep its previous behavior.
f54e702
@kamipo kamipo added a commit to kamipo/rails that referenced this pull request Dec 30, 2016
@kamipo kamipo Fix MySQL PK dumping correctly
Follow up to #26266.

Currently master branch was lost 1fa6c9e (#22090) and #18220 due
to #26266 and #26266 has another regression #27374.

| primary key definition     | before #26266                     | after #26266                      |
|----------------------------|-----------------------------------|-----------------------------------|
| id: :primary_key (default) | int auto_increment PRIMARY KEY    | bigint auto_increment PRIMARY KEY |
| id: :integer               | int PRIMARY KEY                   | int PRIMARY KEY                   |
| id: :integer, default: nil | int PRIMARY KEY                   | int PRIMARY KEY                   |
| id: :bigint                | bigint auto_increment PRIMARY KEY | bigint PRIMARY KEY                |
| id: :bigint,  default: nil | bigint PRIMARY KEY                | bigint PRIMARY KEY                |

1fa6c9e (#22090) and #18220 are to ease to create an auto incremental
(or not) bigint primary key and to dump the primary key correctly.

Since #26266, changed default primary key, and 1fa6c9e (#22090)
and #18220 were lost. And so currently an auto incremental *int* primary
key cannot dump (lose auto increment).

To fix the issue, `integer` and `bigint` primary key has auto increment
as implicit default, and legacy migration still keep its previous
behavior as before #26266.

| primary key definition     | suggested fix                     |
|----------------------------|-----------------------------------|
| id: :primary_key (default) | bigint auto_increment PRIMARY KEY |
| id: :integer               | int auto_increment PRIMARY KEY    |
| id: :integer, default: nil | int PRIMARY KEY                   |
| id: :bigint                | bigint auto_increment PRIMARY KEY |
| id: :bigint,  default: nil | bigint PRIMARY KEY                |
5fe15c8
@kamipo kamipo added a commit to kamipo/rails that referenced this pull request Dec 30, 2016
@kamipo kamipo Restore custom primary key tests lost at #26266
Some custom primary key tests (added at #17631, #17696, #18220, #18228)
were lost at #26266. Restore the tests to improve test coverage.
67bc8b3
@indirect
Member
indirect commented Dec 31, 2016 edited

A tiny note of cautionary experience: BIGINT grows beyond the size that can be represented as a number in JavaScript. If you send IDs to any front-end code, this means you have to represent IDs as strings instead of numbers.

Lest anyone say this is not a real problem and no one needs to worry about it until later, here are two real-world examples of this problem off the top of my head: Twitter IDs, which was a multi-month agonizingly painful change, and last month at one of my consulting clients when a data migration that created just a few records with a randomly selected high IDs broke the entire production front-end JS app until they were deleted from the database.

@kamipo kamipo added a commit to kamipo/rails that referenced this pull request Jan 16, 2017
@kamipo kamipo Fix MySQL PK dumping correctly
Follow up to #26266.

Currently master branch was lost 1fa6c9e (#22090) and #18220 due
to #26266 and #26266 has another regression #27374.

| primary key definition     | before #26266                     | after #26266                      |
|----------------------------|-----------------------------------|-----------------------------------|
| id: :primary_key (default) | int auto_increment PRIMARY KEY    | bigint auto_increment PRIMARY KEY |
| id: :integer               | int PRIMARY KEY                   | int PRIMARY KEY                   |
| id: :integer, default: nil | int PRIMARY KEY                   | int PRIMARY KEY                   |
| id: :bigint                | bigint auto_increment PRIMARY KEY | bigint PRIMARY KEY                |
| id: :bigint,  default: nil | bigint PRIMARY KEY                | bigint PRIMARY KEY                |

1fa6c9e (#22090) and #18220 are to ease to create an auto incremental
(or not) bigint primary key and to dump the primary key correctly.

Since #26266, changed default primary key, and 1fa6c9e (#22090)
and #18220 were lost. And so currently an auto incremental *int* primary
key cannot dump (lose auto increment).

To fix the issue, `integer` and `bigint` primary key has auto increment
as implicit default, and legacy migration still keep its previous
behavior as before #26266.

| primary key definition     | suggested fix                     |
|----------------------------|-----------------------------------|
| id: :primary_key (default) | bigint auto_increment PRIMARY KEY |
| id: :integer               | int auto_increment PRIMARY KEY    |
| id: :integer, default: nil | int PRIMARY KEY                   |
| id: :bigint                | bigint auto_increment PRIMARY KEY |
| id: :bigint,  default: nil | bigint PRIMARY KEY                |
49c39cc
@kamipo kamipo added a commit to kamipo/rails that referenced this pull request Jan 16, 2017
@kamipo kamipo Restore custom primary key tests lost at #26266
Some custom primary key tests (added at #17631, #17696, #18220, #18228)
were lost at #26266. Restore the tests to improve test coverage.
facc301
@kamipo kamipo added a commit to kamipo/rails that referenced this pull request Jan 19, 2017
@kamipo kamipo Restore custom primary key tests lost at #26266
Some custom primary key tests (added at #17631, #17696, #18220, #18228)
were lost at #26266. Restore the tests to improve test coverage.
fb9a1b1
@kamipo kamipo added a commit to kamipo/rails that referenced this pull request Jan 19, 2017
@kamipo kamipo `primary_key` and `references` columns should be identical type
Follow up to #26266.

The default type of `primary_key` and `references` were changed to
`bigint` since #26266. But legacy migration and sqlite3 adapter should
keep its previous behavior.
8e747c5
@kamipo kamipo added a commit to kamipo/rails that referenced this pull request Jan 19, 2017
@kamipo kamipo `primary_key` and `references` columns should be identical type
Follow up to #26266.

The default type of `primary_key` and `references` were changed to
`bigint` since #26266. But legacy migration and sqlite3 adapter should
keep its previous behavior.
5de340d
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment