id	summary	reporter	owner	description	type	status	component	version	severity	resolution	keywords	cc	stage	has_patch	needs_docs	needs_tests	needs_better_patch	easy	ui_ux
20571	Using savepoints within transaction.atomic() can result in the entire transaction being incorrectly and silently rolled back	Chris Lamb	Aymeric Augustin	"Using savepoints within transaction.atomic() can result in the entire transaction being incorrectly and silently rolled back. Here is a slightly contrived example:

{{{#!python
from django.db import transaction, DatabaseError

with transaction.atomic():
    user = User.objects.all()[0]
    user.last_login = datetime.datetime.utcnow() # any change to demonstrate problem
    user.save()

    sid = transaction.savepoint()
    try:
        # Will always fail
        User.objects.create(pk=user.pk)
        transaction.savepoint_commit(sid)
    except DatabaseError:
        transaction.savepoint_rollback(sid)

# outside of atomic() context - user.last_login change has not been committed!
}}}

What happens is the .create() call fails and marks the outermost atomic block as requiring a rollback, so even though we call savepoint_rollback (which does exactly right thing), once we leave the outer transaction.atomic(), the entire transaction is rolled back. In the example above, this means that the change to last_login is not persisted in the database. Rather insiduously, it is done entirely silently. :)

(The outer transaction.atomic() seems a little contrived but note that it is equivalent to the wrapper added by ATOMIC_REQUESTS.)

I'm not entirely sure what the solution is; the transaction.atomic(savepoint=False) call within ModelBase.save_base simply does not (and cannot) know that some other code will manually handle the savepoint rollback and thus no choice but to mark the entire transaction as borked. The only way it could know is if .create and .save took yet another kwarg, but this seems bizarre and would not be at all obvious.

Maybe manual savepoints should be become a private API after all (contradicting the current note in the documentation). Alternatively, it could be clarified that ""manual"" savepoint management they should not be used in conjunction with atomic() blocks (and thus ATOMIC_REQUESTS). The behaviour above is certainly not obvious at all.

(Hm, update_or_create uses savepoints manually, I hope that isn't breaking installations?)"	Bug	closed	Database layer (models, ORM)	1.6-alpha-1	Release blocker	fixed		Chris Lamb	Accepted	0	0	0	0	0	0