Opened 7 years ago

Closed 5 years ago

Last modified 5 years ago

#12180 closed Bug (fixed)

ProgrammingError thrown with autocommit: True if first query on PostgreSQL >= 8.2 is an INSERT

Reported by: Christophe Pettus Owned by: Christophe Pettus
Component: Database layer (models, ORM) Version: 1.3
Severity: Normal Keywords:
Cc: brett@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: yes Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

There is a bug in the handling of InsertQuery.connection.features.can_return_id_from_insert, which is causing Django 1.1.1 to throw a ProgrammingError exception when inserting a new object/record into the database, using PostgreSQL 8.4.1, using psycopg2, if that INSERT is the first thing done by a view on a particular connection to the database, when DATABASE_OPTIONS autocommit: True is set.

The details are:

  1. Django uses InsertQuery.connection.features.can_return_id_from_insert to decide whether or not append a RETURNING clause to the INSERT, so that it can get the primary key of a newly-inserted object (db/models/sql/subqueries.py, lines 311-315).
  1. That flag is set in the _cursor method of DatabaseWrapper (db/backends/postgresql_psycopg2/base.py, lines 106-121), but the cursor hasn't been created yet in step #1, so can_return_id_from_insert is always False.
  1. But Django then issues the query (creating the cursor, and correctly setting can_return_id_from_insert to True), and thus expects a return value to come back the INSERT statement, but since the RETURNING clause wasn't added in step #1, it throws a ProgrammingError exception when it tries to get the expected return value (db/models/sql/subqueries.py, lines 323-324).

This appears to be a bad interaction with autocommit: True, since the code is clearly expecting a transaction to already have been opened at this point, which would have set the version information and the can_return_id_from_insert to True.

Attachments (2)

12180.diff (663 bytes) - added by Christophe Pettus 7 years ago.
12180-2.diff (1.3 KB) - added by Christophe Pettus 7 years ago.
Improved patch (removed some now-redundant code)

Download all attachments as: .zip

Change History (20)

comment:1 Changed 7 years ago by Christophe Pettus

Looking at it a bit further, I'm not quite sure why can_return_id_from_insert depends on uses_autocommit.

comment:2 Changed 7 years ago by Christophe Pettus

Looking at #10467, I now get it: autocommit = True is being used as a (checked) assertion that the PG version is >= 8.2, so that the initially-constructed SQL on an INSERT doesn't include a RETURNING for PG versions <8.2... sadly, it seems to have just moved the bug to a different place.

Changed 7 years ago by Christophe Pettus

Attachment: 12180.diff added

comment:3 Changed 7 years ago by Christophe Pettus

I've attached a proposed patch. The patch just sets "self.features.can_return_id_from_insert" based on the autocommit setting. This is, of course, a hack (although not really any more hacky than the current code). This allows SQL-generation operations before cursor creation to get a correct value for self.features.can_return_id_from_insert. Of course, autocommit could be lying and the version of PostgreSQL could be such that autocommit (and self.features.can_return_id_from_insert) don't apply, but this will be checked before any queries are actually sent to the db.

Changed 7 years ago by Christophe Pettus

Attachment: 12180-2.diff added

Improved patch (removed some now-redundant code)

comment:4 Changed 7 years ago by Christophe Pettus

I've attached another patch, this one with some redundant code removed. This doesn't solve the full issue described in #10509, but it does not reopen the prior bug of sending an INSERT involving a RETURNING to a version of PostgreSQL < 8.2, as the autocommit will cause the version to be checked, and an error raised if it's not >=8.2.

comment:5 Changed 7 years ago by Christophe Pettus

Owner: changed from nobody to Christophe Pettus
Status: newassigned

comment:6 Changed 7 years ago by Christophe Pettus

Has patch: set

comment:7 Changed 7 years ago by Christophe Pettus

milestone: 1.2

comment:8 Changed 7 years ago by Christophe Pettus

Needs tests: set

comment:9 Changed 7 years ago by Russell Keith-Magee

Triage Stage: UnreviewedAccepted

comment:10 Changed 7 years ago by Russell Keith-Magee

milestone: 1.21.3

Not critical for 1.2

comment:11 Changed 7 years ago by Brett Hoerner

Cc: brett@… added

comment:12 Changed 7 years ago by Brett Hoerner

I'm not sure why this isn't considered critical for 1.2, but I'd at least like to note that Xof's patch works well for me (as this was a blocker for our 1.2 deploy).

comment:13 Changed 6 years ago by Jannis Leidel

This seems like a reasonable fix but needs tests.

comment:14 Changed 6 years ago by Matt McClanahan

milestone: 1.3
Severity: Normal
Type: Bug

comment:15 Changed 6 years ago by Karen Tracey

#15818 reported this again.

comment:16 Changed 6 years ago by anonymous

Easy pickings: unset
Version: 1.11.3

I've been using this patch really solves the problem. I wonder, when will be included in an official version?

comment:17 Changed 5 years ago by Ramiro Morales

Resolution: fixed
Status: assignedclosed

In [16443]:

Removed more code for handling of PostgreSQL versions older than 8.2; use always "INSERT... RETURNING..." rather than "INSERT...; SELECT CURRVAL...". Thanks Christoph Pettus for the report and hints. Fixes #12180. Refs [16423].

comment:18 Changed 5 years ago by Tobias McNulty

UI/UX: unset

Note that, for anyone needing this fix on Django 1.3, it appears to be possible to implement the required functionality from this patch by adding the following to your urls.py (with a hefty comment explaining why, of course):

from django.db.backends.postgresql_psycopg2.base import DatabaseFeatures
DatabaseFeatures.can_return_id_from_insert = True
Note: See TracTickets for help on using tickets.
Back to Top