Opened 4 years ago

Last modified 4 years ago

#31730 assigned Bug

manage.py sqlsequencereset not implemented for sqlite3

Reported by: axil Owned by: axil
Component: Database layer (models, ORM) Version: dev
Severity: Normal Keywords: sql sequence reset sqlite3
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description (last modified by axil)

Currently ./manage.py 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)

Change History (11)

comment:2 by axil, 4 years ago

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.

comment:3 by axil, 4 years ago

Description: modified (diff)

comment:4 by Simon Charette, 4 years ago

Patch needs improvement: set
Triage Stage: UnreviewedAccepted

TIL about the sqlsequencereset command!

It seems to be barely tested

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.

comment:5 by axil, 4 years ago

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.

Version 0, edited 4 years ago by axil (next)

comment:6 by axil, 4 years ago

Should sequence_reset_sql automatically cascade to the autocreated many-to-many tables?

Last edited 4 years ago by axil (previous) (diff)

comment:7 by axil, 4 years ago

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)

comment:8 by axil, 4 years ago

Version: 3.0master

in reply to:  4 comment:9 by axil, 4 years ago

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

comment:10 by axil, 4 years ago

Added a deprecation warning to the sequence_reset_sql method. Switched the tests to the new sql_refresh_sequences function. (changeset)

comment:11 by axil, 4 years ago

Owner: changed from nobody to axil
Status: newassigned

Implemented and tested the sql_sequence_reset refactoring for oracle backend (changeset)

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