[Django] #35180: PostgreSQL pattern ops indexes are dropped when changing between CharField and TextField
Django
and TextField
-------------------------------------+-------------------------------------
Reporter: Robin Ray | Owner: nobody
Type: Bug | Status: new
Component: Database | Version: 5.0
layer (models, ORM) | Keywords: index postgres
Severity: Normal | migration
Triage Stage: | Has patch: 0
Unreviewed |
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-------------------------------------+-------------------------------------
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 demonstratest the issue:
https://github.com/robin-ray/alter_text_index_repro
--
Ticket URL: <https://code.djangoproject.com/ticket/35180>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
and TextField
-------------------------------------+-------------------------------------
Reporter: Robin Ray | Owner: nobody
Type: Bug | Status: new
(models, ORM) |
Severity: Normal | Resolution:
Keywords: index postgres | Triage Stage:
migration | Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Old description:
> 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 demonstratest the issue:
> https://github.com/robin-ray/alter_text_index_repro
New description:
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
--
--
Ticket URL: <https://code.djangoproject.com/ticket/35180#comment:1>
Django
and TextField
-------------------------------------+-------------------------------------
Reporter: Robin Ray | Owner:
Type: Bug | Status: assigned
Component: Database layer | Version: 5.0
(models, ORM) |
Severity: Normal | Resolution:
Keywords: index postgres | Triage Stage:
migration | Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* owner: nobody => timbaginski
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/35180#comment:2>
Django
and TextField
-------------------------------------+-------------------------------------
Reporter: Robin Ray | Owner:
| timbaginski
Type: Bug | Status: assigned
(models, ORM) |
Severity: Normal | Resolution:
migration |
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* cc: Simon Charette, Mariusz Felisiak (added)
* version: 5.0 => dev
* stage: Unreviewed => Accepted
Comment:
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:
{{{#!python
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.
With the patch above no test fail, so at least this is a starting point
for a conversation.
--
Ticket URL: <https://code.djangoproject.com/ticket/35180#comment:3>
Django
and TextField
-------------------------------------+-------------------------------------
Reporter: Robin Ray | Owner:
| timbaginski
Type: Bug | Status: assigned
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Keywords: index postgres | Triage Stage: Accepted
migration |
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/35180#comment:4>
Django
and TextField
-------------------------------------+-------------------------------------
Reporter: Robin Ray | Owner:
| timbaginski
Component: Database layer | Version: dev
(models, ORM) |
Keywords: index postgres | Triage Stage:
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: assigned => closed
* resolution: => duplicate
* stage: Accepted => Unreviewed
Comment:
As far as I'm aware, it is a
[https://github.com/django/django/pull/17807#discussion_r1479565648
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/35180#comment:5>
Django
and TextField
-------------------------------------+-------------------------------------
Reporter: Robin Ray | Owner:
| timbaginski
Type: Bug | Status: closed
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution: duplicate
Keywords: index postgres | Triage Stage:
migration | Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Replying to [comment:3 Natalia Bidart]:
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)
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/35180#comment:6>
Django
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
--
Ticket URL: <https://code.djangoproject.com/ticket/35180#comment:7>