Cannot rollback to a savepoint explicitly after an IntegrityError when autocommit is disabled

[Aymeric Augustin]

I think the following code should work. Currently it crashes. (I haven't investigated the ramifications yet.)

>>> transaction.set_autocommit(False)
>>> sid = transaction.savepoint()
>>> User.objects.create(username='foobar')
<User: foobar>
>>> try:
...     User.objects.create(username='foobar')
... except IntegrityError:
...     print("duplicate")
... 
duplicate
>>> transaction.savepoint_rollback(sid)
Traceback (most recent call last):
  File "$VIRTUAL_ENV/lib/python3.5/site-packages/django/core/management/commands/shell.py", line 69, in handle
    self.run_shell(shell=options['interface'])
  File "$VIRTUAL_ENV/lib/python3.5/site-packages/django/core/management/commands/shell.py", line 61, in run_shell
    raise ImportError
ImportError

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "<console>", line 1, in <module>
  File "$VIRTUAL_ENV/lib/python3.5/site-packages/django/db/transaction.py", line 66, in savepoint_rollback
    get_connection(using).savepoint_rollback(sid)
  File "$VIRTUAL_ENV/lib/python3.5/site-packages/django/db/backends/base/base.py", line 328, in savepoint_rollback
    self._savepoint_rollback(sid)
  File "$VIRTUAL_ENV/lib/python3.5/site-packages/django/db/backends/base/base.py", line 288, in _savepoint_rollback
    cursor.execute(self.ops.savepoint_rollback_sql(sid))
  File "$VIRTUAL_ENV/lib/python3.5/site-packages/django/db/backends/utils.py", line 79, in execute
    return super(CursorDebugWrapper, self).execute(sql, params)
  File "$VIRTUAL_ENV/lib/python3.5/site-packages/django/db/backends/utils.py", line 59, in execute
    self.db.validate_no_broken_transaction()
  File "$VIRTUAL_ENV/lib/python3.5/site-packages/django/db/backends/base/base.py", line 429, in validate_no_broken_transaction
    "An error occurred in the current transaction. You can't "
django.db.transaction.TransactionManagementError: An error occurred in the current transaction. You can't execute queries until the end of the 'atomic' block.

[Aymeric Augustin]

I found this while following up on the django-developers discussion triggered by https://code.djangoproject.com/ticket/26323. This may be a minimal reproduction of the bug described in that ticket.

[Aymeric Augustin]

Further investigation shows that:

• You can forcibly mark the connection as usable with transaction.set_rollback(True). This is an advanced feature; it seems reasonable to use it in this advanced use case.

• If you rollback with transaction.rollback(), the same problem happens. I think that's what Tore originally reported. It seems safe to reset the needs_rollback flag in that case.

• In order to reset the needs_rollback flag safely in transaction.savepoint_rollback(), we'd have to know where the error occured, relatively to the savepoints stack.

That's where things get messy. The ORM uses transaction.atomic(savepoint=False) to guarantee the consistency of database operations that may require multiple queries while minimizing overhead.

The documentation recommends that the contents of any try / except IntegrityError: block be wrapped in with transaction.atomic(). If they aren't, there's no savepoint to rollback to and needs_rollback = True leaks.

Unfortunately, needs_rollback is managed by the Atomic class. The DatabaseWrapper class for the current database merely checks it to prevent illegal operations. DatabaseWrapped is ill-equipped to record when exceptions happen.

I'm inclined to:

• add self.needs_rollback = False in transaction.rollback()
• recommend that developers call transaction.set_rollback(False) after rolling back to a known-good, or at least hoped-good savepoint

I'm not rejecting the possibility of being smarter with savepoints. I'm just extremely reluctant to take risks with database transactions. Complicated code would be unacceptable here.

[Tore Lundqvist]

"add self.needs_rollback = False in transaction.rollback()" solved my problem in #26323

[Aymeric Augustin]

Thanks for testing!

There's a good chance I'll end up doing this or something equivalent. If you're patching Django, patch it that way :-)

[Tore Lundqvist]

Replying to aaugustin:

Thanks for testing!

There's a good chance I'll end up doing this or something equivalent. If you're patching Django, patch it that way :-)

I will, Thanks!

[Tim Graham]

A subset of the issue was fixed in 2742901ac210361bc2c4b662870d35a1be5a142c:

Fixed #27504 -- Allowed using the ORM after an error and rollback when autocommit is off.