[Django] #32770: GinIndex with OpClass and Cast results in Postgres syntax error in migration
Django
migration
--------------------------------------+------------------------
Reporter: syastrov | Owner: nobody
Type: Bug | Status: new
Component: Migrations | Version: 3.2
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 1
UI/UX: 0 |
--------------------------------------+------------------------
Given the following model + index:
{{{
from django.db import models
from django.contrib.postgres.indexes import GinIndex, OpClass
from django.db.models.functions import Cast
class MyModel(models.Model):
class Meta:
indexes = [
GinIndex(OpClass(Cast("id", output_field=models.TextField()),
name='gin_trgm_ops'), name='foobar')
]
}}}
After running makemigrations and running the migration, it produces the
SQL:
{{{
CREATE INDEX "foobar" ON "myapp_mymodel" USING gin ((CAST("id" AS text)
gin_trgm_ops));
}}}
Which is a syntax error on Postgres (version 12). The reason is the extra
parentheses around CAST.
--
Ticket URL: <https://code.djangoproject.com/ticket/32770>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
migration
Reporter: Seth Yastrov | Owner: nobody
Type: Bug | Status: closed
Component: Database layer | Version: 3.2
(models, ORM) | Resolution:
Severity: Normal | worksforme
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
-------------------------------------+-------------------------------------
Changes (by Mariusz Felisiak):
* cc: Hannes Ljungberg (added)
* resolution: => worksforme
* status: new => closed
* component: Migrations => Database layer (models, ORM)
* easy: 1 => 0
Comment:
Extra parentheses shouldn't be an issue on PostgreSQL. Also I cannot
reproduce this crash on PostgreSQL 12.6, see the following test.
{{{
def test_trigram_op_class_cast_gin_index(self):
index_name = 'trigram_op_class_castgin'
index = GinIndex(OpClass(Cast('id', CharField(max_length=255)),
name='gin_trgm_ops'), name=index_name)
with connection.schema_editor() as editor:
editor.add_index(Scene, index)
with editor.connection.cursor() as cursor:
cursor.execute(self.get_opclass_query, [index_name])
self.assertCountEqual(cursor.fetchall(), [('gin_trgm_ops',
index_name)])
constraints = self.get_constraints(Scene._meta.db_table)
self.assertIn(index_name, constraints)
self.assertIn(constraints[index_name]['type'], GinIndex.suffix)
with connection.schema_editor() as editor:
editor.remove_index(Scene, index)
self.assertNotIn(index_name,
self.get_constraints(Scene._meta.db_table))
}}}
{{{
CREATE INDEX "trigram_op_class_castgin" ON "postgres_tests_scene" USING
gin ((CAST("id" AS varchar(255))) gin_trgm_ops)
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/32770#comment:1>
Django
migration
Reporter: Seth Yastrov | Owner: nobody
Type: Bug | Status: closed
Component: Database layer | Version: 3.2
(models, ORM) | Resolution:
Severity: Normal | worksforme
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
-------------------------------------+-------------------------------------
Comment (by Hannes Ljungberg):
I'm pretty sure that you're seeing this error because of not adding
`django.contrib.postgres` to `INSTALLED_APPS`, see:
https://docs.djangoproject.com/en/3.2/ref/contrib/postgres/indexes
/#opclass-expressions
--
Ticket URL: <https://code.djangoproject.com/ticket/32770#comment:2>
Django
-------------------------------------+-------------------------------------
Reporter: Seth Yastrov | Owner: nobody
Type: Bug | Status: new
Component: Database layer | Version: 3.2
(models, ORM) |
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
-------------------------------------+-------------------------------------
* status: closed => new
* resolution: worksforme =>
Comment:
Thank you Hannes Ljungberg!
I did not notice that the documentation says to add
`django.contrib.postgres` to `INSTALLED_APPS`. That fixes the problem.
Would it be an idea to make a check that `django.contrib.postgres` is in
`INSTALLED_APPS` when making use of `OpClass` so that instead of
encountering a Postgres syntax error and not knowing what to do (and
assuming there is a bug in Django), you would get a helpful error message
telling how to correct the problem?
Therefore I'm reopening the issue, and hoping that this check can be made,
as despite this requirement being mentioned in the docs, the current
behavior is quite unintuitive.
--
Ticket URL: <https://code.djangoproject.com/ticket/32770#comment:3>
Django
OpClass().
-------------------------------------+-------------------------------------
Reporter: Seth Yastrov | Owner: nobody
Component: Database layer | Version: 3.2
(models, ORM) |
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
-------------------------------------+-------------------------------------
* status: new => closed
* type: Bug => New feature
* resolution: => needsinfo
Comment:
> Therefore I'm reopening the issue, and hoping that this check can be
made, as despite this requirement being mentioned in the docs, the current
behavior is quite unintuitive.
It would be really complicated. First of all, we would need to mix-up
logic from a contrib app and the ORM. Secondly `OpClass()` don't need to
be the topmost expression, so the flatten list of expression would be
necessary. Thirdly we don't have similar checks for fields from
`django.contrib.postgres` app which also require including
`'django.contrib.postgres'` in `INSTALLED_APPS`, e.g. `HStoreField`. I
don't think it's worth complexity, however we can reconsider this decision
if someone provides PoC.
--
Ticket URL: <https://code.djangoproject.com/ticket/32770#comment:4>
Django
OpClass().
-------------------------------------+-------------------------------------
Reporter: Seth Yastrov | Owner: nobody
Type: New feature | Status: closed
Component: Database layer | Version: 3.2
(models, ORM) |
Severity: Normal | Resolution: needsinfo
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
-------------------------------------+-------------------------------------
* cc: Simon Charette (added)
Comment:
In the light of continuous reports of users running into this qwirk
(#35431 being the latest one) I wonder if we could
1. Have all fields defined in `contrib.postgres` have their `check` method
make sure that `'django.contrib.postgres' in INSTALLED_APPS`
2. Adjust `Model._check_indexes` to do something similar to what we did
with `Constraint` check delegation in
0fb104dda287431f5ab74532e45e8471e22b58c8
3. Have both `contrib.postgres` indexes and constraints perform the same
`'django.contrib.postgres' in INSTALLED_APPS` check
Thoughts?
--
Ticket URL: <https://code.djangoproject.com/ticket/32770#comment:5>
Django
OpClass().
-------------------------------------+-------------------------------------
Reporter: Seth Yastrov | Owner: nobody
Type: New feature | Status: closed
Component: Database layer | Version: 3.2
(models, ORM) |
Severity: Normal | Resolution: needsinfo
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
-------------------------------------+-------------------------------------
I'd like to re-open my plea for the above to take place.
[https://forum.djangoproject.com/t/issue-with-postgres-gin-indexes/39842/8
I helped another user who ran into the same issue today] and it was from
clear what exactly was to blame.
Now that 2. from comment:5 is accepted under #36273 would there be
interest in re-opening this ticket?
--
Ticket URL: <https://code.djangoproject.com/ticket/32770#comment:6>
Django
OpClass().
-------------------------------------+-------------------------------------
Reporter: Seth Yastrov | Owner: nobody
Type: New feature | Status: closed
Component: Database layer | Version: 3.2
(models, ORM) |
Severity: Normal | Resolution: needsinfo
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
-------------------------------------+-------------------------------------
Hello, "user who ran into this same issue" here, I am in favor of A.
reopening this issue, and B. adding this check so it is clear what needs
to be done, instead of chasing my tail for a day and a half.
Thanks!
--
Ticket URL: <https://code.djangoproject.com/ticket/32770#comment:7>
Django
OpClass().
Component: contrib.postgres | Version: dev
Severity: Normal | Resolution:
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
----------------------------------+--------------------------------------
Changes (by Tim Graham):
* component: Database layer (models, ORM) => contrib.postgres
* resolution: needsinfo =>
Comment:
It seems feasible now that Simon outlined a plan.
--
Ticket URL: <https://code.djangoproject.com/ticket/32770#comment:8>
Django
OpClass().
Reporter: Seth Yastrov | Owner: Clifford
| Gama
Type: New feature | Status: assigned
Severity: Normal | Resolution:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
-------------------------------------+-------------------------------------
* owner: nobody => Clifford Gama
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/32770#comment:9>
Django
Reporter: Seth Yastrov | Owner: Clifford
Cleanup/optimization | Status: assigned
Severity: Normal | Resolution:
Needs tests: 0 | Patch needs improvement: 0
-------------------------------------+-------------------------------------
* stage: Unreviewed => Accepted
* summary:
using OpClass().
* type: New feature => Cleanup/optimization
Comment:
Clifford note that this ticket won't be completable until #36273 is
completed as the check that should be added
`django.contrib.postgres.PostgresIndex` requires that `check` method be
exposed to subclasses to avoid coupling core with contrib.
--
Ticket URL: <https://code.djangoproject.com/ticket/32770#comment:10>
Django
the fields, indexes, and constraints it provides.
-------------------------------------+-------------------------------------
Reporter: Seth Yastrov | Owner: Clifford
Type: | Gama
Cleanup/optimization | Status: assigned
Component: contrib.postgres | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
-------------------------------------+-------------------------------------
Thanks Simon👍. I noticed, and cc'd myself in #36273 so as to stay up-to-
date with the goings-on there.
--
Ticket URL: <https://code.djangoproject.com/ticket/32770#comment:11>
Django
the fields, indexes, and constraints it provides.
-------------------------------------+-------------------------------------
Reporter: Seth Yastrov | Owner: Clifford
Type: | Gama
Cleanup/optimization | Status: assigned
Component: contrib.postgres | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
-------------------------------------+-------------------------------------
* has_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/32770#comment:12>
Django
the fields, indexes, and constraints it provides.
-------------------------------------+-------------------------------------
Reporter: Seth Yastrov | Owner: Clifford
Type: | Gama
Cleanup/optimization | Status: assigned
Component: contrib.postgres | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
-------------------------------------+-------------------------------------
* needs_better_patch: 0 => 1
Comment:
Patch is looking good but I think it would be preferable to use a single
error error code over one per object type since the check is specific to
`postgres` and not the object type themselves.
--
Ticket URL: <https://code.djangoproject.com/ticket/32770#comment:13>
Django
the fields, indexes, and constraints it provides.
-------------------------------------+-------------------------------------
Reporter: Seth Yastrov | Owner: Clifford
Type: | Gama
Cleanup/optimization | Status: assigned
Component: contrib.postgres | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
-------------------------------------+-------------------------------------
* needs_better_patch: 1 => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/32770#comment:14>
Django
the fields, indexes, and constraints it provides.
-------------------------------------+-------------------------------------
Reporter: Seth Yastrov | Owner: Clifford
Type: | Gama
Cleanup/optimization | Status: assigned
Component: contrib.postgres | Version: dev
Severity: Normal | Resolution:
| checkin
Has patch: 1 | Needs documentation: 0
-------------------------------------+-------------------------------------
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/32770#comment:15>
Django
the fields, indexes, and constraints it provides.
-------------------------------------+-------------------------------------
Reporter: Seth Yastrov | Owner: Clifford
Type: | Gama
Component: contrib.postgres | Version: dev
Severity: Normal | Resolution: fixed
| checkin
-------------------------------------+-------------------------------------
* resolution: => fixed
* status: assigned => closed
Comment:
In [changeset:"74b31cd26b9ad4ad85f131850a734f02aae988bb" 74b31cd2]:
{{{#!CommitTicketReference repository=""
revision="74b31cd26b9ad4ad85f131850a734f02aae988bb"
Fixed #32770 -- Added system check to ensure django.contrib.postgres is
installed when using its features.
Added postgres.E005 to validate 'django.contrib.postgres' is in
INSTALLED_APPS
when using:
* PostgreSQL-specific fields (ArrayField, HStoreField, range fields,
SearchVectorField),
* PostgreSQL indexes (PostgresIndex and all subclasses), and
* ExclusionConstraint
The check provides immediate feedback during system checks rather than
failing
later with obscure runtime and database errors.
Thanks to Simon Charette and Sarah Boyce for reviews.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/32770#comment:16>