Opened 6 years ago

Closed 5 years ago

Last modified 5 years ago

#24318 closed Bug (fixed)

Can't configure Postgres isolation level if using recent psycopg2

Reported by: Carl Meyer Owned by: Aymeric Augustin
Component: Database layer (models, ORM) Version: 1.7
Severity: Release blocker Keywords:
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

According to our docs, you should be able to set OPTIONS['isolation_level'] on a database connection to configure the Postgres isolation level. But according to a report on django-users, which I confirmed by code inspection, on at least Django 1.7+ setting OPTIONS['isolation_level'] has no effect if you are using psycopg2 2.4.2+.

The only place the configured isolation_level is referenced is in the backend's _set_autocommit() method, and there it is only used if using an older psycopg2.

In 1.7 we had a _set_isolation_level() method on the PG backend, but it was never called, and has since been removed in master.

This bug probably dates back to the transaction changes in Django 1.6, though I haven't confirmed that.

Change History (13)

comment:1 Changed 5 years ago by Tim Graham

Cc: Aymeric Augustin added
Severity: NormalRelease blocker
Triage Stage: UnreviewedAccepted

It looks like we could use connection.set_session() to also set the isolation level in the if self.psycopg2_version >= (2, 4, 2) branch.

Aymeric, was this simply an oversight?

comment:2 Changed 5 years ago by Aymeric Augustin

Historically support for isolation levels in the psycopg2 backend only existed for people who wanted to switch to autocommit. This is now the default.

That said, I'm not sure that any isolation level other than the default actually works well with Django, and I don't remember if the docs reflect that.

comment:3 Changed 5 years ago by Carl Meyer

I think get_or_create may have issues on a different isolation level? Otherwise I'm not aware of areas where Django would work poorly, but there certainly may be others. AFAIK the default level for MySQL is "REPEATABLE READ" (whereas for PostgreSQL it's "READ COMMITTED"), and I can't see anywhere we change that in the MySQL backend. And for SQLite AFAIK the only available isolation level is SERIALIZABLE. So unless I'm missing something I think Django core already implicitly "supports" three different transaction isolation levels, depending on your database backend.

I'm not sure the feature historically existed _only_ for switching to autocommit - I've run into people who wanted "REPEATABLE READ" on PostgreSQL for whatever reason (often because they were converting a project from MySQL).

I don't feel strongly, but we should either remove the documentation about setting the isolation level, or make it possible again, and given that it's a small change I'd slightly favor the latter. Perhaps the docs should be updated to note that Django does not test against PostgreSQL with any transaction isolation level other than the default.

comment:4 Changed 5 years ago by Carl Meyer

Of course, if this bug really has existed since 1.6 and wasn't noticed until now, that's a strong indication that nobody is actually using this feature (or if they are, they can't tell the difference when it doesn't work.)

comment:5 Changed 5 years ago by Tim Graham

I guess [e0449316] preceded the rest of the transactions overhaul in 1.6. I'd favor removing the "Isolation level" section from the docs rather than saying "this only works with psycopg2 < 2.4.2" as that seems a bit odd to explain. What do you think? (wrote all this before reading Carl's last two comments)

Last edited 5 years ago by Tim Graham (previous) (diff)

comment:6 in reply to:  4 Changed 5 years ago by Aymeric Augustin

psycopg2 2.4.2+ correctly separates the notions of autocommit and isolation levels. I don't want to mix them again in Django

We can fix this bug by setting the isolation level with set_session in init_connection_state.

We could also tell people to configure the default isolation level in their database server's configuration, but since it's easy to fix the regression, I don't like this solution.

comment:7 Changed 5 years ago by Aymeric Augustin

Cc: Aymeric Augustin removed
Owner: changed from nobody to Aymeric Augustin
Status: newassigned

For the record here's the report on django-users: https://groups.google.com/d/msg/django-users/HTc5HNwa4iQ/hMCaGsWhc2YJ

comment:8 Changed 5 years ago by Aymeric Augustin

Has patch: set

PR: https://github.com/django/django/pull/4132

The patch is quite straightforward. I'm confident it's the correct solution. Can someone double-check?

I will backport the first commit of the pull request to 1.7 and 1.8.

comment:9 Changed 5 years ago by Tim Graham

Triage Stage: AcceptedReady for checkin

comment:10 Changed 5 years ago by Aymeric Augustin

Triage Stage: Ready for checkinAccepted

Sorry, my first attempt wasn't very good. So much for confidence. I updated the PR with a second attempt.

comment:11 Changed 5 years ago by Aymeric Augustin <aymeric.augustin@…>

Resolution: fixed
Status: assignedclosed

In 76356d963c131252e1ce4285083fd21fd0bdedd9:

Fixed #24318 -- Set the transaction isolation level with psycopg >= 2.4.2.

comment:12 Changed 5 years ago by Aymeric Augustin <aymeric.augustin@…>

In 87e9cad4a4481e7cd91e2e7bac46efe54000a932:

[1.8.x] Fixed #24318 -- Set the transaction isolation level with psycopg >= 2.4.2.

Backport of 76356d96 from master

comment:13 Changed 5 years ago by Aymeric Augustin <aymeric.augustin@…>

In 9b7d512d5fb88844d920929b3cbb9d6c38b8b891:

[1.7.x] Fixed #24318 -- Set the transaction isolation level with psycopg >= 2.4.2.

Backport of 76356d96 from master

Note: See TracTickets for help on using tickets.
Back to Top