Opened 6 years ago

Closed 4 years ago

Last modified 4 years ago

#12180 closed Bug (fixed)

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

Reported by: Xof Owned by: Xof
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 Xof 6 years ago.
12180-2.diff (1.3 KB) - added by Xof 6 years ago.
Improved patch (removed some now-redundant code)

Download all attachments as: .zip

Change History (20)

comment:1 Changed 6 years ago by Xof

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

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

comment:2 Changed 6 years ago by Xof

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 6 years ago by Xof

comment:3 Changed 6 years ago by Xof

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 6 years ago by Xof

Improved patch (removed some now-redundant code)

comment:4 Changed 6 years ago by Xof

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 6 years ago by Xof

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

comment:6 Changed 6 years ago by Xof

  • Has patch set

comment:7 Changed 6 years ago by Xof

  • milestone set to 1.2

comment:8 Changed 6 years ago by Xof

  • Needs tests set

comment:9 Changed 5 years ago by russellm

  • Triage Stage changed from Unreviewed to Accepted

comment:10 Changed 5 years ago by russellm

  • milestone changed from 1.2 to 1.3

Not critical for 1.2

comment:11 Changed 5 years ago by bretthoerner

  • Cc brett@… added

comment:12 Changed 5 years ago by bretthoerner

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 5 years ago by jezdez

This seems like a reasonable fix but needs tests.

comment:14 Changed 4 years ago by mattmcc

  • milestone 1.3 deleted
  • Severity set to Normal
  • Type set to Bug

comment:15 Changed 4 years ago by kmtracey

#15818 reported this again.

comment:16 Changed 4 years ago by anonymous

  • Easy pickings unset
  • Version changed from 1.1 to 1.3

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

comment:17 Changed 4 years ago by ramiro

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

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 4 years ago by tobias

  • 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