Re: [Django] #10164: AutoField is broken for sqlite backend
Django
-------------------------------------+-------------------------------------
Reporter: malte | Owner: nobody
Type: Bug | Status: new
Milestone: 1.4 | Component: Database layer
Version: 1.2 | (models, ORM)
Resolution: | Severity: Normal
Triage Stage: Accepted | Keywords: sqlite
Needs documentation: 1 | autoincrement
Patch needs improvement: 1 | Has patch: 1
| Needs tests: 1
| Easy pickings: 0
-------------------------------------+-------------------------------------
Changes (by lukeplant):
* needs_docs: 0 => 1
* needs_tests: 0 => 1
* easy: => 0
Comment:
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.
--
Ticket URL: <http://code.djangoproject.com/ticket/10164#comment:19>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
-------------------------------------+-------------------------------------
Reporter: malte | Owner: nobody
Type: Bug | Status: new
(models, ORM) | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: sqlite | Needs documentation: 1
autoincrement | Patch needs improvement: 1
Easy pickings: 0 |
-------------------------------------+-------------------------------------
* ui_ux: => 0
Comment:
#16992 reports a MySQL/InooDB particularty: The DB engine reuses
AUTO_INCREMENT when/after is restarted.
--
Ticket URL: <https://code.djangoproject.com/ticket/10164#comment:21>
Django
-------------------------------------+-------------------------------------
Reporter: malte | Owner: nobody
Type: Bug | Status: new
Component: Database layer | Version: 1.2
(models, ORM) | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: sqlite | Needs documentation: 1
autoincrement | Patch needs improvement: 1
Has patch: 1 | UI/UX: 0
Needs tests: 1 |
Easy pickings: 0 |
-------------------------------------+-------------------------------------
--
Ticket URL: <https://code.djangoproject.com/ticket/10164#comment:22>
Django
-------------------------------------+-------------------------------------
Reporter: malte | Owner: nobody
Type: Bug | Status: new
Component: Database layer | Version: 1.2
(models, ORM) | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: sqlite | Needs documentation: 1
autoincrement | Patch needs improvement: 1
Has patch: 1 | UI/UX: 0
Needs tests: 1 |
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (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.
--
Ticket URL: <https://code.djangoproject.com/ticket/10164#comment:23>
Django
-------------------------------------+-------------------------------------
Reporter: malte | Owner: nobody
Type: Bug | Status: new
Component: Database layer | Version: 1.2
(models, ORM) | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: sqlite | Needs documentation: 1
autoincrement | Patch needs improvement: 1
Has patch: 1 | UI/UX: 0
Needs tests: 1 |
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (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 ?
--
Ticket URL: <https://code.djangoproject.com/ticket/10164#comment:24>
Django
-------------------------------------+-------------------------------------
Reporter: malte | Owner: nobody
Type: Bug | Status: new
Component: Database layer | Version: 1.2
(models, ORM) | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: sqlite | Needs documentation: 1
autoincrement | Patch needs improvement: 1
Has patch: 1 | UI/UX: 0
Needs tests: 1 |
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by anonymous):
Is there any workaround to have not re-used ids for Django 1.4.3 model
with SQLite?
--
Ticket URL: <https://code.djangoproject.com/ticket/10164#comment:25>
Django
-------------------------------------+-------------------------------------
Reporter: malte | Owner: nobody
Type: Bug | Status: new
Component: Database layer | Version: 1.2
(models, ORM) | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: sqlite | Needs documentation: 1
autoincrement | Patch needs improvement: 1
Has patch: 1 | UI/UX: 0
Needs tests: 1 |
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (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 throw away database, and
none of us are likely to write a patch for this.
But patches welcome!
--
Ticket URL: <https://code.djangoproject.com/ticket/10164#comment:26>
Django
-------------------------------------+-------------------------------------
Reporter: malte | Owner: nobody
Type: Bug | Status: new
Component: Database layer | Version: 1.2
(models, ORM) | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: sqlite | Needs documentation: 1
autoincrement | Patch needs improvement: 1
Has patch: 1 | UI/UX: 0
Needs tests: 1 |
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (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.
--
Ticket URL: <https://code.djangoproject.com/ticket/10164#comment:27>
Django
-------------------------------------+-------------------------------------
Reporter: malte | Owner: nobody
Type: Bug | Status: new
Component: Database layer | Version: 1.2
(models, ORM) | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: sqlite | Needs documentation: 1
autoincrement | Patch needs improvement: 1
Has patch: 1 | UI/UX: 0
Needs tests: 1 |
Easy pickings: 0 |
-------------------------------------+-------------------------------------
* cc: mike@… (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/10164#comment:28>
Django
-------------------------------------+-------------------------------------
Reporter: malte | Owner: nobody
Type: Bug | Status: new
Component: Database layer | Version: 1.2
(models, ORM) | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: sqlite | Needs documentation: 1
autoincrement | Patch needs improvement: 1
Has patch: 1 | UI/UX: 0
Needs tests: 1 |
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by gcc):
Submitted PR https://github.com/django/django/pull/1517 with test and
documentation, and changes requested by @lukeplant.
--
Ticket URL: <https://code.djangoproject.com/ticket/10164#comment:29>
Django
-------------------------------------+-------------------------------------
Reporter: malte | Owner: nobody
Type: Bug | Status: new
Component: Database layer | Version: 1.2
(models, ORM) | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: sqlite | Needs documentation: 1
autoincrement | Patch needs improvement: 1
Has patch: 1 | UI/UX: 0
Needs tests: 1 |
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (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.
--
Ticket URL: <https://code.djangoproject.com/ticket/10164#comment:30>
Django
-------------------------------------+-------------------------------------
Reporter: malte | Owner: nobody
Type: Bug | Status: new
Component: Database layer | Version: 1.2
(models, ORM) | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: sqlite | Needs documentation: 1
autoincrement | Patch needs improvement: 1
Has patch: 1 | UI/UX: 0
Needs tests: 1 |
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (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.
--
Ticket URL: <https://code.djangoproject.com/ticket/10164#comment:31>
Django
-------------------------------------+-------------------------------------
Reporter: malte | Owner: nobody
Type: Bug | Status: new
Component: Database layer | Version: 1.2
(models, ORM) | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: sqlite | Needs documentation: 1
autoincrement | Patch needs improvement: 1
Has patch: 1 | UI/UX: 0
Needs tests: 1 |
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (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...
--
Ticket URL: <https://code.djangoproject.com/ticket/10164#comment:32>
Django
-------------------------------------+-------------------------------------
Reporter: malte | Owner: nobody
Type: Bug | Status: new
Component: Database layer | Version: 1.2
(models, ORM) | Resolution:
Severity: Normal | Triage Stage: Accepted
autoincrement | Patch needs improvement: 1
Has patch: 1 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Changes (by timo):
* needs_docs: 1 => 0
* needs_tests: 1 => 0
Comment:
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 [https://docs.djangoproject.com/en/dev/topics/migrations/#sqlite
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/10164#comment:33>
Django
-------------------------------------+-------------------------------------
Reporter: malte | Owner: nobody
Component: Database layer | Version: 1.2
Severity: Normal | Triage Stage: Accepted
Keywords: sqlite | Needs documentation: 0
autoincrement | Patch needs improvement: 1
Has patch: 1 | UI/UX: 0
Needs tests: 0 |
Easy pickings: 0 |
-------------------------------------+-------------------------------------
* status: new => closed
* resolution: => fixed
Comment:
In [changeset:"eade315da1c8372ac1dfcf1fd20ea87f454d71ac"]:
{{{
#!CommitTicketReference repository=""
revision="eade315da1c8372ac1dfcf1fd20ea87f454d71ac"
Fixed #10164 -- Made AutoField increase monotonically on SQLite
Thanks malte for the report.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/10164#comment:34>
Django
-------------------------------------+-------------------------------------
Reporter: malte | Owner: nobody
Component: Database layer | Version: 1.2
(models, ORM) | Resolution:
Keywords: sqlite | Needs documentation: 0
autoincrement | Patch needs improvement: 1
Has patch: 1 | UI/UX: 0
Needs tests: 0 |
Easy pickings: 0 |
-------------------------------------+-------------------------------------
* status: closed => new
* resolution: fixed =>
Comment:
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
--
Ticket URL: <https://code.djangoproject.com/ticket/10164#comment:35>
Django
-------------------------------------+-------------------------------------
Reporter: malte | Owner: nobody
Type: Bug | Status: new
Component: Database layer | Version: 1.2
(models, ORM) | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: sqlite | Needs documentation: 0
autoincrement | Patch needs improvement: 1
Has patch: 1 | UI/UX: 0
Needs tests: 0 |
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by Tim Graham <timograham@…>):
In [changeset:"910a5760f6f5ffc42b4b264b08057207b8e26106"]:
{{{
#!CommitTicketReference repository=""
revision="910a5760f6f5ffc42b4b264b08057207b8e26106"
Improved release notes for ticket #10164
Thanks Aymeric for the suggestions.
refs #10164
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/10164#comment:36>
Django
-------------------------------------+-------------------------------------
Reporter: malte | Owner: nobody
Component: Database layer | Version: 1.2
Severity: Normal | Triage Stage: Accepted
Keywords: sqlite | Needs documentation: 0
autoincrement | Patch needs improvement: 1
Has patch: 1 | UI/UX: 0
Needs tests: 0 |
Easy pickings: 0 |
-------------------------------------+-------------------------------------
* status: new => closed
* resolution: => fixed
--
Ticket URL: <https://code.djangoproject.com/ticket/10164#comment:37>
Django
-------------------------------------+-------------------------------------
Reporter: malte | Owner: nobody
Type: Bug | Status: closed
Component: Database layer | Version: 1.2
(models, ORM) | Resolution: fixed
Severity: Normal | Triage Stage: Accepted
Keywords: sqlite | Needs documentation: 0
autoincrement | Patch needs improvement: 1
Has patch: 1 | UI/UX: 0
Needs tests: 0 |
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by Ramiro Morales <cramm0@…>):
In [changeset:"6a6136141b133aff7583d51d687719961d261244"]:
{{{
#!CommitTicketReference repository=""
revision="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.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/10164#comment:38>