Opened 14 years ago

Closed 13 years ago

Last modified 12 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 14 years ago.
12180-2.diff (1.3 KB ) - added by Christophe Pettus 14 years ago.
Improved patch (removed some now-redundant code)

Download all attachments as: .zip

Change History (20)

comment:1 by Christophe Pettus, 14 years ago

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

comment:2 by Christophe Pettus, 14 years ago

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.

by Christophe Pettus, 14 years ago

Attachment: 12180.diff added

comment:3 by Christophe Pettus, 14 years ago

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.

by Christophe Pettus, 14 years ago

Attachment: 12180-2.diff added

Improved patch (removed some now-redundant code)

comment:4 by Christophe Pettus, 14 years ago

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 by Christophe Pettus, 14 years ago

Owner: changed from nobody to Christophe Pettus
Status: newassigned

comment:6 by Christophe Pettus, 14 years ago

Has patch: set

comment:7 by Christophe Pettus, 14 years ago

milestone: 1.2

comment:8 by Christophe Pettus, 14 years ago

Needs tests: set

comment:9 by Russell Keith-Magee, 14 years ago

Triage Stage: UnreviewedAccepted

comment:10 by Russell Keith-Magee, 14 years ago

milestone: 1.21.3

Not critical for 1.2

comment:11 by Brett Hoerner, 14 years ago

Cc: brett@… added

comment:12 by Brett Hoerner, 14 years ago

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 by Jannis Leidel, 14 years ago

This seems like a reasonable fix but needs tests.

comment:14 by Matt McClanahan, 13 years ago

milestone: 1.3
Severity: Normal
Type: Bug

comment:15 by Karen Tracey, 13 years ago

#15818 reported this again.

comment:16 by anonymous, 13 years ago

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 by Ramiro Morales, 13 years ago

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 by Tobias McNulty, 12 years ago

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