Opened 19 years ago

Closed 3 years ago

#56 closed New feature (wontfix)

Primary key columns should be UNSIGNED

Reported by: Manuzhai <mail@…> Owned by: Caio Ariede
Component: Database layer (models, ORM) Version: dev
Severity: Normal Keywords: mysql
Cc: lukasz.m.dobrzanski@…, miloslav.pojman@…, zeraien, cmawebsite@…, markus.magnuson@… Triage Stage: Unreviewed
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

This gets you twice the integer space, and seems to me to be a nice sanity check.

Don't know if the same would go for Postgres.

Not enough into the code right know to produce a patch myself.

Attachments (5)

mysql_creation.diff (680 bytes ) - added by buzzar 14 years ago.
Adding UNSIGNED the AutoField definition for MySQL
test_proj.zip (5.1 KB ) - added by ssspiochld 14 years ago.
Test project (tests/modeltests/custom_pk could be used but it would require to change the employee_code field definition to AutoField)
56_autoincrement_unsigned.diff (8.7 KB ) - added by pzinovkin 12 years ago.
56_bigint.diff (9.5 KB ) - added by pzinovkin 12 years ago.
Alternative approach to fix #56
56_bigint_new.diff (13.1 KB ) - added by pzinovkin 12 years ago.
with tests

Download all attachments as: .zip

Change History (55)

comment:1 by Adrian Holovaty, 19 years ago

Resolution: fixed
Status: newclosed

Fixed in [171]. Thanks!

comment:2 by Julian Bez, 16 years ago

milestone: 1.0
Resolution: fixed
Status: closedreopened
Version: 1.1

This improvement got lost somehow. I spent half an hour looking in the svn logs to see if there was a reason why it's not still an unsigned integer, but I found none.

comment:3 by Karen Tracey <kmtracey@…>, 16 years ago

It 'got lost' intentionally with the fix for #1500. I do not quite follow the statement there: "Unsigned int would be better, but because of the way AutoField? gets resolved to a signed integer (see #1165) and the fact that a PostgreSQL serial is signed, it cannot be without much more work." so do not know if it still applies as a reason why MySQL AutoFields cannot be unsigned. However it appears to have been an intentional change at one point so someone who does understand it should verify that switching back to unsigned won't cause any problems before flip-flopping the value again.

comment:4 by Julian Bez, 16 years ago

What #1500 says is, that if a foreign key points to an AutoField, we would need to resolve it to the same unsigned integer in MySQL. Currently foreign keys become IntegerFields.

If we want to switch keys to unsigned for MySQL, it seems some kind of ForeignKeyField would be needed, which could then be, like AutoField, unsigned.

comment:5 by Julian Bez, 16 years ago

This issue let me to a bug: #8316

With the patch there, we would have a KeyField and could change AutoField and KeyField to unsigned for MySQL.

comment:6 by Malcolm Tredinnick, 16 years ago

milestone: 1.0post-1.0

This is an really an enhancement request. If somebody really needs it right now they can alter the table column (plus it won't affect existing setups anyway). It can wait until after 1.0 for reconsideration.

comment:7 by philliptemple@…, 15 years ago

Triage Stage: AcceptedReady for checkin

Change line 10 in django/db/backends/mysql/creation.py from:

'AutoField':         'integer AUTO_INCREMENT',

To:

'AutoField':         'integer UNSIGNED AUTO_INCREMENT',

In lines 709-713 of django/db/models/fields/related.py it casts PositiveIntegerField and SmallPositiveIntegerField, as well as AutoField, to IntegerField().db_type(). This makes me assume we do not need to make any changes by changing AutoField to UNSIGNED matching PositiveIntegerField which is already 'integer UNSIGNED'.

Phillip.

comment:8 by (none), 15 years ago

milestone: post-1.0

Milestone post-1.0 deleted

comment:9 by Thomas Steinacher <tom@…>, 14 years ago

Any reason why Django still uses signed ints for auto-incremented primary keys?

CREATE TABLE `app_model` (
    `id` integer AUTO_INCREMENT NOT NULL PRIMARY KEY,

This seems silly to me.

comment:10 by None, 14 years ago

Owner: changed from Adrian Holovaty to None
Status: reopenednew

Are people still interested in this change? Seems useful and wouldn't break anything from what i can tell? If so i can make a patch.

comment:11 by Thomas Steinacher <tom@…>, 14 years ago

Yes, this is important, as using a signed integer is just wrong. There are never IDs below zero and you lose one bit of information.

comment:12 by Piotr Czachur, 14 years ago

Yeah, we want it. It's also important because database won't allow to insert invalid data (negative IDs) - which is always good.
In fact MySQL by default will allow to insert such value (it will modify it before doing insert) and it will generate warning, but in STRICT/TRADITIONAL mode it will reject such value.

comment:13 by Adam Nelson, 14 years ago

Triage Stage: Ready for checkinDesign decision needed

Resetting triage stage so hopefully a patch goes into the next major release or this ticket gets closed.

by buzzar, 14 years ago

Attachment: mysql_creation.diff added

Adding UNSIGNED the AutoField definition for MySQL

comment:14 by anonymous, 14 years ago

Cc: lukasz.m.dobrzanski@… added
Has patch: set
milestone: 1.3
Version: SVN

comment:15 by anonymous, 14 years ago

Keywords: mysql added

by ssspiochld, 14 years ago

Attachment: test_proj.zip added

Test project (tests/modeltests/custom_pk could be used but it would require to change the employee_code field definition to AutoField)

comment:16 by ssspiochld, 14 years ago

As it was claimed by @adamnelson as Design decision needed I wont change the status as accepted/ready for checkin or should I?

comment:17 by Adam Nelson, 14 years ago

Maybe send the ticket to the django-developers group ... It needs a commiter to approve and this ticket is so old it might be hidden.

comment:18 by ssspiochld, 14 years ago

The patch from http://code.djangoproject.com/ticket/1500 extended the byte size of field
used for AutoField (mediumint->int) however by omitting UNSIGNED field from definition the id-space
is not used in the most efficient way. More about the MySQL field definitions and id-space can be found at
http://dev.mysql.com/doc/refman/5.0/en/numeric-types.html

By applying the patch two profits are met (thats what tests in test_proj.zip check):

  • negative values (invalid) for AutoField wont be permitted
  • the usable id-space will become exceeded two times (from 231 to 232)

comment:19 by Russell Keith-Magee, 14 years ago

milestone: 1.32.0
Triage Stage: Design decision neededAccepted

Accepted for 2.0. We can't do this in a particularly friendly backwards compatible way, because it would require an ALTER TABLE for all existing foreign keys.

comment:20 by None, 14 years ago

Owner: None removed

comment:21 by Thomas Steinacher <tom@…>, 14 years ago

@russellm: What makes this backwards-incompatible? People who don't ALTER TABLE should not notice the change.

comment:22 by Russell Keith-Magee, 14 years ago

milestone: 2.0
Needs documentation: set
Needs tests: set
Patch needs improvement: set

The concern isn't about the table itself; it's about the foreign keys referencing the table. A new table pointing to an old table would have a foreign key that would allow invalid values; if you recreate a base table that has foreign keys pointing at it, you will be able to create instances that you can't refer to.

However, talking this over with Malcolm some more, we think we've got a way to do this. Lets add an 'unsigned=False' option to AutoField. By default, it needs to be False for backwards compatibility; when 2.0 comes around, we can change this to be unsigned by default.

The change needs to be made for other backends (e.g., serial under Postgres needs to be changed to bigserial if unsigned=True), but this provides a way to access unsigned representations in a completely opt-in way for people who want them, and provides a simple way to migrate forward.

comment:23 by Miloslav Pojman, 13 years ago

Cc: miloslav.pojman@… added

comment:24 by Łukasz Rekucki, 13 years ago

Severity: normalNormal
Type: enhancementNew feature

comment:25 by zeraien, 13 years ago

Cc: zeraien added
Easy pickings: unset
UI/UX: unset

+1

in reply to:  22 comment:26 by anonymous, 13 years ago

Replying to russellm:

However, talking this over with Malcolm some more, we think we've got a way to do this. Lets add an 'unsigned=False' option to AutoField. By default, it needs to be False for backwards compatibility; when 2.0 comes around, we can change this to be unsigned by default.

This makes a lot of sence else there will be issues with developers creating new models and finding all sorts of referencing issues. In the next major release this can then be changed to unsigned by default and will remove the need for the developer to set the flag by default.

by pzinovkin, 12 years ago

comment:27 by pzinovkin, 12 years ago

Patch needs improvement: unset
Triage Stage: AcceptedDesign decision needed

Submitted patch with unsigned option for AutoField.
Patch includes rel_db_type_cleanup.diff from #13774

comment:28 by pzinovkin, 12 years ago

Triage Stage: Design decision neededAccepted

comment:29 by pzinovkin, 12 years ago

https://github.com/django/django/pull/62 or see 56_bigint.diff

Alternative approach to fix ticket 56.
Based on discussion https://github.com/django/django/pull/49
This approach doesn't change AutoField behavior. Only those who need extended int range may use it.
It keeps values ranges consistent between databases. Also it works nice with related fields.

Last edited 12 years ago by pzinovkin (previous) (diff)

by pzinovkin, 12 years ago

Attachment: 56_bigint.diff added

Alternative approach to fix #56

comment:30 by pzinovkin, 12 years ago

Needs documentation: unset

comment:31 by Anssi Kääriäinen, 12 years ago

To me the alternate approach seems fine. It does need some tests though, mainly to make sure the related field type matching works properly.

The alternate approach doesn't directly resolve this ticket's issue though. Unsigned is different than bigint. I wonder if adding also unsigned flag should be done?

comment:32 by pzinovkin, 12 years ago

https://github.com/django/django/pull/308

There is no need to use unsigned field to increase id-space. It will only introduce inconsistency between database engines.

comment:33 by pzinovkin, 12 years ago

Needs tests: unset

by pzinovkin, 12 years ago

Attachment: 56_bigint_new.diff added

with tests

comment:34 by tim@…, 11 years ago

I disagree with pzinovkin. Using unsigned integers basically doubles the amount of id's that can be used. That has benefits no matter which integer type is used. But in the case of an INT, 2 billion id's versus 4 billion for the same cost is substantial. Different RDBMS' are not built the same so trying to treat them all the same is a potential recipe for poor scalability.

comment:35 by Chris Wilson, 11 years ago

Owner: set to Chris Wilson
Status: newassigned

comment:36 by Chris Wilson, 11 years ago

Owner: Chris Wilson removed
Status: assignednew

Created a pull request with pzinovkin's patch and tests.

comment:37 by Chris Wilson, 11 years ago

Summary: MySQL id columns should be UNSIGNEDPrimary key columns should be UNSIGNED

comment:38 by kbcmdba@…, 10 years ago

MySQL 5.5.29 and up (plus other versions in other releases of MySQL) now require AUTO_INCREMENT columns to be UNSIGNED. The return value of LAST_INSERT_ID() had to change to BIGINT UNSIGNED because users who had BIGINT UNSIGNED column types were unable to get the ID value when the ID was greater than 9,223,372,036,554,775,807 (BIGINT MAX / 2 - 1 or about 9 quintillion) but still less than the highest possible BIGINT VALUE (18,446,744,023,709,551,615 or about 18 quintillion).

It no longer makes sense to allow negative AUTO_INCREMENT values by default in MySQL persistence layers. I strongly recommend changing the default data type on auto_increment columns to explicitly become UNSIGNED, especially when the persistence layer is known to be MySQL.

Here's the specific MySQL 5.5.29 release note:

  • Incompatible Change: LAST_INSERT_ID(expr) did not work for expr values greater than the largest signed BIGINT value. Such arguments now are accepted, with some consequences for compatibility with previous versions:

LAST_INSERT_ID() now returns a BIGINT UNSIGNED value, not a BIGINT (signed) value.
LAST_INSERT_ID(expr) now returns an unsigned integer value, not a signed integer value.
For AUTO_INCREMENT columns, negative values are no longer supported.

(Bug #20964, Bug #11745891)

comment:39 by Tim Graham, 10 years ago

Patch needs improvement: set

The DatabaseCreation classes are now deprecated, so the patch will need to be refreshed to use SchemaEditor instead.

comment:40 by Christopher D'Cunha, 10 years ago

Owner: set to Christopher D'Cunha
Status: newassigned

comment:41 by Collin Anderson, 9 years ago

Cc: cmawebsite@… added

comment:42 by Markus Amalthea Magnuson, 9 years ago

Cc: markus.magnuson@… added
Owner: changed from Christopher D'Cunha to Markus Amalthea Magnuson

I've rewritten pzinovkin's patch based on the previous pull request from gcc:

https://github.com/alimony/django/compare/master...alimony:unsigned-primary-keys

Did not open a pull request yet as it's not ready, most importantly the tests are failing:

As far as I can see, the new test_can_use_large_primary_keys test can never have passed on postgres. There is no concept of unsigned in postgres, meaning a primary key bigger than 2147483648 only fits in a bigint/bigserial. The latter is used for the AutoField itself in this patch, but as someone pointed out, having a foreign key reference such large primary key fails.

The updated AutoField in the patch determines the field type of a foreign key that references itself like this:

def rel_db_type(self, connection):
    db_type = 'PositiveIntegerField' if self.unsigned else 'IntegerField'
    return self._internal_to_db_type(db_type, connection)

But both PositiveIntegerField and IntegerField resolve to integer in the case of postgres.

Some ways it can ever do anything else, that I can think of now, are:

  1. Set related_fields_match_type to True for postgres.
  2. Make PositiveIntegerField mean a bigint for postgres.
  3. Make the rel_db_type method of the new AutoField do some special treatment to return e.g. BigIntegerField for postgres if self.unsigned is true.

(The first two are of course out of the question.)

I want to write proper tests for this patch to make sure everything behaves on every database, but I guess there needs to be some sort of consensus first on how to solve the above. And people with far more knowledge on Django than me will have to reach that consensus :)

comment:43 by Markus Amalthea Magnuson, 8 years ago

Owner: Markus Amalthea Magnuson removed
Status: assignednew

I'm not actively working on this, feel free to pick up where I left off, anyone.

comment:44 by Collin Anderson, 7 years ago

Patch needs improvement: unset

https://github.com/django/django/pull/8183

I just ran into a situation where I have a legacy mysql table that has a bigint(20) unsigned auto_increment primary key that I want to point a new ForeignKey to. Django is creating the foreign key as bigint(20) rather than bigint(20) unsigned, and mysql won't allow a foreign key constraint between the two.

So basically, I need the legacy table primary key to be a PositiveBigAutoField, and the ForeignKey needs to be a PositiveBigIntegerField.

I'm not super attached to adding this, but I think it could solve this ancient ticket.

Last edited 7 years ago by Collin Anderson (previous) (diff)

comment:45 by Tim Graham, 7 years ago

Patch needs improvement: set

comment:46 by Collin Anderson, 7 years ago

Patch needs improvement: unset

Patch should address the latest feedback.

comment:47 by Tim Graham, 7 years ago

Patch needs improvement: set

comment:48 by Caio Ariede, 5 years ago

Owner: set to Caio Ariede
Status: newassigned

I'll try to revive this ticket

comment:50 by Mariusz Felisiak, 3 years ago

Patch needs improvement: unset
Resolution: wontfix
Status: assignedclosed
Triage Stage: AcceptedUnreviewed

I don't think it's necessary anymore to change the default behavior. We have PositiveBigIntegerField in Django 3.1 (#30987) and a way to change the default AutoField in Django 3.2+ (#31007). As a workaround folks can use DEFAULT_AUTO_FIELD = 'django.db.models.BigAutoField' or an explicit primary key id = models.PositiveBigIntegerField(primary_key=True), see the mailing list discussion.

Note: See TracTickets for help on using tickets.
Back to Top