[Django] #35778: Use native JSONObject on Postgres 16+ with server side bindings

[Django]

#35778: Use native JSONObject on Postgres 16+ with server side bindings
-------------------------------------+-------------------------------------
Reporter: john-parton | Type: New
| feature
Status: new | Component: Database
| layer (models, ORM)
Version: dev | 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
-------------------------------------+-------------------------------------
JSONObject on Postgres 16 with server side bindings recently resulted in a
crash. The most recent fix is to fallback to the use of jsonb_build_object
on postgres 16 when using server side bindings.

See https://code.djangoproject.com/ticket/35734
And https://github.com/django/django/pull/18549

It is possible to use the native JSONObject with server side bindings, but
it requires a little bit of use of `cast`.

See <commit missing, was force-pushed overwritten at some point, need to
find it again>

There are two minor issues:

1. Should Postgres 16 *without* server-side bindings use "cast" even
though it's not strictly necessary? It it desirable or preferable to keep
the generated SQL the same when toggling the server-side binding feature?
I mentioned digging through logs as one example where it might matter.
2. Use of both cast and native json will require at least a minor change
to escaping. This is because we use the double-colon operator to cast and
the native json syntax uses a single colon to separate key-value pairs.
This creates a parsing ambiguity which results in a syntax error (on at
least one version of postgres). For solutions, they're all pretty similar
2a. Update the `as_native` function to wrap the keys in parenthesis,
effectively resolving the ambiguity. (This does raise yet another
question, a question within a question: should we go ahead and wrap the
keys in parenthesis on ALL backends? I think Oracle doesn't necessary
require that for example.)
2b. Update the Cast function to always wrap values in parenthesis in all
contexts. This seems like overkill.
2c. Change postgres from using the double-colon operator to the CAST(x AS
type) syntax. This also seems like overkill, and results in sql being
generated that is less postgres-y, if that makes sense.
--
Ticket URL: <https://code.djangoproject.com/ticket/35778>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

[Django]

#35778: Use native JSONObject on Postgres 16+ with server side bindings
-------------------------------------+-------------------------------------

Reporter: john-parton | Owner: (none)

Type: New feature | Status: new

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

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

Description changed by john-parton:

Old description:

New description:

JSONObject on Postgres 16 with server side bindings recently resulted in a
crash. The most recent fix is to fallback to the use of jsonb_build_object
on postgres 16 when using server side bindings.

See https://code.djangoproject.com/ticket/35734
And https://github.com/django/django/pull/18549

It is possible to use the native JSONObject with server side bindings, but
it requires a little bit of use of `cast`.

See

https://github.com/django/django/commit/0f53d48115ba0295cefea33512dc146caad39443

There are two minor issues:

1. Should Postgres 16 *without* server-side bindings use "cast" even
though it's not strictly necessary? It it desirable or preferable to keep
the generated SQL the same when toggling the server-side binding feature?
I mentioned digging through logs as one example where it might matter.
2. Use of both cast and native json will require at least a minor change
to escaping. This is because we use the double-colon operator to cast and
the native json syntax uses a single colon to separate key-value pairs.
This creates a parsing ambiguity which results in a syntax error (on at
least one version of postgres). For solutions, they're all pretty similar

Options for minor issue 2:

a. Update the `as_native` function to wrap the keys in parenthesis,
effectively resolving the ambiguity. (This does raise yet another
question, a question within a question: should we go ahead and wrap the
keys in parenthesis on ALL backends? I think Oracle doesn't necessary
require that for example.)

b. Update the Cast function to always wrap values in parenthesis in all
contexts. This seems like overkill.

c. Change postgres from using the double-colon operator to the CAST(x AS
type) syntax. This also seems like overkill, and results in sql being
generated that is less postgres-y, if that makes sense.

--

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

[Django]

#35778: Use native JSONObject on Postgres 16+ with server side bindings
-------------------------------------+-------------------------------------
Reporter: john-parton | Owner: (none)
Type: New feature | Status: new
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by john-parton):

My general opinion/gut feeling is:

1. Always cast, even when server-side bindings is off. It's one fewer
conditional which means the function is lower complexity.
2. Wrap keys in parenthesis for all backends that use the as_native
method. Again, fewer conditionals, which means lower complexity.

But I'm happy to be swayed.
--
Ticket URL: <https://code.djangoproject.com/ticket/35778#comment:2>

[Django]

#35778: Use native JSONObject on Postgres 16+ with server side bindings
-------------------------------------+-------------------------------------
Reporter: john-parton | Owner: (none)
Type: New feature | Status: new
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by john-parton):

In order to help facilitate discussion, I went ahead and opened a pull
request with my preferred changes:
https://github.com/django/django/pull/18622

When there's consensus on the the two issues:

1. Whether to cast keys on postgres 16+ without server side bindings; and
2. The best way to ensure that keys are escaped to avoid the colon/double-
colon parsing error

I'll update the PR and we can proceed.
--
Ticket URL: <https://code.djangoproject.com/ticket/35778#comment:3>

[Django]

#35778: Use native JSONObject on Postgres 16+ with server side bindings
-------------------------------------+-------------------------------------
Reporter: john-parton | Owner: (none)
Type: New feature | Status: new
Component: Database layer | Version: dev
(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 john-parton):

* has_patch: 0 => 1

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

[Django]

#35778: Use native JSONObject on Postgres 16+ with server side bindings
-------------------------------------+-------------------------------------

Reporter: john-parton | Owner: john-
| parton
Type: New feature | Status: assigned

Component: Database layer | Version: dev
(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 john-parton):

* owner: (none) => john-parton
* status: new => assigned

--
Ticket URL: <https://code.djangoproject.com/ticket/35778#comment:5>

[Django]

#35778: Use native JSONObject on Postgres 16+ with server side bindings
-------------------------------------+-------------------------------------
Reporter: john-parton | Owner: john-
| parton
Type: New feature | Status: assigned
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | 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: Simon Charette, Sarah Boyce, Mariusz Felisiak (added)
* stage: Unreviewed => Accepted

Comment:

Accepting following the referenced PR conversation. I'm unclear about the
best solution given the presented alternatives. Sarah, Simon, Mariusz:
would you have a recommendation?
--
Ticket URL: <https://code.djangoproject.com/ticket/35778#comment:6>

[Django]

#35778: Use native JSONObject on Postgres 16+ with server side bindings
-------------------------------------+-------------------------------------
Reporter: john-parton | Owner: john-
| parton
Type: New feature | Status: assigned
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | 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):

I'm of opinion that we should try to generate the same SQL with and
without server-side bindings enabled. That should put us in a better place
for eventual support for prepared statements (#20516).

As for the parenthesis wrapping I think that another option that wasn't
discussed is to use a `Func(expression, template="CAST(%(expressions)s AS
text)")` instead of `Cast` in this particular case to avoid the parsing
ambiguity in `as_postgres` while leaving `Cast.as_postgres` alone.
--
Ticket URL: <https://code.djangoproject.com/ticket/35778#comment:7>

[Django]

#35778: Use native JSONObject on Postgres 16+ with server side bindings
-------------------------------------+-------------------------------------
Reporter: john-parton | Owner: john-
| parton
Type: New feature | Status: assigned
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | 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 Sage Abdullah):

* cc: Sage Abdullah (added)

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

[Django]

#35778: Use native JSONObject on Postgres 16+ with server side bindings
-------------------------------------+-------------------------------------
Reporter: john-parton | Owner: john-
| parton
Type: New feature | Status: assigned
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | 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 john-parton):

Good info with prepared statements. That meshes with my intuition.

Regarding `Func(expression, template="CAST(%(expressions)s AS text)")`,
that has a bit of code smell to me, abstraction inversion.

I sort of question the wisdom of using the `::` operation on postgres at
all. The comment
https://github.com/django/django/blob/6765b6adf924c1bc8792a4a454d5a788c1abc98e/django/db/models/functions/comparison.py#L52-L61
suggests it makes the SQL "more readable", which is true in the context
that you're more familiar with postgres than any RDMS. I'd argue that the
Cast(...) function is possibly "more readable" than the double colon in
the case that the user is more familiar with other databases or just SQL
in general, and *relevant to this issue* it is more readable in the case
that there are colons used elsewhere in the expression, i.e. JSON_OBJECT.
(It's so unreadable with the double colons that even the parser can't
"read" it, haha)

I think an argument can be made to just delete those 10 lines and have the
output SQL closer to standards. It seems like part of the underlying
philosophy of moving postgres from JSONB_BUILD_OBJECT to JSON_OBJECT is to
settle on standards, so in that sense, reworking Cast() is in line with
the overall spirit of the project.
--
Ticket URL: <https://code.djangoproject.com/ticket/35778#comment:9>

[Django]

#35778: Use native JSONObject on Postgres 16+ with server side bindings
-------------------------------------+-------------------------------------
Reporter: john-parton | Owner: john-
| parton
Type: New feature | Status: assigned
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | 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 john-parton):

Pros in favor or removing `::` in favor of `CAST`

* Less code
* Generated SQL on Postgres will match other RDMS output in many cases
* It's more explicit
* "Standardization"


Cons

* It's not _strictly_ required to solve this specific issue
* Users more familiar with Postgres likely would find the double-colon
notation more readable in most circumstances
* Perhaps there's a goal to make the generated SQL look as if it were
written by an experienced programmer writing in that specific dialect.
"Idiomatic" postgres?
--
Ticket URL: <https://code.djangoproject.com/ticket/35778#comment:10>

[Django]

#35778: Use native JSONObject on Postgres 16+ with server side bindings
-------------------------------------+-------------------------------------
Reporter: john-parton | Owner: john-
| parton
Type: New feature | Status: assigned
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | 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):

Another con of changing `::` at this point is that will change the SQL
generated in any usage `Cast` in expression indices which might cause
people these indices not to be used anymore when users upgrade their
version of Django.

For example, say that you have a model of the form

{{{#!python
class People(models.Model):
phone_number = models.IntegerField()

class Meta:
indexes = [
Index(name="phone_number_str", Cast("phone_number",
models.TextField()))
]
}}}

Then the resulting index will be on `(phone_number)::text` and queries of
the form `filter(phone_number__startswith="123")` will generate `WHERE
(phone_number)::text LIKE '123%'` will be able to use the index.

If we change `Cast` to use `CAST` instead of `::` are confident that the
Postgres planer will be smart enough to use the pre-existing
`(phone_number)::text` it for queries of the form `CAST(phone_number AS
text) LIKE '123%'`? From past experience the expression has to exactly
match for the planner to consider the index and we can't expect users to
rebuild all of their indices every time they upgrade their Django version.
--
Ticket URL: <https://code.djangoproject.com/ticket/35778#comment:11>

[Django]

#35778: Use native JSONObject on Postgres 16+ with server side bindings
-------------------------------------+-------------------------------------
Reporter: john-parton | Owner: john-
| parton
Type: New feature | Status: assigned
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | 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 john-parton):

Darn. That's a pretty massive con, I would agree. Let's say that's a non-
option then. Thank you for your insight.

So with respect to my open PR, it sounds like we should use the double-
colon as well, in the (probably unlikely case) that the field in question
has an index on it. Based on what you said, if we use
"CAST(%(expression)s)" it might result in an index a user defined with the
expectation it be used, going unused due to the text not being exactly the
same in the query. Make sense?
--
Ticket URL: <https://code.djangoproject.com/ticket/35778#comment:12>

[Django]

#35778: Use native JSONObject on Postgres 16+ with server side bindings
-------------------------------------+-------------------------------------
Reporter: john-parton | Owner: john-
| parton
Type: New feature | Status: assigned
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | 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 john-parton):

In the (probably unlikely) case that a user has created an index using
`JSONObject`, isn't it also possible that migrating from
`JSONB_BUILD_OBJECT` to `JSON_OBJECT` might break those indexes in much
the same way as your Cast example?

{{{#!python
class People(models.Model):
phone_number = models.IntegerField()

class Meta:
indexes = [

Index(name="phone_number_obj",
JSONObject(phone_number="phone_number"))
]
}}}

If the migration was run a while ago, the index would use the
JSONB_BUILD_OBJECT function, but now queries will use the JSON_OBJECT
function.

Is it worth noting SOMEWHERE in a release note or something? It seems
really unlikely to me that someone would index their data this way.
--
Ticket URL: <https://code.djangoproject.com/ticket/35778#comment:13>

[Django]

#35778: Use native JSONObject on Postgres 16+ with server side bindings
-------------------------------------+-------------------------------------
Reporter: john-parton | Owner: john-
| parton
Type: New feature | Status: assigned
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | 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):

Changing to `JSONB_BUILD_OBJECT` would also be affected but as you've
pointed out it's unlikely and was less unlikely than usage of `Cast`. I
think it might be worth a release notes yes, IIRC we did it when we
changed how `Concat` behaved.
--
Ticket URL: <https://code.djangoproject.com/ticket/35778#comment:14>

[Django]

#35778: Use native JSONObject on Postgres 16+ with server side bindings
-------------------------------------+-------------------------------------
Reporter: john-parton | Owner: john-
| parton
Type: New feature | Status: assigned
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | 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 john-parton):

I was going to take a crack at adding a release note, modeled after the
release note you referenced for Concat, but I'm not sure I've found the
correct release note. Which one did you have in mind?
--
Ticket URL: <https://code.djangoproject.com/ticket/35778#comment:15>

[Django]

#35778: Use native JSONObject on Postgres 16+ with server side bindings
-------------------------------------+-------------------------------------
Reporter: john-parton | Owner: john-
| parton
Type: New feature | Status: assigned
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | 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 john-parton):

I believe this is pretty much ready to go. If we need a release note, I
would appreciate a little help.
--
Ticket URL: <https://code.djangoproject.com/ticket/35778#comment:16>

[Django]

#35778: Use native JSONObject on Postgres 16+ with server side bindings
-------------------------------------+-------------------------------------
Reporter: john-parton | Owner: john-
| parton
Type: New feature | Status: assigned
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | 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 Natalia Bidart):

Thank you John, this is indeed in the review queue and we'll get to this
as soon as we can.
--
Ticket URL: <https://code.djangoproject.com/ticket/35778#comment:17>

[Django]

#35778: Use native JSONObject on Postgres 16+ with server side bindings
-------------------------------------+-------------------------------------
Reporter: john-parton | Owner: john-

Type: | parton
Cleanup/optimization | Status: assigned

Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | 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 Sarah Boyce):

* type: New feature => Cleanup/optimization

--
Ticket URL: <https://code.djangoproject.com/ticket/35778#comment:18>

[Django]

#35778: Use native JSONObject on Postgres 16+ with server side bindings
-------------------------------------+-------------------------------------
Reporter: john-parton | Owner: john-
Type: | parton
Cleanup/optimization | Status: assigned
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | 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/35778#comment:19>

[Django]

#35778: Use native JSONObject on Postgres 16+ with server side bindings
-------------------------------------+-------------------------------------
Reporter: john-parton | Owner: john-
Type: | parton

Cleanup/optimization | Status: closed

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

Severity: Normal | 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:"78c9a2703151432f8e35cb63fca5a566fee5f8ed" 78c9a270]:
{{{#!CommitTicketReference repository=""
revision="78c9a2703151432f8e35cb63fca5a566fee5f8ed"
Fixed #35778 -- Used JSON_OBJECT database function on PostgreSQL 16+ with
server-side bindings.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/35778#comment:20>