Opened 6 years ago

Closed 6 years ago

#10881 closed (duplicate)

db.backends.postgresql.operations.sequence_reset_sql and M2M fields with non-integer PKs

Reported by: gordyt Owned by: nobody
Component: Database layer (models, ORM) Version: 1.0
Severity: Keywords: postgresql, operations, sequence_reset_sql, loaddata
Cc: Triage Stage: Unreviewed
Has patch: yes Needs documentation: no
Needs tests: yes Patch needs improvement: no
Easy pickings: UI/UX:

Description

Near the end of the command python manage.py loaddata ... the sequence_reset_sql is called. This part of the method is run for models that contain many-to-many fields:

    for f in model._meta.many_to_many:
        output.append("%s setval('%s', coalesce(max(%s), 1), max(%s) %s null) %s %s;" % \
            (style.SQL_KEYWORD('SELECT'),
            style.SQL_FIELD(qn('%s_id_seq' % f.m2m_db_table())),
            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()))))

Under normal circumstances this is fine. But if you are using your own model to manage the M2M relationship (specified via the through=xxx parameter in the ManyToManyField) and that model uses a non-integer primary key, then this part of the method generates invalid SQL code.

I have attached two files to this ticket. The models.py file has additional comments at the top that shows the output of running the loaddata command. It is self-contained except that is uses the UUIDField from the django-extensions project. This version of a UUIDField is a simple CharField extension.

The file strtest_dump.json is a very small file created by the dumpdata command. It has data for 3 authors and 1 book and the entries to join them together.

Attachments (3)

models.py (3.2 KB) - added by gordyt 6 years ago.
Sample models to illustrate problem with additional comments at the top
srtest_dump.json (1.4 KB) - added by gordyt 6 years ago.
very small sample JSON file to try and load that illustrates the problem
postgresql-sequence_reset_sql.diff (1.5 KB) - added by jcd@… 6 years ago.
Prevents sequence_reset_sql() from trying to handle m2ms defined with through=MyModel kwarg as m2ms.

Download all attachments as: .zip

Change History (7)

Changed 6 years ago by gordyt

Sample models to illustrate problem with additional comments at the top

Changed 6 years ago by gordyt

very small sample JSON file to try and load that illustrates the problem

Changed 6 years ago by jcd@…

Prevents sequence_reset_sql() from trying to handle m2ms defined with through=MyModel kwarg as m2ms.

comment:1 Changed 6 years ago by jcd@…

  • Has patch set
  • Needs documentation unset
  • Needs tests set
  • Patch needs improvement unset

The logic for resetting many_to_many sequences is a bit simplistic because when django auto-generates an intermediate table, it has control over that table, and certain things can't change about it. Note also that the pk column is statically set to "id". If you define your own intermediary table using ManyToManyField(through=MyModel), this can break, but the model will have been dealt with already by the loop above which handles AutoFields for each model. So the loop through model._meta.many_to_many should actually ignore m2ms that are through a defined model. I'm attaching a patch that does this. Once this is done, you still shouldn't need the code to handle non-integer primary keys, because they don't have a sequence that needs to be reset. This only applies to models with AutoFields and ManyToManyFields without the through keyword arg set.

This is my first patch, and I haven't figured out how to get the test suite to run yet, so I'm marking "needs tests".

comment:2 Changed 6 years ago by anonymous

A test should establish that without the patch, m2ms defined with the through kwarg can cause sequence_reset_sql to fail in two ways:

  1. trying to reset a sequence on primary key fields that have no sequence (non-AutoField pks)
  2. trying to reset the wrong sequence on primary key fields that have db_column set to a value other than "id".

With the patch, both of these issues are solved. If no one else has time to do this, I will try to get my test suite working, and code up a test.

comment:3 Changed 6 years ago by jcd@…

Sorry. The comment at 05/13/09 14:27:19 by anonymous was me

comment:4 Changed 6 years ago by jcd@…

  • Resolution set to duplicate
  • Status changed from new to closed

This ticket is now a part of the larger issue in #11107

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