Opened 10 years ago

Closed 10 years ago

Last modified 10 years ago

#21452 closed Bug (fixed)

Connections may be set to autocommit under PostgreSQL when AUTOCOMMIT is set to False.

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

Description

django/db/backends/postgresql_psycopg2/base.py contains this code:

            if conn_tz != tz:
                # Set the time zone in autocommit mode (see #17062)
                self.set_autocommit(True)
                self.connection.cursor().execute(
                        self.ops.set_time_zone_sql(), [tz])

This will leave the connection in autocommit mode regardless of the AUTOCOMMIT setting for the database connection.

I haven't confirmed the bug; this report is just based on my understanding of the code.

Change History (7)

comment:1 by Anssi Kääriäinen, 10 years ago

Severity: NormalRelease blocker
Triage Stage: UnreviewedAccepted
Version: master1.6

Actually I think the code happens to (almost) work both when autocommit is on or off.

If autocommit is on in settings, then set_autocommit(True) gets called from connect() method of backends/__init__.py:BaseDatabaseWrapper. End result is that autocommit is enabled.

If autocommit is off in settings, then self.connection.set_isolation_level(self.isolation_level) gets called as last thing in init_connection_state(). This sets autocommit off. However, if self.set_autocommit(True) is called from the code block mentioned in ticket's description, then autocommit will be off for the connection, but Django will think the connection is in autocommit mode (self.autocommit is True).

Also, after init_connection_state() but before set_autocommit() is called from BaseDatabaseWrapper's connect() the connection will be in non-autocommit mode. This is also why #21453's use case doesn't work as expected.

I think we need to clean up how isolation level changes are done for new connections, and also backpatch to 1.6.x.

Last edited 10 years ago by Anssi Kääriäinen (previous) (diff)

comment:2 by Aymeric Augustin, 10 years ago

Owner: changed from nobody to Aymeric Augustin
Status: newassigned

Yes, now I remember psycopg2's APIs for isolation levels interact with autocommit. Django even used to activate autocommit through set_isolation_level.

I think I carefully refactored this area without changing the behavior when I wrote the new transaction management, but mistakes are always possible.

I'm adding this to my TODO list for the sprints in Berlin next week.

comment:3 by Aymeric Augustin <aymeric.augustin@…>, 10 years ago

Resolution: fixed
Status: assignedclosed

In 1afe7488322f9b53ea00f47fb07c4374a5f2150e:

Fixed #21452 -- Non-autocommit connections to PostgreSQL.

When settings.DATABASESdefaultAUTOCOMMIT = False, the connection
wasn't in autocommit mode but Django pretended it was.

Thanks Anssi for analysing this issue.

Refs #17062.

comment:4 by Aymeric Augustin <aymeric.augustin@…>, 10 years ago

In cb4a000adbb750da62daab318a9f4da8e3d4fdcd:

[1.6.x] Fixed #21452 -- Non-autocommit connections to PostgreSQL.

When settings.DATABASESdefaultAUTOCOMMIT = False, the connection
wasn't in autocommit mode but Django pretended it was.

Thanks Anssi for analysing this issue.

Refs #17062.

Backport of 1afe7488 from master

comment:5 by Aymeric Augustin, 10 years ago

Resolution: fixed
Status: closednew

comment:6 by Aymeric Augustin <aymeric.augustin@…>, 10 years ago

Resolution: fixed
Status: newclosed

In 18d75e079245d7ba789d69eacda2aaf7603d678e:

[1.6.x] Fixed a test that was failing with PostGIS.

Fixed #21452 again.

comment:7 by Aymeric Augustin <aymeric.augustin@…>, 10 years ago

In 225a6ed2cfecc609760a36c3b20369daeec52e07:

Fixed a test that was failing with PostGIS.

Fixed #21452 again.

Forward-port of 18d75e07 from stable/1.6.x.

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