DatabaseWrapper.needs_rollback and transaction.rollback()

[Mark Young]

After a validationError occurs, why is the transaction considered dirty, blocking all db reads/writes? In this example: https://bitbucket.org/marky1991/django-test/raw/59c9ff89e4b12b4a831c36171139cb022735201b/test1.py , I don't really expect a TransactionManagementError at all, as the failure in question is a django model ValidationError, so no rollback should be needed, since the save never actually hits the db. At the minimum, however, I would expect a transaction.rollback() to resolve the issue, but it does not.

The traceback: https://dpaste.de/ooTy

Is there a django bug here or are my expectations not correct?

I have found this: https://code.djangoproject.com/ticket/26340 , where Aymeric Augustin said that he thought that transaction.rollback should indeed set DatabaseWrapper.needs_rollback to False, as I would expect.

Thanks

[Aymeric Augustin]

Hello Mark,

Indeed this is the bug described in https://code.djangoproject.com/ticket/26340.

It hasn’t been fixed yet because things become complicated with savepoints and because it isn’t obvious that it cannot introduce data corruption bugs.

Perhaps it would make sense to fix the bug at least for the whole transaction in rollback() and leave the ticket open for nested transactions with savepoints.

Best regards,

-- 

Aymeric.

--
You received this message because you are subscribed to the Google Groups "Django developers (Contributions to Django itself)" group.
To unsubscribe from this group and stop receiving emails from it, send an email to django-develop...@googlegroups.com.
To post to this group, send email to django-d...@googlegroups.com.
Visit this group at https://groups.google.com/group/django-developers.
To view this discussion on the web visit https://groups.google.com/d/msgid/django-developers/df66a920-2537-48dc-a0e2-4db2de5a54fd%40googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[Mark Young]

Re fixing transaction.rollback: What would be the preferred way to fix this? I haven't personally tested it (a coworker has and reports that it doesn't work), but BaseDatabaseWrapper.set_rollback requires that in_atomic_block be True when set_rollback is called, which is not the case in this example or in general, I think (I still don't entirely know what in_atomic_block is really supposed to mean). I could manually do get_connection(using).needs_rollback = False .

Once I know the preferred fix, I'll work on the patch.

One thing I still don't understand is why needs_rollback was set to True in the first place in my simple example. Reading the attribute name naively, we don't really need to emit a db rollback at all, as the bad save never hit the db at all, as it failed in the model validations. (In practice, this doesn't matter too much, as there's no real harm in making unneeded-as-far-as-the-db-is-concerned rollback calls. Asking more in hopes of understanding what's going on better)

Thanks for the help

[Aymeric Augustin]

> On 16 Nov 2016, at 16:35, Mark Young <mark...@gmail.com> wrote:
>
> Once I know the preferred fix, I'll work on the patch.

Unfortunately the hard part is to figure out what the preferred fix is :-/

It's likely to be a trivial one liner. Tests may be a bit more involved.

> One thing I still don't understand is why needs_rollback was set to True in the first place in my simple example.

You should take a look at what the Atomic class does. Database connections track in_atomic_block and needs_rollback to allow Atomic to perform its function.

--
Aymeric.

[Mark Young]

I created an issue for this https://code.djangoproject.com/ticket/27504 , and created a pull request as well: https://github.com/django/django/pull/7577.