Opened 9 months ago
Closed 5 days ago
#35180 closed Bug (fixed)
PostgreSQL pattern ops indexes are dropped when changing between CharField and TextField
Reported by: | Robin Ray | Owned by: | bcail |
---|---|---|---|
Component: | Database layer (models, ORM) | Version: | dev |
Severity: | Normal | Keywords: | index postgres migration |
Cc: | Simon Charette, Mariusz Felisiak, bcail | Triage Stage: | Ready for checkin |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description (last modified by )
When converting an indexed CharField to a TextField (and vice versa) on PostgreSQL, Django drops the existing pattern ops _like
index for the column but does not recreate it with the new pattern ops. When reversing the migration, Django does not recreate the initial _like
index.
I have been able to reproduce this behavior in Django 3.2 and 5.0.
Reproduction
- Given a model with a CharField that has
db_index=True
on a PostgreSQL database - Inspect the database indexes or migration SQL and see that the indexed CharField has two indexes, one of which ends in
_like
and usesvarchar_pattern_ops
- Change the CharField to a TextField and generate a migration
- Run the migration or inspect the SQL with the
sqlmigrate
manage command - Inspect the database indexes or migration SQL and see that the indexed TextField does not have a
_like
index - Reverse the migration
- Inspect the database indexes or migration SQL and see that the indexed CharField no longer has a
_like
index
Here is an example project that demonstrates the issue: https://github.com/robin-ray/alter_text_index_repro
Change History (13)
comment:1 by , 9 months ago
Description: | modified (diff) |
---|
comment:2 by , 9 months ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
follow-up: 7 comment:3 by , 9 months ago
Cc: | added |
---|---|
Triage Stage: | Unreviewed → Accepted |
Version: | 5.0 → dev |
comment:4 by , 9 months ago
EDIT: in my previous comment I mentioned that no test was failing with my proposed patch but I needed to run a wider set of tests. In fact there are failures=3, errors=6
so we should discuss the proper fix in more depth.
comment:5 by , 9 months ago
Resolution: | → duplicate |
---|---|
Status: | assigned → closed |
Triage Stage: | Accepted → Unreviewed |
As far as I'm aware, it is a duplicate of #28646 and changing a data type is not crucial for this issue. It's important that the altered field has db_index
set to True
, in such cases all indexes are deleted in the BaseDatabaseSchemaEditor._alter_field()
but in some cases we miss recreating _like
indexes.
comment:7 by , 9 months ago
Replying to Natalia Bidart:
Hello Robin Ray, thank you for your report and for helping making Django better.
As far as I can see, this is a valid issue. When I first read the description, I was sure there were already similar reports so I searched a bit for possible duplicates. I couldn't find an exact duplicate but there are related issues reported (and fixed in some cases) in #25412, #34505, and a few others involving collations.
It seems that the code at fault is the
_alter_field
indjango/db/backends/postgresql/schema.py
that skips creating the index becauseold_field.db_index
andnew_field.db_index
are both True. I'm not an expert in this area so I have added some people as cc in this ticket, but I think this guard needs to also consider whether the column being altered is also changing its type:
diff --git a/django/db/backends/postgresql/schema.py b/django/db/backends/postgresql/schema.py index 842830be30..975a9f1e93 100644 --- a/django/db/backends/postgresql/schema.py +++ b/django/db/backends/postgresql/schema.py @@ -295,10 +295,12 @@ class DatabaseSchemaEditor(BaseDatabaseSchemaEditor): new_db_params, strict, ) + type_changed = old_type != new_type + old_db_index = old_field.db_index or old_field.unique + new_db_index = new_field.db_index or new_field.unique + needs_index = (not old_db_index and new_db_index) or (type_changed and new_db_index) # Added an index? Create any PostgreSQL-specific indexes. - if (not (old_field.db_index or old_field.unique) and new_field.db_index) or ( - not old_field.unique and new_field.unique - ): + if needs_index: like_index_statement = self._create_like_index_sql(model, new_field) if like_index_statement is not None: self.execute(like_index_statement)NOTE: please do not consider this diff as suitable for a patch without further discussion.
I've been looking at it and noticed the exact same thing. I think you just need to add a condition to check if the old field was type varchar or text and got changed.
# Added an index or deleted index due to type change? # Create any PostgreSQL-specific indexes. if (not (old_field.db_index or old_field.unique) and new_field.db_index) or ( not old_field.unique and new_field.unique) or ((old_field.db_index or old_field.unique) and ( (old_type.startswith("varchar") and not new_type.startswith("varchar")) or (old_type.startswith("text") and not new_type.startswith("text"))) ): like_index_statement = self._create_like_index_sql(model, new_field) if like_index_statement is not None: self.execute(like_index_statement)
EDIT: I'm new to open source contribution, so my apologies if my claiming of the ticket and subsequent lack of communication caused any headache
comment:8 by , 7 weeks ago
Cc: | added |
---|
I know this was closed as a duplicate, but could we please look into re-opening it? I've opened a [PR https://github.com/django/django/pull/18593] that tests changing a CharField to a TextField and fixes the issue by checking for a type change on an indexed field.
comment:10 by , 6 weeks ago
Resolution: | duplicate |
---|---|
Status: | closed → new |
Re-opening, at least for the new patch to be evaluated.
comment:11 by , 6 weeks ago
Has patch: | set |
---|
comment:12 by , 5 weeks ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
Triage Stage: | Unreviewed → Accepted |
After a quick look, the patch looks sensible. This requires an in-depth review so I'll accept to get this ticket moved to the review queue.
comment:13 by , 6 days ago
Triage Stage: | Accepted → Ready for checkin |
---|
Hello Robin Ray, thank you for your report and for helping making Django better.
As far as I can see, this is a valid issue. When I first read the description, I was sure there were already similar reports so I searched a bit for possible duplicates. I couldn't find an exact duplicate but there are related issues reported (and fixed in some cases) in #25412, #34505, and a few others involving collations.
It seems that the code at fault is the
_alter_field
indjango/db/backends/postgresql/schema.py
that skips creating the index becauseold_field.db_index
andnew_field.db_index
are both True. I'm not an expert in this area so I have added some people as cc in this ticket, but I think this guard needs to also consider whether the column being altered is also changing its type:NOTE: please do not consider this diff as suitable for a patch without further discussion.