Repository: 3YOURMIND/django-migration-linter

Issue:
Raise backend specific deployment implications
For instance, certain operations will potentially lock a table, which can have implications during deployment (lots of operations require the table => we don't acquire the lock waiting to migrate / one we have the lock, long migration, locking the table and making production hang)

https://github.com/yandex/zero-downtime-migrations might be interesting to help out for these warnings

Discussion:
codecov-commenter: # [Codecov](https://codecov.io/gh/3YOURMIND/django-migration-linter/pull/156?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None) Report
> Merging [#156](https://codecov.io/gh/3YOURMIND/django-migration-linter/pull/156?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None) (3392da1) into [main](https://codecov.io/gh/3YOURMIND/django-migration-linter/commit/a119b4ba1fdfd27bf950e109771c6fd3e41d48dc?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None) (a119b4b) will **increase** coverage by `0.08%`.
> The diff coverage is `91.66%`.

[![Impacted file tree graph](https://codecov.io/gh/3YOURMIND/django-migration-linter/pull/156/graphs/tree.svg?width=650&height=150&src=pr&token=pgNNXGbCf7&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None)](https://codecov.io/gh/3YOURMIND/django-migration-linter/pull/156?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None)

```diff
@@            Coverage Diff             @@
##             main     #156      +/-   ##
==========================================
+ Coverage   93.03%   93.11%   +0.08%     
==========================================
  Files          77       77              
  Lines        1593     1627      +34     
==========================================
+ Hits         1482     1515      +33     
- Misses        111      112       +1     
```


| [Impacted Files](https://codecov.io/gh/3YOURMIND/django-migration-linter/pull/156?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None) | Coverage Δ | |
|---|---|---|
| [django\_migration\_linter/sql\_analyser/mysql.py](https://codecov.io/gh/3YOURMIND/django-migration-linter/pull/156/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None#diff-ZGphbmdvX21pZ3JhdGlvbl9saW50ZXIvc3FsX2FuYWx5c2VyL215c3FsLnB5) | `91.66% <ø> (ø)` | |
| [django\_migration\_linter/sql\_analyser/sqlite.py](https://codecov.io/gh/3YOURMIND/django-migration-linter/pull/156/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None#diff-ZGphbmdvX21pZ3JhdGlvbl9saW50ZXIvc3FsX2FuYWx5c2VyL3NxbGl0ZS5weQ==) | `95.65% <ø> (ø)` | |
| [django\_migration\_linter/migration\_linter.py](https://codecov.io/gh/3YOURMIND/django-migration-linter/pull/156/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None#diff-ZGphbmdvX21pZ3JhdGlvbl9saW50ZXIvbWlncmF0aW9uX2xpbnRlci5weQ==) | `82.50% <60.00%> (-0.56%)` | :arrow_down: |
| [django\_migration\_linter/sql\_analyser/analyser.py](https://codecov.io/gh/3YOURMIND/django-migration-linter/pull/156/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None#diff-ZGphbmdvX21pZ3JhdGlvbl9saW50ZXIvc3FsX2FuYWx5c2VyL2FuYWx5c2VyLnB5) | `94.11% <100.00%> (ø)` | |
| [django\_migration\_linter/sql\_analyser/base.py](https://codecov.io/gh/3YOURMIND/django-migration-linter/pull/156/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None#diff-ZGphbmdvX21pZ3JhdGlvbl9saW50ZXIvc3FsX2FuYWx5c2VyL2Jhc2UucHk=) | `100.00% <100.00%> (ø)` | |
| [django\_migration\_linter/sql\_analyser/postgresql.py](https://codecov.io/gh/3YOURMIND/django-migration-linter/pull/156/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None#diff-ZGphbmdvX21pZ3JhdGlvbl9saW50ZXIvc3FsX2FuYWx5c2VyL3Bvc3RncmVzcWwucHk=) | `100.00% <100.00%> (ø)` | |
| [tests/unit/test\_sql\_analyser.py](https://codecov.io/gh/3YOURMIND/django-migration-linter/pull/156/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None#diff-dGVzdHMvdW5pdC90ZXN0X3NxbF9hbmFseXNlci5weQ==) | `100.00% <100.00%> (ø)` | |
| [django\_migration\_linter/sql\_analyser/utils.py](https://codecov.io/gh/3YOURMIND/django-migration-linter/pull/156/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None#diff-ZGphbmdvX21pZ3JhdGlvbl9saW50ZXIvc3FsX2FuYWx5c2VyL3V0aWxzLnB5) | `100.00% <0.00%> (+15.38%)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/3YOURMIND/django-migration-linter/pull/156?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/3YOURMIND/django-migration-linter/pull/156?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None). Last update [a119b4b...3392da1](https://codecov.io/gh/3YOURMIND/django-migration-linter/pull/156?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None).

David-Wobrock: Thanks for the review @skarzi ! :muscle: Feel free to comment more, but be aware that I'm still working on this branch.
So it might still get quite some changes ;)

Patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 75223f0..1ee0f0b 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -4,6 +4,9 @@
 * the positional argument `GIT_COMMIT_ID` becomes an optional argument with the named parameter ` --git-commit-id [GIT_COMMIT_ID]`
 * the `lintmigrations` command takes now two positional arguments: `lintmigrations [app_label] [migration_name]`
 
+New features:
+* raise warning when create or dropping an index in a non-concurrent manner using postgresql
+
 Miscellaneous:
 * Add complete and working support for `toml` configuration files
 * Add code coverage to the linter
diff --git a/django_migration_linter/migration_linter.py b/django_migration_linter/migration_linter.py
index 3ce96ae..c83a9ea 100644
--- a/django_migration_linter/migration_linter.py
+++ b/django_migration_linter/migration_linter.py
@@ -159,17 +159,19 @@ class MigrationLinter(object):
             return
 
         sql_statements = self.get_sql(app_label, migration_name)
-        errors, ignored = analyse_sql_statements(
+        errors, ignored, warnings = analyse_sql_statements(
             sql_statements,
             settings.DATABASES[self.database]["ENGINE"],
             self.exclude_migration_tests,
         )
 
-        err, ignored_data, warnings = self.analyse_data_migration(migration)
+        err, ignored_data, warnings_data = self.analyse_data_migration(migration)
         if err:
             errors += err
         if ignored_data:
             ignored += ignored_data
+        if warnings_data:
+            warnings += warnings_data
 
         if self.warnings_as_errors:
             errors += warnings
@@ -616,7 +618,7 @@ class MigrationLinter(object):
             else:
                 sql_statements.append(runsql.sql)
 
-            sql_errors, sql_ignored = analyse_sql_statements(
+            sql_errors, sql_ignored, sql_warnings = analyse_sql_statements(
                 sql_statements,
                 settings.DATABASES[self.database]["ENGINE"],
                 self.exclude_migration_tests,
@@ -625,6 +627,8 @@ class MigrationLinter(object):
                 error += sql_errors
             if sql_ignored:
                 ignored += sql_ignored
+            if sql_warnings:
+                warning += sql_warnings
 
         # And analysse the reverse SQL
         if runsql.reversible and runsql.reverse_sql != RunSQL.noop:
@@ -644,7 +648,7 @@ class MigrationLinter(object):
             else:
                 sql_statements.append(runsql.reverse_sql)
 
-            sql_errors, sql_ignored = analyse_sql_statements(
+            sql_errors, sql_ignored, sql_warnings = analyse_sql_statements(
                 sql_statements,
                 settings.DATABASES[self.database]["ENGINE"],
                 self.exclude_migration_tests,
@@ -653,5 +657,7 @@ class MigrationLinter(object):
                 error += sql_errors
             if sql_ignored:
                 ignored += sql_ignored
+            if sql_warnings:
+                warning += sql_warnings
 
         return error, ignored, warning
diff --git a/django_migration_linter/sql_analyser/analyser.py b/django_migration_linter/sql_analyser/analyser.py
index d69f873..d98610a 100644
--- a/django_migration_linter/sql_analyser/analyser.py
+++ b/django_migration_linter/sql_analyser/analyser.py
@@ -29,4 +29,4 @@ def analyse_sql_statements(
 ):
     sql_analyser = get_sql_analyser(database_vendor, exclude_migration_tests)
     sql_analyser.analyse(sql_statements)
-    return sql_analyser.errors, sql_analyser.ignored
+    return sql_analyser.errors, sql_analyser.ignored, sql_analyser.warnings
diff --git a/django_migration_linter/sql_analyser/base.py b/django_migration_linter/sql_analyser/base.py
index 4b60ebf..584ebbc 100644
--- a/django_migration_linter/sql_analyser/base.py
+++ b/django_migration_linter/sql_analyser/base.py
@@ -48,24 +48,28 @@ class BaseAnalyser(object):
             or re.search("ALTER TABLE .* RENAME TO", sql),
             "msg": "RENAMING tables",
             "mode": "one_liner",
+            "type": "error",
         },
         {
             "code": "NOT_NULL",
             "fn": has_not_null_column,
             "msg": "NOT NULL constraint on columns",
             "mode": "transaction",
+            "type": "error",
         },
         {
             "code": "DROP_COLUMN",
             "fn": lambda sql, **kw: re.search("DROP COLUMN", sql),
             "msg": "DROPPING columns",
             "mode": "one_liner",
+            "type": "error",
         },
         {
             "code": "DROP_TABLE",
             "fn": lambda sql, **kw: sql.startswith("DROP TABLE"),
             "msg": "DROPPING table",
             "mode": "one_liner",
+            "type": "error",
         },
         {
             "code": "RENAME_COLUMN",
@@ -73,6 +77,7 @@ class BaseAnalyser(object):
             or re.search("ALTER TABLE .* RENAME COLUMN", sql),
             "msg": "RENAMING columns",
             "mode": "one_liner",
+            "type": "error",
         },
         {
             "code": "ALTER_COLUMN",
@@ -84,12 +89,14 @@ class BaseAnalyser(object):
                 "You may ignore this migration.)"
             ),
             "mode": "one_liner",
+            "type": "error",
         },
         {
             "code": "ADD_UNIQUE",
             "fn": has_add_unique,
             "msg": "ADDING unique constraint",
             "mode": "transaction",
+            "type": "error",
         },
     ]
 
@@ -98,6 +105,7 @@ class BaseAnalyser(object):
     def __init__(self, exclude_migration_tests):
         self.exclude_migration_tests = exclude_migration_tests or []
         self.errors = []
+        self.warnings = []
         self.ignored = []
         self.migration_tests = update_migration_tests(
             self.base_migration_tests, self.migration_tests
@@ -113,21 +121,26 @@ class BaseAnalyser(object):
 
     @property
     def one_line_migration_tests(self):
-        return [test for test in self.migration_tests if test["mode"] == "one_liner"]
+        return (test for test in self.migration_tests if test["mode"] == "one_liner")
 
     @property
     def transaction_migration_tests(self):
-        return [test for test in self.migration_tests if test["mode"] == "transaction"]
+        return (test for test in self.migration_tests if test["mode"] == "transaction")
 
     def _test_sql(self, test, sql):
         if test["fn"](sql, errors=self.errors):
-            err = self.build_error_dict(migration_test=test, sql_statement=sql)
             if test["code"] in self.exclude_migration_tests:
-                logger.debug("Testing %s -- IGNORED", sql)
-                self.ignored.append(err)
+                action = "IGNORED"
+                list_to_add = self.ignored
+            elif test["type"] == "warning":
+                action = "WARNING"
+                list_to_add = self.warnings
             else:
-                logger.debug("Testing %s -- ERROR", sql)
-                self.errors.append(err)
+                action = "ERROR"
+                list_to_add = self.errors
+            logger.debug("Testing %s -- %s", sql, action)
+            err = self.build_error_dict(migration_test=test, sql_statement=sql)
+            list_to_add.append(err)
         else:
             logger.debug("Testing %s -- PASSED", sql)
 
diff --git a/django_migration_linter/sql_analyser/mysql.py b/django_migration_linter/sql_analyser/mysql.py
index 4178c32..487a85e 100644
--- a/django_migration_linter/sql_analyser/mysql.py
+++ b/django_migration_linter/sql_analyser/mysql.py
@@ -11,6 +11,7 @@ class MySqlAnalyser(BaseAnalyser):
                 "ALTER TABLE .* MODIFY .* (?!NULL);?$", sql
             ),
             "mode": "one_liner",
+            "type": "error",
         }
     ]
 
diff --git a/django_migration_linter/sql_analyser/postgresql.py b/django_migration_linter/sql_analyser/postgresql.py
index 5da16c2..140aba3 100644
--- a/django_migration_linter/sql_analyser/postgresql.py
+++ b/django_migration_linter/sql_analyser/postgresql.py
@@ -1,5 +1,31 @@
+import re
+
 from .base import BaseAnalyser
 
 
 class PostgresqlAnalyser(BaseAnalyser):
-    migration_tests = []
+    migration_tests = [
+        {
+            "code": "CREATE_INDEX",
+            "fn": lambda sql, **kw: re.search("CREATE (UNIQUE )?INDEX", sql)
+            and not re.search("INDEX CONCURRENTLY", sql),
+            "msg": "CREATE INDEX locks table",
+            "mode": "one_liner",
+            "type": "warning",
+        },
+        {
+            "code": "DROP_INDEX",
+            "fn": lambda sql, **kw: re.search("DROP INDEX", sql)
+            and not re.search("INDEX CONCURRENTLY", sql),
+            "msg": "DROP INDEX locks table",
+            "mode": "one_liner",
+            "type": "warning",
+        },
+        {
+            "code": "REINDEX",
+            "fn": lambda sql, **kw: sql.startswith("REINDEX"),
+            "msg": "REINDEX locks table",
+            "mode": "one_liner",
+            "type": "warning",
+        },
+    ]
diff --git a/django_migration_linter/sql_analyser/sqlite.py b/django_migration_linter/sql_analyser/sqlite.py
index 21503c6..9a947d1 100644
--- a/django_migration_linter/sql_analyser/sqlite.py
+++ b/django_migration_linter/sql_analyser/sqlite.py
@@ -27,6 +27,7 @@ class SqliteAnalyser(BaseAnalyser):
             "fn": lambda sql, **kw: re.search("ALTER TABLE .* RENAME TO", sql)
             and "__old" not in sql
             and "new__" not in sql,
+            "type": "error",
         },
         {
             "code": "DROP_TABLE",
@@ -37,6 +38,7 @@ class SqliteAnalyser(BaseAnalyser):
             and not any(sql.startswith("CREATE TABLE") for sql in sql_statements),
             "msg": "DROPPING table",
             "mode": "transaction",
+            "type": "error",
         },
         {
             "code": "NOT_NULL",
@@ -50,8 +52,14 @@ class SqliteAnalyser(BaseAnalyser):
                 for sql in sql_statements
             ),
             "mode": "transaction",
+            "type": "error",
+        },
+        {
+            "code": "ADD_UNIQUE",
+            "fn": has_add_unique,
+            "mode": "transaction",
+            "type": "error",
         },
-        {"code": "ADD_UNIQUE", "fn": has_add_unique, "mode": "transaction"},
     ]
 
     @staticmethod
diff --git a/docs/incompatibilities.md b/docs/incompatibilities.md
index 4f7b0ef..883a79f 100644
--- a/docs/incompatibilities.md
+++ b/docs/incompatibilities.md
@@ -39,3 +39,6 @@ You can ignore check through the `--exclude-migration-test` option and specifyin
 |`RUNPYTHON_MODEL_IMPORT`           | Missing apps.get_model() calls for model
 |`RUNPYTHON_MODEL_VARIABLE_NAME`    | The model variable name is different from the model class itself
 |`RUNSQL_REVERSIBLE`                | RunSQL data migration is not reversible (missing reverse SQL)
+|`CREATE_INDEX`                     | (Postgresql specific) Creating an index without the concurrent keyword will lock the table and may generate downtime
+|`DROP_INDEX`                       | (Postgresql specific) Dropping an index without the concurrent keyword will lock the table and may generate downtime
+|`REINDEX`                          | (Postgresql specific) Reindexing will lock the table and may generate downtime


Test patch:
diff --git a/tests/unit/test_sql_analyser.py b/tests/unit/test_sql_analyser.py
index 4b646ca..bb30d99 100644
--- a/tests/unit/test_sql_analyser.py
+++ b/tests/unit/test_sql_analyser.py
@@ -13,14 +13,22 @@ class SqlAnalyserTestCase(unittest.TestCase):
             sql_statements=sql, database_vendor=self.database_vendor
         )
 
-    def assertValidSql(self, sql):
-        errors, _ = self.analyse_sql(sql)
+    def assertValidSql(self, sql, allow_warnings=False):
+        errors, _, warnings = self.analyse_sql(sql)
         self.assertEqual(0, len(errors), "Found errors in sql: {}".format(errors))
+        if not allow_warnings:
+            self.assertEqual(
+                0, len(warnings), "Found warnings in sql: {}".format(errors)
+            )
 
     def assertBackwardIncompatibleSql(self, sql):
-        errors, _ = self.analyse_sql(sql)
+        errors, _, _ = self.analyse_sql(sql)
         self.assertNotEqual(0, len(errors), "Found no errors in sql")
 
+    def assertWarningSql(self, sql):
+        _, _, warnings = self.analyse_sql(sql)
+        self.assertNotEqual(0, len(warnings), "Found no warnings in sql")
+
 
 class MySqlAnalyserTestCase(SqlAnalyserTestCase):
     database_vendor = "mysql"
@@ -183,7 +191,7 @@ class PostgresqlAnalyserTestCase(SqlAnalyserTestCase):
             'CREATE INDEX "app_add_manytomany_field_b_many_to_many_b_id_953b185b" ON "app_add_manytomany_field_b_many_to_many"("b_id");',
             'CREATE INDEX "app_add_manytomany_field_b_many_to_many_a_id_4b44832a" ON "app_add_manytomany_field_b_many_to_many"("a_id");',
         ]
-        self.assertValidSql(sql)
+        self.assertValidSql(sql, allow_warnings=True)
 
     def test_make_column_not_null_with_django_default(self):
         sql = [
@@ -203,3 +211,29 @@ class PostgresqlAnalyserTestCase(SqlAnalyserTestCase):
             'ALTER TABLE "app_drop_default_a" ALTER COLUMN "col" SET DEFAULT `\'empty\';',
         ]
         self.assertValidSql(sql)
+
+    def test_create_index_non_concurrently(self):
+        sql = "CREATE INDEX ON films ((lower(title)));"
+        self.assertWarningSql(sql)
+        sql = "CREATE UNIQUE INDEX title_idx ON films (title);"
+        self.assertWarningSql(sql)
+
+    def test_create_index_concurrently(self):
+        sql = "CREATE INDEX CONCURRENTLY ON films (lower(title));"
+        self.assertValidSql(sql)
+        sql = "CREATE UNIQUE INDEX CONCURRENTLY title_idx ON films (title);"
+        self.assertValidSql(sql)
+
+    def test_drop_index_non_concurrently(self):
+        sql = "DROP INDEX ON films"
+        self.assertWarningSql(sql)
+
+    def test_drop_index_concurrently(self):
+        sql = "DROP INDEX CONCURRENTLY ON films;"
+        self.assertValidSql(sql)
+
+    def test_reindex(self):
+        sql = "REINDEX INDEX my_index;"
+        self.assertWarningSql(sql)
+        sql = "REINDEX TABLE my_table;"
+        self.assertWarningSql(sql)