diff --git a/django_migration_linter/sql_analyser/postgresql.py b/django_migration_linter/sql_analyser/postgresql.py index 33e0b23..778ac76 100644 --- a/django_migration_linter/sql_analyser/postgresql.py +++ b/django_migration_linter/sql_analyser/postgresql.py @@ -5,6 +5,36 @@ import re from .base import BaseAnalyser, Check, CheckMode, CheckType +def has_create_index_in_transaction(sql_statements: list[str], **kwargs) -> bool: + """Return if a migration opens a transaction, acquires EXCLUSIVE lock, then indexes. + + Any locks that are obtained after a transaction is opened will not be released + until that transaction either commits or rolls back. + + Accordingly, it's extremely important that a migration avoid any long-running + queries while exclusive locks are held. + + Building an index is generally a long-running operation. A standard index + creation (that is, avoiding use of `CONCURRENTLY`) can be safe to run on + tables which are small in size and/or read-heavy. (`CREATE INDEX` only + prevents writes to a table; reads are allowed during index creation). + + This check is a stricter version of `CREATE_INDEX` -- if a team wishes + to build indices nonconcurrently, it's imperative to be mindful of locks. + """ + if not (sql_statements and sql_statements[0].startswith("BEGIN")): + return False + + for i, sql in enumerate(sql_statements): + # If any statements acquire an exclusive lock, complain about index creation later. + # Nearly every single `ALTER TABLE` command requires an exclusive lock: + # https://www.postgresql.org/docs/current/sql-altertable.html + # (Most common example is `ALTER TABLE... ADD COLUMN`, then later `CREATE INDEX`) + if sql.startswith("ALTER TABLE"): + return has_create_index(sql_statements[i + 1 :]) + return False + + def has_create_index(sql_statements: list[str], **kwargs) -> bool: regex_result = None for sql in sql_statements: @@ -32,6 +62,13 @@ class PostgresqlAnalyser(BaseAnalyser): mode=CheckMode.TRANSACTION, type=CheckType.WARNING, ), + Check( + code="CREATE_INDEX_EXCLUSIVE", + fn=has_create_index_in_transaction, + message="CREATE INDEX prolongs transaction, delaying lock release", + mode=CheckMode.TRANSACTION, + type=CheckType.ERROR, + ), Check( code="DROP_INDEX", fn=lambda sql, **kw: re.search("DROP INDEX", sql) diff --git a/docs/incompatibilities.md b/docs/incompatibilities.md index bce15d9..1867339 100644 --- a/docs/incompatibilities.md +++ b/docs/incompatibilities.md @@ -40,6 +40,7 @@ You can ignore checks through the `--exclude-migration-tests` option by specifyi | `RUNPYTHON_MODEL_VARIABLE_NAME` | The model variable name is different from the model class itself | Warning | | `RUNSQL_REVERSIBLE` | RunSQL data migration is not reversible (missing reverse SQL) | Warning | | `CREATE_INDEX` | (Postgresql specific) Creating an index without the concurrent keyword will lock the table and may generate downtime | Warning | +| `CREATE_INDEX_EXCLUSIVE` | (Postgresql specific) Creating an index in a transaction (after an `EXCLUSIVE` lock) prolongs the exclusive lock | Error | | `DROP_INDEX` | (Postgresql specific) Dropping an index without the concurrent keyword will lock the table and may generate downtime | Warning | | `REINDEX` | (Postgresql specific) Reindexing will lock the table and may generate downtime | Warning | diff --git a/tests/fixtures.py b/tests/fixtures.py index 7262d1e..38224c4 100644 --- a/tests/fixtures.py +++ b/tests/fixtures.py @@ -15,3 +15,4 @@ ADD_MANYTOMANY_FIELD = "app_add_manytomany_field" DATA_MIGRATIONS = "app_data_migrations" MAKE_NOT_NULL_WITH_DJANGO_DEFAULT = "app_make_not_null_with_django_default" MAKE_NOT_NULL_WITH_LIB_DEFAULT = "app_make_not_null_with_lib_default" +CREATE_INDEX_EXCLUSIVE = "app_create_index_exclusive" diff --git a/tests/functional/test_migration_linter.py b/tests/functional/test_migration_linter.py index 6b19170..5318ba5 100644 --- a/tests/functional/test_migration_linter.py +++ b/tests/functional/test_migration_linter.py @@ -148,3 +148,7 @@ class PostgresqlBackwardCompatibilityDetectionTestCase( def test_detect_alter_column(self): app = fixtures.ALTER_COLUMN self._test_linter_finds_errors(app) + + def test_create_index_exclusive(self): + app = fixtures.CREATE_INDEX_EXCLUSIVE + self._test_linter_finds_errors(app) diff --git a/tests/test_project/app_create_index_exclusive/__init__.py b/tests/test_project/app_create_index_exclusive/__init__.py new file mode 100644 index 0000000..e69de29 diff --git a/tests/test_project/app_create_index_exclusive/migrations/0001_initial.py b/tests/test_project/app_create_index_exclusive/migrations/0001_initial.py new file mode 100644 index 0000000..2d0e94c --- /dev/null +++ b/tests/test_project/app_create_index_exclusive/migrations/0001_initial.py @@ -0,0 +1,30 @@ +# Generated by Django 4.1.7 on 2023-03-09 21:46 + +from __future__ import annotations + +from django.db import migrations, models + + +class Migration(migrations.Migration): + + initial = True + + dependencies = [] + + operations = [ + migrations.CreateModel( + name="User", + fields=[ + ( + "id", + models.AutoField( + auto_created=True, + primary_key=True, + serialize=False, + verbose_name="ID", + ), + ), + ("name", models.TextField()), + ], + ), + ] diff --git a/tests/test_project/app_create_index_exclusive/migrations/0002_user_email.py b/tests/test_project/app_create_index_exclusive/migrations/0002_user_email.py new file mode 100644 index 0000000..b245727 --- /dev/null +++ b/tests/test_project/app_create_index_exclusive/migrations/0002_user_email.py @@ -0,0 +1,20 @@ +# Generated by Django 4.1.7 on 2023-03-09 21:47 + +from __future__ import annotations + +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ("app_create_index_exclusive", "0001_initial"), + ] + + operations = [ + migrations.AddField( + model_name="user", + name="email", + field=models.EmailField(db_index=True, max_length=254, null=True), + ), + ] diff --git a/tests/test_project/app_create_index_exclusive/migrations/__init__.py b/tests/test_project/app_create_index_exclusive/migrations/__init__.py new file mode 100644 index 0000000..e69de29 diff --git a/tests/test_project/app_create_index_exclusive/models.py b/tests/test_project/app_create_index_exclusive/models.py new file mode 100644 index 0000000..6d60a51 --- /dev/null +++ b/tests/test_project/app_create_index_exclusive/models.py @@ -0,0 +1,8 @@ +from __future__ import annotations + +from django.db import models + + +class User(models.Model): + name = models.TextField() + email = models.EmailField(db_index=True, null=True) diff --git a/tests/test_project/settings.py b/tests/test_project/settings.py index 271dc4c..b23edb2 100644 --- a/tests/test_project/settings.py +++ b/tests/test_project/settings.py @@ -56,6 +56,7 @@ INSTALLED_APPS = [ "tests.test_project.app_data_migrations", "tests.test_project.app_make_not_null_with_django_default", "tests.test_project.app_make_not_null_with_lib_default", + "tests.test_project.app_create_index_exclusive", ] MIDDLEWARE = [ diff --git a/tests/unit/test_sql_analyser.py b/tests/unit/test_sql_analyser.py index b664142..8f58d6a 100644 --- a/tests/unit/test_sql_analyser.py +++ b/tests/unit/test_sql_analyser.py @@ -315,6 +315,23 @@ class PostgresqlAnalyserTestCase(SqlAnalyserTestCase): sql = "REINDEX TABLE my_table;" self.assertWarningSql(sql) + def test_create_index_exclusive(self): + sql = [ + "BEGIN;", + 'ALTER TABLE "users" ADD COLUMN "email" varchar(254) NULL;', + 'CREATE INDEX "user_email" ON "users" ("email");', + "COMMIT;", + ] + self.assertBackwardIncompatibleSql(sql, code="CREATE_INDEX_EXCLUSIVE") + + def test_create_index_exclusive_no_lock(self): + sql = [ + 'ALTER TABLE "users" ADD COLUMN "email" varchar(254) NULL;', + 'CREATE INDEX "user_email" ON "users" ("email");', + ] + # Will warn about `CREATE_INDEX`, but *not* `CREATE_INDEX_EXCLUSIVE` + self.assertValidSql(sql, allow_warnings=True) + class SqlUtilsTestCase(unittest.TestCase): def test_unknown_analyser_string(self):