[Django] #31730: manage.py sqlsequencereset not implemented for sqlite3
Django
-------------------------------------+-------------------------------------
Reporter: axil | Owner: nobody
Type: Bug | Status: new
Component: Database | Version: 3.0
layer (models, ORM) | Keywords: sql sequence reset
Severity: Normal | sqlite3
Triage Stage: | Has patch: 1
Unreviewed |
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-------------------------------------+-------------------------------------
Currently `./manage.py sqlsequencereset`
([https://docs.djangoproject.com/en/3.0/ref/django-admin/#sqlsequencereset
docs]) never prints anything for the sqlite3 backend.
It happens because the function `sequence_reset_sql` defined in
`django/django/db/backends/base/operations.py`
is not implemented for the sqlite3 backend in
`django/django/db/backends/sqlite3/operations.py`
`django/django/contrib/sites/management.py` makes use of this function
{{{
# We set an explicit pk instead of relying on auto-incrementation,
# so we need to reset the database sequence. See #17415.
sequence_sql = connections[using].ops.sequence_reset_sql(no_style(),
[Site])
if sequence_sql:
if verbosity >= 2:
print("Resetting sequence")
}}}
Here `sequence_reset_sql` also does nothing for sqlite3, but sqlite3 is
pretty lenient to
the sequences, so it didn't result in an exception.
Same applies to its usage in `manage.py loaddata`: sqlite3 fixes the
sequences automatically.
What it cannot do automatically is reset the sequence to zero as
demonstrated in the new test
`django/tests/backends/tests.py:SequenceResetTest.test_reset_sequence`
{{{
def test_reset_sequence(self):
Post.objects.create(name='1st post', text='hello world')
Post.objects.all().delete()
# Reset the sequences for the database
commands =
connections[DEFAULT_DB_ALIAS].ops.sequence_reset_sql(no_style(), [Post])
with connection.cursor() as cursor:
for sql in commands:
cursor.execute(sql)
# If we create a new object now, it should have a PK greater
# than the PK we specified manually.
obj = Post.objects.create(name='New post', text='goodbye world')
self.assertEqual(obj.pk, 1)
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/31730>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
-------------------------------------+-------------------------------------
Reporter: axil | Owner: nobody
Type: Bug | Status: new
(models, ORM) |
Severity: Normal | Resolution:
Keywords: sql sequence reset | Triage Stage:
sqlite3 | Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by axil):
https://github.com/django/django/pull/13093
--
Ticket URL: <https://code.djangoproject.com/ticket/31730#comment:1>
Django
-------------------------------------+-------------------------------------
Reporter: axil | Owner: nobody
Type: Bug | Status: new
(models, ORM) |
Severity: Normal | Resolution:
Keywords: sql sequence reset | Triage Stage:
sqlite3 | Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by axil):
The fix is related and partly based on the code from the recent ticket
#31479 as well as on the corresponding functions for postgres and oracle.
--
Ticket URL: <https://code.djangoproject.com/ticket/31730#comment:2>
Django
-------------------------------------+-------------------------------------
Reporter: axil | Owner: nobody
Type: Bug | Status: new
(models, ORM) |
Severity: Normal | Resolution:
Keywords: sql sequence reset | Triage Stage:
sqlite3 | Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Old description:
> Currently `./manage.py sqlsequencereset`
> ([https://docs.djangoproject.com/en/3.0/ref/django-
> admin/#sqlsequencereset docs]) never prints anything for the sqlite3
New description:
Currently `./manage.py sqlsequencereset`
([https://docs.djangoproject.com/en/3.0/ref/django-admin/#sqlsequencereset
docs]) never prints anything for the sqlite3 backend.
It happens because the function `sequence_reset_sql` defined in
`django/django/db/backends/base/operations.py`
is not implemented for the sqlite3 backend in
`django/django/db/backends/sqlite3/operations.py`
`django/django/contrib/sites/management.py` makes use of this function
{{{
# We set an explicit pk instead of relying on auto-incrementation,
# so we need to reset the database sequence. See #17415.
sequence_sql = connections[using].ops.sequence_reset_sql(no_style(),
[Site])
if sequence_sql:
if verbosity >= 2:
print("Resetting sequence")
}}}
Here `sequence_reset_sql` also does nothing for sqlite3, but sqlite3 is
pretty lenient to
the sequences, so it didn't result in an exception.
Same applies to its usage in `manage.py loaddata`: sqlite3 fixes the
sequences automatically.
Leniency of sqlite is also the explanation of how
`django/tests/backends/tests.py:SequenceResetTest.test_generic_relation`
test contrives to pass with `sequence_reset_sql` being noop for sqlite3.
What it cannot do automatically is reset the sequence to zero as
demonstrated in the new test
`django/tests/backends/tests.py:SequenceResetTest.test_reset_sequence`
{{{
def test_reset_sequence(self):
Post.objects.create(name='1st post', text='hello world')
Post.objects.all().delete()
# Reset the sequences for the database
commands =
connections[DEFAULT_DB_ALIAS].ops.sequence_reset_sql(no_style(), [Post])
with connection.cursor() as cursor:
for sql in commands:
cursor.execute(sql)
# If we create a new object now, it should have a PK greater
# than the PK we specified manually.
obj = Post.objects.create(name='New post', text='goodbye world')
self.assertEqual(obj.pk, 1)
}}}
--
--
Ticket URL: <https://code.djangoproject.com/ticket/31730#comment:3>
Django
-------------------------------------+-------------------------------------
Reporter: axil | Owner: nobody
Type: Bug | Status: new
(models, ORM) |
Severity: Normal | Resolution:
sqlite3 |
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* needs_better_patch: 0 => 1
* stage: Unreviewed => Accepted
Comment:
TIL about the `sqlsequencereset` command!
It seems to be barely tested
-
https://github.com/django/django/blob/3152146e3abd029be2457d2e780599d399db0fe2/tests/empty_models/test_commands.py#L20
-
https://github.com/django/django/blob/b9cf764be62e77b4777b3a75ec256f6209a57671/tests/bash_completion/tests.py#L81
And not implemented on both SQLite and MySQL!
Before any code is added here I think that we should add backend agnostics
tests for the command that execute the provided SQL and assert it has the
desired effects. This will likely require skipping the tests on SQLite and
MySQL at first.
In a following commit the logic that identifies the sequences that need to
be reset should be moved to the command itself to avoid the huge amount of
boilerplate repeated in the current PostgreSQL and Oracle
`DatabaseOperations` implementations. That will also avoid leaking
`db.models` abstraction to the `db.backends` ones. We should probably use
`connection.introspection.sequence_list()` to ''JOIN'' its return value to
the specified model subset tables. In the end I think the signature of the
of the `sequence_reset_sql` method should be something like `(style,
sequences)`.
Then we should be able to add support for both SQLite and MySQL.
--
Ticket URL: <https://code.djangoproject.com/ticket/31730#comment:4>
Django
-------------------------------------+-------------------------------------
Reporter: axil | Owner: nobody
Type: Bug | Status: new
(models, ORM) |
Severity: Normal | Resolution:
Keywords: sql sequence reset | Triage Stage: Accepted
sqlite3 |
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by axil):
> Before any code is added here
It might have come unnoticed but there is the code already ;)
https://github.com/django/django/blob/6f33f7dcf3ef20d0585bece747887c7054cf2189/django/db/backends/sqlite3/operations.py#L232-L257
> I think that we should add backend agnostics tests for the command that
execute the provided SQL and assert it has the desired effects.
One of the tests that I've added
(django/tests/backends/tests.py:SequenceResetTest.test_reset_sequence)
looks pretty backend agnostic to me:
https://github.com/axil/django/blob/7de27842c038d087257ebdd8e964f8921cd68295/tests/backends/tests.py#L186-L203
I've improved it a bit now.
>In a following commit the logic that identifies the sequences that need
to be reset should be moved to the command itself
Actually of the logic identifying the sequences has already been moved to
the django.core.commands.sqlsequencerest:
https://github.com/django/django/blob/6f33f7dcf3ef20d0585bece747887c7054cf2189/django/core/management/commands/sqlsequencereset.py#L17-L25
or to be precise to `app_config.get_models(include_auto_created=True)`. By
that reason
> to avoid the huge amount of boilerplate repeated in the current
PostgreSQL and Oracle DatabaseOperations implementations.
this boilerplate can be cut in half as I suggest in #31731. In what is
left after that yes, postgresql and oracle share the same logic and can be
refactored, but sqlite needs a somewhat different SQL statement so I see
little room for refactoring here.
> That will also avoid leaking db.models abstraction to the db.backends
ones
Yes, my patch doesn't do that.
--
Ticket URL: <https://code.djangoproject.com/ticket/31730#comment:5>
Django
-------------------------------------+-------------------------------------
Reporter: axil | Owner: nobody
Type: Bug | Status: new
(models, ORM) |
Severity: Normal | Resolution:
Keywords: sql sequence reset | Triage Stage: Accepted
sqlite3 |
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by axil):
The main question for me here is whether or not should
`sequence_reset_sql` automatically cascade to the autocreated many-to-many
tables. From its main use cases described in #31731 (and considering that
de facto this feature does not work right now and noone is reporting any
problems) it looks as if it shouldn't.
--
Ticket URL: <https://code.djangoproject.com/ticket/31730#comment:6>
Django
-------------------------------------+-------------------------------------
Reporter: axil | Owner: nobody
Type: Bug | Status: new
(models, ORM) |
Severity: Normal | Resolution:
Keywords: sql sequence reset | Triage Stage: Accepted
sqlite3 |
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by axil):
> In the end I think the signature of the of the sequence_reset_sql method
should be something like (style, sequences).
Yes, it looks strange that two similar named functions have different
signatures and do very different things:
{{{
def sequence_reset_by_name_sql(self, style, sequences):
}}}
resets sequence to 1
{{{
def sequence_reset_sql(self, style, model_list):
}}}
fixes sequences to max(id)
--
Ticket URL: <https://code.djangoproject.com/ticket/31730#comment:7>
Django
-------------------------------------+-------------------------------------
Reporter: axil | Owner: nobody
Type: Bug | Status: new
(models, ORM) |
Severity: Normal | Resolution:
Keywords: sql sequence reset | Triage Stage: Accepted
sqlite3 |
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* version: 3.0 => master
--
Ticket URL: <https://code.djangoproject.com/ticket/31730#comment:8>
Django
-------------------------------------+-------------------------------------
Reporter: axil | Owner: nobody
Type: Bug | Status: new
(models, ORM) |
Severity: Normal | Resolution:
Keywords: sql sequence reset | Triage Stage: Accepted
sqlite3 |
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by axil):
Replying to [comment:4 Simon Charette]:
> In a following commit the logic that identifies the sequences that need
to be reset should be moved to the command itself to avoid the huge amount
of boilerplate repeated in the current PostgreSQL and Oracle
`DatabaseOperations` implementations. That will also avoid leaking
`db.models` abstraction to the `db.backends` ones. We should probably use
`connection.introspection.sequence_list()` to ''JOIN'' its return value to
the specified model subset tables. In the end I think the signature of the
of the `sequence_reset_sql` method should be something like `(style,
sequences)`.
I've done this part in a separate PR:
https://github.com/django/django/pull/13103
So far for postgres only. Please have a look.
--
Ticket URL: <https://code.djangoproject.com/ticket/31730#comment:9>
Django
-------------------------------------+-------------------------------------
Reporter: axil | Owner: nobody
Type: Bug | Status: new
(models, ORM) |
Severity: Normal | Resolution:
Keywords: sql sequence reset | Triage Stage: Accepted
sqlite3 |
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by axil):
Added a deprecation warning to the `sequence_reset_sql` method. Switched
the tests to the new `sql_refresh_sequences` function.
([[https://github.com/axil/django/commit/f6e4cb098d9e3380b28c5e50b0917a2c01b95711|changeset]])
--
Ticket URL: <https://code.djangoproject.com/ticket/31730#comment:10>
Django
-------------------------------------+-------------------------------------
Type: Bug | Status: assigned
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: sql sequence reset | Triage Stage: Accepted
sqlite3 |
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* owner: nobody => axil
* status: new => assigned
Comment:
Implemented and tested the sql_sequence_reset refactoring for oracle
backend
([[https://github.com/axil/django/commit/139f6fc373bf18a3c13bd301542d638a78a8479a|changeset]])
--
Ticket URL: <https://code.djangoproject.com/ticket/31730#comment:11>