Code

Opened 9 years ago

Last modified 13 days ago

#56 new New feature

Primary key columns should be UNSIGNED

Reported by: Manuzhai <mail@…> Owned by:
Component: Database layer (models, ORM) Version: master
Severity: Normal Keywords: mysql
Cc: lukasz.m.dobrzanski@…, miloslav.pojman@…, zeraien Triage Stage: Accepted
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 4 years ago.
Adding UNSIGNED the AutoField definition for MySQL
test_proj.zip (5.1 KB) - added by ssspiochld 4 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 2 years ago.
56_bigint.diff (9.5 KB) - added by pzinovkin 2 years ago.
Alternative approach to fix #56
56_bigint_new.diff (13.1 KB) - added by pzinovkin 20 months ago.
with tests

Download all attachments as: .zip

Change History (43)

comment:1 Changed 9 years ago by adrian

  • Resolution set to fixed
  • Status changed from new to closed

Fixed in [171]. Thanks!

comment:2 Changed 6 years ago by julianb

  • milestone set to 1.0
  • Resolution fixed deleted
  • Status changed from closed to reopened
  • Version 1.1 deleted

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 Changed 6 years ago by Karen Tracey <kmtracey@…>

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 Changed 6 years ago by julianb

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 Changed 6 years ago by julianb

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 Changed 6 years ago by mtredinnick

  • milestone changed from 1.0 to 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 Changed 5 years ago by philliptemple@…

  • Triage Stage changed from Accepted to 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:8 Changed 5 years ago by anonymous

  • milestone post-1.0 deleted

Milestone post-1.0 deleted

comment:9 Changed 4 years ago by Thomas Steinacher <tom@…>

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 Changed 4 years ago by glassresistor

  • Owner changed from adrian to glassresistor
  • Status changed from reopened to 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 Changed 4 years ago by Thomas Steinacher <tom@…>

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 Changed 4 years ago by zimnyx

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 Changed 4 years ago by adamnelson

  • Triage Stage changed from Ready for checkin to Design decision needed

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

Changed 4 years ago by buzzar

Adding UNSIGNED the AutoField definition for MySQL

comment:14 Changed 4 years ago by anonymous

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

comment:15 Changed 4 years ago by anonymous

  • Keywords mysql added

Changed 4 years ago by ssspiochld

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

comment:16 Changed 4 years ago by ssspiochld

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 Changed 4 years ago by adamnelson

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 Changed 4 years ago by ssspiochld

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 Changed 4 years ago by russellm

  • milestone changed from 1.3 to 2.0
  • Triage Stage changed from Design decision needed to 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 Changed 4 years ago by glassresistor

  • Owner glassresistor deleted

comment:21 Changed 4 years ago by Thomas Steinacher <tom@…>

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

comment:22 follow-up: Changed 4 years ago by russellm

  • milestone 2.0 deleted
  • 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 Changed 3 years ago by mila

  • Cc miloslav.pojman@… added

comment:24 Changed 3 years ago by lrekucki

  • Severity changed from normal to Normal
  • Type changed from enhancement to New feature

comment:25 Changed 3 years ago by zeraien

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

+1

comment:26 in reply to: ↑ 22 Changed 3 years ago by anonymous

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.

Changed 2 years ago by pzinovkin

comment:27 Changed 2 years ago by pzinovkin

  • Patch needs improvement unset
  • Triage Stage changed from Accepted to Design decision needed

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

comment:28 Changed 2 years ago by pzinovkin

  • Triage Stage changed from Design decision needed to Accepted

comment:29 Changed 2 years ago by pzinovkin

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 2 years ago by pzinovkin (previous) (diff)

Changed 2 years ago by pzinovkin

Alternative approach to fix #56

comment:30 Changed 2 years ago by pzinovkin

  • Needs documentation unset

comment:31 Changed 21 months ago by akaariai

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 Changed 20 months ago by pzinovkin

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 Changed 20 months ago by pzinovkin

  • Needs tests unset

Changed 20 months ago by pzinovkin

with tests

comment:34 Changed 18 months ago by tim@…

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 Changed 11 months ago by gcc

  • Owner set to gcc
  • Status changed from new to assigned

comment:36 Changed 11 months ago by gcc

  • Owner gcc deleted
  • Status changed from assigned to new

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

comment:37 Changed 11 months ago by gcc

  • Summary changed from MySQL id columns should be UNSIGNED to Primary key columns should be UNSIGNED

comment:38 Changed 13 days ago by kbcmdba@…

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)

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as new
The owner will be changed from (none) to anonymous. Next status will be 'assigned'
as The resolution will be set. Next status will be 'closed'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.