Opened 16 years ago

Closed 16 years ago

Last modified 13 years ago

#10467 closed (fixed)

Remove use of "RETURNING" in PostgreSQL backends by default

Reported by: Malcolm Tredinnick Owned by: Malcolm Tredinnick
Component: Database layer (models, ORM) Version: 1.0
Severity: Keywords:
Cc: guillermo.gutierrez@…, richard.davies@… Triage Stage: Unreviewed
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

Turns out the "RETURNING" change in r10029 isn't supported in PostgreSQL 8.1. So that new feature is only going to be possible in later PostgreSQL versions and we can't use the one statement version in the default code path, unless there's an alternative compatible version.

Change History (9)

comment:1 by Erin Kelly, 16 years ago

Maybe this should be a separate ticket, but it's close enough that I'll be lazy and just add it to this one. The existing "RETURNING" support doesn't seem to be complete, since the "SELECT CURRVAL" query is still happening:

In [1]: from django.db import connection

In [2]: from testapp.models import P

In [3]: p = P()

In [4]: p.id

In [5]: p.save()

In [6]: p.id
Out[6]: 1L

In [7]: connection.queries
Out[7]:
[{'sql': 'INSERT INTO "testapp_p" ("id") VALUES (DEFAULT) RETURNING "testapp_p"."id"',
  'time': '0.004'},
 {'sql': 'SELECT CURRVAL(\'"testapp_p_id_seq"\')', 'time': '0.001'}]

It looks like one of the diffs for subqueries.py from the patch in #3460 didn't make it into the final commit.

comment:2 by Malcolm Tredinnick, 16 years ago

(In [10034]) Return last insert ID correctly when the feature is enabled.

This was overlooked when merging the patch from #3460 in r10029.
Thank to Ian Kelly for noticing. Refs #10467.

comment:3 by Malcolm Tredinnick, 16 years ago

Resolution: fixed
Status: newclosed

(In [10035]) Fixed #10467 -- Fixed generated INSERT SQL for PostgreSQL 8.1 and earlier.

I introduced a bad regression in r10029, forgetting to check that some
syntax was supported. For now, you can't use autocommit=True with 8.1
and earlier (it's still available for later versions). I'll fix the
broader issue later and re-enable it for those versions, but I want to
get the SQL regression for the default path out of the code right now.

comment:4 by offmessage, 16 years ago

Resolution: fixed
Status: closedreopened

I'm afraid this one isn't quite fixed. If the very first database operation on a connection is a save() you still get the error reporting in #10474: syntax error at or near "RETURNING". It looks like the test for PostgreSQL version is only done at the point a query is issued. I'm afraid I'm new to Django and haven't been able to propose a patch.

Simple test case:

>>> from django.db import connection                                    
>>> connection.features.can_return_id_from_insert                       
True                                                                    
>>> connection._version
Traceback (most recent call last):
  File "<console>", line 1, in ?
AttributeError: 'DatabaseWrapper' object has no attribute '_version'
>>> from test.test.models import TestCase                           
>>> t = TestCase(teststring="hello")                                    
>>> t.save()                                                            
Traceback (most recent call last):                                      
  File "<console>", line 1, in ?                                        
  File "/home/andy/projects/django10474/code/django/django/db/models/base.py", line 330, in save                                                                       
    self.save_base(force_insert=force_insert, force_update=force_update)         
  File "/home/andy/projects/django10474/code/django/django/db/models/base.py", line 402, in save_base                                                                  
    result = manager._insert(values, return_id=update_pk)                        
  File "/home/andy/projects/django10474/code/django/django/db/models/manager.py", line 168, in _insert                                                                 
    return insert_query(self.model, values, **kwargs)                            
  File "/home/andy/projects/django10474/code/django/django/db/models/query.py", line 1054, in insert_query                                                             
    return query.execute_sql(return_id)                                          
  File "/home/andy/projects/django10474/code/django/django/db/models/sql/subqueries.py", line 320, in execute_sql                                                      
    cursor = super(InsertQuery, self).execute_sql(None)                          
  File "/home/andy/projects/django10474/code/django/django/db/models/sql/query.py", line 2098, in execute_sql
    cursor.execute(sql, params)
  File "/home/andy/projects/django10474/code/django/django/db/backends/util.py", line19, in execute
    return self.cursor.execute(sql, params)
ProgrammingError: syntax error at or near "RETURNING" at character 63

and

>>> from django.db import connection
>>> connection.features.can_return_id_from_insert
True
>>> from test.test.models import TestCase
>>> TestCase.objects.all()
[]
>>> connection._version
(8, 1)
>>> connection.features.can_return_id_from_insert
False
>>> t = TestCase(teststring="hello")
>>> t.save()
>>>

comment:5 by Malcolm Tredinnick, 16 years ago

#10509 has been opened to track the bigger problem here. For now, I'm disabling returning the ID values unless autocommit requires it (for PostgreSQL). That avoids the immediate problem, but it's only (deliberate) symptom patching.

comment:6 by Malcolm Tredinnick, 16 years ago

Resolution: fixed
Status: reopenedclosed

(In [10065]) More fixing of PostgreSQL < 8.2 problems with the psycopg2 backend.

Affects the postgresql_psycopg2 backend only. We now don't use the
"RETURNING" syntax in SQL INSERT statements unless it's required by the
autocommit behaviour. This fixes an edge-case that could cause crashes
with earlier PostgreSQL versions, but the broader problem remains to be
fixed (which is #10509).

Fixed #10467. Refs #10509.

comment:7 by Guillermo Gutiérrez, 16 years ago

Cc: guillermo.gutierrez@… added

comment:8 by Richard Davies <richard.davies@…>, 16 years ago

Cc: richard.davies@… added

comment:9 by Jacob, 13 years ago

milestone: 1.1 beta

Milestone 1.1 beta deleted

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