Some indexes dropped in migration are not recreated

[sparrowt]

Problem

In the following cases (possibly more) any indexes on the column are dropped, but aren't recreated afterwards:
(a) when a column is made null-able (i.e. adding null=True)
(b) when a column is renamed
(c) when a table is renamed

This is quite a major issue because it is silently leaving the database in the wrong state - the migration doesn't fail, but now certain columns with db_index=True won't actually have an index which could cause very slow queries.

Cause

As far as I can see this is a triple-regression introduced (before this fork was created) by 2e60754 (ESSolutions/django-mssql-backend#24).

That commit added new calls to _delete_indexes in 2 places within _alter_field causing the first two cases listed above:

(a) ESSolutions/django-mssql-backend@2e6075456a#diff-e4c1520d8b49ccbf46382bd2eed4e740R400
(b) ESSolutions/django-mssql-backend@2e6075456a#diff-e4c1520d8b49ccbf46382bd2eed4e740R334-R335

and added an explicit index deletion here in alter_db_table before renaming a table causing the third case:

(c) ESSolutions/django-mssql-backend@2e6075456a#diff-e4c1520d8b49ccbf46382bd2eed4e740R222-R225

but in none of those cases is the index re-instated afterwards, even if the field still has db_index=True. Index restoration only happens in certain specific cases (e.g. type change / removal of nullability) which doesn't include the above 3 cases.

Reproduction

I've added 3 tests in #49 which fail due to the bugs described above, but pass if run on older versions like django-mssql-backend v2.4.2 (before ESSolutions/django-mssql-backend#24 was merged).

History

I previously filed this on the project from which this was forked ESSolutions/django-mssql-backend#58 - given there was no feedback to my proposed solution there, I hope it's ok to file an equivalent issue here. Hopefully this more official project might be able to provide some insight. This was my comment (the only one on that issue) re possible fixes:

Considering how to solve this, one approach would be to expand the condition under # Restore an index, or to start explicitly keeping track of which indexes have been dropped and need re-creating.

However before that we should ask: is it actually necessary for all indexes to be dropped in these 3 cases?

If it can be avoided then it will save wasting time during migration of a large table. MS SQL Server seems to allow those 3 operations to occur while the index is there (it didn't explode in those cases using v2.4.2).

@OskarPersson do you know what the reasons were for adding all these "delete index before doing X" in 2e60754?

[absci]

Issue acknowledged. We will review it and propose a better solution in the future.

[absci]

The issue has been fixed in the latest release. Feel free to reopen if there is any other concern.

[sparrowt]

Many thanks @absci

It's a slight tangent from the original issue here, so if it's valid perhaps deserves a new issue, but I'm slightly confused by this change which adds an index creation to post_actions 6048db2#diff-8ec363b9558cceda1508c9b4ddc74ebe8af8d41f3d70b7fdf140e35dd80a3d00R443-R444

1. Why does it enqueue recreation of an index regardless of whether the field actually specifies db_index=True or not (i.e. new_field.db_index) - surely the index should only be recreated if it was actually dropped and is still required? I realise d1cc2d8 changed this behaviour slightly re Alter ForeignKey from non-nullable to nullable, including data migration, in squashed migration, leads to duplicate index created #77 (adding and not (old_field.db_index and new_field.db_index)) but it still seems wrong - if new_field.db_index is False then the code will still create an index in post_actions which should be there AFAICS (?)
2. Also why should TextField (and now JSONField) be specifically excluded? (again, with no reference to whether db_index is set)

[absci]

Many thanks @absci

It's a slight tangent from the original issue here, so if it's valid perhaps deserves a new issue, but I'm slightly confused by this change which adds an index creation to post_actions 6048db2#diff-8ec363b9558cceda1508c9b4ddc74ebe8af8d41f3d70b7fdf140e35dd80a3d00R443-R444

1. Why does it enqueue recreation of an index regardless of whether the field actually specifies db_index=True or not (i.e. new_field.db_index) - surely the index should only be recreated if it was actually dropped and is still required? I realise d1cc2d8 changed this behaviour slightly re Alter ForeignKey from non-nullable to nullable, including data migration, in squashed migration, leads to duplicate index created #77 (adding and not (old_field.db_index and new_field.db_index)) but it still seems wrong - if new_field.db_index is False then the code will still create an index in post_actions which should be there AFAICS (?)
2. Also why should TextField (and now JSONField) be specifically excluded? (again, with no reference to whether db_index is set)

1. Yes, you're right. There's some issue here I'm currently trying to fix it.
2. TextField and JSONField are nvarchar(max). SQL Server can't create indexes on that type, so I excluded those. I'm not sure if there are any alternatives currently.

[sparrowt]

Awesome, thank you!

The tests for the original problem in this issue are now rebased and ✔ and ready to merge if you're happy: #49

[absci]

Awesome, thank you!

The tests for the original problem in this issue are now rebased and ✔ and ready to merge if you're happy: #49

Merged, thanks for your contribution.