Fixed #34841 -- Avoided rendering apps on state still requiring mutation

[shangxiao]

https://code.djangoproject.com/ticket/34841

[shangxiao]

We may no longer need to set preserve=True here since state is cloned already.

[charettes]

I'm not sure this is adequate, but my migration understanding is rusty.

For migrations that are planed to be unapplied (migration in migrations_to_run) we do want their rendered state to be used below when calling self.unapply_migration as it will inevitably result in database_forwards being called for each operation and the latter still relies on .state to perform schema changes (see ticket-29898).

Is the speedup you're experiencing due to the fact that the state_forwards calls performed by Migration.unapply for each of its operations are now cheaper because the apps are not invalidated?

From what I understand the changes you are proposing do

1. Avoid eager model rendering for the last migration that needs to be unapplied
2. Make state_forwards and state cloning within Migration.unapply for each operation really cheap for this particular migration
3. Cause O=len(last_migration.operations) model rendering from scratch for each database_backwards call

I think this change might make unapplying a single migration with a single operation faster at the expense of slowing down other scenarios. Does that add up with your testing?

[shangxiao]

@charettes Ah that explains why we render apps on state 👍

The slowdown was only caused by the block that mutates solely for post-migration state:

        # Generate the post migration state by starting from the state before
        # the last migration is unapplied and mutating it to include all the
        # remaining applied migrations.
        last_unapplied_migration = plan[-1][0]
        state = states[last_unapplied_migration]
        del state.apps
        for index, (migration, _) in enumerate(full_plan):
            if migration == last_unapplied_migration:
                for migration, _ in full_plan[index:]:
                    if migration in applied_migrations:
                        migration.mutate_state(state, preserve=False)
                break

As you pointed out, we should indeed leave it rendered for this:

        for migration, _ in plan:
            self.unapply_migration(states[migration], migration, fake=fake)
            applied_migrations.remove(migration)

(I confirmed this by when testing reversing to zero: my patch showed a significant slow down as it's my largest migration.)

So rethinking this, the changes should be isolated to that post-migration mutating block. I think this is more appropriate:

--- a/django/db/migrations/executor.py
+++ b/django/db/migrations/executor.py
@@ -224,6 +224,7 @@ class MigrationExecutor:
         # remaining applied migrations.
         last_unapplied_migration = plan[-1][0]
         state = states[last_unapplied_migration]
+        del state.apps
         for index, (migration, _) in enumerate(full_plan):
             if migration == last_unapplied_migration:
                 for migration, _ in full_plan[index:]: 

^ testing that change fixes the issue of reversing the last migration and doesn't affect reversing to zero.

[shangxiao]

@jarshwah, hi. Interested in checking whether this patch speeds up reverse migrations for you?

[shangxiao]

This speeds up mutation - but do we need to re-render state.apps after mutation is finished for the benefit of any post-migrate signal? 🤔

[charettes]

yes, there's no way around complete model rendering here due to post_migrate eager rendering of models

I could see how disposing of rendered models like you did you help here though. What about using state.bulk_update() instead?

[shangxiao]

Assuming you meant state.apps.bulk_update()? Unfortunately still 20s to unapply only the final migration.

[shangxiao]

Results of del state.apps on my project:

|-------------------------+--------+----------------|
|                         | Normal | del state.apps |
|-------------------------+--------+----------------|
| Unapply final migration | 20.5s  | 3.7s           |
| Unapply all migrations  | 177.8s | 175.08s        |
|-------------------------+--------+----------------|

FYI this project is making use of django-pghistory so there's constant removal & re-addition of triggers used to track changes on tables. This may be affecting things: not only because of db operations but it also stays in the " Rendering model states..." state for quite a while.

[felixxm]

Can we add a comment to explain why this is helpful?

# Delete apps to avoid ...

[shangxiao]

Yep I have to go back and remember why that was myself 😅

[felixxm]

@shangxiao Do you have time to keep working on this?

[shangxiao]

@shangxiao Do you have time to keep working on this?

Yep 👍

[felixxm]

@charettes Do you think this is an acceptable improvement?

[shangxiao]

If we want a better comment I'd need to dig into the state mutation to remember what specifically is expensive.

[felixxm]

@shangxiao Thanks 👍

I think it's worth trying.

[charettes]

@felixxm no objections from me, this should only cause a slowdown on custom migrations.Operation subclasses out there that use state.apps in their state_forwards as none of the core ones do anymore so I think this is a good tradeoff.