[Django] #26340: Cannot rollback to a savepoint explicitly after an IntegrityError when autocommit is disabled

[Django]

#26340: Cannot rollback to a savepoint explicitly after an IntegrityError when
autocommit is disabled
-------------------------------------+-------------------------------------
Reporter: aaugustin | Owner: nobody
Type: Bug | Status: new
Component: Database | Version: 1.9
layer (models, ORM) |
Severity: Normal | Keywords:
Triage Stage: | Has patch: 0
Unreviewed |
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-------------------------------------+-------------------------------------
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.
}}}

--
Ticket URL: <https://code.djangoproject.com/ticket/26340>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

[Django]

#26340: Cannot rollback to a savepoint explicitly after an IntegrityError when
autocommit is disabled
-------------------------------------+-------------------------------------
Reporter: aaugustin | Owner: nobody
Type: Bug | Status: new

Component: Database layer | Version: 1.9
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by aaugustin):

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.

--
Ticket URL: <https://code.djangoproject.com/ticket/26340#comment:1>

[Django]

#26340: Cannot rollback to a savepoint explicitly after an IntegrityError when
autocommit is disabled
-------------------------------------+-------------------------------------
Reporter: aaugustin | Owner: nobody
Type: Bug | Status: new

Component: Database layer | Version: 1.9
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed

Has patch: 0 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by aaugustin):

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.

--
Ticket URL: <https://code.djangoproject.com/ticket/26340#comment:2>

[Django]

#26340: Cannot rollback to a savepoint explicitly after an IntegrityError when
autocommit is disabled
-------------------------------------+-------------------------------------
Reporter: aaugustin | Owner: nobody
Type: Bug | Status: new

Component: Database layer | Version: 1.9
(models, ORM) |
Severity: Normal | Resolution:

Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by timgraham):

* stage: Unreviewed => Accepted

--
Ticket URL: <https://code.djangoproject.com/ticket/26340#comment:3>

[Django]

#26340: Cannot rollback to a savepoint explicitly after an IntegrityError when
autocommit is disabled
-------------------------------------+-------------------------------------
Reporter: aaugustin | Owner: nobody
Type: Bug | Status: new

Component: Database layer | Version: 1.9
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted

Has patch: 0 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by tltx):

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

--
Ticket URL: <https://code.djangoproject.com/ticket/26340#comment:4>

[Django]

#26340: Cannot rollback to a savepoint explicitly after an IntegrityError when
autocommit is disabled
-------------------------------------+-------------------------------------
Reporter: aaugustin | Owner: nobody
Type: Bug | Status: new

Component: Database layer | Version: 1.9
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted

Has patch: 0 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by aaugustin):

* cc: tltx (added)

Comment:

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 :-)

--
Ticket URL: <https://code.djangoproject.com/ticket/26340#comment:5>

[Django]

#26340: Cannot rollback to a savepoint explicitly after an IntegrityError when
autocommit is disabled
-------------------------------------+-------------------------------------
Reporter: aaugustin | Owner: nobody
Type: Bug | Status: new

Component: Database layer | Version: 1.9
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted

Has patch: 0 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by tltx):

Replying to [comment:5 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!

--
Ticket URL: <https://code.djangoproject.com/ticket/26340#comment:6>

[Django]

#26340: Cannot rollback to a savepoint explicitly after an IntegrityError when
autocommit is disabled
-------------------------------------+-------------------------------------

Reporter: Aymeric Augustin | Owner: nobody
Type: Bug | Status: new

Component: Database layer | Version: 1.9
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted

Has patch: 0 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by 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.

--
Ticket URL: <https://code.djangoproject.com/ticket/26340#comment:7>