[Django] #21160: in_bulk() fails on SQLite when passing more than 999 ids
Django
----------------------------------------------+----------------------------
Reporter: NiGhTTraX | Owner: nobody
Type: Bug | Status: new
Component: Database layer (models, ORM) | Version: master
Severity: Release blocker | Keywords: in_bulk sqlite
Triage Stage: Unreviewed | 999 1000
Easy pickings: 0 | Has patch: 1
| UI/UX: 0
----------------------------------------------+----------------------------
As we all know, SQLite doesn't allow more than 999 variables in a query,
failing with the 'too many SQL variables' error if you pass in more. This
is a problem when using the in_bulk() method.
I've looked through the code and found out that all backends define a
bulk_batch_size ops feature and it is set to 999 in the SQLite backend.
https://github.com/django/django/blob/master/django/db/backends/sqlite3/base.py#L140
This ops feature is only used in _batched_insert. I've written a patch
that makes use of it in the in_bulk method as well. You can check it here
[link pending]
I've also written two tests that test passing in a large number of ids and
also the efficiency in this case. The efficiency test runs only on
databases that don't support 1000 variables in a query.
Any comments are welcome.
--
Ticket URL: <https://code.djangoproject.com/ticket/21160>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: NiGhTTraX | Owner: nobody
Type: Bug | Status: new
(models, ORM) | Resolution:
Severity: Release blocker | Triage Stage:
Keywords: in_bulk sqlite 999 | Unreviewed
1000 | Needs documentation: 0
Has patch: 1 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Changes (by NiGhTTraX):
* needs_better_patch: => 0
* needs_tests: => 0
* needs_docs: => 0
Comment:
Pull request: https://github.com/django/django/pull/1679
For the love of me I can't find the way to edit the original description.
--
Ticket URL: <https://code.djangoproject.com/ticket/21160#comment:1>
Django
Reporter: NiGhTTraX | Owner: nobody
Type: Bug | Status: new
(models, ORM) | Resolution:
Keywords: in_bulk sqlite 999 | Unreviewed
1000 | Needs documentation: 0
Has patch: 1 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
* severity: Release blocker => Normal
--
Ticket URL: <https://code.djangoproject.com/ticket/21160#comment:2>
Django
Reporter: NiGhTTraX | Owner: nobody
Type: Bug | Status: new
(models, ORM) | Resolution:
Severity: Normal | Triage Stage:
Keywords: in_bulk sqlite 999 | Unreviewed
1000 | Needs documentation: 0
Has patch: 1 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Old description:
> As we all know, SQLite doesn't allow more than 999 variables in a query,
> failing with the 'too many SQL variables' error if you pass in more. This
> is a problem when using the in_bulk() method.
>
> I've looked through the code and found out that all backends define a
> bulk_batch_size ops feature and it is set to 999 in the SQLite backend.
> https://github.com/django/django/blob/master/django/db/backends/sqlite3/base.py#L140
>
> This ops feature is only used in _batched_insert. I've written a patch
> that makes use of it in the in_bulk method as well. You can check it here
> [link pending]
>
> I've also written two tests that test passing in a large number of ids
> and also the efficiency in this case. The efficiency test runs only on
> databases that don't support 1000 variables in a query.
>
> Any comments are welcome.
New description:
As we all know, SQLite doesn't allow more than 999 variables in a query,
failing with the 'too many SQL variables' error if you pass in more. This
is a problem when using the in_bulk() method.
I've looked through the code and found out that all backends define a
bulk_batch_size ops feature and it is set to 999 in the SQLite backend.
https://github.com/django/django/blob/master/django/db/backends/sqlite3/base.py#L140
This ops feature is only used in _batched_insert. I've written a patch
that makes use of it in the in_bulk method as well. You can check it here:
https://github.com/django/django/pull/1679
I've also written two tests that test passing in a large number of ids and
also the efficiency in this case. The efficiency test runs only on
databases that don't support 1000 variables in a query.
Any comments are welcome.
--
--
Ticket URL: <https://code.djangoproject.com/ticket/21160#comment:3>
Django
Reporter: NiGhTTraX | Owner: nobody
Type: Bug | Status: new
(models, ORM) | Resolution:
Keywords: in_bulk sqlite 999 | Needs documentation: 0
1000 | Patch needs improvement: 0
Has patch: 1 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Changes (by aaugustin):
* stage: Unreviewed => Accepted
Comment:
At first sight, the patch looks reasonable.
--
Ticket URL: <https://code.djangoproject.com/ticket/21160#comment:4>
Django
Reporter: NiGhTTraX | Owner: nobody
Type: Bug | Status: new
(models, ORM) | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: in_bulk sqlite 999 | Needs documentation: 0
Has patch: 1 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by NiGhTTraX):
Any other thoughts on this?
--
Ticket URL: <https://code.djangoproject.com/ticket/21160#comment:5>
Django
Reporter: NiGhTTraX | Owner: nobody
Type: Bug | Status: new
(models, ORM) | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: in_bulk sqlite 999 | Needs documentation: 0
Has patch: 1 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
* needs_better_patch: 0 => 1
Comment:
Updated patch can be found from
https://github.com/django/django/pull/3046.
The usage of bulk_batch_size() is incorrect in the patch. The method is
documented to receive a list of objects, but for the use case in this
ticket it receives an iterable of field values. We can either change the
method definition so that it can receive either an iterable of field
values, or a list of objects. Or we can just add a new feature
"max_query_params" and use that instead.
I slightly prefer the approach of adding max_query_params in to database
features. Then we can make supports_1000_query_params to be a property
which is calculated based on max_query_params, and also make the base
bulk_batch_size() to use the max_query_params feature.
--
Ticket URL: <https://code.djangoproject.com/ticket/21160#comment:6>
Django
Reporter: Andrei Picus | Owner: felixxm
Type: Bug | Status: assigned
Component: Database layer | Version: master
(models, ORM) |
Keywords: in_bulk sqlite 999 | Triage Stage: Accepted
1000 |
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by felixxm):
* owner: nobody => felixxm
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/21160#comment:7>
Django
Reporter: Andrei Picus | Owner: felixxm
Type: Bug | Status: assigned
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: in_bulk sqlite 999 | Triage Stage: Accepted
1000 |
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by GitHub <noreply@…>):
In [changeset:"f42c7cc87baa83000624fadc1e434c44efc1fba8" f42c7cc]:
{{{
#!CommitTicketReference repository=""
revision="f42c7cc87baa83000624fadc1e434c44efc1fba8"
Refs #21160 -- Replaced DatabaseFeatures.supports_1000_query_parameters by
a DatabaseFeatures.max_query_params.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/21160#comment:8>
Django
Reporter: Andrei Picus | Owner: felixxm
Type: Bug | Status: assigned
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: in_bulk sqlite 999 | Triage Stage: Accepted
1000 |
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* needs_better_patch: 1 => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/21160#comment:9>
Django
Reporter: Andrei Picus | Owner: felixxm
Type: Bug | Status: assigned
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
1000 | checkin
Has patch: 1 | 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/21160#comment:10>
Django
Reporter: Andrei Picus | Owner: felixxm
Component: Database layer | Version: master
(models, ORM) |
Keywords: in_bulk sqlite 999 | Triage Stage: Ready for
1000 | checkin
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:"1b6f05e91f5d1f7aaf937522ead62836ab50fed8" 1b6f05e9]:
{{{
#!CommitTicketReference repository=""
revision="1b6f05e91f5d1f7aaf937522ead62836ab50fed8"
Fixed #21160 -- Fixed QuerySet.in_bulk() crash on SQLite when requesting
more than 999 ids.
Thanks Andrei Picus and Anssi Kääriäinen for the initial patch
and Tim Graham for the review.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/21160#comment:11>