From eb59c472fbbfe948544fda0d7950ef6a8e3fefbd Mon Sep 17 00:00:00 2001 From: Chris Conn Date: Wed, 15 Jan 2025 09:40:18 +0000 Subject: [PATCH] Fix #264 -- Don't detect an index creation during a transaction with an exclusive lock, when the table is being created Improve documentation on this issue. --- CHANGELOG.md | 3 ++ docs/incompatibilities.md | 36 +++++++++---------- .../sql_analyser/postgresql.py | 8 +++-- tests/unit/test_sql_analyser.py | 24 +++++++++++++ 4 files changed, 50 insertions(+), 21 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1f9e2b4..e40ebc3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,8 @@ ## 5.x.y +Bug: +- Don't detect an index creation during a transaction with an exclusive lock, when the table is being created (#264) + Miscellaneous: - Add support for Python 3.13 diff --git a/docs/incompatibilities.md b/docs/incompatibilities.md index a8cd365..4fa8931 100644 --- a/docs/incompatibilities.md +++ b/docs/incompatibilities.md @@ -25,24 +25,24 @@ On data migrations, the linter will show a warning when: You can ignore checks through the `--exclude-migration-tests` option by specifying any of the codes: -| Code | Description | Default type | -|------------------------------------|----------------------------------------------------------------------------------------------------------------------|--------------| -| `NOT_NULL` | Not NULL constraint on columns | Error | -| `DROP_COLUMN` | Dropping columns | Error | -| `DROP_TABLE` | Dropping tables | Error | -| `RENAME_COLUMN` | Renaming columns | Error | -| `RENAME_TABLE` | Renaming tables | Error | -| `ALTER_COLUMN` | Altering columns (could be backward compatible) | Error | -| `ADD_UNIQUE` | Add unique constraints | Error | -| `RUNPYTHON_REVERSIBLE` | RunPython data migration is not reversible (missing reverse code) | Warning | -| `RUNPYTHON_ARGS_NAMING_CONVENTION` | By convention, RunPython names two arguments: apps, schema_editor | Warning | -| `RUNPYTHON_MODEL_IMPORT` | Missing apps.get_model() calls for model | Error | -| `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 | Warning | -| `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 | +| Code | Description | Default type | +|------------------------------------|-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|--------------| +| `NOT_NULL` | Not NULL constraint on columns | Error | +| `DROP_COLUMN` | Dropping columns | Error | +| `DROP_TABLE` | Dropping tables | Error | +| `RENAME_COLUMN` | Renaming columns | Error | +| `RENAME_TABLE` | Renaming tables | Error | +| `ALTER_COLUMN` | Altering columns (could be backward compatible) | Error | +| `ADD_UNIQUE` | Add unique constraints | Error | +| `RUNPYTHON_REVERSIBLE` | RunPython data migration is not reversible (missing reverse code) | Warning | +| `RUNPYTHON_ARGS_NAMING_CONVENTION` | By convention, RunPython names two arguments: apps, schema_editor | Warning | +| `RUNPYTHON_MODEL_IMPORT` | Missing apps.get_model() calls for model | Error | +| `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 acquiring an `EXCLUSIVE` lock (e.g. most `ALTER TABLE` statements acquire one) prolongs the exclusive lock on the table. Using concurrently clause does not address the issue. On the contrary, it prolongs the transaction, making it more dangerous in this situation. | Warning | +| `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 | ## Details about backward incompatibilities diff --git a/src/django_migration_linter/sql_analyser/postgresql.py b/src/django_migration_linter/sql_analyser/postgresql.py index 28e0624..109fb6c 100644 --- a/src/django_migration_linter/sql_analyser/postgresql.py +++ b/src/django_migration_linter/sql_analyser/postgresql.py @@ -33,7 +33,7 @@ def has_create_index_in_transaction(sql_statements: list[str], **kwargs) -> bool # (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 :], ignore_concurrently=False) + return has_create_index(sql_statements, ignore_concurrently=False) return False @@ -41,7 +41,7 @@ def has_create_index( sql_statements: list[str], ignore_concurrently: bool = True, **kwargs ) -> bool: regex_result = None - for sql in sql_statements: + for i, sql in enumerate(sql_statements): regex_result = re.search(r"CREATE (UNIQUE )?INDEX.*ON (.*) \(", sql) if ignore_concurrently and re.search("INDEX CONCURRENTLY", sql): regex_result = None @@ -50,9 +50,11 @@ def has_create_index( if not regex_result: return False + preceding_sql_statements = sql_statements[:i] concerned_table = regex_result.group(2) table_is_added_in_transaction = any( - sql.startswith(f"CREATE TABLE {concerned_table}") for sql in sql_statements + sql.startswith(f"CREATE TABLE {concerned_table}") + for sql in preceding_sql_statements ) return not table_is_added_in_transaction diff --git a/tests/unit/test_sql_analyser.py b/tests/unit/test_sql_analyser.py index 6dbce6b..fdd1c83 100644 --- a/tests/unit/test_sql_analyser.py +++ b/tests/unit/test_sql_analyser.py @@ -315,6 +315,13 @@ class PostgresqlAnalyserTestCase(SqlAnalyserTestCase): 'CREATE INDEX ON "films" ((lower("title")));', ] self.assertValidSql(sql) + sql = [ + 'CREATE TABLE "films" ("title" text);', + 'ALTER TABLE "films" ADD CONSTRAINT "title_not_empty" CHECK (char_length("title") > 0)' + 'CREATE INDEX ON "films" ((lower("title")));', + ] + self.assertValidSql(sql) + # Not the same column or tables. sql = [ 'CREATE TABLE "some_table" ("title" text);', 'CREATE INDEX ON "films" ((lower("title")));', @@ -363,6 +370,23 @@ class PostgresqlAnalyserTestCase(SqlAnalyserTestCase): ] self.assertWarningSql(sql, code="CREATE_INDEX_EXCLUSIVE") + def test_create_index_exclusive_with_prior_table_creation(self): + sql = [ + "BEGIN;", + 'CREATE TABLE "users" ("email" text);', + 'CREATE INDEX "user_email" ON "users" ("email");', + "COMMIT;", + ] + self.assertValidSql(sql) + sql = [ + "BEGIN;", + 'CREATE TABLE "users" ("id" number);', + 'ALTER TABLE "users" ADD COLUMN "email" varchar(254) NULL;', + 'CREATE INDEX "user_email" ON "users" ("email");', + "COMMIT;", + ] + self.assertValidSql(sql) + def test_create_concurrently_index_exclusive(self): sql = [ "BEGIN;",