Add savepoint protection to Model.save()

[Richard Davies <richard.davies@…>]

[8314] introduced savepoints - these are a no-op for all databases except for Postgresql, on which they must be used to continue using a database connection after an IntegrityError.

IntegrityError can be raised in save() in several cases:

• because of a conflict on a unique field (example below)
• when using force_insert or force_update if this is not possible
• if there is a race between when save() decides whether to insert/update and when it actually does so

At present, the user should wrap calls to save() with savepoints if they are likely to fail for any of these reasons.

This patch changes save() to always wrap the call to save_base() in savepoints, meaning that all of the above cases leave a working database connection and the user does not need to wrap. It means that "user" code wrapping save() can be removed from several places in Django and its test suite. The complete test suite still passes on Postgresql.

Possibly the savepoints should move _inside_ save_base(), so that direct calls also benefit from them. Doing this naively leads to extra test suite failures, which I have not investigated.

On performance: we are now making many more savepoint calls, which could have performance implications. These calls are no-ops on non-Postgresql backends, so should have little impact there. On Postgresql, time to run the complete test suite was unchanged on my development machine. So, I conclude that there are no performance implications.

---

Example of IntegrityError from unique field conflict:

I have a simple model:

================
class Test(models.Model):
  data1 = models.IntegerField(unique=True)
  data2 = models.IntegerField()
================

and a view using it:

================
def render(request):
  a = Test()
  a.data1 = 1
  a.data2 = 10
  a.save()

  b = Test()
  b.data1 = 1
  b.data2 = 20
  try:
    b.save()
  except IntegrityError:
    # Expected since data1 not unique
    # savepoint protection needed here for postgresql today
    pass

  c = Test()
  c.data1 = 2
  c.data2 = 30
  # Today: c is saved by most database backends, but not by postgresql.
  #        savepoint protection would be needed above for postgresql.
  #
  # With patch: c is saved by all database backends.
  c.save()

  return HttpResponse('Test')

[Richard Davies <richard.davies@…>]

Add savepoint protection to Model.save()

[Malcolm Tredinnick]

Django should not be in the business of deciding how users wish to handle database-level errors. That is why saving doesn't do this right now. This is essentially a disguised version of what you were asking for in #8739, by the way. It is up to the caller to decide whether they want to roll things back entirely, or go back to an earlier savepoint they set or what.

At the moment, I don't think this change should go in, but I want to think about what we can do to possibly make some things easier. The current behaviour feels the most natural to me for now, since there definitely isn't a one-size fits all and the extra database interaction overhead isn't something we should impose on everybody.

[Richard Davies <richard.davies@…>]

I hadn't thought of it as the same as #8739, but you're right. However, in #8739 I thought it was limited to force_insert/force_update (advanced features), whereas now I realize it also happens in the more common case of normal save() and unique fields.

If it helps your decision, my thinking was as follows:

• The error "current transaction is aborted, commands ignored until end of transaction block" is Postgresql-specific, as far as I know
• Django savepoints are also Postgresql-specific, and [8314] says that they were added to enable rollback in case of this error
• The options are
  1. [current] Savepoints are the caller's responsibility. Ideally the caller should always use them, but they only actually do anything on Postgresql, so it's easy to write code (like the example in the ticket), which is official incorrect but actually works on all other databases.
  2. [proposed] Savepoints become the responsibility of save(), so behavior is consistent across database backends.

For what it's worth, I did the speed tests, and can't measure the overhead.

[Richard Davies <richard.davies@…>]

I've thought a bit more about this design decision (and improved the documentation at ​http://docs.djangoproject.com/en/dev/topics/db/transactions/ of the current state in [10791]). I'm recording my thoughts here for now, and will then bring this design decision to django-developers for Django 1.2.

There are two prongs to what I'd like to see:

(1) (As above): Add savepoint protection inside Model.save(), so there is no need for a user to protect calls to save() with unique fields, force_insert/force_update, etc.

(2) (Extra): Disaggregate savepoint support, to distinguish between when a backend simply supports savepoints (can_savepoint) and when a backend such as Postgresql also needs to have savepoint protection inside Model.save() (needs_savepoint_after_exception). At present uses_savepoints covers both meanings, which are logically distinct.

In addition, currently savepoints are a no-op when unsupported. After change 1, they will be used much less frequently in user code, and I would make this an error, since a user who asks for a savepoint rollback and does not get it should not continue blindly on!

[Aymeric Augustin]

Change UI/UX from NULL to False.

[Aymeric Augustin]

Change Easy pickings from NULL to False.

[Anssi Kääriäinen]

I think this ticket should be closed as wontfix. This is a big behavior change leading to backwards compatibility issues, and in addition _will_ have performance implications - save 10000 objects and you create 10000 subtransactions which _does_ cost you.