[Django] #31731: Dead code in the sequence_reset_sql for postgres and oracle
Django
-------------------------------------+-------------------------------------
Reporter: axil | Owner: nobody
Type: Bug | Status: new
Component: Database | Version: 3.0
layer (models, ORM) | Keywords: sql sequence reset
Severity: Normal | postgres oracle
Triage Stage: | Has patch: 0
Unreviewed |
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-------------------------------------+-------------------------------------
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 included the fixture from ticket 11107 as
the testcase and it passes 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/31731>
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:
postgres oracle | Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Old description:
New description:
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.
--
--
Ticket URL: <https://code.djangoproject.com/ticket/31731#comment:1>
Django
-------------------------------------+-------------------------------------
Reporter: axil | Owner: nobody
Cleanup/optimization |
Component: Database layer | Version: 3.0
(models, ORM) |
Severity: Normal | Resolution:
postgres oracle |
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* type: Bug => Cleanup/optimization
* stage: Unreviewed => Accepted
Comment:
This should likely be included in the same PR that tackles #31730 and move
the sequence identification logic to the command.
--
Ticket URL: <https://code.djangoproject.com/ticket/31731#comment:2>
Django
-------------------------------------+-------------------------------------
Reporter: axil | Owner:
Cleanup/optimization | Status: assigned
Component: Database layer | Version: 3.0
(models, ORM) |
Severity: Normal | Resolution:
Keywords: sql sequence reset | Triage Stage: Accepted
postgres oracle |
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* owner: nobody => Ravindar24
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/31731#comment:3>
Django
-------------------------------------+-------------------------------------
Type: | Sharma
Cleanup/optimization | Status: assigned
Component: Database layer | Version: 3.0
(models, ORM) |
Severity: Normal | Resolution:
postgres oracle | checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* has_patch: 0 => 1
* stage: Accepted => Ready for checkin
Comment:
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
--
Ticket URL: <https://code.djangoproject.com/ticket/31731#comment:4>
Django
-------------------------------------+-------------------------------------
Type: | Sharma
Cleanup/optimization | Status: assigned
Component: Database layer | Version: 3.0
(models, ORM) |
Severity: Normal | Resolution:
postgres oracle |
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* stage: Ready for checkin => Accepted
Comment:
Ravindar, you shouldn't mark your own PRs as "Ready for checkin".
--
Ticket URL: <https://code.djangoproject.com/ticket/31731#comment:5>
Django
-------------------------------------+-------------------------------------
Type: | Sharma
Cleanup/optimization | Status: assigned
Component: Database layer | Version: 3.0
(models, ORM) |
Severity: Normal | Resolution:
Keywords: sql sequence reset | Triage Stage: Accepted
postgres oracle |
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Ravindar Sharma):
Replying to [comment:5 felixxm]:
> Thanks felixxm and i will take care of that it should not be repeated
in future.
--
Ticket URL: <https://code.djangoproject.com/ticket/31731#comment:6>
Django
-------------------------------------+-------------------------------------
Type: | Sharma
Cleanup/optimization | Status: assigned
Component: Database layer | Version: 3.0
(models, ORM) |
Severity: Normal | Resolution:
postgres oracle | checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/31731#comment:7>
Django
-------------------------------------+-------------------------------------
Type: | Sharma
Component: Database layer | Version: 3.0
(models, ORM) |
Keywords: sql sequence reset | Triage Stage: Ready for
postgres oracle | checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"18d4eac7fce6dfab6149ec7aba72f3c765b5722c" 18d4eac7]:
{{{
#!CommitTicketReference repository=""
revision="18d4eac7fce6dfab6149ec7aba72f3c765b5722c"
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.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/31731#comment:8>