Opened 15 years ago

Closed 13 years ago

#10476 closed Bug (wontfix)

Fix(?) model insert handling when autcommit=True for PostgreSQL <= 8.1

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

Description

Need to wrap some isolation level changes around inserts when using older PostgreSQL versions so that we can reliably return the last insert ID. This is necessary because we use that value to set the primary key attribute on the model instance and there's potentially no other way to get the information for some models (with no other unique constraints).

When this is fixed, settings.DATABASE_OPTIONS[autocommit] = True can be an option again for those server versions.

Attachments (2)

remove_unnecessary_test.diff (1.7 KB ) - added by Richard Davies <richard.davies@…> 15 years ago.
Remove unnecessary test - it works already!
remove_unnecessary_test_v2.diff (2.1 KB ) - added by Richard Davies <richard.davies@…> 15 years ago.
Wider clean up of tests in this area

Download all attachments as: .zip

Change History (15)

comment:1 by Malcolm Tredinnick, 15 years ago

Triage Stage: UnreviewedAccepted

comment:2 by Malcolm Tredinnick, 15 years ago

milestone: 1.1 beta1.1

This doesn't have to be a "beta" fix. It's a bug-fix on the original feature. If we get time to do it before 1.1, that's cool. If not, it's a documentation note to say it doesn't work on those versions yet. Somebody trying to squeeze maximum performance from PostgreSQL at this level should consider using the latest version anyway.

comment:3 by Malcolm Tredinnick, 15 years ago

milestone: 1.1

Can wait until later. Right now, an error is correctly reported. I want to fix it properly so that everybody doesn't have to pay all the version checking penalties we currently do for this sort of thing (some kind of DATABASE_OPTIONS specification of version number, probably). Anyway, something to add for 1.2.

comment:4 by Richard Davies <richard.davies@…>, 15 years ago

Cc: richard.davies@… added

by Richard Davies <richard.davies@…>, 15 years ago

Remove unnecessary test - it works already!

comment:5 by Richard Davies <richard.davies@…>, 15 years ago

Has patch: set
milestone: 1.1

It turns out that the current code already works on PostgreSQL <= 8.1. I'm attaching a patch which simply removes the check (and some documentation of the limitation). This means that we'll be able to get database-native autocommit into 1.1. for all versions of PostgreSQL - good!

The secret is that django/db/backends/postgresql/operations.py already defines last_insert_id() in terms of 'select currval' which is defined to be a session-local value (see http://www.postgresql.org/docs/7.3/static/functions-sequence.html or any other version).

When django/db/models/sql/subqueries.py defines InsertQuery.execute_sql(), the version which uses return last_insert_id() is already session-safe, even in database-native autocommit mode without a wrapping transaction. The version which uses the 'returning' syntax on Postgresql >= 8.2 is simply more efficient, not more session-safe.

by Richard Davies <richard.davies@…>, 15 years ago

Wider clean up of tests in this area

comment:6 by Richard Davies <richard.davies@…>, 15 years ago

The last patch leaves the 'if version >= 8.2' rather awkwardly inside an 'if uses_autocommit'.

I don't think that outer 'if' statement is necessary, even in light of #10467 comment 4 and #10509, so here's a version which removes it, and hence allows the 'returning' syntax to be used whenever version >= 8.2, regardless of uses_autocommit.

I think that [10065] was confused between two things:

  • setting 'can_return_id_from_insert=True' inside _cursor() rather than by default, which is necessary and the point of #10509
  • only doing this if uses_autocommit, which was overcautious and unnecessary

comment:7 by Russell Keith-Magee, 15 years ago

milestone: 1.1

Like Malcolm said, this can wait until v1.2. We won't ever release v1.1 if we keep adding tickets that aren't essential.

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

milestone: 1.2

Adding to the 1.2 milestone - hopefully we can get this completed patch committed this time.

comment:9 by Richard Davies <richard.davies@…>, 15 years ago

I have just checked that this patch still applies and works with the current SVN head.

I mentioned it in the 1.2 features discussion, and it was categorized under "Small, or Just Bugs" as a small non-controversial feature addition.

As such, I'd like to see it checked in before Django 1.2 beta, please.

comment:10 by James Bennett, 14 years ago

milestone: 1.21.3

Patch hasn't been updated since before 1.1, so I'd be wary of checking it in now for 1.2. Also, the comments above indicate this is more a feature addition, and unfortunately 1.2's feature-frozen.

comment:11 by Adam Nelson, 13 years ago

Looks like this won't make it into milestone:1.3 either?

comment:12 by Chris Beaven, 13 years ago

milestone: 1.3
Severity: Normal
Type: Bug

comment:13 by Adam Nelson, 13 years ago

Resolution: wontfix
Status: newclosed

PostgreSQL 8.1 is no longer supported by the Postgres community. I think this ticket should simply be closed at this point.

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