#876 :limit for primary key columns - Ruby on Rails

Type	To find
responsible:me	tickets assigned to you
tagged:"@high"	tickets tagged @high
milestone:next	tickets in the upcoming milestone
state:invalid	tickets with the state invalid
created:"last week"	tickets created last week
sort:number, importance, updated	tickets sorted by #, importance or updated
Combine keywords for powerful searching.
Use advanced searching »

This project is archived and is in readonly mode.

#876 ✓stale

:limit for primary key columns

Reported by James Le Cuirot | August 21st, 2008 @ 12:04 PM | in 3.0.2

The primary key size for MySQL is currently hardcoded to int(11) in ActiveRecord. I have attempted to add support for the :limit option so you can do something like this.


create_table :people, :id => false do |t|
  t.primary_key :id, :limit => 8
  ...
end

The attached patches (for edge and 2.1) make this work for migrations but I had trouble on the schema dumper side of things and gave up because it was getting too messy. Still, we might be able to come up with the right solution through some discussion here.

The problem was that information about the primary key is retrieved through pk_and_sequence_for. This would effectively have to become pk_and_sequence_and_limit_for and the primary key would have to be explicitly given in the create_table block unless the limit size matches the default. This alone wasn't pretty but then I realised I needed access to extract_limit from the MysqlColumn class before the instances of this class are created. Like I said, messy!

So, any ideas?

Comments and changes to this ticket

You flagged this item as spam.
James Le Cuirot August 21st, 2008 @ 12:12 PM
Thinking about it, I suppose the primary key could always be given explicitly in the create_table block. That would make the underlying code a bit simpler. That doesn't solve the problem though.
You flagged this item as spam.
James Le Cuirot August 21st, 2008 @ 04:47 PM
- Tag changed from “2.1, activerecord, edge, enhancement, experimental, migrations, mysql, patch” to “2.1, activerecord, edge, enhancement, migrations, mysql, patch, tests”
All by best ideas come to me in the shower. :D I later realised that always creating the primary key explicitly in the block does solve the problem since the column instances have been created by that point. Here are new patches, complete with a new test. I had to modify the test for custom primary key names slightly.
You flagged this item as spam.
James Le Cuirot August 21st, 2008 @ 05:18 PM
Sorry, one more fix. Changed the default :limit from 11 to 4, otherwise it'll add :limit in the schema when it doesn't need to since 4 is returned by int, not 11.
- primary_key_limits-2.1.patch 8.2 KB
josh November 22nd, 2008 @ 06:50 PM
- State changed from “new” to “stale”
Still and issue? If so can you rebase for 2.2
You flagged this item as spam.
James Le Cuirot November 22nd, 2008 @ 09:08 PM
- Tag changed from “2.1, activerecord, edge, enhancement, migrations, mysql, patch, tests” to “2.1, 2.2, activerecord, edge, enhancement, migrations, mysql, patch, tests”
Yes, this is still needed. Here's the patch rebased against 2-2-stable. It also applies cleanly against master.
- primary_key_limits-2.2.patch 8.3 KB
josh November 23rd, 2008 @ 06:32 AM
- State changed from “stale” to “open”
Pratik March 8th, 2009 @ 12:49 PM
- Assigned user set to “Pratik”
- State changed from “open” to “wontfix”
Why is the patch changing the default limit in NATIVE_DATABASE_TYPES ? Also, the default schema dumper shouldn't be changed here.

Considering the complexity of the patch and the gains by it, I'm inclined to suggest you just use change_column instead of trying to change the limit when creating the table. Does that work for you ?

Thanks.
You flagged this item as spam.
James Le Cuirot March 8th, 2009 @ 01:09 PM
change_column ignores the limit option for primary keys so that doesn't work.

Although the patch looks a little big, it actually simplifies the existing code by making primary keys less of a special case. I think this is ideally what you want.

The change in NATIVE_DATABASE_TYPES isn't really a change. Remember that the byte sizes in MySQL get translated to string lengths by the type_to_sql function. 4 is the equivalent byte size for a string length of 11.

I used this patch a lot in a previous project and never had a single problem with it.
You flagged this item as spam.
James Le Cuirot March 8th, 2009 @ 01:13 PM
Actually, to expand on what I just said about change_column not working, it's not merely a case of allowing the limit option to take effect because output from the schema dumper still doesn't reflect the new size.
Pratik March 8th, 2009 @ 01:19 PM
Alright. So this patch should probably make change_column work too, if it's not too much work.

Also, I'm not yet so sure about changing schema dumper to always have :id => false. I wonder if it's possible to do that only if the supplied :limit, :name are different from the default

But the rest looks good.

Thanks !
You flagged this item as spam.
James Le Cuirot March 8th, 2009 @ 03:04 PM
The patch already fixes change_column. There is still a " type != :primary_key" condition in the base definition of type_to_sql but the MySQL definition deals with primary keys before this line is reached. The condition should be left there for the other databases because I don't think they allow you to set the size of the primary key like MySQL does.

Preventing :id => false except when necessary makes it a little messier but very similar to how it was originally so it's no big deal. Here's an updated patch with that included against current edge.
You flagged this item as spam.
James Le Cuirot March 8th, 2009 @ 03:09 PM
Just realised the call to "compact" in the schema dumper can now be removed.
- primary_key_limits-edge.patch 8.5 KB
You flagged this item as spam.
James Le Cuirot March 24th, 2009 @ 11:11 PM
Sorry but I'm concerned that this will get overlooked unless it is reopened.
Pratik March 24th, 2009 @ 11:29 PM
- State changed from “wontfix” to “open”
Rizwan Reza March 26th, 2010 @ 01:10 PM
- Milestone cleared.
- Tag changed from “2.1, 2.2, activerecord, edge, enhancement, migrations, mysql, patch, tests” to “3.0, activerecord, enhancement, migrations, mysql, patch, tests”
Pratik, this looks like a viable addition. Since this doesn't apply on master anymore, would you want a patch that applies cleanly?
Rizwan Reza March 27th, 2010 @ 08:30 AM
This adds the behavior and now passes with all tests.
- 0001-primary_key-now-supports-limit.patch 10 KB
Rizwan Reza March 27th, 2010 @ 08:33 AM
- State changed from “open” to “resolved”
(from [41e5c7ed44fedb95636ef9b7a792c46ea03309bd]) primary_key now supports :limit. [#876 state:resolved]

Signed-off-by: wycats wycats@gmail.com
http://github.com/rails/rails/commit/41e5c7ed44fedb95636ef9b7a792c4...
Repository March 27th, 2010 @ 10:07 AM
(from [0cb3311d06c02649fb7444c34b6fdf2214ab85f5]) Revert "primary_key now supports :limit. [#876 state:resolved]" since it broke AR test suite.

This reverts commit 41e5c7ed44fedb95636ef9b7a792c46ea03309bd.
http://github.com/rails/rails/commit/0cb3311d06c02649fb7444c34b6fdf...
José Valim March 27th, 2010 @ 10:07 AM
- State changed from “resolved” to “open”
I had to revert since it broke AR test suite.

José Valim March 27th, 2010 @ 10:08 AM

This is the failing test:

1) Failure: test_mysql_schema_dump_should_honor_primary_keys_limits(SchemaDumperTest)

[./test/cases/schema_dumper_test.rb:176:in `test_mysql_schema_dump_should_honor_primary_keys_limits'
 /Users/jose/Work/github/rails/activesupport/lib/active_support/testing/setup_and_teardown.rb:64:in `__send__'
 /Users/jose/Work/github/rails/activesupport/lib/active_support/testing/setup_and_teardown.rb:64:in `run'
 /Users/jose/Work/github/rails/activesupport/lib/active_support/callbacks.rb:412:in `_run_setup_callbacks'
 /Users/jose/Work/github/rails/activesupport/lib/active_support/testing/setup_and_teardown.rb:62:in `run']:

limit option not found on primary key.
<"|t|\n t.string "nick", :null => false\n t.string "name"\n "> expected to be =~
</t.primary_key\s+"id",\s+:limit => \d+$/>.

Rizwan Reza March 27th, 2010 @ 10:10 AM
Strange, I tested this and it was passing. :(

Are you on Ruby 1.9?
José Valim March 27th, 2010 @ 10:12 AM
- Assigned user changed from “Pratik” to “José Valim”
1.8.7
Rizwan Reza March 27th, 2010 @ 10:19 AM
José, can you test this again please? You might need to recreate databases. Thanks.
José Valim March 27th, 2010 @ 10:25 AM
The error appears when running the test suite with rake test_sqlite3. I recreated sqlite database, still get it.
Rizwan Reza March 27th, 2010 @ 10:27 AM
Thanks, fix coming. :)
Rizwan Reza March 27th, 2010 @ 10:48 AM
The tests now pass for the whole ActiveRecord suite. Thanks for the mention, José. :-)

Needless to say, this also fixes the parsing error on 1.9.
- 0001-primary_key-now-supports-limit-for-mysql.patch 10 KB
Rizwan Reza March 27th, 2010 @ 10:55 AM
- State changed from “open” to “resolved”
Fixed: http://github.com/rails/rails/commit/faeca694b3d4afebf6b623b493e867...
You flagged this item as spam.
James Le Cuirot March 27th, 2010 @ 11:26 AM
- Assigned user changed from “José Valim” to “Rizwan Reza”
Thanks guys. I tried to update the patch myself last night but I couldn't get 3.0 to go. Hadn't tried it before.

Just one concern. In schema_dumper.rb, the spec[:limit]= line used to come after the spec[:type]= line. The spec[:limit]= line is conditional on spec[:type] != 'decimal'. That will always be false now. I think you need to rearrange this section a bit.
You flagged this item as spam.
James Le Cuirot March 27th, 2010 @ 11:26 AM
Sorry, I meant that will always be TRUE now.
Repository March 27th, 2010 @ 01:40 PM
- State changed from “resolved” to “open”
(from [ff522cf4bcb31420baee62aa3c24c98959d80cdf]) Revert "primary_key now supports :limit for MySQL". Break Sam Ruby app. To reproduce, start a new application, create a scaffold and run test suite. [#876 state:open]

This reverts commit faeca694b3d4afebf6b623b493e86731e773c462.
http://github.com/rails/rails/commit/ff522cf4bcb31420baee62aa3c24c9...
José Valim March 27th, 2010 @ 09:39 PM
Please also check comments here:

http://github.com/rails/rails/commit/41e5c7ed44fedb95636ef9b7a792c4...
Rizwan Reza March 27th, 2010 @ 11:56 PM
That error was fixed in the second commit. I am still unsure what is going on. Will dive through a Sam Ruby app to test it. Thanks José and sorry for the troubles. :D
Dan Pickett May 15th, 2010 @ 02:03 AM
- Tag changed from “3.0, activerecord, enhancement, migrations, mysql, patch, tests” to “3.0, activerecord, bugmash, enhancement, migrations, mysql, patch, tests”
You flagged this item as spam.
Michael Raidel May 15th, 2010 @ 12:00 PM
verified when cherry-picking the original commit to the current HEAD

there is an error when running the tests with mysql and fixtures and using a limit below 4. The reason: the automatically calculated ids for the fixtures which are usually above the range of limits between 1-3 (maximum of 8388607).

Is this patch intended to be used for lowering the limit? My use-case would probably always be increasing the limit to allow for bigger ids. In this case we could just disallow the use of limits below 4. If one would want limits below 4 we would have to adjust the automatically generated number for ids.
Rizwan Reza June 8th, 2010 @ 04:38 PM
- State changed from “open” to “stale”
I'm calling off on this one. Please submit a patch if it's still applicable.
You flagged this item as spam.
James Le Cuirot June 8th, 2010 @ 04:52 PM
I might look at it again once I move up to Rails 3. I don't use MySQL anymore but I don't want to see the effort go to waste. To answer Michael's question, it was to increase the limit.
Jeremy Kemper October 15th, 2010 @ 11:01 PM
- Milestone set to “3.0.2”
- Importance changed from “” to “Low”
[bulk edit]