[Django] #34840: Django 4.2 casts text fields when testing IS NULL, preventing use of partial indexes
Django
partial indexes
-------------------------------------+-------------------------------------
Reporter: Alex | Owner: nobody
Vandiver |
Type: | Status: new
Uncategorized |
Component: Database | Version: 4.2
layer (models, ORM) |
Severity: Normal | Keywords:
Triage Stage: | Has patch: 0
Unreviewed |
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-------------------------------------+-------------------------------------
The Zulip project has a model with unique constraints which are expressed
as two non-overlapping partial indexes:
{{{
class UserCount(models.Model):
user = models.ForeignKey(UserProfile, on_delete=models.CASCADE)
realm = models.ForeignKey(Realm, on_delete=models.CASCADE)
property = models.CharField(max_length=32)
subgroup = models.CharField(max_length=16, null=True)
end_time = models.DateTimeField()
value = models.BigIntegerField()
class Meta:
constraints = [
UniqueConstraint(
fields=["user", "property", "subgroup", "end_time"],
condition=Q(subgroup__isnull=False),
name="unique_user_count",
),
UniqueConstraint(
fields=["user", "property", "end_time"],
condition=Q(subgroup__isnull=True),
name="unique_user_count_null_subgroup",
),
]
}}}
However, since commit 09ffc5c1212d4ced58b708cbbf3dfbfb77b782ca in Django
4.2, a query of the form:
{{{
UserCount.objects.get(
property="messages_read::hour",
subgroup=None,
end_time=timezone_now(),
user_id=user_profile.id,
realm_id=realm.id,
)
}}}
...generates this SQL:
{{{
SELECT "analytics_usercount"."id", "analytics_usercount"."property",
"analytics_usercount"."subgroup", "analytics_usercount"."end_time",
"analytics_usercount"."value", "analytics_usercount"."user_id",
"analytics_usercount"."realm_id" FROM "analytics_usercount" WHERE
("analytics_usercount"."end_time" =
'2023-09-13T19:16:34.195355+00:00'::timestamptz AND
"analytics_usercount"."property" = 'messages_read::hour' AND
"analytics_usercount"."realm_id" = 4715 AND
"analytics_usercount"."subgroup"::text IS NULL AND
"analytics_usercount"."user_id" = 428054) LIMIT 21
}}}
The cast of `"analytics_usercount"."subgroup"::text IS NULL` causes
PostgreSQL to not be able to use the unique partial index:
{{{
Limit (cost=48.30..49.42 rows=1 width=61)
-> Bitmap Heap Scan on analytics_usercount (cost=48.30..49.42 rows=1
width=61)
Recheck Cond: (((property)::text = 'messages_read::hour'::text)
AND (realm_id = 4715) AND (end_time = '2023-09-13 19:00:00+00'::timestamp
with time zone) AND (user_id = 428054))
Filter: ((subgroup)::text IS NULL)
-> BitmapAnd (cost=48.30..48.30 rows=1 width=0)
-> Bitmap Index Scan on
analytics_usercount_property_591dbec1_idx (cost=0.00..4.88 rows=158
width=0)
Index Cond: (((property)::text =
'messages_read::hour'::text) AND (realm_id = 4715) AND (end_time =
'2023-09-13 19:00:00+00'::timestamp with time zone))
-> Bitmap Index Scan on analytics_usercount_e8701ad4
(cost=0.00..43.17 rows=3626 width=0)
Index Cond: (user_id = 428054)
}}}
Dropping the explicit cast causes it to use the index:
{{{
Limit (cost=0.57..2.80 rows=1 width=61)
-> Index Scan using unique_user_count_null_subgroup on
analytics_usercount (cost=0.57..2.80 rows=1 width=61)
Index Cond: ((user_id = 428054) AND ((property)::text =
'messages_read::hour'::text) AND (end_time = '2023-09-13
19:00:00+00'::timestamp with time zone))
Filter: (realm_id = 4715)
}}}
...and improving the query runtime significantly.
It's not clear to me from 09ffc5c1212d4ced58b708cbbf3dfbfb77b782ca what
about psycopg3 requires this cast.
--
Ticket URL: <https://code.djangoproject.com/ticket/34840>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
partial indexes
-------------------------------------+-------------------------------------
Type: Bug | Status: new
Component: Database layer | Version: 4.2
(models, ORM) |
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* type: Uncategorized => Bug
* severity: Normal => Release blocker
* stage: Unreviewed => Accepted
Comment:
We should try to avoid casts if possible (IIRC they are only required for
server-side bindings).
Out of curiosity, if you try re-creating your `UniqueConstraint` on 4.2,
are they created with casts?
--
Ticket URL: <https://code.djangoproject.com/ticket/34840#comment:1>
Django
partial indexes
-------------------------------------+-------------------------------------
Reporter: Alex Vandiver | Owner: nobody
Type: Bug | Status: new
Component: Database layer | Version: 4.2
(models, ORM) |
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Alex Vandiver):
Yes, the indexes as created on 4.2 ''do'' have the cast in them. The diff
of our schema across the 4.2 upgrade is:
{{{
$ diff -U0 <(rg -U '(?s)CREATE[^;]*;' before.sql) <(rg -U
'(?s)CREATE[^;]*;' after.sql)
--- /dev/fd/63 2023-09-13 12:55:09.595790167 -0700
+++ /dev/fd/62 2023-09-13 12:55:09.595790167 -0700
@@ -1095,4 +1095,4 @@
-CREATE UNIQUE INDEX unique_installation_count ON
zulip.analytics_installationcount USING btree (property, subgroup,
end_time) WHERE (subgroup IS NOT NULL);
-CREATE UNIQUE INDEX unique_installation_count_null_subgroup ON
zulip.analytics_installationcount USING btree (property, end_time) WHERE
(subgroup IS NULL);
-CREATE UNIQUE INDEX unique_realm_count ON zulip.analytics_realmcount
USING btree (realm_id, property, subgroup, end_time) WHERE (subgroup IS
NOT NULL);
-CREATE UNIQUE INDEX unique_realm_count_null_subgroup ON
zulip.analytics_realmcount USING btree (realm_id, property, end_time)
WHERE (subgroup IS NULL);
+CREATE UNIQUE INDEX unique_installation_count ON
zulip.analytics_installationcount USING btree (property, subgroup,
end_time) WHERE ((subgroup)::text IS NOT NULL);
+CREATE UNIQUE INDEX unique_installation_count_null_subgroup ON
zulip.analytics_installationcount USING btree (property, end_time) WHERE
((subgroup)::text IS NULL);
+CREATE UNIQUE INDEX unique_realm_count ON zulip.analytics_realmcount
USING btree (realm_id, property, subgroup, end_time) WHERE
((subgroup)::text IS NOT NULL);
+CREATE UNIQUE INDEX unique_realm_count_null_subgroup ON
zulip.analytics_realmcount USING btree (realm_id, property, end_time)
WHERE ((subgroup)::text IS NULL);
@@ -1100,4 +1100,4 @@
-CREATE UNIQUE INDEX unique_stream_count ON zulip.analytics_streamcount
USING btree (stream_id, property, subgroup, end_time) WHERE (subgroup IS
NOT NULL);
-CREATE UNIQUE INDEX unique_stream_count_null_subgroup ON
zulip.analytics_streamcount USING btree (stream_id, property, end_time)
WHERE (subgroup IS NULL);
-CREATE UNIQUE INDEX unique_user_count ON zulip.analytics_usercount USING
btree (user_id, property, subgroup, end_time) WHERE (subgroup IS NOT
NULL);
-CREATE UNIQUE INDEX unique_user_count_null_subgroup ON
zulip.analytics_usercount USING btree (user_id, property, end_time) WHERE
(subgroup IS NULL);
+CREATE UNIQUE INDEX unique_stream_count ON zulip.analytics_streamcount
USING btree (stream_id, property, subgroup, end_time) WHERE
((subgroup)::text IS NOT NULL);
+CREATE UNIQUE INDEX unique_stream_count_null_subgroup ON
zulip.analytics_streamcount USING btree (stream_id, property, end_time)
WHERE ((subgroup)::text IS NULL);
+CREATE UNIQUE INDEX unique_user_count ON zulip.analytics_usercount USING
btree (user_id, property, subgroup, end_time) WHERE ((subgroup)::text IS
NOT NULL);
+CREATE UNIQUE INDEX unique_user_count_null_subgroup ON
zulip.analytics_usercount USING btree (user_id, property, end_time) WHERE
((subgroup)::text IS NULL);
}}}
So if the application's schema was inserted with 4.2, then its partial
indexes work with the current code. If it was inserted with Django < 4.2,
then the indexes don't work with >= 4.2.
This also means that there's a bit of a pickle -- you can't just strip off
the `::text` in the query in some later 4.2 bugfix release, because that
will also make any 4.2-inserted indexes with `::text` not work with the
newly `::text`-less query.
Replying to [comment:1 Simon Charette]:
> We should try to avoid casts if possible (IIRC they are only required
for server-side bindings).
>
> Out of curiosity, if you try re-creating your `UniqueConstraint` on 4.2,
are they created with casts? By that I'm in no mean implying that a
solution to this problem is to force the re-creation of constraints on
each releases but I want to have an idea of the scope of the problem.
>
> If that's the case it means that reverting to no cast in a minor release
might break Postgres query planer the other way around as constraint
created on 4.2 include `::text` cast but queries on a future version
removing them don't.
--
Ticket URL: <https://code.djangoproject.com/ticket/34840#comment:2>
Django
partial indexes
-------------------------------------+-------------------------------------
Reporter: Alex Vandiver | Owner: nobody
Type: Bug | Status: new
Component: Database layer | Version: 4.2
(models, ORM) |
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* cc: Florian Apolloner, Mariusz Felisiak (added)
Comment:
> This also means that there's a bit of a pickle -- you can't just strip
off the ::text in the query in some later 4.2 bugfix release, because that
will also make any 4.2-inserted indexes with ::text not work with the
newly ::text-less query.
Right, that's what I feared. I think we should favour reverting to the
previous behaviour if possible though as there are likely to be more
constraints created prior to 4.2 in the wild than after even at this
point. This is definitely something that should be mentioned in the
release notes though.
Any thoughts here Florian and Mariusz?
--
Ticket URL: <https://code.djangoproject.com/ticket/34840#comment:3>
Django
partial indexes
-------------------------------------+-------------------------------------
Reporter: Alex Vandiver | Owner: nobody
Type: Bug | Status: new
Component: Database layer | Version: 4.2
(models, ORM) |
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Mariusz Felisiak):
Replying to [comment:3 Simon Charette]:
> Any thoughts here Florian and Mariusz?
I'd prefer to avoid casting, however, it is necessary when server-side
binding is enabled.
--
Ticket URL: <https://code.djangoproject.com/ticket/34840#comment:4>
Django
partial indexes
-------------------------------------+-------------------------------------
Reporter: Alex Vandiver | Owner: nobody
Type: Bug | Status: new
Component: Database layer | Version: 4.2
(models, ORM) |
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* cc: Simon Charette (added)
Comment:
What do you think of making the casting conditional to the enablement of
the `server_side_binding` option in this case?
That would contain the issue to the already existing buckets or problems
that come with enabling this option that we're already aware of (e.g.
grouping by parameterized annotation).
Happy to prepare a patch if you believe that it's worth pursuing.
--
Ticket URL: <https://code.djangoproject.com/ticket/34840#comment:5>
Django
partial indexes
-------------------------------------+-------------------------------------
Reporter: Alex Vandiver | Owner: nobody
Type: Bug | Status: new
Component: Database layer | Version: 4.2
(models, ORM) |
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Mariusz Felisiak):
Replying to [comment:5 Simon Charette]:
> What do you think of making the casting conditional to the enablement of
the `server_side_binding` option in this case?
Yeah, I was thinking the same thing. This is probably the only reasonable
option we have.
--
Ticket URL: <https://code.djangoproject.com/ticket/34840#comment:6>
Django
partial indexes
-------------------------------------+-------------------------------------
Reporter: Alex Vandiver | Owner: nobody
Type: Bug | Status: new
Component: Database layer | Version: 4.2
(models, ORM) |
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Florian Apolloner):
This doesn't just affect `::text` but all casts?
--
Ticket URL: <https://code.djangoproject.com/ticket/34840#comment:7>
Django
partial indexes
-------------------------------------+-------------------------------------
Reporter: Alex Vandiver | Owner: nobody
Type: Bug | Status: new
Component: Database layer | Version: 4.2
(models, ORM) |
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Mariusz Felisiak):
Replying to [comment:7 Florian Apolloner]:
> This doesn't just affect `::text` but all casts?
As far as I'm aware, from the lookup perspective it
[https://github.com/django/django/blob/814e7bc22062eeae4be9f189e89027e28d5dd290/django/db/backends/postgresql/operations.py#L158-L166
affects] mainly `__isnull`.
--
Ticket URL: <https://code.djangoproject.com/ticket/34840#comment:8>
Django
partial indexes
-------------------------------------+-------------------------------------
Type: Bug | Status: assigned
Component: Database layer | Version: 4.2
(models, ORM) |
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* owner: nobody => murtatah
* status: new => assigned
Comment:
Hey everyone,
I'm excited to tackle this one! The issue (#34840) looks like a great
challenge, and I'd love to help resolve it. 😊
I'll start working on a solution and keep you posted on my progress. If
you have any insights or suggestions along the way, please feel free to
share them.while this is the first time I'm working on something like this
so I hope I can do it right as much as possible!
Thanks for the opportunity!
--
Ticket URL: <https://code.djangoproject.com/ticket/34840#comment:9>
Django
partial indexes
-------------------------------------+-------------------------------------
Reporter: Alex Vandiver | Owner: murtatah
Type: Bug | Status: assigned
Component: Database layer | Version: 4.2
(models, ORM) |
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Florian Apolloner):
Replying to [comment:8 Mariusz Felisiak]:
> Replying to [comment:7 Florian Apolloner]:
> > This doesn't just affect `::text` but all casts?
>
> As far as I'm aware, from the lookup perspective it
[https://github.com/django/django/blob/814e7bc22062eeae4be9f189e89027e28d5dd290/django/db/backends/postgresql/operations.py#L158-L166
affects] mainly `__isnull`.
Oh right, and that is mostly a result of handling text columns as
`unknown` oids instead of `text` which has/had it's own share of problems.
Replying to [comment:5 Simon Charette]:
> What do you think of making the casting conditional to the enablement of
the `server_side_binding` option in this case?
Thinking about this more I am not sure this is a good idea; after all
people should be free to change that setting and Django should behave the
same. I feel like we are running into limitations of our ORM here, do we
really need the `::text` cast here for server side bindings? Isn't
postgresql able to properly deduce the types? After all, in the query as
well as in the index we reference the column `subgroup` which has a well
defined type (or am I mistaken here). We should only need type casts if we
involve bind parameters.
Though that begs the next question: Assume you generate an index for a
text column named `x`. Would a query with `.filter(x__startswith='test')`
use the index if the index is created on `x` but the query uses `x::text
like 'test%'`?
--
Ticket URL: <https://code.djangoproject.com/ticket/34840#comment:10>
Django
partial indexes
-------------------------------------+-------------------------------------
Reporter: Alex Vandiver | Owner: murtatah
Type: Bug | Status: assigned
Component: Database layer | Version: 4.2
(models, ORM) |
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Florian Apolloner):
Running the tests with server side param binding enabled and having
removed the cast for `IS NULL` results in three failures:
{{{
======================================================================
ERROR: test_model_validation_with_condition
(constraints.tests.UniqueConstraintTests.test_model_validation_with_condition)
Partial unique constraints are not ignored by
----------------------------------------------------------------------
Traceback (most recent call last):
File "/usr/lib64/python3.11/unittest/case.py", line 57, in
testPartExecutor
yield
File "/usr/lib64/python3.11/unittest/case.py", line 623, in run
self._callTestMethod(testMethod)
^^^^^^^^^^^^^^^^^
File "/usr/lib64/python3.11/unittest/case.py", line 579, in
_callTestMethod
if method() is not None:
^^^^^^^^^^^^^^^^^
File "/home/florian/sources/django.git/django/test/testcases.py", line
1425, in skip_wrapper
return test_func(*args, **kwargs)
^^^^^^^^^^^^^^^^^
File "/home/florian/sources/django.git/tests/constraints/tests.py", line
804, in test_model_validation_with_condition
).validate_constraints()
^^^^^^^^^^^^^^^^^
File "/home/florian/sources/django.git/django/db/models/base.py", line
1497, in validate_constraints
raise ValidationError(errors)
^^^^^^^^^^^^^^^^^
django.core.exceptions.ValidationError: {'__all__': ['Constraint
“name_without_color_uniq” is violated.']}
======================================================================
ERROR: test_validate_condition
(constraints.tests.UniqueConstraintTests.test_validate_condition)
----------------------------------------------------------------------
Traceback (most recent call last):
File "/usr/lib64/python3.11/unittest/case.py", line 57, in
testPartExecutor
yield
File "/usr/lib64/python3.11/unittest/case.py", line 623, in run
self._callTestMethod(testMethod)
^^^^^^^^^^^^^^^^^
File "/usr/lib64/python3.11/unittest/case.py", line 579, in
_callTestMethod
if method() is not None:
^^^^^^^^^^^^^^^^^
File "/home/florian/sources/django.git/django/test/testcases.py", line
1425, in skip_wrapper
return test_func(*args, **kwargs)
^^^^^^^^^^^^^^^^^
File "/home/florian/sources/django.git/tests/constraints/tests.py", line
879, in test_validate_condition
constraint.validate(
^^^^^^^^^^^^^^^^^
File "/home/florian/sources/django.git/django/db/models/constraints.py",
line 461, in validate
raise ValidationError(
^^^^^^^^^^^^^^^^^
django.core.exceptions.ValidationError: ['Constraint
“name_without_color_uniq” is violated.']
======================================================================
ERROR: test_validate_expression_condition
(constraints.tests.UniqueConstraintTests.test_validate_expression_condition)
----------------------------------------------------------------------
Traceback (most recent call last):
File "/usr/lib64/python3.11/unittest/case.py", line 57, in
testPartExecutor
yield
File "/usr/lib64/python3.11/unittest/case.py", line 623, in run
self._callTestMethod(testMethod)
^^^^^^^^^^^^^^^^^
File "/usr/lib64/python3.11/unittest/case.py", line 579, in
_callTestMethod
if method() is not None:
^^^^^^^^^^^^^^^^^
File "/home/florian/sources/django.git/tests/constraints/tests.py", line
965, in test_validate_expression_condition
constraint.validate(
^^^^^^^^^^^^^^^^^
File "/home/florian/sources/django.git/django/db/models/constraints.py",
line 461, in validate
raise ValidationError(
^^^^^^^^^^^^^^^^^
django.core.exceptions.ValidationError: ['Constraint
“name_lower_without_color_uniq” is violated.']
----------------------------------------------------------------------
Ran 16695 tests in 201.401s
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/34840#comment:11>
Django
partial indexes
-------------------------------------+-------------------------------------
Reporter: Alex Vandiver | Owner: murtatah
Type: Bug | Status: assigned
Component: Database layer | Version: 4.2
(models, ORM) |
Severity: Release blocker | Resolution:
| checkin
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/34840#comment:12>
Django
partial indexes
-------------------------------------+-------------------------------------
Reporter: Alex Vandiver | Owner: murtatah
Type: Bug | Status: assigned
Component: Database layer | Version: 4.2
(models, ORM) |
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Ready for
| checkin
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by murtatah):
fix: Correct value format for CharArrayModel in test_isnull_lookup_cast
In the test_isnull_lookup_cast method, the CharArrayModel instance was
created with an incorrect value format for the 'field' attribute. This
caused a PostgreSQL error when trying to create the instance.
This commit fixes the issue by using a list to represent an array and
encloses the value properly in curly braces, ensuring the correct format
for the 'field' attribute.i made some test check the status of it.
--
Ticket URL: <https://code.djangoproject.com/ticket/34840#comment:13>
Django
partial indexes
-------------------------------------+-------------------------------------
Reporter: Alex Vandiver | Owner: murtatah
Type: Bug | Status: assigned
Component: Database layer | Version: 4.2
(models, ORM) |
Severity: Release blocker | Resolution:
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* stage: Ready for checkin => Accepted
Comment:
Please don't see tickets with your own merge requests to "ready for
checkin". Additionally, simply removing the cast is not the correct fix
(at the very least it is most likely possible to just drop the if etc…)
--
Ticket URL: <https://code.djangoproject.com/ticket/34840#comment:14>
Django
partial indexes
-------------------------------------+-------------------------------------
Reporter: Alex Vandiver | Owner: murtatah
Type: Bug | Status: assigned
Component: Database layer | Version: 4.2
(models, ORM) |
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Florian Apolloner):
Regarding my comment
https://code.djangoproject.com/ticket/34840#comment:11 -- The validation
error is wrong there, it is actually a database area that is masked
(https://github.com/django/django/pull/15659#pullrequestreview-1629960210).
I wonder if we could teach the ORM to only add casts when using bind
params, there is no reason to add those for column refs. That is unless
our testsuite doesn't miss some further important edge cases :)
At the very least we will need instructions (release notes, maybe a blog
post) for people on how to check whether they are affected and how to fix
it.
--
Ticket URL: <https://code.djangoproject.com/ticket/34840#comment:15>
Django
partial indexes
-------------------------------------+-------------------------------------
| Felisiak
Type: Bug | Status: assigned
Component: Database layer | Version: 4.2
(models, ORM) |
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* owner: murtatah => Mariusz Felisiak
Comment:
I wonder if we could extend Simon's solution for #34754
(3434dbd39d373df7193ad006b970c09c1a909ea3). The following patch passes all
tests (with and without server side binding parameters):
{{{#!diff
diff --git a/django/db/backends/postgresql/operations.py
b/django/db/backends/postgresql/operations.py
index 93eb982f4b..e536681914 100644
--- a/django/db/backends/postgresql/operations.py
+++ b/django/db/backends/postgresql/operations.py
@@ -155,13 +155,6 @@ class DatabaseOperations(BaseDatabaseOperations):
def lookup_cast(self, lookup_type, internal_type=None):
lookup = "%s"
- if lookup_type == "isnull" and internal_type in (
- "CharField",
- "EmailField",
- "TextField",
- ):
- return "%s::text"
-
# Cast text lookups to text to allow things like
filter(x__contains=4)
if lookup_type in (
"iexact",
diff --git a/django/db/models/lookups.py b/django/db/models/lookups.py
index 896d3eee44..eb887f5548 100644
--- a/django/db/models/lookups.py
+++ b/django/db/models/lookups.py
@@ -624,11 +624,17 @@ class IsNull(BuiltinLookup):
raise ValueError(
"The QuerySet value for an isnull lookup must be True or
False."
)
- if isinstance(self.lhs, Value) and self.lhs.value is None:
+ if isinstance(self.lhs, Value):
if self.rhs:
- raise FullResultSet
+ if self.lhs.value is None:
+ raise FullResultSet
+ else:
+ raise EmptyResultSet
else:
- raise EmptyResultSet
+ if self.lhs.value is None:
+ raise EmptyResultSet
+ else:
+ raise FullResultSet
sql, params = self.process_lhs(compiler, connection)
if self.rhs:
return "%s IS NULL" % sql, params
diff --git a/django/db/models/query_utils.py
b/django/db/models/query_utils.py
index 4f3358eb8d..830a7fdb6f 100644
--- a/django/db/models/query_utils.py
+++ b/django/db/models/query_utils.py
@@ -10,7 +10,7 @@ import inspect
import logging
from collections import namedtuple
-from django.core.exceptions import FieldError
+from django.core.exceptions import EmptyResultSet, FieldError
from django.db import DEFAULT_DB_ALIAS, DatabaseError, connections
from django.db.models.constants import LOOKUP_SEP
from django.utils import tree
@@ -140,6 +140,8 @@ class Q(tree.Node):
except DatabaseError as e:
logger.warning("Got a database error calling check() on %r:
%s", self, e)
return True
+ except EmptyResultSet:
+ return False
def deconstruct(self):
path = "%s.%s" % (self.__class__.__module__,
self.__class__.__name__)
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/34840#comment:16>
Django
partial indexes
-------------------------------------+-------------------------------------
Reporter: Alex Vandiver | Owner: Mariusz
| Felisiak
Type: Bug | Status: assigned
Component: Database layer | Version: 4.2
(models, ORM) |
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Mariusz Felisiak):
Alex, does it work for you?
--
Ticket URL: <https://code.djangoproject.com/ticket/34840#comment:17>
Django
partial indexes
-------------------------------------+-------------------------------------
Reporter: Alex Vandiver | Owner: Mariusz
| Felisiak
Type: Bug | Status: assigned
Component: Database layer | Version: 4.2
(models, ORM) |
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Florian Apolloner):
That is some creative thinking Mariusz. So just so I get this right:
Django "optimizes" away part of the asked query if it can determine the
outcome of the clause/lookup/etc? If that is the case: wow, never realized
that :)
--
Ticket URL: <https://code.djangoproject.com/ticket/34840#comment:18>
Django
partial indexes
-------------------------------------+-------------------------------------
Reporter: Alex Vandiver | Owner: Mariusz
| Felisiak
Type: Bug | Status: assigned
Component: Database layer | Version: 4.2
(models, ORM) |
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Simon Charette):
Florian, yeah that's the gist of it.
This approach has existed for a while in the ORM to avoid performing a
query for `filter(somefield__in=[])` as SQL doesn't allow `"column" IN ()`
anyway and we've leaned on it recently to prevent some full table scan on
Postgres (#27397) and issues with `NULL` handling when dealing with
`JSONField` (#34754).
I think it makes sense to use this approach in cases where Python input
poses type ambiguity at the database level like it did with the `IN`
operator, after all we were forced to add `::text` cast because Postgres
doesn't know what the type of `%s IS NULL` should be.
In all cases the adjustments to `Q.check` to handle `EmptyResultSet`
should be added as other lookups could generate it, the same can be said
with `FullResultSet`.
I think we should not let the discussion about the silencing of
`DatabaseError` die off though and consider a deprecation path to raise
the awareness of the issue. Maybe a possible alternative solution that
doesn't warrant expression introspection (e.g. special casing `RawSQL`)
could be a `Constraint` kwarg that allows disabling model level validation
entirely? That could be a way to silence the warning during the
deprecation period.
--
Ticket URL: <https://code.djangoproject.com/ticket/34840#comment:19>
Django
partial indexes
-------------------------------------+-------------------------------------
Reporter: Alex Vandiver | Owner: Mariusz
| Felisiak
Type: Bug | Status: assigned
Component: Database layer | Version: 4.2
(models, ORM) |
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Florian Apolloner):
Replying to [comment:19 Simon Charette]:
> This approach has existed for a while in the ORM to avoid performing a
query for `filter(somefield__in=[])` as SQL doesn't allow `"column" IN ()`
anyway and we've leaned on it recently to prevent some full table scan on
Postgres (#27397) and issues with `NULL` handling when dealing with
`JSONField` (#34754).
>
> I think it makes sense to use this approach in cases where Python input
poses type ambiguity at the database level like it did with the `IN`
operator, after all we were forced to add `::text` cast because Postgres
doesn't know what the type of `%s IS NULL` should be.
Makes sense. And as we see we only need the `::text` cast in very special
situations, a `.filter(field__isnull=True)` won't require it…
> In all cases the adjustments to `Q.check` to handle `EmptyResultSet`
should be added as other lookups could generate it, the same can be said
about `FullResultSet`.
ACK, and we need tests for both cases. I see that the `WhereNode` properly
propagates `EmptyResultSet`/`FullResultSet` -- I wonder if there are other
cases where we don't do that yet (but I guess we will need to find them on
a case by case basis).
> I think we should not let the discussion about the silencing of
`DatabaseError` die off though and consider a deprecation path to raise
the awareness of the issue. Maybe a possible alternative solution that
doesn't warrant expression introspection (e.g. special casing `RawSQL`)
could be a `Constraint` kwarg that allows disabling model level validation
entirely? That could be a way to silence the warning during the
deprecation period.
+1. Though that should probably be a separate PR/ticket?
As for the indexing issues for people which already created indexes on 4.2
-- we should probably (aside from the release notes) also mention it
explicitly in a blog post? This is something that can be very hard to
find.
--
Ticket URL: <https://code.djangoproject.com/ticket/34840#comment:20>
Django
partial indexes
-------------------------------------+-------------------------------------
Reporter: Alex Vandiver | Owner: Mariusz
| Felisiak
Type: Bug | Status: assigned
Component: Database layer | Version: 4.2
(models, ORM) |
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Alex Vandiver):
Replying to [comment:17 Mariusz Felisiak]:
> Alex, does it work for you?
My parse of it is that it'll drop the `::text` cast in the query, which
means that the partial index, as created pre-4.2, work as expected -- so
yup!
(For the meantime, we've manually recreated the indexes with the cast in
them in production, so we get their utility -- but we'll happily swap
those back to the cast-less variety when we upgrade to a Django with this
fix)
--
Ticket URL: <https://code.djangoproject.com/ticket/34840#comment:21>
Django
partial indexes
-------------------------------------+-------------------------------------
Reporter: Alex Vandiver | Owner: Mariusz
| Felisiak
Type: Bug | Status: assigned
Component: Database layer | Version: 4.2
(models, ORM) |
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* has_patch: 0 => 1
Comment:
[https://github.com/django/django/pull/17295 PR]
--
Ticket URL: <https://code.djangoproject.com/ticket/34840#comment:22>
Django
partial indexes
-------------------------------------+-------------------------------------
Reporter: Alex Vandiver | Owner: Mariusz
| Felisiak
Component: Database layer | Version: 4.2
(models, ORM) |
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"779cd28acb1f7eb06f629c0ea4ded99b5ebb670a" 779cd28]:
{{{
#!CommitTicketReference repository=""
revision="779cd28acb1f7eb06f629c0ea4ded99b5ebb670a"
Fixed #34840 -- Avoided casting string base fields on PostgreSQL.
Thanks Alex Vandiver for the report.
Regression in 09ffc5c1212d4ced58b708cbbf3dfbfb77b782ca.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/34840#comment:23>
Django
partial indexes
-------------------------------------+-------------------------------------
Reporter: Alex Vandiver | Owner: Mariusz
| Felisiak
Type: Bug | Status: closed
Component: Database layer | Version: 4.2
(models, ORM) |
Severity: Release blocker | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Mariusz Felisiak <felisiak.mariusz@…>):
In [changeset:"2566f92544d587de30c4b50b292c59128554e76c" 2566f92]:
{{{
#!CommitTicketReference repository=""
revision="2566f92544d587de30c4b50b292c59128554e76c"
[5.0.x] Fixed #34840 -- Avoided casting string base fields on PostgreSQL.
Thanks Alex Vandiver for the report.
Regression in 09ffc5c1212d4ced58b708cbbf3dfbfb77b782ca.
Backport of 779cd28acb1f7eb06f629c0ea4ded99b5ebb670a from main.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/34840#comment:24>
Django
partial indexes
-------------------------------------+-------------------------------------
Reporter: Alex Vandiver | Owner: Mariusz
| Felisiak
Type: Bug | Status: closed
Component: Database layer | Version: 4.2
(models, ORM) |
Severity: Release blocker | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Mariusz Felisiak <felisiak.mariusz@…>):
In [changeset:"a148461f1fa7aceb2ea6c9dc203b67a170884445" a148461f]:
{{{
#!CommitTicketReference repository=""
revision="a148461f1fa7aceb2ea6c9dc203b67a170884445"
[4.2.x] Fixed #34840 -- Avoided casting string base fields on PostgreSQL.
Thanks Alex Vandiver for the report.
Regression in 09ffc5c1212d4ced58b708cbbf3dfbfb77b782ca.
Backport of 779cd28acb1f7eb06f629c0ea4ded99b5ebb670a from main.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/34840#comment:25>
Django
partial indexes
-------------------------------------+-------------------------------------
Reporter: Alex Vandiver | Owner: Mariusz
| Felisiak
Type: Bug | Status: closed
Component: Database layer | Version: 4.2
(models, ORM) |
Severity: Release blocker | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by GitHub <noreply@…>):
In [changeset:"9fd3a0ffc8fb8e532b0d5717f99fb15aff175a6c" 9fd3a0f]:
{{{
#!CommitTicketReference repository=""
revision="9fd3a0ffc8fb8e532b0d5717f99fb15aff175a6c"
Refs #34840 -- Fixed test_validate_nullable_textfield_with_isnull_true()
on databases that don's support table check constraints.
Thanks Tim Graham for the report.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/34840#comment:26>
Django
partial indexes
-------------------------------------+-------------------------------------
Reporter: Alex Vandiver | Owner: Mariusz
| Felisiak
Type: Bug | Status: closed
Component: Database layer | Version: 4.2
(models, ORM) |
Severity: Release blocker | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Mariusz Felisiak <felisiak.mariusz@…>):
In [changeset:"c3cf518a804be9bf56883bf2c93c1d0229b18b8b" c3cf518]:
{{{
#!CommitTicketReference repository=""
revision="c3cf518a804be9bf56883bf2c93c1d0229b18b8b"
[5.0.x] Refs #34840 -- Fixed
test_validate_nullable_textfield_with_isnull_true() on databases that
don's support table check constraints.
Thanks Tim Graham for the report.
Backport of 9fd3a0ffc8fb8e532b0d5717f99fb15aff175a6c from main
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/34840#comment:27>
Django
partial indexes
-------------------------------------+-------------------------------------
Reporter: Alex Vandiver | Owner: Mariusz
| Felisiak
Type: Bug | Status: closed
Component: Database layer | Version: 4.2
(models, ORM) |
Severity: Release blocker | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Mariusz Felisiak <felisiak.mariusz@…>):
In [changeset:"b6bb2f809958ec32f6cb8a4f315ffcf490ee7a71" b6bb2f80]:
{{{
#!CommitTicketReference repository=""
revision="b6bb2f809958ec32f6cb8a4f315ffcf490ee7a71"
[4.2.x] Refs #34840 -- Fixed
test_validate_nullable_textfield_with_isnull_true() on databases that
don's support table check constraints.
Thanks Tim Graham for the report.
Backport of 9fd3a0ffc8fb8e532b0d5717f99fb15aff175a6c from main
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/34840#comment:28>
Django
partial indexes
-------------------------------------+-------------------------------------
Reporter: Alex Vandiver | Owner: Mariusz
| Felisiak
Type: Bug | Status: closed
Component: Database layer | Version: 4.2
(models, ORM) |
Severity: Release blocker | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by GitHub <noreply@…>):
In [changeset:"73b2c63127297e214d3bfdc8d9a96837691fc2a0" 73b2c63]:
{{{
#!CommitTicketReference repository=""
revision="73b2c63127297e214d3bfdc8d9a96837691fc2a0"
Refs #34840 -- Improved release note describing index regression.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/34840#comment:29>
Django
partial indexes
-------------------------------------+-------------------------------------
Reporter: Alex Vandiver | Owner: Mariusz
| Felisiak
Type: Bug | Status: closed
Component: Database layer | Version: 4.2
(models, ORM) |
Severity: Release blocker | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Mariusz Felisiak <felisiak.mariusz@…>):
In [changeset:"12a19321b4b8abec05e27af8c5931d2ff836bbec" 12a1932]:
{{{
#!CommitTicketReference repository=""
revision="12a19321b4b8abec05e27af8c5931d2ff836bbec"
[5.0.x] Refs #34840 -- Improved release note describing index regression.
Backport of 73b2c63127297e214d3bfdc8d9a96837691fc2a0 from main
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/34840#comment:30>
Django
partial indexes
-------------------------------------+-------------------------------------
Reporter: Alex Vandiver | Owner: Mariusz
| Felisiak
Type: Bug | Status: closed
Component: Database layer | Version: 4.2
(models, ORM) |
Severity: Release blocker | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Mariusz Felisiak <felisiak.mariusz@…>):
In [changeset:"caec4f4a6fc0f0ba60e9d4e4e7f3bfb6f6d492a7" caec4f4a]:
{{{
#!CommitTicketReference repository=""
revision="caec4f4a6fc0f0ba60e9d4e4e7f3bfb6f6d492a7"
[4.2.x] Refs #34840 -- Improved release note describing index regression.
Backport of 73b2c63127297e214d3bfdc8d9a96837691fc2a0 from main
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/34840#comment:31>