#34466 closed Bug (fixed)
Django 4.2 overwrites user-specified psycopg cursor_factory
| Reported by: | Anders Kaseorg | Owned by: | Anders Kaseorg |
|---|---|---|---|
| Component: | Database layer (models, ORM) | Version: | 4.2 |
| Severity: | Release blocker | Keywords: | |
| Cc: | Daniele Varrazzo, Florian Apolloner | 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
Zulip configures a custom cursor_factory that wraps psycopg2.extensions.cursor to collect timing statistics for logging. But this no longer works in Django 4.2 due to 09ffc5c1212d4ced58b708cbbf3dfbfb77b782ca (#33308) and 0e2649fdf40cedc5be7e2c0e5f7711f315e36b84 (#34255) because DatabaseWrapper.get_new_connection now unconditionally overwrites connection.cursor_factory (even for psycopg2).
The configured cursor_factory is being passed to get_new_connection as conn_params["cursor_factory"]. get_new_connection should leave that alone if it’s set. (And if it’s not, it might also be cleaner for to pass the default cursor_factory via a keyword argument to psycopg[2].connect too, rather than mutating it later.)
Change History (8)
comment:1 by , 3 years ago
| Has patch: | set |
|---|---|
| Owner: | changed from to |
| Status: | new → assigned |
follow-up: 5 comment:2 by , 3 years ago
| Needs tests: | set |
|---|---|
| Triage Stage: | Unreviewed → Accepted |
Sounds like a legit request, we didn't limit this functionality on purpose but rather by accident.
Assuming we fix this we should also make it more pythonic (and I guess I am at fault for it being like it is now) and not check for is True but only truthiness. Either way, a test ensuring that we do not regress here would be good.
comment:4 by , 3 years ago
| Needs tests: | unset |
|---|
comment:5 by , 3 years ago
| Needs documentation: | set |
|---|---|
| Patch needs improvement: | set |
| Severity: | Normal → Release blocker |
Replying to Florian Apolloner:
Assuming we fix this we should also make it more pythonic (and I guess I am at fault for it being like it is now) and not check for
is Truebut only truthiness.
We use is True intentionally, to avoid errors when passing wrong truthy values, e.g."server_side_binding": os.environ.get("USE_SSB") (where os.environ.get("USE_SSB") returns "False").
comment:6 by , 3 years ago
| Needs documentation: | unset |
|---|---|
| Patch needs improvement: | unset |
| Triage Stage: | Accepted → Ready for checkin |
Submitted a patch at https://github.com/django/django/pull/16736.