Code

Opened 5 years ago

Closed 7 months ago

Last modified 4 months ago

#10164 closed Bug (fixed)

sqlite3 backend: AutoField values aren't monotonically increasing

Reported by: malte Owned by: nobody
Component: Database layer (models, ORM) Version: 1.2
Severity: Normal Keywords: sqlite autoincrement
Cc: ivazquez, mattimustang, mike@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description

If you try to use the 'AutoField' with sqlite, django will create a primary key table. This is not an autoincrementing table, as per sqlites manual.
In order to create a true autoinc field in sqlite (one that guarantees all rows unique id:s in the autoinc field) you must suffix the field creation line with 'AUTOINCREMENT', and this must come last in the field syntax.

The attached path implements a data_types_suffix dictionary, that can be optionally added to each backend in its creation.py file, to accomplish adding the 'AUTOINCREMENT' directive. Sqlite and its special way to create autoinc fields is the only case I know of that needs this functionality.

Attachments (2)

sqlite_autoincrement_fix.diff (2.1 KB) - added by malte 5 years ago.
fixes the problem described in the ticket
Django-1.3-sqlite.patch (2.2 KB) - added by Jonttu 3 years ago.
Patch modified for 1.3

Download all attachments as: .zip

Change History (40)

Changed 5 years ago by malte

fixes the problem described in the ticket

comment:1 Changed 5 years ago by ivazquez

  • Cc ivazquez added
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

comment:2 Changed 5 years ago by jacob

  • Triage Stage changed from Unreviewed to Design decision needed

I'm positive that AutoField isn't "broken" for sqlite: if it was, we'd be seeing a million test failures on sqlite since nearly every test case in the tree uses an AutoField. Since currently all tests pass on sqlite, I have to assume there's something more going on here.

So, can you clarify what you mean by "broken"? What's the problem you're seeing?

comment:3 Changed 5 years ago by mtredinnick

Presumably this is in reference to this django-users thread. There was a bit of confusion as to the problem being reported there initially, but I think I'm happy saying in the end that there isn't really a bug here, except for a minor documentation/expectation issue.

We say (in the docs) that AutoField is an always-incrementing field, but it could go back and reuse some old numbers, the way we've implemented it for !SQLite. I'd probably vote for just fixing the docs here, simply because I prefer maintaining consistency in the SQL we produce, unless there's a functionality bug.

comment:4 Changed 4 years ago by fwenzel

This should be included. The patch looks like a good solution for it.

Rationale:
In spots where database IDs are exposed publicly, for example as paths: /item/1, deleting the latest item and adding a new one will result in the same ID (and thus, path) being reassigned in SQLite only, and only because Django is not using the auto_increment feature SQLite provides.

For the sake of consistency between DB backends, I suggest accepting the patch.

If needed, I could add a test ensuring the PKs Django generates increase monotonously.

comment:5 Changed 4 years ago by fwenzel

And by monotonously I mean monotonically, of course :)

comment:6 Changed 4 years ago by ramiro

#13852 also reported this.

comment:7 Changed 4 years ago by mattimustang

Hi,

This issue becomes a problem when using GenericRelations and sqlite3.

from django.db import models
from django.contrib.contenttypes.models import ContentType
from django.contrib.contenttypes import generic

class TaggedItem(models.Model):
    tag = models.SlugField()
    content_type = models.ForeignKey(ContentType)
    object_id = models.PositiveIntegerField()
    content_object = generic.GenericForeignKey('content_type', 'object_id')

    def __unicode__(self):
        return self.tag

class Item(models.Model):
    name = models.CharField()

    def __unicode__(self):
        return self.name

Now if I create an Item and assign a tag to it, delete the Item, then create a new Item the tags remain and now point to the new Item. e.g.

>>> from myapp.models import TaggedItem, Item
>>> from django.contrib.contenttypes.models import ContentType
>>> i1 = Item(name='one')
>>> i1.save()
>>> i1.id
1
>>> t = TaggedItem(content_object=i1, tag='foo')
>>> t.save()
>>> item_type = ContentType.objects.get_for_model(i1)
>>> TaggedItem.objects.filter(content_type__pk=item_type.id, object_id=1)
[<TaggedItem: foo>]
>>> i1.delete()
>>> i2 = Item(name='two')
>>> i2.save()
>>> i2.id
1
>>> TaggedItem.objects.filter(content_type__pk=item_type.id, object_id=1)
[<TaggedItem: foo>]

Changing sqlite3 backend to autoincrement would make it behave like postgres. I don't have mysql or oracle available to test their behaviour.

comment:8 Changed 4 years ago by mattimustang

  • Cc mattimustang added

comment:9 Changed 3 years ago by whardier

Is there a status update for this? This appears to not be the case with Django 1.2.3 at the moment.

Simply altering the table after the fact appears to be a fine solution for me.

comment:10 Changed 3 years ago by philipn

mattimustang, mysql and oracle behave the same way as postgresql.

This bug has hit me a few times. If all tests pass, is there any reason this shouldn't be committed?

comment:11 Changed 3 years ago by lukeplant

mattimustang, the problem with your example is that as soon as you delete the original object, you ought to delete the TaggedItem pointing to it, so that is an example of user error.

More compelling use cases are:

  • URLs that expose PKs, and ought to become 404s when the object is deleted, as fwenzel said.
  • Log/audit records that reference the PK, which become misleading when the object is deleted and the PK re-used, and then horribly confused. This is the most compelling example IMO, as your audit records are all being silently corrupted (in effect), and you won't find out until you try to re-construct some data from your audit tables.

comment:12 Changed 3 years ago by mattimustang

Luke,

Your second use case is exactly the problem I have. The example code I wrote above was just to illustrate the issue in as simple a way as possible.

comment:13 Changed 3 years ago by Jonttu

I have a model caching system that relies on unique-not-reused pks and would like to see this patch as part of future django releases

comment:14 Changed 3 years ago by lfagundes

  • Version changed from 1.0 to 1.2

+ 1

Both Postgres and MySQL do not reuse primary keys, but in django's current implementation sqlite does. My tests could run in 2 minutes in sqlite if this bug was fixed (and it looks like the big discussion is considering this a bug), but instead I have to wait for 10 minutes for them to run in MySQL.

Please, include this patch in future releases.

comment:15 follow-up: Changed 3 years ago by johnsmith

  • milestone set to 1.4

2 years seems like a long time for this nobrainer to be outstanding.

Auto increment that doesn't reuse primary keys is the default behavior of the overwhelming majority of production databases (eg MySQL & Postgres), why wouldn't we want that behavior in SQLite (the 'normal' dev environment) as well? Is anyone going to complain if SQLite followed this behavior as well (I think not)?

comment:16 in reply to: ↑ 15 Changed 3 years ago by lukeplant

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

Replying to johnsmith:

2 years seems like a long time for this nobrainer to be outstanding.

We need compelling reasons to change something like this, because it is entirely possible that someone is depending on the existing behaviour in production i.e. people using SQLite for production. So just getting consistency for the sake of people using SQLite for testing only isn't in itself the most important consideration.

And we certainly didn't have a compelling reason for the change 2 years ago. I think we do now though, and no-one has responded otherwise, so I'm going to accept. If Jacob or Malcolm are still unsure, we probably need to have a discussion on django-devs.

The patch needs some work to be updated for trunk. Also, if there is a nice way to do it other than adding an ad-hoc feature addition to the DB backend that is needed by just this exception, that would be good.

We also need tests, either by testing the SQL generated, or by a functional test that ensures that IDs are not re-used.

comment:17 Changed 3 years ago by SmileyChris

  • Severity set to Normal
  • Summary changed from AutoField is broken for sqlite backend [patch included] to AutoField is broken for sqlite backend
  • Type set to Bug

comment:18 Changed 3 years ago by Jonttu

This problem exists also in South

Changed 3 years ago by Jonttu

Patch modified for 1.3

comment:19 Changed 3 years ago by lukeplant

  • Easy pickings unset
  • Needs documentation set
  • Needs tests set

This still needs tests (sorry for not setting the flag before).

I'm still bothered by the creation of a new method on Field to cope with one peculiarity of a single backend, i.e. the need for the autoincrement keyword and the fact that it only allows this keyword at the end, not at the beginning. But I can't see a nicer way of doing it. As to the current patch:

In Field.db_type_suffix, I don't see any need for using the string interpolation using the dictionary. Since we have no use case for that, it would be better to remove it.

We should put db_types_suffix as an empty dictionary on BaseDatabaseCreation, and adjust the error handling in Field.db_type_suffix accordingly i.e. so it only catches KeyError.

We also need some docs - a note in the release notes, for the following reasons:

1) It could potentially cause breakage.
2) The change will require a manual migration for anyone using SQLite in production, if they care about the change. Unfortunately there is no way to do an 'alter column' in SQLite, so this migration is non-trivial.

Many thanks.

comment:20 Changed 3 years ago by jacob

  • milestone 1.4 deleted

Milestone 1.4 deleted

comment:21 Changed 3 years ago by ramiro

  • UI/UX unset

#16992 reports a similar MySQL/InnoDB particularity: The DB engine reuses AUTO_INCREMENT when/after it is restarted.

Last edited 2 years ago by ramiro (previous) (diff)

comment:22 Changed 2 years ago by ramiro

  • Summary changed from AutoField is broken for sqlite backend to sqlite3 backend: AutoField values aren't monotonically increasing

comment:23 Changed 19 months ago by tt@…

I was bitten by this today, and to me the AUTOINCREMENT behavior seems more reasonable than the "integer primary key" behavior.
However, I see that the potential for breakage is very much in place, so changing the default might not be the greatest of ideas.

However, if we could grow an option on the AutoField to get AUTOINCREMENT behavior, I would be very +1 on that.

comment:24 Changed 14 months ago by ad-65

I spent the afternoon tracking down a problem where my audit logs were all messed up. It was caused by this issue and using generic relations. If a row is deleted in SQLite without the 'AUTOINCREMENT' keyword the id is re-used, screwing up my audit trail in the process. I think there are probably quite a few people using SQLite for development and MySQL/PostgreSQL for production. It would be nice if the behaviour was the same.

Having said that I understand about not wanting to change core behaviour after the fact. Would it be possible to add some kind of configuration option to get the 'AUTOINCREMENT' keyword in the table creation SQL as suggested in https://code.djangoproject.com/ticket/10164#comment:23 ?

comment:25 Changed 13 months ago by anonymous

Is there any workaround to have not re-used ids for Django 1.4.3 model with SQLite?

comment:26 Changed 13 months ago by lukeplant

To recent commenters:

This bug is accepted - the current behaviour is regarded as a bug, and we will accept good patches fixing this problem. The limitation is someone willing to write a good patch. You can see my comments on the previous patch above.

However, I should say that you really should consider a better database for production. Even for development, differences between development and production can really bite. Because of this, all experienced Django developers only use SQLite as a toy, strictly for quick throw-away databases and experiments, and so none of us are likely to write a patch for this.

But patches are welcome!

Last edited 13 months ago by lukeplant (previous) (diff)

comment:27 Changed 13 months ago by pacemkr@…

I was also bitten by this today. SQLite is very convenient for early stages of a new project.

contentypes + this issue results in some very weird behavior when you delete something.

comment:28 Changed 12 months ago by carbonXT

  • Cc mike@… added

comment:29 Changed 8 months ago by gcc

Submitted PR https://github.com/django/django/pull/1517 with test and documentation, and changes requested by @lukeplant.

comment:30 Changed 8 months ago by akaariai

I wonder about migrations support for this. We have that now in core...

Otherwise the approach looks good to me. The backwards incompatiblity of this seems minor.

comment:31 Changed 8 months ago by gcc

@akaariai how would we do migrations using what's in core? Isn't it up to developers to freeze their models before and after the upgrade to ensure that a migration takes place, if they want it?

Otherwise we don't automatically muck about with their tables, which seems like a good thing to me.

comment:32 Changed 8 months ago by akaariai

Truthfully, I don't know. I have to ask and see if there is anything that can be done here. It would be nice to have a upgrade script one can run if that is wanted, but which isn't ran automatically. I don't know if that is possible with current migrations framework...

comment:33 Changed 8 months ago by timo

  • Needs documentation unset
  • Needs tests unset

I don't think the migrations framework is really designed to handle this type of thing, particularly since it's SQLite specific. See the migration docs for how migrations work for SQLite. I don't feel we need to provide much help with a respect to a migration here, particularly if a lack of solution is going to further delay fixing this. I've left some minor comments on the PR.

comment:34 Changed 8 months ago by Tim Graham <timograham@…>

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

In eade315da1c8372ac1dfcf1fd20ea87f454d71ac:

Fixed #10164 -- Made AutoField increase monotonically on SQLite

Thanks malte for the report.

comment:35 Changed 8 months ago by aaugustin

  • Resolution fixed deleted
  • Status changed from closed to new

I'm sorry, but the release notes look wrong to me. Do you think my proposal makes sense?
https://github.com/django/django/commit/eade315da1c8372ac1dfcf1fd20ea87f454d71ac#commitcomment-4034261

comment:36 Changed 7 months ago by Tim Graham <timograham@…>

In 910a5760f6f5ffc42b4b264b08057207b8e26106:

Improved release notes for ticket #10164

Thanks Aymeric for the suggestions.

refs #10164

comment:37 Changed 7 months ago by timo

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

comment:38 Changed 4 months ago by Ramiro Morales <cramm0@…>

In 6a6136141b133aff7583d51d687719961d261244:

Implemented #10164 for new schema migration code.

Made it use 'AUTOINCREMENT' suffix for PK creation. This way it doeesn't
regress when compared with the 'traditional' DB backend creation
infrastructure.

Refs #10164.

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


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

 
Note: See TracTickets for help on using tickets.