sqlite3 backend: AutoField values aren't monotonically increasing

[malte]

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.

[malte]

fixes the problem described in the ticket

[Jacob]

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?

[Malcolm Tredinnick]

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.

[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.

[fwenzel]

And by monotonously I mean monotonically, of course :)

[Ramiro Morales]

#13852 also reported this.

[Matthew Flanagan]

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.

[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.

[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?

[Luke Plant]

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.

[Matthew Flanagan]

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.

[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

[lfagundes]

+ 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.

[johnsmith]

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

[Luke Plant]

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.

[Jonttu]

This problem exists also in South

[Jonttu]

Patch modified for 1.3

[Luke Plant]

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.

[Jacob]

Milestone 1.4 deleted

[Ramiro Morales]

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

[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.

[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 ?

[anonymous]

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

[Luke Plant]

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!

[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.

[Chris Wilson]

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

[Anssi Kääriäinen]

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.

[Chris Wilson]

@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.

[Anssi Kääriäinen]

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...

[Tim Graham]

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.

[Tim Graham <timograham@…>]

In eade315da1c8372ac1dfcf1fd20ea87f454d71ac:

Fixed #10164 -- Made AutoField increase monotonically on SQLite

Thanks malte for the report.

[Aymeric Augustin]

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

[Tim Graham <timograham@…>]

In 910a5760f6f5ffc42b4b264b08057207b8e26106:

Improved release notes for ticket #10164

Thanks Aymeric for the suggestions.

refs #10164

[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.