[Django] #36416: id_list argument to in_bulk() does not account for composite primary keys when batching

[Django]

#36416: id_list argument to in_bulk() does not account for composite primary keys
when batching
-------------------------------------+-------------------------------------
Reporter: Jacob | Owner: Jacob Walls
Walls |
Type: Bug | Status: assigned
Component: Database | Version: 5.2
layer (models, ORM) |
Severity: Release | Keywords:
blocker |
Triage Stage: | Has patch: 0
Unreviewed |
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-------------------------------------+-------------------------------------
The untested `id_list` argument to `in_bulk()` divides large lists into
batches, but similar to #36118, it did not account for composite primary
keys, leading to errors like this on SQLite:

{{{
django.db.utils.OperationalError: too many SQL variables
}}}

#36118 mentioned that other uses like this remained to be audited.

Failing test (I may adjust this in the PR to run faster by mocking a lower
query param limit, but this shows the OperationalError):
{{{#!diff
diff --git a/tests/composite_pk/tests.py b/tests/composite_pk/tests.py
index 91cbee0635..ba290a796f 100644
--- a/tests/composite_pk/tests.py
+++ b/tests/composite_pk/tests.py
@@ -147,6 +147,18 @@ class CompositePKTests(TestCase):
result = Comment.objects.in_bulk([self.comment.pk])
self.assertEqual(result, {self.comment.pk: self.comment})

+ def test_in_bulk_batching(self):
+ num_requiring_batching = (connection.features.max_query_params //
2) + 1
+ comments = [
+ Comment(id=i, tenant=self.tenant)
+ for i in range(2, num_requiring_batching + 1)
+ ]
+ Comment.objects.bulk_create(comments)
+ id_list = list(Comment.objects.values_list("pk", flat=True))
+ with self.assertNumQueries(2):
+ comment_dict = Comment.objects.in_bulk(id_list=id_list)
+ self.assertQuerySetEqual(comment_dict, id_list)
+
def test_iterator(self):
"""
Test the .iterator() method of composite_pk models.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/36416>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

[Django]

#36416: id_list argument to in_bulk() does not account for composite primary keys
when batching
-------------------------------------+-------------------------------------

Reporter: Jacob Walls | Owner: Jacob

| Walls
Type: Bug | Status: assigned

Component: Database layer | Version: 5.2
(models, ORM) |
Severity: Release blocker | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Jacob Walls):

* has_patch: 0 => 1

Comment:

[https://github.com/django/django/pull/19502 PR]
--
Ticket URL: <https://code.djangoproject.com/ticket/36416#comment:1>

[Django]

#36416: id_list argument to in_bulk() does not account for composite primary keys
when batching
-------------------------------------+-------------------------------------
Reporter: Jacob Walls | Owner: Jacob
| Walls
Type: Bug | Status: assigned
Component: Database layer | Version: 5.2
(models, ORM) |
Severity: Release blocker | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Description changed by Jacob Walls:

Old description:

New description:

The `id_list` argument to `in_bulk()` divides large lists into batches,

but similar to #36118, it did not account for composite primary keys,
leading to errors like this on SQLite:

{{{
django.db.utils.OperationalError: too many SQL variables
}}}

#36118 mentioned that other uses like this remained to be audited.

The id_list argument is tested, but the batching was
[https://djangoci.com/view/%C2%ADCoverage/job/django-
coverage/HTML_20Coverage_20Report/z_d81526da7cfdb7e4_query_py.html#t1155
not covered].

--
Ticket URL: <https://code.djangoproject.com/ticket/36416#comment:2>

[Django]

#36416: id_list argument to in_bulk() does not account for composite primary keys
when batching
-------------------------------------+-------------------------------------
Reporter: Jacob Walls | Owner: Jacob
| Walls
Type: Bug | Status: assigned
Component: Database layer | Version: 5.2
(models, ORM) |
Severity: Release blocker | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Natalia Bidart):

* cc: Simon Charette (added)

Comment:

Hello Jacob, thank you for the report and for providing a test! I'm having
a bit of an issue when running the test, it seems to hang and never
finishes. I've waited over 15 minutes. I have printed the values used in
the tests and I have:
{{{
connection.features.max_query_params=250000
num_requiring_batching=125001
}}}
while high, these do not seem impossible. Specifically, the line that
hangs is:
{{{
Comment.objects.in_bulk(id_list=id_list)
}}}
I would like to understand more about what/why this is happening before
accepting. Is this failing for you on `main` without your proposed fix? Or
is it hanging as well?
--
Ticket URL: <https://code.djangoproject.com/ticket/36416#comment:3>

[Django]

#36416: id_list argument to in_bulk() does not account for composite primary keys
when batching
-------------------------------------+-------------------------------------
Reporter: Jacob Walls | Owner: Jacob
| Walls
Type: Bug | Status: assigned
Component: Database layer | Version: 5.2
(models, ORM) |
Severity: Release blocker | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Jacob Walls):

I left a
[https://github.com/django/django/pull/19502#discussion_r2105896691 note]
here that I believe the slowness is simply due to the unfixed #36380 (and
I added mocking in order to avoid creating such a large batch either way).
The draft test finishes for me when I apply the fix for #36380 -- sorry I
didn't pull these pieces together into the ticket description.
--
Ticket URL: <https://code.djangoproject.com/ticket/36416#comment:4>

[Django]

#36416: id_list argument to in_bulk() does not account for composite primary keys
when batching
-------------------------------------+-------------------------------------
Reporter: Jacob Walls | Owner: Jacob
| Walls
Type: Bug | Status: assigned
Component: Database layer | Version: 5.2
(models, ORM) |
Severity: Release blocker | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Jacob Walls):

(And yes, the draft test provided with the ticket is hanging on `main` for
me: when CTRL-C'ing out of the hang, you can see from the trace that the
issue is #36380.)

The draft test on the ticket fails on 5.2.1 if you make adjust it to run:
- provide `user=self.user` in the `Comment(...` instantiation
- on SQLite: adjust `num_requiring_batching` to circa 999 to see
`OperationalError` because below Django 6.0 `max_query_params` is too
protective (#36143)
--
Ticket URL: <https://code.djangoproject.com/ticket/36416#comment:5>

[Django]

#36416: id_list argument to in_bulk() does not account for composite primary keys
when batching
-------------------------------------+-------------------------------------
Reporter: Jacob Walls | Owner: Jacob
| Walls
Type: Bug | Status: assigned
Component: Database layer | Version: 5.2
(models, ORM) |
Severity: Release blocker | Resolution:

Keywords: | Triage Stage: Accepted

Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Natalia Bidart):

* cc: Csirmaz Bendegúz (added)
* stage: Unreviewed => Accepted

Comment:

Replying to [comment:4 Jacob Walls]:

> I left a
[https://github.com/django/django/pull/19502#discussion_r2105896691 note]
here that I believe the slowness is simply due to the unfixed #36380 (and
I added mocking in order to avoid creating such a large batch either way).
The draft test finishes for me when I apply the fix for #36380 -- sorry I
didn't pull these pieces together into the ticket description.

Thank you, I missed this. When running the given test with the fixes for
#36380, I also get the `django.db.utils.OperationalError: too many SQL
variables`.
--
Ticket URL: <https://code.djangoproject.com/ticket/36416#comment:6>

[Django]

#36416: id_list argument to in_bulk() does not account for composite primary keys
when batching
-------------------------------------+-------------------------------------
Reporter: Jacob Walls | Owner: Jacob
| Walls
Type: Bug | Status: assigned
Component: Database layer | Version: 5.2
(models, ORM) |
Severity: Release blocker | Resolution:

Keywords: | Triage Stage: Ready for
| checkin

Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Sarah Boyce):

* stage: Accepted => Ready for checkin

--
Ticket URL: <https://code.djangoproject.com/ticket/36416#comment:7>

[Django]

#36416: id_list argument to in_bulk() does not account for composite primary keys
when batching
-------------------------------------+-------------------------------------
Reporter: Jacob Walls | Owner: Jacob
| Walls

Type: Bug | Status: closed

Component: Database layer | Version: 5.2
(models, ORM) |

Severity: Release blocker | Resolution: fixed

Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Sarah Boyce <42296566+sarahboyce@…>):

* resolution: => fixed
* status: assigned => closed

Comment:

In [changeset:"26313bc21932d0d3af278ab387549d63b1f64575" 26313bc]:
{{{#!CommitTicketReference repository=""
revision="26313bc21932d0d3af278ab387549d63b1f64575"
Fixed #36416 -- Made QuerySet.in_bulk() account for composite pks in
id_list.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/36416#comment:8>

[Django]

#36416: id_list argument to in_bulk() does not account for composite primary keys
when batching
-------------------------------------+-------------------------------------
Reporter: Jacob Walls | Owner: Jacob
| Walls
Type: Bug | Status: closed
Component: Database layer | Version: 5.2
(models, ORM) |
Severity: Release blocker | Resolution: fixed
Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Sarah Boyce <42296566+sarahboyce@…>):

In [changeset:"2bf4c5b9eaaf0a36cb0fb6c060625a5fb2fcf6c9" 2bf4c5b9]:
{{{#!CommitTicketReference repository=""
revision="2bf4c5b9eaaf0a36cb0fb6c060625a5fb2fcf6c9"
[5.2.x] Fixed #36416 -- Made QuerySet.in_bulk() account for composite pks
in id_list.

Backport of 26313bc21932d0d3af278ab387549d63b1f64575 from main.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/36416#comment:9>

[Django]

#36416: id_list argument to in_bulk() does not account for composite primary keys
when batching
-------------------------------------+-------------------------------------
Reporter: Jacob Walls | Owner: Jacob
| Walls
Type: Bug | Status: closed
Component: Database layer | Version: 5.2
(models, ORM) |
Severity: Release blocker | Resolution: fixed
Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by nessita <124304+nessita@…>):

In [changeset:"a68e8565cdd4fc3f8b738fc516095dab142b9d65" a68e8565]:
{{{#!CommitTicketReference repository=""
revision="a68e8565cdd4fc3f8b738fc516095dab142b9d65"
Refs #34378, #36143, #36416 -- Fixed isolation of
LookupTests.test_in_bulk_preserve_ordering_with_batch_size().

`max_query_params` is a property, so it must be patched on the class.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/36416#comment:10>

[Django]

#36416: id_list argument to in_bulk() does not account for composite primary keys
when batching
-------------------------------------+-------------------------------------
Reporter: Jacob Walls | Owner: Jacob
| Walls
Type: Bug | Status: closed
Component: Database layer | Version: 5.2
(models, ORM) |
Severity: Release blocker | Resolution: fixed
Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Natalia <124304+nessita@…>):

In [changeset:"a4e27c0c6bcc37ad7d7f6098e5d44edbbcca2c42" a4e27c0]:
{{{#!CommitTicketReference repository=""
revision="a4e27c0c6bcc37ad7d7f6098e5d44edbbcca2c42"
[5.2.x] Refs #34378, #36143, #36416 -- Fixed isolation of

LookupTests.test_in_bulk_preserve_ordering_with_batch_size().

`max_query_params` is a property, so it must be patched on the class.

Backport of a68e8565cdd4fc3f8b738fc516095dab142b9d65 from main.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/36416#comment:11>

[Django]

#36416: id_list argument to in_bulk() does not account for composite primary keys
when batching
-------------------------------------+-------------------------------------
Reporter: Jacob Walls | Owner: Jacob
| Walls
Type: Bug | Status: closed
Component: Database layer | Version: 5.2
(models, ORM) |
Severity: Release blocker | Resolution: fixed
Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Sarah Boyce <42296566+sarahboyce@…>):

In [changeset:"d3cf24e9b415b41f570c9f426b2cd113b5fdb4de" d3cf24e9]:
{{{#!CommitTicketReference repository=""
revision="d3cf24e9b415b41f570c9f426b2cd113b5fdb4de"
Refs #36430, #36416, #34378 -- Simplified batch size calculation in
QuerySet.in_bulk().
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/36416#comment:12>