[Django] #36430: bulk_batch_size() special-cases single field on SQLite according to outdated limit

[Django]

#36430: bulk_batch_size() special-cases single field on SQLite according to
outdated limit
-------------------------------------+-------------------------------------
Reporter: Jacob Walls | Type:
| Cleanup/optimization
Status: new | Component: Database
| layer (models, ORM)
Version: dev | Severity: Normal
Keywords: | Triage Stage:
SQLITE_MAX_COMPOUND_SELECT, | Unreviewed
bulk_create |
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
On SQLite, `bulk_batch_size()`
[https://github.com/django/django/blob/1a744343999c9646912cee76ba0a2fa6ef5e6240/django/db/backends/sqlite3/operations.py#L49
special-cases] a field list of length 1 and applies the
`SQLITE_MAX_COMPOUND_SELECT` limit to arrive at a value of 500.

I think this must date from before bulk inserts used `VALUES` syntax,
which became available in SQLite in 2012, see
[https://github.com/laravel/framework/issues/25262#issuecomment-414836191
discussion in Laravel].

When the list of fields exceeds 1, we go through the `elif` branch and
arrive at much higher limits. I'm pretty sure this shows that the limit of
500 for `fields=["pk"]` is overly protective and can just be removed.

I don't have a unit test to provide, but you can play with changing the
limit from 500 to 501 and see that `test_large_delete` still passes (no
trouble selecting 501 objects).

(I found this while trying to
[https://github.com/django/django/pull/19502#discussion_r2118612666
refactor] a different call site away from `max_query_params` in the hopes
of just calling `bulk_batch_size()` until I saw how overly protective it
was.)

I think this would be a good idea to resolve before anyone invests effort
in [https://github.com/django/django/pull/19427/files#r2062643293 reading
the dynamic limit] for this potentially irrelevant param.
--
Ticket URL: <https://code.djangoproject.com/ticket/36430>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

[Django]

#36430: bulk_batch_size() special-cases single field on SQLite according to
outdated limit
-------------------------------------+-------------------------------------

Reporter: Jacob Walls | Owner: (none)
Type: | Status: new
Cleanup/optimization |
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:

Keywords: | Triage Stage:
SQLITE_MAX_COMPOUND_SELECT, | Unreviewed
bulk_create |
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Old description:

> On SQLite, `bulk_batch_size()`
> [https://github.com/django/django/blob/1a744343999c9646912cee76ba0a2fa6ef5e6240/django/db/backends/sqlite3/operations.py#L49
> special-cases] a field list of length 1 and applies the
> `SQLITE_MAX_COMPOUND_SELECT` limit to arrive at a value of 500.
>
> I think this must date from before bulk inserts used `VALUES` syntax,
> which became available in SQLite in 2012, see
> [https://github.com/laravel/framework/issues/25262#issuecomment-414836191
> discussion in Laravel].
>
> When the list of fields exceeds 1, we go through the `elif` branch and
> arrive at much higher limits. I'm pretty sure this shows that the limit
> of 500 for `fields=["pk"]` is overly protective and can just be removed.
>
> I don't have a unit test to provide, but you can play with changing the
> limit from 500 to 501 and see that `test_large_delete` still passes (no
> trouble selecting 501 objects).
>
> (I found this while trying to
> [https://github.com/django/django/pull/19502#discussion_r2118612666
> refactor] a different call site away from `max_query_params` in the hopes
> of just calling `bulk_batch_size()` until I saw how overly protective it
> was.)
>
> I think this would be a good idea to resolve before anyone invests effort
> in [https://github.com/django/django/pull/19427/files#r2062643293 reading
> the dynamic limit] for this potentially irrelevant param.

New description:

On SQLite, `bulk_batch_size()`
[https://github.com/django/django/blob/1a744343999c9646912cee76ba0a2fa6ef5e6240/django/db/backends/sqlite3/operations.py#L49
special-cases] a field list of length 1 and applies the
`SQLITE_MAX_COMPOUND_SELECT` limit to arrive at a value of 500.

I think this must date from before bulk inserts used `VALUES` syntax,
which became available in SQLite in 2012, see
[https://github.com/laravel/framework/issues/25262#issuecomment-414836191
discussion in Laravel].

When the list of fields exceeds 1, we go through the `elif` branch and
arrive at much higher limits. I'm pretty sure this shows that the limit of
500 for `fields=["pk"]` is overly protective and can just be removed.

I don't have a unit test to provide, but you can play with changing the
limit from 500 to 501 and see that `test_large_delete` still passes (no

trouble selecting 501 objects). (You can also adjust
`test_max_batch_size()` to provide a s

(I found this while trying to
[https://github.com/django/django/pull/19502#discussion_r2118612666
refactor] a different call site away from `max_query_params` in the hopes
of just calling `bulk_batch_size()` until I saw how overly protective it
was.)

I think this would be a good idea to resolve before anyone invests effort
in [https://github.com/django/django/pull/19427/files#r2062643293 reading
the dynamic limit] for this potentially irrelevant param.

--

Comment (by Jacob Walls):

Here's a rough test after all to demo, just adjust `test_max_batch_size`
like this:


{{{#!diff
diff --git a/tests/bulk_create/tests.py b/tests/bulk_create/tests.py
index d590a292de..fd30ca48f7 100644
--- a/tests/bulk_create/tests.py
+++ b/tests/bulk_create/tests.py
@@ -296,7 +296,7 @@ class BulkCreateTests(TestCase):
@skipUnlessDBFeature("has_bulk_insert")
def test_max_batch_size(self):
objs = [Country(name=f"Country {i}") for i in range(1000)]
- fields = ["name", "iso_two_letter", "description"]
+ fields = ["pk"]
max_batch_size = connection.ops.bulk_batch_size(fields, objs)
with self.assertNumQueries(ceil(len(objs) / max_batch_size)):
Country.objects.bulk_create(objs)
}}}

----
The test fails as it was *more* efficient than expected:
{{{#!py
======================================================================
FAIL: test_max_batch_size
(bulk_create.tests.BulkCreateTests.test_max_batch_size)
----------------------------------------------------------------------
Traceback (most recent call last):
File "/Users/.../django/tests/bulk_create/tests.py", line 301, in
test_max_batch_size
with self.assertNumQueries(ceil(len(objs) / max_batch_size)):
~~~~~~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
AssertionError: 1 != 2 : 1 queries executed, 2 expected
Captured queries were:
1. INSERT INTO "bulk_create_country" ("name", "iso_two_letter",
"description") VALUES ('Country 0', '', ''), ('Country 1', '', ''),
('Country 2', '', ''), ('Country 3', '', ...

----------------------------------------------------------------------
Ran 2 tests in 0.025s

FAILED (failures=1)
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/36430#comment:1>

[Django]

#36430: bulk_batch_size() special-cases single field on SQLite according to
outdated limit
-------------------------------------+-------------------------------------
Reporter: Jacob Walls | Owner: (none)
Type: | Status: new
Cleanup/optimization |
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage:
SQLITE_MAX_COMPOUND_SELECT, | Unreviewed
bulk_create |
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Description changed by Jacob Walls:

Old description:

> On SQLite, `bulk_batch_size()`
> [https://github.com/django/django/blob/1a744343999c9646912cee76ba0a2fa6ef5e6240/django/db/backends/sqlite3/operations.py#L49
> special-cases] a field list of length 1 and applies the
> `SQLITE_MAX_COMPOUND_SELECT` limit to arrive at a value of 500.
>
> I think this must date from before bulk inserts used `VALUES` syntax,
> which became available in SQLite in 2012, see
> [https://github.com/laravel/framework/issues/25262#issuecomment-414836191
> discussion in Laravel].
>
> When the list of fields exceeds 1, we go through the `elif` branch and
> arrive at much higher limits. I'm pretty sure this shows that the limit
> of 500 for `fields=["pk"]` is overly protective and can just be removed.
>
> I don't have a unit test to provide, but you can play with changing the
> limit from 500 to 501 and see that `test_large_delete` still passes (no

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

[Django]

#36430: bulk_batch_size() special-cases single field on SQLite according to
outdated limit
-------------------------------------+-------------------------------------
Reporter: Jacob Walls | Owner: (none)
Type: | Status: new
Cleanup/optimization |
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:

Keywords: | Triage Stage: Accepted
SQLITE_MAX_COMPOUND_SELECT, |

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

Changes (by Sarah Boyce):

* cc: Sage Abdullah (added)
* stage: Unreviewed => Accepted

--
Ticket URL: <https://code.djangoproject.com/ticket/36430#comment:3>

[Django]

#36430: bulk_batch_size() special-cases single field on SQLite according to
outdated limit
-------------------------------------+-------------------------------------

Reporter: Jacob Walls | Owner: Jacob
Type: | Walls
Cleanup/optimization | Status: assigned

Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
SQLITE_MAX_COMPOUND_SELECT, |
bulk_create |
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Jacob Walls):

* owner: (none) => Jacob Walls
* status: new => assigned

Comment:

I can take a swing at this given that we'll want to handle this
[https://github.com/django/django/pull/19502#discussion_r2123992360
mentioned cleanup] at the same time.
--
Ticket URL: <https://code.djangoproject.com/ticket/36430#comment:4>

[Django]

#36430: bulk_batch_size() special-cases single field on SQLite according to
outdated limit
-------------------------------------+-------------------------------------
Reporter: Jacob Walls | Owner: Jacob
Type: | Walls
Cleanup/optimization | Status: assigned
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
SQLITE_MAX_COMPOUND_SELECT, |
bulk_create |
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Sage Abdullah):

Thanks Jacob for reporting this and thanks Sarah for cc-ing me to the
ticket.

As mentioned,
[https://github.com/django/django/pull/19427/files#r2062643293 I did
identify this in my previous PR]. However, I wasn't sure why Django even
uses `SQLITE_MAX_COMPOUND_SELECT ` for a bulk query. I tried to look into
where it might've come from but I didn't find any good leads.

> I think this must date from before bulk inserts used `VALUES` syntax,
which became available in SQLite in 2012, see
[https://github.com/laravel/framework/issues/25262#issuecomment-414836191
discussion in Laravel].

Aaand, I think you're correct! I didn't know `VALUES` wasn't supported
back then. My findings:

- The `500` limit a.k.a. `SQLITE_MAX_COMPOUND_SELECT` was added in
0a0a0d66b316598f7c296e8bf75749a14ce3ac49. As noted in the tests in that
commit, Django used `UNION`ed selects to do the bulk insert for some
reason.
- Related: a27582484cf814554907d2d1ad077852de36963f and #19351
- Looking at c27932ec938217d4fbb0adad23c0d0708f83f690 and #33460, yep.
It's pretty recent.

If it's not needed anymore, I'm in favour of removing this. I'm not 100%
sure, but it seems `bulk_batch_size` is only used for
[https://github.com/django/django/blob/953095d1e603fe0f8f01175b1409ca23818dcff9/django/db/models/query.py#L1909-L1915
bulk_create],
[https://github.com/django/django/blob/953095d1e603fe0f8f01175b1409ca23818dcff9/django/db/models/query.py#L928-L930
bulk_update], and
[https://github.com/django/django/blob/953095d1e603fe0f8f01175b1409ca23818dcff9/django/db/models/deletion.py#L233-L235
deletion], so I don't think any of them needs to consider the "max items
in a `SELECT` statement" limit on SQLite.
--
Ticket URL: <https://code.djangoproject.com/ticket/36430#comment:5>

[Django]

#36430: bulk_batch_size() special-cases single field on SQLite according to
outdated limit
-------------------------------------+-------------------------------------
Reporter: Jacob Walls | Owner: Jacob
Type: | Walls
Cleanup/optimization | Status: assigned
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
SQLITE_MAX_COMPOUND_SELECT, |
bulk_create |

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/19522 PR]
--
Ticket URL: <https://code.djangoproject.com/ticket/36430#comment:6>

[Django]

#36430: bulk_batch_size() special-cases single field on SQLite according to
outdated limit
-------------------------------------+-------------------------------------
Reporter: Jacob Walls | Owner: Jacob
Type: | Walls
Cleanup/optimization | Status: assigned
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:

Keywords: | Triage Stage: Ready for
SQLITE_MAX_COMPOUND_SELECT, | checkin

bulk_create |
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/36430#comment:7>

[Django]

#36430: bulk_batch_size() special-cases single field on SQLite according to
outdated limit
-------------------------------------+-------------------------------------
Reporter: Jacob Walls | Owner: Jacob
Type: | Walls
Cleanup/optimization | Status: assigned
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:

Keywords: | Triage Stage: Accepted
SQLITE_MAX_COMPOUND_SELECT, |

bulk_create |
Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 1

Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Jacob Walls):

* needs_better_patch: 0 => 1
* stage: Ready for checkin => Accepted

--
Ticket URL: <https://code.djangoproject.com/ticket/36430#comment:8>

[Django]

#36430: bulk_batch_size() special-cases single field on SQLite according to
outdated limit
-------------------------------------+-------------------------------------
Reporter: Jacob Walls | Owner: Jacob
Type: | Walls
Cleanup/optimization | Status: assigned
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
SQLITE_MAX_COMPOUND_SELECT, |
bulk_create |
Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0

Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by Jacob Walls):

* needs_better_patch: 1 => 0

--
Ticket URL: <https://code.djangoproject.com/ticket/36430#comment:9>

[Django]

#36430: bulk_batch_size() special-cases single field on SQLite according to
outdated limit
-------------------------------------+-------------------------------------
Reporter: Jacob Walls | Owner: Jacob
Type: | Walls
Cleanup/optimization | Status: assigned
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:

Keywords: | Triage Stage: Ready for
SQLITE_MAX_COMPOUND_SELECT, | checkin

bulk_create |
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/36430#comment:10>

[Django]

#36430: bulk_batch_size() special-cases single field on SQLite according to
outdated limit
-------------------------------------+-------------------------------------
Reporter: Jacob Walls | Owner: Jacob
Type: | Walls

Cleanup/optimization | Status: closed

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

Severity: Normal | Resolution: fixed

Keywords: | Triage Stage: Ready for
SQLITE_MAX_COMPOUND_SELECT, | checkin
bulk_create |
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:"a2ce4900a63f91f0cc685ac157762610c199c391" a2ce4900]:
{{{#!CommitTicketReference repository=""
revision="a2ce4900a63f91f0cc685ac157762610c199c391"
Fixed #36430 -- Removed artificially low limit on single field bulk
operations on SQLite.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/36430#comment:11>

[Django]

#36430: bulk_batch_size() special-cases single field on SQLite according to
outdated limit
-------------------------------------+-------------------------------------
Reporter: Jacob Walls | Owner: Jacob
Type: | Walls
Cleanup/optimization | Status: closed
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Ready for
SQLITE_MAX_COMPOUND_SELECT, | checkin
bulk_create |
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/36430#comment:12>