Code

Opened 5 years ago

Closed 5 years ago

Last modified 3 years ago

#10467 closed (fixed)

Remove use of "RETURNING" in PostgreSQL backends by default

Reported by: mtredinnick Owned by: mtredinnick
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: UI/UX:

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.

Attachments (0)

Change History (9)

comment:1 Changed 5 years ago by ikelly

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

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

(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 Changed 5 years ago by mtredinnick

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

(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 Changed 5 years ago by offmessage

  • Resolution fixed deleted
  • Status changed from closed to reopened

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

#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 Changed 5 years ago by mtredinnick

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

(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 Changed 5 years ago by terrex

  • Cc guillermo.gutierrez@… added

comment:8 Changed 5 years ago by Richard Davies <richard.davies@…>

  • Cc richard.davies@… added

comment:9 Changed 3 years ago by jacob

  • milestone 1.1 beta deleted

Milestone 1.1 beta deleted

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.