atch] transaction.commit()/rollback() should be aware of nested calls to transaction.managed(True)

patch adding nested transactions to @commit_on_success

I had the same Problem: an upper layer calling a lower layer and both used transactions. The problem is, that the databases I'm aware of do not support nested transactions. I played a little with Postgres 8.x SAVEPOINTs but then opted for a simple counting solution mderk. Attatched patch implements that for @commit_on_success. Unfortunately I included some other cruft in the patch. Only look at the patch to django/db/transaction.py or get a fixed patch at http://c0re.23.nu/c0de/misc/django-20060703-nested_transactions.diff

At least with @commit_on_success I would consider the current (pre-patch) behaviour as broken. To use @commit_on_success you must guarantee that you never directly or indirectly calll an other function decorated by @commit_on_success - something you can not do in any reasonable complex application.

Nope, the patch doesn't resolve the issue, because not the decorators should be patched, but commit() itself, because it could be called from a nested commit_manually-decorated function or whatever. I guess the commit should look at state[thread_ident][-2] (not an elegant way, but...), something like this:

def commit():
    """
    Does the commit itself and resets the dirty flag, only when the transaction is not nested.
    """
    thread_ident = thread.get_ident()
    top = state.get(thread_ident, None)
    if top and len(top) > 1 and top[-2]:
        return
    connection._commit()
    set_clean()

The behaviuor is similar to commit_unless_managed(), but it looks to the previous 'level', if possible. Of course, the this commit() should be in enter_transaction_management/leave_transaction_management block to work as expected.

I agree, that the current behaviour is broken, so classify this as a defect.

I don't agree with you here. if I write transaction.commit() or rollback() I want a commit or a rollback to happen. No magic wanted. While when I use the higher level @commit_on_success I yust want this funktion to be transaction protected.

Making commit not issue COMMIT to the database seems thereforto me a bad design choice. But what's about commit_unless_nested() or rollback_unless_nested()?

But what's about commit_unless_nested() or rollback_unless_nested()?

The problem here is that You can call a @commit_manually-decorated function, that insist on doing commit/rollback in the function body (it will raise an exception if you don't), or any function that omits the transaction decorator syntax, but calls a commit/rollback explicitly (e.g. when only custom SQL is used in the function). So, it means that you can not safely call any code with emplicit or explicit commit/rollback in it within another transaction block. Another reason is BC - you should replace all the manual commits/rollbacks to commit_unless_nested() or rollback_unless_nested() all over the place, and to decide where it is appropriate or not. And I don't think that You really do want a commit or a rollback to happen if You are in the nested transaction block - It would definitely break integrity of the application. The maximum the transaction.commit() shoild do in this case is to make a savepoint if available. But if the surrounding block wants to rollback, it should be able to do so (it won't be able if its transaction was already commited in the nested block).

I guess "assigned" means accepted

I got the same problem, what status about this issue?

Thanks.

No one care about this issue?

I do care, but my proposal would affect BC, and nobody came with a BC-compatible alternative proposal yet. Max

It seems like this issue wouldn't be accepted into 1.0 release,

http://groups.google.com/group/django-developers/browse_thread/thread/5ce124e7526dad

Maybe we should do it by ourself with our own decorator?

Django seems wouldn't do anything that will break BC-compatible.

 Introduction
------------

I stumbled across a problem with nested transactions and created a
general fix before I found |this ticket|.  At its core, the solution I
developed is to change how ``manage`` interacts with
``enter_transaction_management``.  Currently, when
``enter_transaction_management`` is called, the previous state is
pushed onto the stack of states.  The procedure ``manage`` can change
this state at any time.  This solution forces the state of a
transactional context to remain managed if the outer context was
managed.

.. |this ticket| trac:: 2227

Basic Solution
--------------

* Make each item on the state stack a 2-tuple of ``(initialized,
  managed)``.  The ``initialized`` flag is used to make sure
  ``manage`` is only called once per block.  The ``managed`` flag is
  used by the ``is_managed`` procedure as before.

* Only allow ``manage`` to be called once per transaction block.  This
  prevents flip-flopping between a managed and unmanaged state and
  relieves ``manage`` of the responsibility of deciding whether to
  commit dirty transactions or not.

* Once a transaction block is set to ``managed = True``, nested
  transaction blocks are forced to be ``managed = True`` even if
  ``manage(False)`` is called.  This change prevents nested
  "autocommit" blocks from committing changes in an outer, manual
  transaction.

* ``connection._commit`` and ``connection._rollback`` are only called
  for top-level transactions or when there is not a current
  transaction.  ``commit`` and ``rollback`` operations are no-ops for
  nested transactions.  This is correct for ``commit``, but slightly
  unfortunate for ``rollback`` because a nested transaction block
  cannot roll itself back.  Such a partial rollback would require
  first-class support for save-points.

Other Changes
-------------

* ``connection._commit`` and ``connection._rollback`` are only called
  from ``commit`` and ``rollback`` respectively.

* The ``state`` and ``dirty`` dictionaries are replaced with a
  ``threading.local`` object (see Compatibility_).

* The patch from |ticket 7241| has been incorporated.

* |ticket 2304| could be fixed by adding a check for
   ``DISABLE_TRANSACTION_MANAGEMENT`` to
   ``TransactionState.really_transact``.

.. |ticket 2304| trac:: 2304
.. |ticket 7241| trac:: 7241

Compatibility
-------------

With this patch applied, Django's ``runtests.py`` passes all tests.
It is backward-compatible with un-nested transactions and database
operations lacking a transactional context (Django, by default,
autocommits).

It may not backward-compatible with:

1. outer transactions that roll back after an inner transaction has
   been committed
   
2. outer transactions that commit after an inner transaction has
   rolled back

A comments at the top of ``transaction.py`` implies that compatibility
with Python 2.3 is a goal.  If this is still the case, an
implementation of ``threading.local`` will need to be added to
``django.utils`` since ``threading.local`` was introduced in Python
2.4.  Such an implementation could use a technique similar to the
current technique (i.e. a dictionary indexed by thread-id).
Alternatively, the ``TransactionState`` class could be refactored to
not use ``threading.local`` as long as Python 2.3 is still supported.

Attachments
-----------

A patch is attached as well as a complete copy of the updated
``transaction.py``.  I've made some test cases.  The tests can be run
by unpacking the attached zip file and running ``./manage.py test``.
I've also attached ``test.py`` which has the relevant parts of the zip
file for easier browsing.

sorry, I'm new to trac. I didn't mean to change the issue name.

maybe it should be better to allow a parameter to the decorator that will say if one wants the method to always use a new transaction for it or work inside the above transaction if one exists (like in .NET). There are cases that you might want one behavior and cases where you might want the other.

if someone thinks where I might want to use the first case the first example that comes to mind is when i want to log in both a file and the db whenever an error appears. This works at the moment but if the changes were to be done it would no longer work like this. can someone modify the decorator to take one parameter and decide from there how to behave?

This seems like a step in the right direction, but not being allowed to call rollback() from within a nested transaction seems like a killer. What are you supposed to do? You'd have to remember that you aren't the top-level transaction level, raise an exception instead of rolling back, and expect whoever is the top-level transaction to rollback for you once the exception propagates out.

That's very brittle. If someone accidentally eats the exception before it gets there, then the transaction might be committed, which is the worst possible failure mode.

Instead, I'd suggest:

• when rollback is called when nested, mark the transaction failed;
• if commit is called when not nested and the transaction was marked failed, raise an exception, because rollback should have been called instead.

This allows calling rollback() to cancel the transaction, and guarantees that the transaction will never be committed, which I think is a critical invariant.

Then, this would work (easily dropped into a decorator):

def foo():
    transaction.enter_transaction_management()
    try:
        # code

        transaction.set_dirty()
        transaction.commit()
    except:
        transaction.rollback()
        raise
    finally:
        transaction.leave_transaction_management()

def fum():
    transaction.enter_transaction_management()
    try:
        foo()
    except:
        transaction.rollback()
        raise
    finally:
        transaction.leave_transaction_management()

I didn't notice that there was already infrastructure for savepoints, which makes it very easy to do this all correctly. I don't know why that's not being used, so I guess I'll just roll my own wrappers to handle this for now.