Fixed #34529, Refs #34525 -- Reduced index operations with Meta.indexes/index_together when optimizing migrations. #16820

felixxm · 2023-05-02T06:32:13Z

This makes squashing migrations an available path for changing Meta.index_together, which is deprecated, to Meta.indexes.

ticket-34529
ticket-34525

tests/migrations/test_autodetector.py

shangxiao

👍

Reading this made me realise there's also some more opportunities for reducing constraints into createmodel - this can be done along with those other potential changes we discussed in the new ticket.

(May also warrant checking if anything could be included)

felixxm · 2023-05-02T08:38:02Z

Reading this made me realise there's also some more opportunities for reducing constraints into createmodel - this can be done along with those other potential changes we discussed in the new ticket.

Yes, but it's not related with index_together deprecation so I didn't want to put it here. Also, not all constraints are deferred ,so this can be more tricky.

felixxm · 2023-05-02T08:38:28Z

buildbot, test on oracle.

nessita

I tried to run this locally on a project that had a 6 migratios:

0001 - Initial migration using the deprecated index_together
0002 - The migrations.RenameIndex operation following the deprecation update path from the 4.2 release notes
0003 up to 0006 various index operations following the report from ticket #34525

When squashing migrations, I get:

(djangodev) [nessita@dali projectfromrepo]$ python manage.py squashmigrations reduceindexops 0006 --noinput
Will squash the following migrations:
 - 0001_initial
 - 0002_rename_mymodel_field_2_field_4_reduceindex_field_2_92cb62_idx
 - 0003_mymodel_reduceindex_field_3_d18611_idx
 - 0004_mymodel_reduceindex_field_3_fb791e_idx
 - 0005_remove_mymodel_reduceindex_field_2_92cb62_idx_and_more
 - 0006_rename_reduceindex_field_1_098efd_idx_custom_name_idx
Optimizing...
Traceback (most recent call last):
  File "/home/nessita/fellowship/projectfromrepo/manage.py", line 22, in <module>
    main()
  File "/home/nessita/fellowship/projectfromrepo/manage.py", line 18, in main
    execute_from_command_line(sys.argv)
  File "/home/nessita/fellowship/django/django/core/management/__init__.py", line 442, in execute_from_command_line
    utility.execute()
  File "/home/nessita/fellowship/django/django/core/management/__init__.py", line 436, in execute
    self.fetch_command(subcommand).run_from_argv(self.argv)
  File "/home/nessita/fellowship/django/django/core/management/base.py", line 412, in run_from_argv
    self.execute(*args, **cmd_options)
  File "/home/nessita/fellowship/django/django/core/management/base.py", line 458, in execute
    output = self.handle(*args, **options)
             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/nessita/fellowship/django/django/core/management/commands/squashmigrations.py", line 172, in handle
    new_operations = optimizer.optimize(operations, migration.app_label)
                     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/nessita/fellowship/django/django/db/migrations/optimizer.py", line 34, in optimize
    result = self.optimize_inner(operations, app_label)
             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/nessita/fellowship/django/django/db/migrations/optimizer.py", line 47, in optimize_inner
    result = operation.reduce(other, app_label)
             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/nessita/fellowship/django/django/db/migrations/operations/models.py", line 345, in reduce
    options_index_together = {
                             ^
TypeError: 'NoneType' object is not iterable

Will debug further but wanted to let you know before this gets merged.

docs/releases/4.2.1.txt

docs/releases/4.2.txt

charettes

Changes are looking good, it should make for a better index_together deprecation experience.

Something I noticed while looking the at the code is that RenameIndex.references_field is not specialized to consider self.old_fields. A similar picture can be drawn for AddIndex and RemoveIndex wrt/ to self.index.fields (and even conditions, expressions, ) which could cause issues with regards to optimizations of complex migration as the optimizer won't be able to break some cycles

django/django/db/migrations/operations/base.py

Lines 108 to 115 in c61219a

    
               def references_field(self, model_name, name, app_label): 
        
                   """ 
        
                   Return True if there is a chance this operation references the given 
        
                   field name, with an app label for accuracy. 
        
                   Used for optimization. If in doubt, return True. 
        
                   """ 
        
                   return self.references_model(model_name, app_label)

In short the code proposed here is correct but it might fail to optimize complex scenarios and require manual interventions.

felixxm · 2023-05-02T13:58:49Z

Thanks for reviews 👍 I pushed small edits.

felixxm · 2023-05-02T13:59:50Z

Will debug further but wanted to let you know before this gets merged.

Good catch 🎯 Fixed.

nessita · 2023-05-02T14:06:09Z

It took me a while but I managed to reproduce the TypeError reported above by using the following migrations (notice the different Django version when generating each migration):

0001_initial.py

# Generated by Django 3.2.18 on 2023-05-02 13:44

from django.db import migrations, models


class Migration(migrations.Migration):

    initial = True

    dependencies = [
    ]

    operations = [
        migrations.CreateModel(
            name='MyModel',
            fields=[
                ('id', models.BigAutoField(auto_created=True, primary_key=True, serialize=False, verbose_name='ID')),
                ('field_1', models.IntegerField()),
                ('field_2', models.TextField()),
                ('field_3', models.IntegerField()),
            ],
            options={
                'index_together': {('field_3',), ('field_1', 'field_2')},
            },
        ),
    ]

0002_rename_mymodel_field_1_field_2_reduceindex_field_1_098efd_idx_and_more.py

# Generated by Django 4.2 on 2023-05-02 13:47

from django.db import migrations


class Migration(migrations.Migration):
    dependencies = [
        ('reduceindexops', '0001_initial'),
    ]

    operations = [
        migrations.RenameIndex(
            model_name='mymodel',
            new_name='reduceindex_field_1_098efd_idx',
            old_fields=('field_1', 'field_2'),
        ),
        migrations.RenameIndex(
            model_name='mymodel',
            new_name='reduceindex_field_3_d18611_idx',
            old_fields=('field_3',),
        ),
    ]

0003_remove_mymodel_reduceindex_field_1_098efd_idx_and_more.py

# Generated by Django 4.2 on 2023-05-02 13:49

from django.db import migrations, models


class Migration(migrations.Migration):
    dependencies = [
        (
            'reduceindexops',
            '0002_rename_mymodel_field_1_field_2_reduceindex_field_1_098efd_idx_and_more',
        ),
    ]

    operations = [
        migrations.RemoveIndex(
            model_name='mymodel',
            name='reduceindex_field_1_098efd_idx',
        ),
        migrations.AddField(
            model_name='mymodel',
            name='field_4',
            field=models.IntegerField(null=True),
        ),
        migrations.AddIndex(
            model_name='mymodel',
            index=models.Index(
                fields=['field_2', 'field_4'],
                name='reduceindex_field_2_92cb62_idx',
            ),
        ),
        migrations.AddIndex(
            model_name='mymodel',
            index=models.Index(
                fields=['field_3', 'field_4'],
                name='reduceindex_field_3_fb791e_idx',
            ),
        ),
    ]

0004_rename_reduceindex_field_2_92cb62_idx_custom_name_idx.py

# Generated by Django 4.2 on 2023-05-02 13:56

from django.db import migrations


class Migration(migrations.Migration):
    dependencies = [
        (
            'reduceindexops',
            '0003_remove_mymodel_reduceindex_field_1_098efd_idx_and_more',
        ),
    ]

    operations = [
        migrations.RenameIndex(
            model_name='mymodel',
            new_name='custom_name_idx',
            old_name='reduceindex_field_2_92cb62_idx',
        ),
    ]

nessita · 2023-05-02T14:14:38Z

Will debug further but wanted to let you know before this gets merged.

Good catch dart Fixed.

Thanks. Were you able to reproduce the issue before submitting the fix?
I also thought that the fix was defaulting to the empty list like this:

--- a/django/db/migrations/operations/models.py
+++ b/django/db/migrations/operations/models.py
@@ -344,7 +344,7 @@ class CreateModel(ModelOperation):
             elif isinstance(operation, RenameIndex):
                 options_index_together = {
                     fields
-                    for fields in self.options.get("index_together")
+                    for fields in self.options.get("index_together", [])
                     if fields != operation.old_fields
                 }
                 if options_index_together:

yet this branch with the above change results in another exception:

$ python manage.py squashmigrations reduceindexops 0004 --noinput
Will squash the following migrations:
 - 0001_initial
 - 0002_rename_mymodel_field_1_field_2_reduceindex_field_1_098efd_idx_and_more
 - 0003_remove_mymodel_reduceindex_field_1_098efd_idx_and_more
 - 0004_rename_reduceindex_field_2_92cb62_idx_custom_name_idx
Optimizing...
Traceback (most recent call last):
  File "/home/nessita/fellowship/projectfromrepo/manage.py", line 22, in <module>
    main()
  File "/home/nessita/fellowship/projectfromrepo/manage.py", line 18, in main
    execute_from_command_line(sys.argv)
  File "/home/nessita/fellowship/django/django/core/management/__init__.py", line 442, in execute_from_command_line
    utility.execute()
  File "/home/nessita/fellowship/django/django/core/management/__init__.py", line 436, in execute
    self.fetch_command(subcommand).run_from_argv(self.argv)
  File "/home/nessita/fellowship/django/django/core/management/base.py", line 412, in run_from_argv
    self.execute(*args, **cmd_options)
  File "/home/nessita/fellowship/django/django/core/management/base.py", line 458, in execute
    output = self.handle(*args, **options)
             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/nessita/fellowship/django/django/core/management/commands/squashmigrations.py", line 172, in handle
    new_operations = optimizer.optimize(operations, migration.app_label)
                     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/nessita/fellowship/django/django/db/migrations/optimizer.py", line 34, in optimize
    result = self.optimize_inner(operations, app_label)
             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/nessita/fellowship/django/django/db/migrations/optimizer.py", line 47, in optimize_inner
    result = operation.reduce(other, app_label)
             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/nessita/fellowship/django/django/db/migrations/operations/models.py", line 362, in reduce
    models.Index(
  File "/home/nessita/fellowship/django/django/db/models/indexes.py", line 36, in __init__
    raise ValueError("Index.fields must be a list or tuple.")
ValueError: Index.fields must be a list or tuple.

nessita · 2023-05-02T14:21:00Z

The ValueError: Index.fields must be a list or tuple. is caused by operation.old_fields being None

felixxm · 2023-05-02T14:31:54Z

Thanks. Were you able to reproduce the issue before submitting the fix? I also thought that the fix was defaulting to the empty list like this:

Fixed, I added a test.

tests/migrations/test_optimizer.py

nessita · 2023-05-02T15:35:47Z

Thanks @felixxm, the latest push did fix errors and I was able to squash migrations.
Do you think is there any chance to add a test for the first fix, the TypeError?

Otherwise this looks good! 👍

…es/index_together when optimizing migrations. This makes squashing migrations an available path for changing Meta.index_together, which is deprecated, to Meta.indexes. Follow up to f810325.

felixxm · 2023-05-02T18:20:56Z

Do you think is there any chance to add a test for the first fix, the TypeError?

This should be covered by test_create_model_rename_index_no_old_fields, without the operation.old_fields check it crashes with TypeError.

nessita · 2023-05-03T01:50:08Z

Do you think is there any chance to add a test for the first fix, the TypeError?

This should be covered by test_create_model_rename_index_no_old_fields, without the operation.old_fields check it crashes with TypeError.

By TypeError I mean the first error that I reported, the one that was fixed with the self.options.get("index_together", []) change. In this branch, if I revert that fix:

--- a/django/db/migrations/operations/models.py
+++ b/django/db/migrations/operations/models.py
@@ -344,7 +344,7 @@ class CreateModel(ModelOperation):
             elif isinstance(operation, RenameIndex) and operation.old_fields:
                 options_index_together = {
                     fields
-                    for fields in self.options.get("index_together", [])
+                    for fields in self.options.get("index_together")
                     if fields != operation.old_fields
                 }
                 if options_index_together:

no test fails:

$ ./runtests.py migrations.test_optimizer
Testing against Django installed in '/home/nessita/fellowship/django/django' with up to 4 processes
Found 44 test(s).
System check identified no issues (0 silenced).
............................................
----------------------------------------------------------------------
Ran 44 tests in 0.031s

OK

This is where I think the code would benefit by having a test ensuring that no crash occurs if self.options.get("index_together") is None.

felixxm · 2023-05-03T04:07:37Z

By TypeError I mean the first error that I reported, the one that was fixed with the self.options.get("index_together", []) change. In this branch, if I revert that fix:
--- a/django/db/migrations/operations/models.py
+++ b/django/db/migrations/operations/models.py
@@ -344,7 +344,7 @@ class CreateModel(ModelOperation):
             elif isinstance(operation, RenameIndex) and operation.old_fields:
                 options_index_together = {
                     fields
-                    for fields in self.options.get("index_together", [])
+                    for fields in self.options.get("index_together")
                     if fields != operation.old_fields
                 }
                 if options_index_together:
no test fails:
This is where I think the code would benefit by having a test ensuring that no crash occurs if self.options.get("index_together") is None.

As I said, this is covered by test_create_model_rename_index_no_old_fields, i.e. without the operation.old_fields check it crashes with TypeError:

diff --git a/django/db/migrations/operations/models.py b/django/db/migrations/operations/models.py
index 9c5bd5fbcb..39b16f4902 100644
--- a/django/db/migrations/operations/models.py
+++ b/django/db/migrations/operations/models.py
@@ -341,10 +341,10 @@ class CreateModel(ModelOperation):
                         managers=self.managers,
                     ),
                 ]
-            elif isinstance(operation, RenameIndex) and operation.old_fields:
+            elif isinstance(operation, RenameIndex):
                 options_index_together = {
                     fields
-                    for fields in self.options.get("index_together", [])
+                    for fields in self.options.get("index_together")
                     if fields != operation.old_fields
                 }
                 if options_index_together:

======================================================================
ERROR: test_create_model_rename_index_no_old_fields (migrations.test_optimizer.OptimizerTests)
----------------------------------------------------------------------
Traceback (most recent call last):
...
    options_index_together = {
TypeError: 'NoneType' object is not iterable

IMO, this is enough.

felixxm commented May 2, 2023

View reviewed changes

tests/migrations/test_autodetector.py Show resolved Hide resolved

felixxm marked this pull request as ready for review May 2, 2023 08:00

felixxm force-pushed the issue-34529 branch from 277ee16 to 1a6bf9f Compare May 2, 2023 08:01

felixxm changed the title ~~Fixed #34539, Refs #34525 -- Reduced index operations with Meta.indexes/index_together when optimizing migrations.~~ Fixed #34529, Refs #34525 -- Reduced index operations with Meta.indexes/index_together when optimizing migrations. May 2, 2023

felixxm force-pushed the issue-34529 branch from 1a6bf9f to 24098d2 Compare May 2, 2023 08:01

shangxiao approved these changes May 2, 2023

View reviewed changes

nessita reviewed May 2, 2023

View reviewed changes

docs/releases/4.2.1.txt Show resolved Hide resolved

docs/releases/4.2.txt Show resolved Hide resolved

charettes reviewed May 2, 2023

View reviewed changes

charettes approved these changes May 2, 2023

View reviewed changes

felixxm force-pushed the issue-34529 branch from 24098d2 to 1240cf7 Compare May 2, 2023 13:58

felixxm force-pushed the issue-34529 branch from 1240cf7 to 76edc7a Compare May 2, 2023 14:31

felixxm commented May 2, 2023

View reviewed changes

tests/migrations/test_optimizer.py Show resolved Hide resolved

Fixed #34529, Refs #34525 -- Reduced index operations with Meta.index…

6cf90c4

…es/index_together when optimizing migrations. This makes squashing migrations an available path for changing Meta.index_together, which is deprecated, to Meta.indexes. Follow up to f810325.

felixxm force-pushed the issue-34529 branch from 76edc7a to 6cf90c4 Compare May 2, 2023 18:20

felixxm merged commit 8e2460d into django:main May 3, 2023
26 checks passed

felixxm deleted the issue-34529 branch May 3, 2023 11:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixed #34529, Refs #34525 -- Reduced index operations with Meta.indexes/index_together when optimizing migrations. #16820

Fixed #34529, Refs #34525 -- Reduced index operations with Meta.indexes/index_together when optimizing migrations. #16820

felixxm commented May 2, 2023 •

edited

shangxiao left a comment

felixxm commented May 2, 2023

felixxm commented May 2, 2023

nessita left a comment

charettes left a comment

felixxm commented May 2, 2023

felixxm commented May 2, 2023

nessita commented May 2, 2023

nessita commented May 2, 2023

nessita commented May 2, 2023

felixxm commented May 2, 2023

nessita commented May 2, 2023 •

edited

felixxm commented May 2, 2023

nessita commented May 3, 2023

felixxm commented May 3, 2023

	def references_field(self, model_name, name, app_label):
	"""
	Return True if there is a chance this operation references the given
	field name, with an app label for accuracy.

	Used for optimization. If in doubt, return True.
	"""
	return self.references_model(model_name, app_label)

Fixed #34529, Refs #34525 -- Reduced index operations with Meta.indexes/index_together when optimizing migrations. #16820

Fixed #34529, Refs #34525 -- Reduced index operations with Meta.indexes/index_together when optimizing migrations. #16820

Conversation

felixxm commented May 2, 2023 • edited

shangxiao left a comment

Choose a reason for hiding this comment

felixxm commented May 2, 2023

felixxm commented May 2, 2023

nessita left a comment

Choose a reason for hiding this comment

charettes left a comment

Choose a reason for hiding this comment

felixxm commented May 2, 2023

felixxm commented May 2, 2023

nessita commented May 2, 2023

nessita commented May 2, 2023

nessita commented May 2, 2023

felixxm commented May 2, 2023

nessita commented May 2, 2023 • edited

felixxm commented May 2, 2023

nessita commented May 3, 2023

felixxm commented May 3, 2023

felixxm commented May 2, 2023 •

edited

nessita commented May 2, 2023 •

edited