Opened 4 years ago

Closed 4 years ago

Last modified 4 years ago

#24921 closed Bug (fixed)

No Database objects can be created with set_autocommit(False)

Reported by: Shabda Owned by: Aymeric Augustin
Component: Database layer (models, ORM) Version: 1.8
Severity: Normal Keywords:
Cc: Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

If you call transaction.set_autocommit(False),

Any further objects creation will fail, because

https://github.com/django/django/blob/04e8d890aec8e996d568565555164a27a6a76057/django/db/models/base.py#L699

This line tries an atomic with savepoint=False, which raises an exception in no autocommit mode.

Change History (19)

comment:1 Changed 4 years ago by Shabda

Owner: changed from nobody to Shabda
Status: newassigned

comment:2 Changed 4 years ago by Aymeric Augustin

Resolution: invalid
Status: assignedclosed

The ORM assumes that Django has control of the database connection, including control of its transactional behavior.

The docs make it pretty clear that you're on your own in that case:

Once you turn autocommit off, you get the default behavior of your database adapter, and Django won't help you.

Other than doing complicated things in cursor.execute() calls, I don't see a use case for disabling autocommit.

Can you clarify what you're trying to do?

comment:3 Changed 4 years ago by Shabda

Things I am doing

transaction.set_autocommit(False)
ModelClass.object.create(...)

I am not sure why this should fail. The PR https://github.com/django/django/pull/4755 has a failing minimal test case.

comment:4 in reply to:  3 Changed 4 years ago by Shabda

To clarify: If this an expected behaviour, then this warning "Once you turn autocommit off, you get the default behavior of your database adapter, and Django won't help you." is not strong enough and it should be calrified that usual ORM methods will fail.

As the docs stand now, it seems to imply that , you get the default transaction behavior, but can use the ORM. I think a doc or code fix is needed, so I am going to reopen the ticket.

comment:5 Changed 4 years ago by Shabda

Resolution: invalid
Status: closednew

comment:6 Changed 4 years ago by Aymeric Augustin

I'm not opposed to docs changes clarifying that you really shouldn't mess with autocommit.

I still don't know why you were trying to do this.

comment:7 Changed 4 years ago by Shabda

So here is the use case:

Lettuce, the BDD lib, doesn't wrap each scenario in transaction. I want to keep each scenario independent. lettuce provides two hooks before_each_scenario and after_each_scenario

http://lettuce.it/reference/terrain.html#before-each-scenario

In before_each_scenario I want to set_autocommit(False) and in after_each_scenario I want to do a transaction.rollback. I can't use the atomic decorator or context as the step functions are evaluated inside a third method.

I believe this can be achieved by using named savepoints, but based on the docs the first way looked more straightforward.

comment:8 Changed 4 years ago by Carl Meyer

I think you could probably actually use transaction.atomic, if lettuce gives you a place to stash some state between the hooks. You'd just call it, get an Atomic object back, and then manually call its __enter__ and __exit__ methods.

It seems to me that there is a valid use case here ("manual transaction control when the code structure, probably because of some other library, won't allow for a context manager or or decorator as the code idiom"), and I don't think the workaround I've suggested above is really adequate API for that use case.

Currently our documentation strongly suggests that the right approach for this case is set_autocommit(False) followed by commit or rollback. If the ORM is completely nonfunctional in that scenario (not just "doesn't handle transactions for you", but completely unusable), then I do think that's a problem that should be fixed.

I guess it could be that the right fix is a new higher-level API on a similar level to atomic, but that is usable as separate function calls instead of a context manager or decorator.

But it also seems to me that the ORM _should_ be able to basically function with autocommit off (even if it handles transactions unpredictably), and that this bug that prevents it from working does not look unsolvable. If it turns out that we can't fix it, then I definitely think we need _much_ stronger warnings in the docs (along the lines of "these APIs documented below are only usable with raw SQL, you cannot use the ORM at all"), and I think we probably also should suggest a reasonable alternative approach for non-context-manager transaction control with a usable ORM.

comment:9 Changed 4 years ago by Shai Berger

Component: UncategorizedDatabase layer (models, ORM)
Triage Stage: UnreviewedAccepted
Type: UncategorizedBug

Accepting because the original analysis is correct and @carljm's comments are spot on.

comment:10 Changed 4 years ago by Aymeric Augustin

Atomic is a context-manager / decorator in order to enforce proper matching between __enter__ and __exit__ calls. If the calls aren't balanced properly, data loss will happen, and it may be hard to diagnose. That's why I've historically rejected the idea of exposing this methods through public APIs.

Django uses the hack carljm suggests in its own test setUp / tearDown functions. See TestCase._enter_atomics and _rollback_atomics. That's exactly the OP's use case, so I suggest using that as a stop gap solution. Don't forget to call set_rollback(True) before exiting, since you want a rollback.

For the ORM to function with autocommit off, we should change the savepoint parameter of atomic to add a third-value, None (three-valued booleans, grrr), which would mean "automatic" and be implemented as follows:

    if self.savepoint is None:
        self.savepoint = not connection.in_atomic_block and not connection.get_autocommit()

In other words, it would mean "no savepoint, unless I'm the outermost atomic block in a connection that is already in a transaction, which means I can't use a plain transaction"

That's backwards compatible. It just makes Django cope with a situation where it would previously rais TransactionManagementError. (EDIT: this used to say "backwards incompatible" by mistake.)

Last edited 4 years ago by Aymeric Augustin (previous) (diff)

comment:11 Changed 4 years ago by Aymeric Augustin

Owner: changed from Shabda to Aymeric Augustin
Status: newassigned

comment:13 Changed 4 years ago by Aymeric Augustin

Indeed, some compatibility code added in Django 1.6 to ease the introduction of the new transaction management system and immediately deprecated was removed in Diango 1.8. That code used to make the scenario you're describing not crash.

comment:14 in reply to:  10 Changed 4 years ago by Carl Meyer

Replying to aaugustin:

That's backwards incompatible. It just makes Django cope with a situation where it would previously rais TransactionManagementError.

Correct me if I'm wrong, Aymeric, but I believe you meant to say "that's backwards compatible" here?

comment:15 Changed 4 years ago by Aymeric Augustin

Yes that's what I meant... I edited my comment.

comment:16 Changed 4 years ago by Aymeric Augustin

Has patch: set

After further analysis, the check that raises an exception in that case can simply be removed. See https://github.com/django/django/pull/5330 for details.

It won't work on SQLite for the usual reason, namely http://bugs.python.org/issue10740.

My original design for transaction was very stringent. Then I relaxed it, for instance I introduced savepoint=False. This is one more logical step in this direction.

I'm hesitating to backport this patch to 1.8 LTS. It could be considered a significant problem in the transaction functionality.

comment:17 Changed 4 years ago by Tim Graham

Triage Stage: AcceptedReady for checkin

comment:18 Changed 4 years ago by Aymeric Augustin <aymeric.augustin@…>

Resolution: fixed
Status: assignedclosed

In 91e9f1c:

Fixed #24921 -- set_autocommit(False) + ORM queries.

This commits lifts the restriction that the outermost atomic block must
be declared with savepoint=False. This restriction was overly cautious.

The logic that makes it safe not to create savepoints for inner blocks
also applies to the outermost block when autocommit is disabled and a
transaction is already active.

This makes it possible to use the ORM after set_autocommit(False).
Previously it didn't work because ORM write operations are protected
with atomic(savepoint=False).

comment:19 Changed 4 years ago by Aymeric Augustin <aymeric.augustin@…>

In 425c5e4:

[1.8.x] Fixed #24921 -- set_autocommit(False) + ORM queries.

This commits lifts the restriction that the outermost atomic block must
be declared with savepoint=False. This restriction was overly cautious.

The logic that makes it safe not to create savepoints for inner blocks
also applies to the outermost block when autocommit is disabled and a
transaction is already active.

This makes it possible to use the ORM after set_autocommit(False).
Previously it didn't work because ORM write operations are protected
with atomic(savepoint=False).

Backport of 91e9f1c from master

Note: See TracTickets for help on using tickets.
Back to Top