#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)
Change History (40)
by , 16 years ago
Attachment: | sqlite_autoincrement_fix.diff added |
---|
comment:1 by , 16 years ago
Cc: | added |
---|
comment:2 by , 16 years ago
Triage Stage: | Unreviewed → 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 by , 16 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 , 15 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:7 by , 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 , 14 years ago
Cc: | added |
---|
comment:9 by , 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 , 14 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 , 14 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 , 14 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 , 14 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 , 14 years ago
Version: | 1.0 → 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.
follow-up: 16 comment:15 by , 14 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)?
comment:16 by , 14 years ago
Patch needs improvement: | set |
---|---|
Triage Stage: | Design decision needed → 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 by , 14 years ago
Severity: | → Normal |
---|---|
Summary: | AutoField is broken for sqlite backend [patch included] → AutoField is broken for sqlite backend |
Type: | → Bug |
comment:19 by , 14 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:21 by , 13 years ago
UI/UX: | unset |
---|
#16992 reports a similar MySQL/InnoDB particularity: The DB engine reuses AUTO_INCREMENT
when/after it is restarted.
comment:22 by , 13 years ago
Summary: | AutoField is broken for sqlite backend → sqlite3 backend: AutoField values aren't monotonically increasing |
---|
comment:23 by , 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 , 12 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 , 12 years ago
Is there any workaround to have not re-used ids for Django 1.4.3 model with SQLite?
comment:26 by , 12 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!
comment:27 by , 12 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 , 12 years ago
Cc: | added |
---|
comment:29 by , 11 years ago
Submitted PR https://github.com/django/django/pull/1517 with test and documentation, and changes requested by @lukeplant.
comment:30 by , 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 , 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 , 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 , 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 , 11 years ago
Resolution: | → fixed |
---|---|
Status: | new → closed |
comment:35 by , 11 years ago
Resolution: | fixed |
---|---|
Status: | closed → 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:37 by , 11 years ago
Resolution: | → fixed |
---|---|
Status: | new → closed |
fixes the problem described in the ticket