Opened 19 years ago
Closed 4 years ago
#56 closed New feature (wontfix)
Primary key columns should be UNSIGNED
Reported by: | 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)
Change History (55)
comment:1 by , 19 years ago
Resolution: | → fixed |
---|---|
Status: | new → closed |
comment:2 by , 16 years ago
milestone: | → 1.0 |
---|---|
Resolution: | fixed |
Status: | closed → reopened |
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 , 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 , 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 , 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 , 16 years ago
milestone: | 1.0 → post-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 , 16 years ago
Triage Stage: | Accepted → Ready 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:9 by , 15 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 , 15 years ago
Owner: | changed from | to
---|---|
Status: | reopened → new |
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 , 15 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 , 15 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 , 15 years ago
Triage Stage: | Ready for checkin → Design decision needed |
---|
Resetting triage stage so hopefully a patch goes into the next major release or this ticket gets closed.
by , 15 years ago
Attachment: | mysql_creation.diff added |
---|
Adding UNSIGNED the AutoField definition for MySQL
comment:14 by , 15 years ago
Cc: | added |
---|---|
Has patch: | set |
milestone: | → 1.3 |
Version: | → SVN |
comment:15 by , 15 years ago
Keywords: | mysql added |
---|
by , 15 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 , 15 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 , 15 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 , 15 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 , 14 years ago
milestone: | 1.3 → 2.0 |
---|---|
Triage Stage: | Design decision needed → Accepted |
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 , 14 years ago
Owner: | removed |
---|
comment:21 by , 14 years ago
@russellm: What makes this backwards-incompatible? People who don't ALTER TABLE should not notice the change.
follow-up: 26 comment:22 by , 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 , 14 years ago
Cc: | added |
---|
comment:24 by , 14 years ago
Severity: | normal → Normal |
---|---|
Type: | enhancement → New feature |
comment:26 by , 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 , 13 years ago
Attachment: | 56_autoincrement_unsigned.diff added |
---|
comment:27 by , 13 years ago
Patch needs improvement: | unset |
---|---|
Triage Stage: | Accepted → Design decision needed |
Submitted patch with unsigned option for AutoField.
Patch includes rel_db_type_cleanup.diff from #13774
comment:28 by , 13 years ago
Triage Stage: | Design decision needed → Accepted |
---|
comment:29 by , 13 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.
comment:30 by , 13 years ago
Needs documentation: | unset |
---|
comment:31 by , 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 , 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 , 12 years ago
Needs tests: | unset |
---|
comment:34 by , 12 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 , 12 years ago
Owner: | set to |
---|---|
Status: | new → assigned |
comment:36 by , 12 years ago
Owner: | removed |
---|---|
Status: | assigned → new |
Created a pull request with pzinovkin's patch and tests.
comment:37 by , 12 years ago
Summary: | MySQL id columns should be UNSIGNED → Primary key columns should be UNSIGNED |
---|
comment:38 by , 11 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.
comment:39 by , 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 , 10 years ago
Owner: | set to |
---|---|
Status: | new → assigned |
comment:41 by , 10 years ago
Cc: | added |
---|
comment:42 by , 9 years ago
Cc: | added |
---|---|
Owner: | changed from | to
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:
- Set
related_fields_match_type
toTrue
for postgres. - Make
PositiveIntegerField
mean abigint
for postgres. - Make the
rel_db_type
method of the newAutoField
do some special treatment to return e.g.BigIntegerField
for postgres ifself.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 , 9 years ago
Owner: | removed |
---|---|
Status: | assigned → new |
I'm not actively working on this, feel free to pick up where I left off, anyone.
comment:44 by , 8 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.
comment:45 by , 8 years ago
Patch needs improvement: | set |
---|
comment:46 by , 8 years ago
Patch needs improvement: | unset |
---|
Patch should address the latest feedback.
comment:47 by , 7 years ago
Patch needs improvement: | set |
---|
comment:50 by , 4 years ago
Patch needs improvement: | unset |
---|---|
Resolution: | → wontfix |
Status: | assigned → closed |
Triage Stage: | Accepted → Unreviewed |
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.
Fixed in [171]. Thanks!