Opened 9 years ago
Last modified 8 years ago
#26340 new Bug
Cannot rollback to a savepoint explicitly after an IntegrityError when autocommit is disabled
Reported by: | Aymeric Augustin | Owned by: | nobody |
---|---|---|---|
Component: | Database layer (models, ORM) | Version: | 1.9 |
Severity: | Normal | Keywords: | |
Cc: | Tore Lundqvist | Triage Stage: | Accepted |
Has patch: | no | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description
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.
Change History (7)
comment:2 by , 9 years ago
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 theneeds_rollback
flag in that case.
- In order to reset the
needs_rollback
flag safely intransaction.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
intransaction.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.
comment:3 by , 9 years ago
Triage Stage: | Unreviewed → Accepted |
---|
comment:4 by , 9 years ago
"add self.needs_rollback = False in transaction.rollback()" solved my problem in #26323
follow-up: 6 comment:5 by , 9 years ago
Cc: | added |
---|
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 :-)
comment:6 by , 9 years ago
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!
comment:7 by , 8 years ago
A subset of the issue was fixed in 2742901ac210361bc2c4b662870d35a1be5a142c:
Fixed #27504 -- Allowed using the ORM after an error and rollback when autocommit is off.
I found this while following up on the django-developers discussion triggered by #26323. This may be a minimal reproduction of the bug described in that ticket.