#35734 closed Bug (fixed)
JSONObject crashes with Postgres 16 and server side bindings
Reported by: | john-parton | Owned by: | john-parton |
---|---|---|---|
Component: | Database layer (models, ORM) | Version: | 5.1 |
Severity: | Release blocker | Keywords: | |
Cc: | Nick Pope | Triage Stage: | Ready for checkin |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description
Tests fail with Postgres 16 and server side bindings
Steps to replicate
- Pull down main
- Up a postgres 16 database (supplied docker-compose.yml)
- Use supplied settings file
- Install requirements
- Run
./runtests.py --settings test_postgres db_functions.comparison
docker-compose.yml
services: postgres: image: postgres:16.3 shm_size: 1g ports: - "5436:5432" environment: POSTGRES_USER: user POSTGRES_PASSWORD: postgres POSTGRES_DB: django
test_postgres.py
from test_sqlite import * # NOQA DATABASES = { "default": { "ENGINE": "django.db.backends.postgresql", "USER": "user", "NAME": "django", "PASSWORD": "postgres", "HOST": "localhost", "PORT": 5436, "OPTIONS": { "server_side_binding": True, }, }, }
Here's the output of my pip freeze for reference
aiohappyeyeballs==2.4.0 aiohttp==3.10.5 aiosignal==1.3.1 aiosmtpd==1.4.6 argon2-cffi==23.1.0 argon2-cffi-bindings==21.2.0 asgiref==3.8.1 atpublic==5.0 attrs==24.2.0 bcrypt==4.2.0 black==24.8.0 certifi==2024.8.30 cffi==1.17.1 charset-normalizer==3.3.2 click==8.1.7 -e git+ssh://git@github.com/django/django.git@aa5293068782dfa2d2173c75c8477f58a9989942#egg=Django docutils==0.21.2 frozenlist==1.4.1 geoip2==4.8.0 h11==0.14.0 idna==3.8 Jinja2==3.1.4 MarkupSafe==2.1.5 maxminddb==2.6.2 multidict==6.0.5 mypy-extensions==1.0.0 numpy==2.1.1 outcome==1.3.0.post0 packaging==24.1 pathspec==0.12.1 pillow==10.4.0 platformdirs==4.2.2 psycopg==3.2.1 psycopg-binary==3.2.1 psycopg-pool==3.2.2 pycparser==2.22 pylibmc==1.6.3 pymemcache==4.0.0 PySocks==1.7.1 pywatchman==2.0.0 PyYAML==6.0.2 redis==5.0.8 requests==2.32.3 selenium==4.24.0 setuptools==74.1.2 sniffio==1.3.1 sortedcontainers==2.4.0 sqlparse==0.5.1 tblib==3.0.0 trio==0.26.2 trio-websocket==0.11.1 typing_extensions==4.12.2 tzdata==2024.1 urllib3==2.2.2 websocket-client==1.8.0 wsproto==1.2.0 yarl==1.9.11
Python 3.12.3
Made this discovery while trying to finish a JSON feature, comment here: https://github.com/django/django/pull/18541#issuecomment-2331630403
Change History (18)
comment:2 by , 2 months ago
Version: | dev → 5.1 |
---|
comment:3 by , 2 months ago
Cc: | added |
---|---|
Severity: | Normal → Release blocker |
Triage Stage: | Unreviewed → Accepted |
Thank you John!
Confirmed this is a regression in 81ccf92f154c6d9eac3e30bac0aa67574d0ace15.
follow-up: 8 comment:4 by , 2 months ago
Thanks. I assume Nick can take a crack at it? Otherwise I can take a look. A little busy at the moment, so not sure when I could get to it.
EDIT
Do we need to add Postgres 16/15 to CI? I know there's already a ton of different configs tested, but it would have caught this if it were in the CI pipeline.
comment:6 by , 2 months ago
Has patch: | set |
---|---|
Needs tests: | set |
comment:7 by , 2 months ago
Owner: | set to |
---|---|
Status: | new → assigned |
comment:8 by , 2 months ago
Replying to john-parton:
Thanks. I assume Nick can take a crack at it? Otherwise I can take a look. A little busy at the moment, so not sure when I could get to it.
Thank you John for your PR and work on this. We'll review soon!
Do we need to add Postgres 16/15 to CI? I know there's already a ton of different configs tested, but it would have caught this if it were in the CI pipeline.
This is a very valid point and I would like us to pursue a more sustainable solution for this testing other than "each one does it on their own laptop".
John, would you be willing to raise this issue in the Django Forum to see if we could get some traction to setup this? I think we could have at least a new entry in the scheduled tests to test all supported version, or at least the oldest (current setup) and the newest.
comment:9 by , 2 months ago
Topic here: https://forum.djangoproject.com/t/postgres-16-ci-postgres-16-in-use/34649
Yeah, I'm not sure what the cost in $$$ to run the CI is. I think this is a pretty unusual corner case, so maybe it's not worth the hassle.
follow-up: 14 comment:11 by , 8 weeks ago
Here is an approach which uses the native JSONObject
function for server side bindings, casting the keys to text. Adding it here for a record.
comment:12 by , 8 weeks ago
Needs tests: | unset |
---|
comment:13 by , 8 weeks ago
Summary: | Tests fail with Postgres 16 and server side bindings → JSONObject crashes with Postgres 16 and server side bindings |
---|
comment:14 by , 8 weeks ago
Replying to Sarah Boyce:
Here is an approach which uses the native
JSONObject
function for server side bindings, casting the keys to text. Adding it here for a record.
I can create a separate ticket for this. "Enhancement", something like "use native JSONObject on Postgres 16+ with server side bindings"?
I'm assuming that it's a "good thing" to prefer the native JSONObject over just using jsonb_build_object all the time. In my experience, there is no _practical_ difference, but I'm assuming the original change was accepted because when support for Postgres 14 and 15 is dropped, we can drop the jsonb_build_object code path with a simple delete. Does that sound right?
comment:15 by , 8 weeks ago
I've created the separate ticket to track use of the native json object with postgres 16+ and server side bindings: https://code.djangoproject.com/ticket/35778#ticket
comment:17 by , 7 weeks ago
Triage Stage: | Accepted → Ready for checkin |
---|
I can confirm this is also an issue with
5.1.1
and5.1
, but not an issue with5.0.9
.Here's where I think the error was introduced.
5.0.9: https://github.com/django/django/blob/8e68f938f376cf2ca22a7e8ff0bcbe1b7a5832d1/django/db/models/functions/comparison.py#L163-L176
5.1: https://github.com/django/django/blob/373cb3037fe4e67adbac9ac43340391e859aa957/django/db/models/functions/comparison.py#L192
Basically on postgres 16, there's an attempt to use "native" JSON, but it doesn't work for nested objects.