Opened 10 months ago

Closed 3 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 Robin Ray)

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

  1. Given a model with a CharField that has db_index=True on a PostgreSQL database
  2. Inspect the database indexes or migration SQL and see that the indexed CharField has two indexes, one of which ends in _like and uses varchar_pattern_ops
  3. Change the CharField to a TextField and generate a migration
  4. Run the migration or inspect the SQL with the sqlmigrate manage command
  5. Inspect the database indexes or migration SQL and see that the indexed TextField does not have a _like index
  6. Reverse the migration
  7. 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 Robin Ray, 10 months ago

Description: modified (diff)

comment:2 by timbaginski, 10 months ago

Owner: changed from nobody to timbaginski
Status: newassigned

comment:3 by Natalia Bidart, 9 months ago

Cc: Simon Charette Mariusz Felisiak added
Triage Stage: UnreviewedAccepted
Version: 5.0dev

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 in django/db/backends/postgresql/schema.py that skips creating the index because old_field.db_index and new_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.

Last edited 9 months ago by Natalia Bidart (previous) (diff)

comment:4 by Natalia Bidart, 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 Mariusz Felisiak, 9 months ago

Resolution: duplicate
Status: assignedclosed
Triage Stage: AcceptedUnreviewed

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.

in reply to:  3 comment:7 by timbaginski, 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 in django/db/backends/postgresql/schema.py that skips creating the index because old_field.db_index and new_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)
Last edited 9 months ago by timbaginski (previous) (diff)

comment:8 by bcail, 2 months ago

Cc: bcail 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 Claude Paroz, 8 weeks ago

Resolution: duplicate
Status: closednew

Re-opening, at least for the new patch to be evaluated.

comment:11 by bcail, 8 weeks ago

Has patch: set

comment:12 by Natalia Bidart, 7 weeks ago

Owner: changed from timbaginski to bcail
Status: newassigned
Triage Stage: UnreviewedAccepted

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 Sarah Boyce, 3 weeks ago

Triage Stage: AcceptedReady for checkin

comment:14 by Sarah Boyce <42296566+sarahboyce@…>, 3 weeks ago

Resolution: fixed
Status: assignedclosed

In 0eaaadd4:

Fixed #35180 -- Recreated PostgreSQL _like indexes when changing between TextField and CharField field types.

Note: See TracTickets for help on using tickets.
Back to Top