Opened 10 months ago
Closed 7 weeks 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 , 10 months ago
Description: | modified (diff) |
---|
comment:2 by , 10 months ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
follow-up: 7 comment:3 by , 10 months ago
Cc: | added |
---|---|
Triage Stage: | Unreviewed → Accepted |
Version: | 5.0 → dev |
comment:4 by , 10 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 , 10 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 , 10 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 same thing, though my code is less elegant. I'm new to open source contribution, so my apologies if my claiming of the ticket and subsequent lack of communication caused any headache
# 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)
comment:8 by , 3 months 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 , 3 months ago
Resolution: | duplicate |
---|---|
Status: | closed → new |
Re-opening, at least for the new patch to be evaluated.
comment:11 by , 3 months ago
Has patch: | set |
---|
comment:12 by , 3 months 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 , 7 weeks 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.
With the patch above no test fail, so at least this is a starting point for a conversation.