Opened 15 years ago

Closed 11 years ago

Last modified 10 years 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, Matthew Flanagan, 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 15 years ago.
fixes the problem described in the ticket
Django-1.3-sqlite.patch (2.2 KB ) - added by Jonttu 13 years ago.
Patch modified for 1.3

Download all attachments as: .zip

Change History (40)

by malte, 15 years ago

fixes the problem described in the ticket

comment:1 by ivazquez, 15 years ago

Cc: ivazquez added

comment:2 by Jacob, 15 years ago

Triage Stage: UnreviewedDesign 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 by Malcolm Tredinnick, 15 years ago

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 by fwenzel, 14 years ago

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 by fwenzel, 14 years ago

And by monotonously I mean monotonically, of course :)

comment:6 by Ramiro Morales, 14 years ago

#13852 also reported this.

comment:7 by Matthew Flanagan, 14 years ago

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 by Matthew Flanagan, 14 years ago

Cc: Matthew Flanagan added

comment:9 by whardier, 14 years ago

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 by philipn, 13 years ago

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 by Luke Plant, 13 years ago

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 by Matthew Flanagan, 13 years ago

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 by Jonttu, 13 years ago

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 by lfagundes, 13 years ago

Version: 1.01.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 by johnsmith, 13 years ago

milestone: 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)?

in reply to:  15 comment:16 by Luke Plant, 13 years ago

Patch needs improvement: set
Triage Stage: Design decision neededAccepted

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 by Chris Beaven, 13 years ago

Severity: Normal
Summary: AutoField is broken for sqlite backend [patch included]AutoField is broken for sqlite backend
Type: Bug

comment:18 by Jonttu, 13 years ago

This problem exists also in South

by Jonttu, 13 years ago

Attachment: Django-1.3-sqlite.patch added

Patch modified for 1.3

comment:19 by Luke Plant, 13 years ago

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 by Jacob, 13 years ago

milestone: 1.4

Milestone 1.4 deleted

comment:21 by Ramiro Morales, 13 years ago

UI/UX: unset

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

Last edited 12 years ago by Ramiro Morales (previous) (diff)

comment:22 by Ramiro Morales, 12 years ago

Summary: AutoField is broken for sqlite backendsqlite3 backend: AutoField values aren't monotonically increasing

comment:23 by tt@…, 12 years ago

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 by ad-65, 11 years ago

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 by anonymous, 11 years ago

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

comment:26 by Luke Plant, 11 years ago

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 11 years ago by Luke Plant (previous) (diff)

comment:27 by pacemkr@…, 11 years ago

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 by Mike Fogel, 11 years ago

Cc: mike@… added

comment:29 by Chris Wilson, 11 years ago

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

comment:30 by Anssi Kääriäinen, 11 years ago

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 by Chris Wilson, 11 years ago

@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 by Anssi Kääriäinen, 11 years ago

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 by Tim Graham, 11 years ago

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 by Tim Graham <timograham@…>, 11 years ago

Resolution: fixed
Status: newclosed

In eade315da1c8372ac1dfcf1fd20ea87f454d71ac:

Fixed #10164 -- Made AutoField increase monotonically on SQLite

Thanks malte for the report.

comment:35 by Aymeric Augustin, 11 years ago

Resolution: fixed
Status: closednew

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 by Tim Graham <timograham@…>, 11 years ago

In 910a5760f6f5ffc42b4b264b08057207b8e26106:

Improved release notes for ticket #10164

Thanks Aymeric for the suggestions.

refs #10164

comment:37 by Tim Graham, 11 years ago

Resolution: fixed
Status: newclosed

comment:38 by Ramiro Morales <cramm0@…>, 10 years ago

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.

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