Opened 15 years ago

Closed 19 months ago

#11156 closed Cleanup/optimization (wontfix)

Unnecessary savepoints with Oracle

Reported by: Richard Davies <richard.davies@…> Owned by:
Component: Database layer (models, ORM) Version: dev
Severity: Normal Keywords: oracle savepoints
Cc: richard.davies@…, Matt Boersma, Erin Kelly Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: yes Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

Savepoints are implemented in the Postgresql and Oracle backends, and provide a useful features to users of both.

In addition, the framework itself wraps various calls in savepoints (e.g. inside django/db/models/query.py:get_or_create). This is to work around a Postgresql-specific oddity that the open transaction needs to be rolled back to a prior savepoint if it experiences a database exception.

This oddity is not present on Oracle, so the extra savepoints are not required. We probably need to split the current single backend flag uses_savepoints into two: can_savepoint and needs_savepoint_after_exception.

See http://groups.google.com/group/django-developers/browse_thread/thread/bca33ecf27ff5d63

Attachments (2)

11156.diff (7.0 KB ) - added by Erin Kelly 14 years ago.
12037.diff (13.1 KB ) - added by Richard Davies <richard.davies@…> 14 years ago.

Download all attachments as: .zip

Change History (18)

comment:1 by Matt Boersma, 15 years ago

Cc: Matt Boersma Erin Kelly added

comment:2 by Alex Gaynor, 15 years ago

Triage Stage: UnreviewedDesign decision needed

comment:3 by Richard Davies <richard.davies@…>, 14 years ago

milestone: 1.2

Adding to the 1.2 milestone, since it was accepted into the medium priority list (ORM-11).

comment:4 by Erin Kelly, 14 years ago

Keywords: oracle savepoints added

by Erin Kelly, 14 years ago

Attachment: 11156.diff added

comment:5 by Erin Kelly, 14 years ago

Has patch: set
Needs tests: set
Triage Stage: Design decision neededAccepted

I've uploaded a patch for this based on Richard's suggestion. Before I commit this, I'd like some feedback from somebody more familiar with the Postgresql isolation level code. Also need to figure out a reasonable test case.

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

Attachment: 12037.diff added

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

Ian - thanks for kicking this off.

I've updated your patch with:

a) Disabling needs_savepoint_on_exception on PostgreSQL before 8.0 where we can't do it anyway

b) A more conservative approach to the PostgreSQL isolation code - we can't use savepoints as currently written when we're in autocommit mode anyway, so just turn them off

c) Using the new only_if_needed flag in various test cases

d) Documentation

I'm not sure what a test case would reasonably be - the desired effect of this patch is that the test suite still runs on both PostgreSQL and Oracle, but is more efficient on Oracle than before - so I suggest that you just double check that spurious savepoints are no longer being generated on Oracle and then commit without a test case.

My only design concern is that I don't think only_if_needed is a great name. I'd be tempted to go with if_needs_savepoint_on_exception, which is rather long but at least explanatory!

comment:7 by James Bennett, 14 years ago

milestone: 1.21.3

So long as this isn't actually a bug (e.g., we're not ending up with an exception), this needs to be punted off 1.2.

comment:8 by Julien Phalip, 13 years ago

Severity: Normal
Type: Cleanup/optimization

comment:9 by Jacob, 12 years ago

milestone: 1.3

Milestone 1.3 deleted

comment:11 by Aymeric Augustin, 12 years ago

UI/UX: unset

Change UI/UX from NULL to False.

comment:12 by Aymeric Augustin, 12 years ago

Easy pickings: unset

Change Easy pickings from NULL to False.

comment:13 by Aymeric Augustin, 11 years ago

Owner: changed from nobody to Aymeric Augustin
Status: newassigned

comment:14 by Aymeric Augustin, 11 years ago

Owner: Aymeric Augustin removed
Status: assignednew

I would rather qualify this as an Oracle oddity: apparently, Oracle transactions don't guarantee integrity!

For example in the following scenario:

  • enter a transaction
  • execute a statement A that succeeds
  • execute a statement B that fails because of an integrity error; it's automatically rolled back
  • execute a statement C that succeeds
  • commit the transaction

you get only the changes resulting from A and C.

I thought transactions were supposed to guarantee that either all the changes were going to be committed or none of them.


I'd like to see a benchmark of how much the extra savepoints hurt before adding new flags and conditionals to the ORM.

This appears rather orthogonal to my recent work on transactions, so I'm going to deassign myself.

comment:15 by Erin Kelly, 11 years ago

It's a bit weird, I agree. The Oracle docs refer to this as "statement-level atomicity":
http://docs.oracle.com/cd/E11882_01/server.112/e10713/transact.htm

Transactions are guaranteed to be atomic (e.g. in case of system failure), but an unsuccessful statement is not considered part of the transaction.

comment:16 by Aymeric Augustin, 11 years ago

To implement this, Oracle must create some sort of "implicit savepoint" before every statement. Indeed, that means that it's never required to create a savepoint around a single statement on Oracle.

It could be much worse, see Response To Errors Within A Transaction in http://www.sqlite.org/lang_transaction.html :)

comment:17 by Simon Charette, 19 months ago

Resolution: wontfix
Status: newclosed

I'm going to wont-fix this one.

No conversation update for the past 9 years and still no performance benchmarks demonstrating that it is a change worth the extra complexity it introduces.

The transaction logic is already complex and Oracle has proven to be the hardest backend to support for the past years. I'm far from convinced than attempting to take advantage of this statement-level atomicity feature will bring more benefits than the maintenance burden it will incur.

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