Opened 6 years ago

Closed 4 years ago

#10476 closed Bug (wontfix)

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

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

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@…> 6 years ago.
Remove unnecessary test - it works already!
remove_unnecessary_test_v2.diff (2.1 KB) - added by Richard Davies <richard.davies@…> 6 years ago.
Wider clean up of tests in this area

Download all attachments as: .zip

Change History (15)

comment:1 Changed 6 years ago by mtredinnick

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

comment:2 Changed 6 years ago by mtredinnick

  • milestone changed from 1.1 beta to 1.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 Changed 6 years ago by mtredinnick

  • milestone 1.1 deleted

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 Changed 6 years ago by Richard Davies <richard.davies@…>

  • Cc richard.davies@… added

Changed 6 years ago by Richard Davies <richard.davies@…>

Remove unnecessary test - it works already!

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

  • Has patch set
  • milestone set to 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.

Changed 6 years ago by Richard Davies <richard.davies@…>

Wider clean up of tests in this area

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

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

  • milestone 1.1 deleted

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 Changed 6 years ago by Richard Davies <richard.davies@…>

  • milestone set to 1.2

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

comment:9 Changed 6 years ago by Richard Davies <richard.davies@…>

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

  • milestone changed from 1.2 to 1.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 Changed 4 years ago by adamnelson

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

comment:12 Changed 4 years ago by SmileyChris

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

comment:13 Changed 4 years ago by adamnelson

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

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