Opened 21 months ago
Closed 12 months 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=Trueon a PostgreSQL database
- Inspect the database indexes or migration SQL and see that the indexed CharField has two indexes, one of which ends in _likeand usesvarchar_pattern_ops
- Change the CharField to a TextField and generate a migration
- Run the migration or inspect the SQL with the sqlmigratemanage command
- Inspect the database indexes or migration SQL and see that the indexed TextField does not have a _likeindex
- Reverse the migration
- Inspect the database indexes or migration SQL and see that the indexed CharField no longer has a _likeindex
Here is an example project that demonstrates the issue: https://github.com/robin-ray/alter_text_index_repro
Change History (13)
comment:1 by , 21 months ago
| Description: | modified (diff) | 
|---|
comment:2 by , 21 months ago
| Owner: | changed from to | 
|---|---|
| Status: | new → assigned | 
follow-up: 7 comment:3 by , 21 months ago
| Cc: | added | 
|---|---|
| Triage Stage: | Unreviewed → Accepted | 
| Version: | 5.0 → dev | 
comment:4 by , 21 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 , 21 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 , 21 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_fieldindjango/db/backends/postgresql/schema.pythat skips creating the index becauseold_field.db_indexandnew_field.db_indexare 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 , 14 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 , 13 months ago
| Resolution: | duplicate | 
|---|---|
| Status: | closed → new | 
Re-opening, at least for the new patch to be evaluated.
comment:11 by , 13 months ago
| Has patch: | set | 
|---|
comment:12 by , 13 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 , 12 months 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_fieldindjango/db/backends/postgresql/schema.pythat skips creating the index becauseold_field.db_indexandnew_field.db_indexare 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.