[Django] #35021: Debug query capturing on psycopg3 disregards execute wrappers
Django
-------------------------------------+-------------------------------------
Reporter: Ran | Owner: nobody
Benita |
Type: Bug | Status: new
Component: Database | Version: 5.0
layer (models, ORM) |
Severity: Normal | Keywords:
Triage Stage: | Has patch: 0
Unreviewed |
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-------------------------------------+-------------------------------------
== Problem
I use [https://docs.djangoproject.com/en/4.2/topics/db/instrumentation
/#connection-execute-wrapper connection.execute_wrapper] as a way to add
comments to certain queries (see code at the end for reference). And I
test the wrapper using `assertNumQueries` (see code at end for reference).
Once I switched from psycopg2 to psycopg3, this test started failing: the
unmodified query is captured, instead of the query modified by the execute
wrapper. Note however that the modified query is what is actually sent to
the DB, so this is only a debug issue.
I expect it's the same for `django.db.backends` debug logs but I haven't
verified this.
== Analysis
I've traced the issue to the following code in
`postgresql.DatabaseOperations.last_executed_query`
https://github.com/django/django/blob/66d58e77de3196404e0820a6fef0a6144186015a/django/db/backends/postgresql/operations.py#L296-L311:
{{{
#!python
if is_psycopg3:
def last_executed_query(self, cursor, sql, params):
try:
return self.compose_sql(sql, params)
except errors.DataError:
return None
else:
def last_executed_query(self, cursor, sql, params):
# https://www.psycopg.org/docs/cursor.html#cursor.query
# The query attribute is a Psycopg extension to the DB API
2.0.
if cursor.query is not None:
return cursor.query.decode()
return None
}}}
psycopg2 uses `cursor.query` which ends up being the modified query.
psycopg3 uses whatever's passed in which ends up being the unmodified
query.
It seems like psycopg3 has an equivalent in
[https://www.psycopg.org/psycopg3/docs/api/cursors.html#psycopg.Cursor._query
cursor._query]. It is documented in the API reference but with a warning
"You shouldn’t consider it part of the public interface of the object: it
might change without warnings. [...] If you would like to build reliable
features using this object, please get in touch so we can try and design
an useful interface for it.". So if this is the desired solution, will
need to work with psycopg to expose a stable interface.
== Reproduction code
Example execute wrapper:
{{{
#!python
def db_comment_wrapper(comment: str) -> AbstractContextManager[None]:
def handler(execute, sql, params, many, context):
clean_comment = escape(comment)
return execute(f'/* {clean_comment} */ {sql}', params, many,
context)
return db_connection.execute_wrapper(handler)
}}}
Test:
{{{
#!python
with self.assertNumQueries(1) as captured:
with db_comment_wrapper('This is a comment'):
list(ContentType.objects.all())
sql = captured[0]['sql']
assert sql.startswith('/* This is a comment */')
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/35021>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
-------------------------------------+-------------------------------------
Type: | Status: new
Cleanup/optimization |
Component: Database layer | Version: 5.0
(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
-------------------------------------+-------------------------------------
* cc: Florian Apolloner, Daniele Varrazzo (added)
* type: Bug => Cleanup/optimization
Comment:
As far as I'm aware, there is no way to support it with psycopg's public
API, so I'd not treat this as a bug.
Daniele, What do you think about extending psycopg's API?
--
Ticket URL: <https://code.djangoproject.com/ticket/35021#comment:1>
Django
-------------------------------------+-------------------------------------
Reporter: Ran Benita | Owner: nobody
Type: | Status: new
Cleanup/optimization |
Component: Database layer | Version: 5.0
(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 Mariusz Felisiak):
> Daniele, What do you think about extending psycopg's API?
I opened an [https://github.com/psycopg/psycopg/issues/696 issue] to
continue a discussion with the `psycopg` maintainers.
--
Ticket URL: <https://code.djangoproject.com/ticket/35021#comment:2>
Django
-------------------------------------+-------------------------------------
Reporter: Ran Benita | Owner: nobody
Type: | Status: new
Cleanup/optimization |
Component: Database layer | Version: 5.0
(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 Florian Apolloner):
I think the new code was written like this because we cannot ever
determine the actual query for server side bindings.
--
Ticket URL: <https://code.djangoproject.com/ticket/35021#comment:3>
Django
-------------------------------------+-------------------------------------
Reporter: Ran Benita | Owner: nobody
Type: | Status: new
Cleanup/optimization |
Component: Database layer | Version: 5.0
(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 Daniele Varrazzo):
I understand that Django use `ClientCursor` only, right? I think that the
first iterations of the backend used to use server-side-binding cursors
and maybe the current implementation of `last_executed_query()` was
catered to that.
I have added a proposal to [https://github.com/psycopg/psycopg/issues/696
the upstream ticket].
--
Ticket URL: <https://code.djangoproject.com/ticket/35021#comment:4>
Django
-------------------------------------+-------------------------------------
Reporter: Ran Benita | Owner: nobody
Type: | Status: new
Cleanup/optimization |
Component: Database layer | Version: 5.0
(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 Florian Apolloner):
Replying to [comment:4 Daniele Varrazzo]:
> I understand that Django use `ClientCursor` only, right? I think that
the first iterations of the backend used to use server-side-binding
cursors and maybe the current implementation of `last_executed_query()`
was catered to that.
Django defaults to `ClientCursor` but it can be changed to server-side-
binding cursors via settings.
--
Ticket URL: <https://code.djangoproject.com/ticket/35021#comment:5>