Add new check for indexing with atomic=True

Briefly, this check helps prevent a long-running index creation from
preventing all reads and writes to a table.

This supplements an existing check
==================================
There is an existing check, `CREATE_INDEX` which warns about creating
indices without the `CONCURRENTLY` argument. It's a helpful check!

However, there are some valid reasons to prefer non-concurrent index
creation. For example, concurrent index creation may struggle to
complete when applied to write-heavy tables (since Postgres will have to
make repeated passes on the table to accommodate the new reads since the
last round).

The danger of `EXCLUSIVE` locks
===============================
If you *do* want to create indices nonconcurrently, there is still a
**huge footgun** to be aware of -- locks which are held in a transaction
are not released until the transaction closes.

Dangerous Django defaults
=========================
By default, any time you add a new Django field with `db_index=True` or
`unique=True`, the automatically generated `Migration` has:

- implicit setting of `atomic=True` (transactions are on)
- an `ALTER TABLE...ADD COLUMN` call (exclusively locks the table)
- a `CREATE INDEX` invocation (*hopefully* quick, due to nullable
  columns)

It's even more dangerous if you combine an `ALTER TABLE` call with
`CREATE INDEX` of an existing, non-nullable column.

Favor `atomic=False` if you must index
======================================
Index-creating migrations should have few `operations`. Since index
creation does not modify row-level data (and only requires locking
against writes), there's no reason to use transactions.

It's prudent to bypass the default `atomic=True` for index creation.

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)

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      |
 

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"

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)

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()),
+            ],
+        ),
+    ]

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),
+        ),
+    ]

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)

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 = [

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):