Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Regression: index dropped in migration and not recreated #58

Open
sparrowt opened this issue Jul 6, 2020 · 3 comments
Open

Regression: index dropped in migration and not recreated #58

sparrowt opened this issue Jul 6, 2020 · 3 comments

Comments

@sparrowt
Copy link

sparrowt commented Jul 6, 2020

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 by 2e60754 (for #24).

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

(a) 2e6075456a#diff-e4c1520d8b49ccbf46382bd2eed4e740R400
(b) 2e6075456a#diff-e4c1520d8b49ccbf46382bd2eed4e740R334-R335

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

(c) 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

See the 3 tests I added on this branch:
master...sparrowt:issue58-index-reinstatement
which fail due to the bugs described above, but pass if run on django-mssql-backend v2.4.2 (before #24)

sparrowt added a commit to sparrowt/django-mssql-backend that referenced this issue Jul 6, 2020
Doubtless there are more cases that should be tested, but
these are the 3 specific ones that I've found to be broken
and reported in issue ESSolutions#58

These tests fail on latest django-mssql-backend (e.g. v2.8.1)
but passed on v2.4.2 before ESSolutions#24 went in.
@sparrowt
Copy link
Author

sparrowt commented Jul 6, 2020

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?

@sparrowt
Copy link
Author

This issue still remains in the new Microsoft-supported fork of this project so I have filed it there 👆

@sparrowt
Copy link
Author

Now fixed in microsoft/mssql-django#14

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant