Opened 5 years ago

Closed 4 years ago

#31731 closed Cleanup/optimization (fixed)

Dead code in the sequence_reset_sql for postgres and oracle

Reported by: axil Owned by: Ravindar Sharma
Component: Database layer (models, ORM) Version: 3.0
Severity: Normal Keywords: sql sequence reset postgres oracle
Cc: Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description (last modified by axil)

In the definition of sequence_reset_sql function in django/db/backends/postgresql/operations.py and django/db/backends/oracle/operations.py files

   def sequence_reset_sql(self, style, model_list):
       from django.db import models
       output = []
       qn = self.quote_name

       for model in model_list:
            # Use `coalesce` to set the sequence for each model to the max pk value if there are records,
            # or 1 if there are none. Set the `is_called` property (the third argument to `setval`) to true
            # if there are records (as the max pk value is already in use), otherwise set it to false.
            # Use pg_get_serial_sequence to get the underlying sequence name from the table name
            # and column name (available since PostgreSQL 8)

            for f in model._meta.local_fields:
                if isinstance(f, models.AutoField):
                    output.append(
                        "%s setval(pg_get_serial_sequence('%s','%s'), "
                        "coalesce(max(%s), 1), max(%s) %s null) %s %s;" % (
                            style.SQL_KEYWORD('SELECT'),
                            style.SQL_TABLE(qn(model._meta.db_table)),
                            style.SQL_FIELD(f.column),
                            style.SQL_FIELD(qn(f.column)),
                            style.SQL_FIELD(qn(f.column)),
                            style.SQL_KEYWORD('IS NOT'),
                            style.SQL_KEYWORD('FROM'),
                            style.SQL_TABLE(qn(model._meta.db_table)),
                        )
                    )
                    break  # Only one AutoField is allowed per model, so don't bother continuing.
            for f in model._meta.many_to_many:
                if not f.remote_field.through:
##                    output.append(
##                       "%s setval(pg_get_serial_sequence('%s','%s'), "
##                        "coalesce(max(%s), 1), max(%s) %s null) %s %s;" % (
##                           style.SQL_KEYWORD('SELECT'),
##                            style.SQL_TABLE(qn(f.m2m_db_table())),
##                            style.SQL_FIELD('id'),
##                            style.SQL_FIELD(qn('id')),
##                            style.SQL_FIELD(qn('id')),
##                            style.SQL_KEYWORD('IS NOT'),
##                            style.SQL_KEYWORD('FROM'),
##                            style.SQL_TABLE(qn(f.m2m_db_table()))
##                        )
##                    )
        return output        

The statement marked with ## never executes.

11 years ago when this code was written (ticket #11107) this if not f.remote_field.through: was a valid way to check if the ManyToManyField uses the user-provided intermediate through table or not. Nowadays bool(f.remote_field.through) always evaluates to True, no matter if it is a user-provided or an automatic table.

But actually it is not necessary anymore. This function is used in three places in django code:

1) manage.py sqlsequencereset uses

models = app_config.get_models(include_auto_created=True)

instead of the original

models = app_config.get_models()

which was there when this ticket 11107 was merged. So now it fetches both automatic and user-defined intermediate tables at the ealier stage, before going.

2) loaddata is unaffected by this branch of execution because it needs to reset the sequences solely of the models that are being loaded and not of any others. To be 100% sure I created a project reproduced the models.py from 11107 and loaded the fixture provided there with manage.py loaddata - it succeeded without entering into that execution branch.

3) django.contrib.sites.management.create_default_site applies it to the Site model

sequence_sql = connections[using].ops.sequence_reset_sql(no_style(), [Site])

which has no manytomany keys, so it is unaffected.

I would suggest to remove the code marked with "##" and check for other usages of f.remote_field.through for similar issues.

Change History (8)

comment:1 by axil, 5 years ago

Description: modified (diff)

comment:2 by Simon Charette, 5 years ago

Triage Stage: UnreviewedAccepted
Type: BugCleanup/optimization

This should likely be included in the same PR that tackles #31730 and move the sequence identification logic to the command.

comment:3 by Ravindar Sharma, 4 years ago

Owner: changed from nobody to Ravindar Sharma
Status: newassigned

comment:4 by Ravindar Sharma, 4 years ago

Has patch: set
Triage Stage: AcceptedReady for checkin

Dead Code has been removed from definition of sequence_reset_sql function in django/db/backends/postgresql/operations.py and django/db/backends/oracle/operations.py files.

Pull Request :- https://github.com/django/django/pull/13180

comment:5 by Mariusz Felisiak, 4 years ago

Triage Stage: Ready for checkinAccepted

Ravindar, you shouldn't mark your own PRs as "Ready for checkin".

in reply to:  5 comment:6 by Ravindar Sharma, 4 years ago

Replying to felixxm:

Thanks felixxm and i will take care of that it should not be repeated in future.

comment:7 by Mariusz Felisiak, 4 years ago

Triage Stage: AcceptedReady for checkin

comment:8 by Mariusz Felisiak <felisiak.mariusz@…>, 4 years ago

Resolution: fixed
Status: assignedclosed

In 18d4eac7:

Fixed #31731 -- Removed unreachable code for resetting sequences of auto-created m2m tables in sequence_reset_sql().

Unreachable because f.remote_field.through is truthy for all m2m
fields.

Resetting sequences of auto-created m2m tables in sequence_reset_sql()
is also unnecessary:

  • in sqlsequencereset since c39ec6dccb389fbb4047e44f5247e18bb76ae598 because auto-created tables are included in model_list,
  • in loaddata because there is no it need to reset sequences for models not loaded directly.
  • in create_default_site() because it doesn't have m2m fields.
Note: See TracTickets for help on using tickets.
Back to Top