Code

Opened 8 months ago

Closed 6 months ago

Last modified 6 months ago

#21452 closed Bug (fixed)

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

Reported by: aaugustin Owned by: aaugustin
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.

Attachments (0)

Change History (7)

comment:1 Changed 8 months ago by akaariai

  • Severity changed from Normal to Release blocker
  • Triage Stage changed from Unreviewed to Accepted
  • Version changed from master to 1.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 8 months ago by akaariai (previous) (diff)

comment:2 Changed 8 months ago by aaugustin

  • Owner changed from nobody to aaugustin
  • Status changed from new to assigned

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 Changed 6 months ago by Aymeric Augustin <aymeric.augustin@…>

  • Resolution set to fixed
  • Status changed from assigned to closed

In 1afe7488322f9b53ea00f47fb07c4374a5f2150e:

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

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

Thanks Anssi for analysing this issue.

Refs #17062.

comment:4 Changed 6 months ago by Aymeric Augustin <aymeric.augustin@…>

In cb4a000adbb750da62daab318a9f4da8e3d4fdcd:

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

When settings.DATABASESdefault?AUTOCOMMIT? = 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 Changed 6 months ago by aaugustin

  • Resolution fixed deleted
  • Status changed from closed to new

comment:6 Changed 6 months ago by Aymeric Augustin <aymeric.augustin@…>

  • Resolution set to fixed
  • Status changed from new to closed

In 18d75e079245d7ba789d69eacda2aaf7603d678e:

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

Fixed #21452 again.

comment:7 Changed 6 months ago by Aymeric Augustin <aymeric.augustin@…>

In 225a6ed2cfecc609760a36c3b20369daeec52e07:

Fixed a test that was failing with PostGIS.

Fixed #21452 again.

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

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.