[Django] #36748: Postgres UNNEST optimisation of bulk_create cannot handle fields with `get_placeholder`.

[Django]

#36748: Postgres UNNEST optimisation of bulk_create cannot handle fields with
`get_placeholder`.
-------------------------------------+-------------------------------------
Reporter: Chris Wesseling | Type: Bug
Status: new | Component: Database
| layer (models, ORM)
Version: 5.2 | Severity: Normal
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
In order to allow fields to define a different placeholder than `"%s"` in
https://github.com/django/django/blob/97acd4d2f92eef8c285bac070d437bf0fd52e071/django/db/models/sql/compiler.py#L1769
`BaseSQLInsertCompiler.assemble_as_sql` does:

{{{
get_placeholders = [getattr(field, "get_placeholder", None) for
field in fields]
}}}

and calls those functions with `(value, compiler, connection)` to generate
the placeholders.

These alternative placeholders are not always UNNESTable. e.g. `"%s::{}"`
for arrays and and ranges, which we tried to work-around by filtering

{{{
# Fields that don't use standard internal types might not be
# unnest'able (e.g. array and geometry types are known to be
# problematic).
or any(
(field.target_field if field.is_relation else
field).get_internal_type()
not in self.connection.data_types
for field in fields
)
}}}

but more general since `get_placeholder` can be a function of `value`, we
can't UNNEST forall `fields, value_rows`, as it breaks the "one
placeholder fits all values" assumption.

Therefore if any field in fields has get_placeholder, we should fallback
to the default, unoptimised implementation.

In fact the addition of
{{{
# Field.get_placeholder takes value as an argument, therefore
the
# resulting placeholder might be dependent on the value.
# in UNNEST requires a single placeholder to "fit all values"
in
# the array.
or any(hasattr(field, "get_placeholder") for field in fields)
}}}

before the mentioned internal_type filter that's already present, handles
all cases in the test suite and never hits the internal_type case, because
all test examples also have a get_placeholder. I don't know if this will
always be the case, so I suggest keeping the present filter too.

I bumped into this regression when upgrading to 5.2 and the call to a
Postgres extension function in such a placeholder, but which has a db_type
that is present in self.connection.data_types, didn't happen anymore for
bulk inserts.
--
Ticket URL: <https://code.djangoproject.com/ticket/36748>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

[Django]

#36748: Postgres UNNEST optimisation of bulk_create cannot handle fields with
`get_placeholder`.
-------------------------------------+-------------------------------------

Reporter: Chris Wesseling | Owner: (none)
Type: Bug | Status: new
Component: Database layer | Version: 5.2
(models, ORM) |
Severity: Normal | 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 Chris Wesseling):

* has_patch: 0 => 1

--
Ticket URL: <https://code.djangoproject.com/ticket/36748#comment:1>

[Django]

#36748: Postgres UNNEST optimisation of bulk_create cannot handle fields with
`get_placeholder`.
-------------------------------------+-------------------------------------
Reporter: Chris Wesseling | Owner: (none)
Type: Bug | Status: new
Component: Database layer | Version: 5.2
(models, ORM) |
Severity: Normal | 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 Chris Wesseling:

Old description:

New description:

**NB This bug can go unnoticed: bulk_create might succeed, but with
different data written to disk than what is expected. And subsequent reads
and seeks will have the wrong results.**

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

[Django]

#36748: Postgres UNNEST optimisation of bulk_create cannot handle fields with
`get_placeholder`.
-------------------------------------+-------------------------------------
Reporter: Chris Wesseling | Owner: (none)
Type: Bug | Status: new
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 Simon Charette):

* severity: Normal => Release blocker
* stage: Unreviewed => Accepted

--
Comment:

Thanks for the excellent report and patch Chris!

The only part missing is a release note for 5.2.9 as this is a bug in a
newly introduced feature and thus still qualifies for backport.

> I don't know if this will always be the case, so I suggest keeping the
present filter too.

I also think it's worth keeping both as well as they are cheap to perform
and could cover other cases in the wild.

Small note to me that this relates to #36727 but the latter won't fix
issue as, just like you said, `get_placeholder` can be a function of
`value`. This only means we'll need even more logic to handle the presence
of either `get_placeholder` or `get_placeholder_sql`.
--
Ticket URL: <https://code.djangoproject.com/ticket/36748#comment:3>

[Django]

#36748: Postgres UNNEST optimisation of bulk_create cannot handle fields with
`get_placeholder`.
-------------------------------------+-------------------------------------
Reporter: Chris Wesseling | Owner: (none)
Type: Bug | Status: new
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
-------------------------------------+-------------------------------------

Description changed by Simon Charette:

--
--
Ticket URL: <https://code.djangoproject.com/ticket/36748#comment:4>

[Django]

#36748: Postgres UNNEST optimisation of bulk_create cannot handle fields with
`get_placeholder`.
-------------------------------------+-------------------------------------
Reporter: Chris Wesseling | Owner: (none)
Type: Bug | Status: new
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
-------------------------------------+-------------------------------------

Comment (by Chris Wesseling):

Replying to [comment:3 Simon Charette]:

> Thanks for the excellent report and patch Chris!
>
> The only part missing is a release note for 5.2.9 as this is a bug in a
newly introduced feature and thus still qualifies for backport.

Thanks for the quick response.
Should I just write one for 5.2.9.txt or 6.0.txt too?
--
Ticket URL: <https://code.djangoproject.com/ticket/36748#comment:5>

[Django]

#36748: Postgres UNNEST optimisation of bulk_create cannot handle fields with
`get_placeholder`.
-------------------------------------+-------------------------------------
Reporter: Chris Wesseling | Owner: (none)
Type: Bug | Status: new
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
-------------------------------------+-------------------------------------

Comment (by Simon Charette):

> Should I just write one for 5.2.9.txt or 6.0.txt too?

We don't document bug fixes unless they are backported to a fully released
series (e.g. 5.2.x) and since 6.0 is only in RC the patch will be
backported to it but won't be part of the release notes.
--
Ticket URL: <https://code.djangoproject.com/ticket/36748#comment:6>

[Django]

#36748: Postgres UNNEST optimisation of bulk_create cannot handle fields with
`get_placeholder`.
-------------------------------------+-------------------------------------
Reporter: Chris Wesseling | Owner: (none)
Type: Bug | Status: new
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 Simon Charette):

* stage: Accepted => Ready for checkin

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

[Django]

#36748: Postgres UNNEST optimisation of bulk_create cannot handle fields with
`get_placeholder`.
-------------------------------------+-------------------------------------

Reporter: Chris Wesseling | Owner: Chris
| Wesseling
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 Jacob Walls):

* owner: (none) => Chris Wesseling
* status: new => assigned

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

[Django]

#36748: Postgres UNNEST optimisation of bulk_create cannot handle fields with
`get_placeholder`.
-------------------------------------+-------------------------------------
Reporter: Chris Wesseling | Owner: Chris
| Wesseling

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 Jacob Walls <jacobtylerwalls@…>):

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

Comment:

In [changeset:"5834643f43a767fe19f2c6d10217b204e7584ec8" 5834643f]:
{{{#!CommitTicketReference repository=""
revision="5834643f43a767fe19f2c6d10217b204e7584ec8"
Fixed #36748 -- Filtered non-standard placeholders from UNNEST queries.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/36748#comment:9>

[Django]

#36748: Postgres UNNEST optimisation of bulk_create cannot handle fields with
`get_placeholder`.
-------------------------------------+-------------------------------------
Reporter: Chris Wesseling | Owner: Chris
| Wesseling
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 Jacob Walls <jacobtylerwalls@…>):

In [changeset:"161e54348577ee9756b3766ab363596ab880560f" 161e5434]:
{{{#!CommitTicketReference repository=""
revision="161e54348577ee9756b3766ab363596ab880560f"
[6.0.x] Fixed #36748 -- Filtered non-standard placeholders from UNNEST
queries.

Backport of 5834643f43a767fe19f2c6d10217b204e7584ec8 from main.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/36748#comment:10>

[Django]

#36748: Postgres UNNEST optimisation of bulk_create cannot handle fields with
`get_placeholder`.
-------------------------------------+-------------------------------------
Reporter: Chris Wesseling | Owner: Chris
| Wesseling
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 Jacob Walls <jacobtylerwalls@…>):

In [changeset:"ac9bdcabe10fb7ac0c7e9ebcd879f5e34bee776f" ac9bdca]:
{{{#!CommitTicketReference repository=""
revision="ac9bdcabe10fb7ac0c7e9ebcd879f5e34bee776f"
[5.2.x] Fixed #36748 -- Filtered non-standard placeholders from UNNEST

queries.

Backport of 5834643f43a767fe19f2c6d10217b204e7584ec8 from main.
}}}
--

Ticket URL: <https://code.djangoproject.com/ticket/36748#comment:11>