[Django] #36960: Django never uses psycopg 3's optimised timestamptzloader

[Django]

#36960: Django never uses psycopg 3's optimised timestamptzloader
-------------------------------------+-------------------------------------
Reporter: Aarni Koskela | Type: Bug
Status: new | Component: Database
| layer (models, ORM)
Version: 6.0 | Severity: Normal
Keywords: postgresql | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
When available (e.g. `psycopg3-c` or `psycopg3-binary` installed), Psycopg
implicitly registers a faster `TimestamptzLoader` implementation
[https://github.com/psycopg/psycopg/blob/master/psycopg_c/psycopg_c/types/datetime.pyx].

However, Django overrides this with a custom class that derives from the
Python `psycopg.types.datetime.TimestamptzLoader` implementation. (You
can't derive from the `final`-marked C speedup class, anyway.)

For performance, it would be pleasant if Django used the faster class when
available. This is made slightly hairy by the fact that, as mentioned, you
can't use the speedup class as a base, and there is no public API in
psycopg to get the speedup class. `context.adapters._get_optimised`
exists, but is not a public API. You could do something like (and I
experimentally did)

{{{
try:
optimised_class =
context.adapters._get_optimised(TimestamptzLoader)
if optimised_class is not TimestamptzLoader:
self.fast_load = optimised_class(oid,
context).load
except Exception:
pass
}}}
and `res = (self.fast_load or super().load)(data)` but that's also
hairy...
--
Ticket URL: <https://code.djangoproject.com/ticket/36960>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

[Django]

#36960: Django never uses psycopg 3's optimised timestamptzloader
-------------------------------------+-------------------------------------

Reporter: Aarni Koskela | Owner: Mahi
| Singhal
Type: Bug | Status: assigned
Component: Database layer | Version: 6.0
(models, ORM) |
Severity: Normal | Resolution:

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

Changes (by Mahi Singhal):

* owner: (none) => Mahi Singhal
* status: new => assigned

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

[Django]

#36960: Django never uses psycopg 3's optimised timestamptzloader
-------------------------------------+-------------------------------------

Reporter: Aarni Koskela | Owner: (none)
Type: Bug | Status: new

Component: Database layer | Version: 6.0
(models, ORM) |
Severity: Normal | Resolution:
Keywords: postgresql | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by Mahi Singhal):

* owner: Mahi Singhal => (none)
* status: assigned => new

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

[Django]

#36960: Django never uses psycopg 3's optimised timestamptzloader
-------------------------------------+-------------------------------------
Reporter: Aarni Koskela | Owner: (none)

Type: | Status: closed
Cleanup/optimization |

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

Severity: Normal | Resolution: wontfix

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

Changes (by Jacob Walls):

* resolution: => wontfix
* status: new => closed
* type: Bug => Cleanup/optimization

Comment:

Thanks for the idea. I played with the
[https://github.com/django/django/pull/20813 provided PR] and added some
timeit statements, and found about a 4x performance improvement. However,
I don't think the provided solution is adequate, since, as you
acknowledge, there is no public interface for this.

We could revisit with one of two things in hand:
- a public API from psycopg for retrieving the c-accelerated internals
(unlikely...)
- a Django-side solution where instead of subclassing the loaders, we can
patch the `load()` method dynamically like this, although the cursor
timezone registration needs investigation:

{{{#!diff
diff --git a/django/db/backends/postgresql/base.py
b/django/db/backends/postgresql/base.py
index 42b37ab3c2..b4fe84be65 100644
--- a/django/db/backends/postgresql/base.py
+++ b/django/db/backends/postgresql/base.py
@@ -441,6 +441,7 @@ class DatabaseWrapper(BaseDatabaseWrapper):
# Register the cursor timezone only if the connection
disagrees, to
# avoid copying the adapter map.
tzloader =
self.connection.adapters.get_loader(TIMESTAMPTZ_OID, Format.TEXT)
+ # This fails, needs investigation.
if self.timezone != tzloader.timezone:
register_tzloader(self.timezone, cursor)
else:
diff --git a/django/db/backends/postgresql/psycopg_any.py
b/django/db/backends/postgresql/psycopg_any.py
index dea4800fce..221c18c6f4 100644
--- a/django/db/backends/postgresql/psycopg_any.py
+++ b/django/db/backends/postgresql/psycopg_any.py
@@ -22,23 +22,9 @@ try:
return ClientCursor(cursor.connection).mogrify(sql, params)

# Adapters.
- class BaseTzLoader(TimestamptzLoader):
- """
- Load a PostgreSQL timestamptz using the a specific timezone.
- The timezone can be None too, in which case it will be chopped.
- """
-
- timezone = None
-
- def load(self, data):
- res = super().load(data)
- return res.replace(tzinfo=self.timezone)
-
def register_tzloader(tz, context):
- class SpecificTzLoader(BaseTzLoader):
- timezone = tz
-
- context.adapters.register_loader("timestamptz", SpecificTzLoader)
+ TimestamptzLoader.load = lambda self, data:
TimestamptzLoader.load(self, data).replace(tz)
+ context.adapters.register_loader("timestamptz",
TimestamptzLoader)

class DjangoRangeDumper(RangeDumper):
"""A Range dumper customized for Django."""
}}}

I don't know how practical that sketched idea would be. It's not very
great python, either, so even then I'm still uncertain.
--
Ticket URL: <https://code.djangoproject.com/ticket/36960#comment:1>

[Django]

#36960: Django never uses psycopg 3's optimised timestamptzloader
-------------------------------------+-------------------------------------
Reporter: Aarni Koskela | Owner: (none)
Type: | Status: closed
Cleanup/optimization |
Component: Database layer | Version: 6.0
(models, ORM) |
Severity: Normal | Resolution: wontfix
Keywords: postgresql | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Aarni Koskela):

> a public API from psycopg for retrieving the c-accelerated internals
(unlikely...)

I'm not sure that's totally implausible; the author/maintainer of
psycopg3, @dvarrazzo-on-GitHub, chimed in on the original issue porting
his psycopg3 backend to Django in
[https://github.com/django/django/pull/15687#issuecomment-1348923713], and
is responsive on the psycopg repo (I've been looking at some perf fixes on
that side of the fence.)

I could make a PR there to propose making `get_optimised` a public API,
and link this thread.
--
Ticket URL: <https://code.djangoproject.com/ticket/36960#comment:2>

[Django]

#36960: Django never uses psycopg 3's optimised timestamptzloader
-------------------------------------+-------------------------------------
Reporter: Aarni Koskela | Owner: (none)

Type: | Status: new

Cleanup/optimization |
Component: Database layer | Version: 6.0
(models, ORM) |
Severity: Normal | Resolution:

Keywords: postgresql | Triage Stage:
| Someday/Maybe

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

Changes (by Jacob Walls):

* resolution: wontfix =>
* stage: Unreviewed => Someday/Maybe
* status: closed => new

Comment:

Great plan. I'll signal that by moving this to Someday/Maybe to represent
we're waiting on upstream input.
--
Ticket URL: <https://code.djangoproject.com/ticket/36960#comment:3>

[Django]

#36960: Django never uses psycopg 3's optimised timestamptzloader
-------------------------------------+-------------------------------------
Reporter: Aarni Koskela | Owner: (none)
Type: | Status: new
Cleanup/optimization |
Component: Database layer | Version: 6.0
(models, ORM) |
Severity: Normal | Resolution:
Keywords: postgresql | Triage Stage:
| Someday/Maybe
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Aarni Koskela):

:+1: Opened an issue for discussion for now instead of going full in with
another PR: https://github.com/psycopg/psycopg/issues/1273
--
Ticket URL: <https://code.djangoproject.com/ticket/36960#comment:4>