Code

Opened 2 years ago

Closed 2 years ago

#17284 closed Bug (wontfix)

last_insert_id in django.db.backends.postgresql.operations.py

Reported by: EErlo Owned by: nobody
Component: Database layer (models, ORM) Version: 1.3
Severity: Normal Keywords: postgres 8.1.23,ORM save
Cc: Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

The new version of function last_insert_id uses the "new" postgres( >= version 8) method to get the sequence of a table (pg_get_serial_sequence), but in postgres 8.1.23(so far) this method isn't 100% good.
In some rare situations, postgres returns NULL, returning None to a saved new model pk, making lots of problems, even on Django Admin Interface, when you need the new-created pk to do something after call the save() method, as the Admin Interface Auth User does. I have made a treatment in django.db.backends.postgresql.operations.py, in the last_insert_id function:

    def last_insert_id(self, cursor, table_name, pk_name):
        # Use pg_get_serial_sequence to get the underlying sequence name
        # from the table name and column name (available since PostgreSQL 8)
        cursor.execute("SELECT CURRVAL(pg_get_serial_sequence('%s','%s'))" % (
            self.quote_name(table_name), pk_name))
        to_return = cursor.fetchone()[0]
        if not to_return or to_return == 'None':

            cursor.execute("select substr(column_default,10,position('::regclass' in column_default)-11) from information_schema.columns where table_name = '%s' and column_name = '%s'" % (table_name, pk_name))
            seq_name = cursor.fetchone()[0]
            cursor.execute("SELECT CURRVAL('%s')"%(seq_name))
            to_return = cursor.fetchone()[0]
        return to_return

I wait for your answer to know if it will be added to the django next release.

Attachments (0)

Change History (5)

comment:1 Changed 2 years ago by ramiro

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Accepted

Some questions so we can evaluate if we should undo that change of strategy for obtainig last interted row ID:

  • Why have you selected 1.3-rc1 as the Django version number in the Version ticket field?
  • What exact revision of Django are you using?
  • What version of psycopg2 are you using?
Version 0, edited 2 years ago by ramiro (next)

comment:2 Changed 2 years ago by akaariai

An assert that something is going to be returned would be a good thing.

Also, that if branch sequence lookup is slow. On my system the lookup takes 10ms, while the actual insert takes 0.5ms. This is probably nothing to worry about, since the if branch is just a fallback.

comment:3 Changed 2 years ago by ramiro

Wait, I had misread this report. This isn't about the changes we introduced for Django 1.4 (the ones that raise the minimum supported Postgres version to 8.2 and switches to always use "INSERT... RETURNING..." for this task) so please disregard ther first paragraph of my comment.

This is about using "SELECT CURRVAL(pg_get_serial_sequence('%s','%s'))" % (table_name, pk_name)

It is strange that this is the first report of this kind of behavior since we introduced usage of pg_get_serial_sequence() in r13328 back in June 2010.

I don't know if a change like the proposed would have a chance to get in a maintenance branch like 1.3.x especially considering that PostgreSQ 8.1 has been deprecated upstream since Nov 2010. Maybe you can remove the usage of pg_get_serial_sequence() reverting back to the previous hard-coded "SELECT CURRVAL('\"%s_%s_seq\"')" % (table_name, pk_name) in your local copy of Django.

It still would be great if you could post the information we asked for.

comment:4 Changed 2 years ago by eduardo.erlo@…

My Django Version: 1.3.1 final ..

django.VERSION = (1, 3, 1, 'final', 0)..

psycopg2.version = '2.2.2 (dt dec ext pq3)'

comment:5 Changed 2 years ago by ramiro

  • Resolution set to wontfix
  • Status changed from new to closed
  • Version changed from 1.3-rc1 to 1.3

wontfixig for the reasons exposed in comment:3: There is no chance to get a change like the proposed one in a hypothetical 1.3.2 release (these releases only containg fixes for release-blocking critical or security issues) and we've dropped support for Postgres older than 8.2 in 1.4.

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.