Opened 5 years ago

Closed 2 years ago

#14970 closed New feature (fixed)

Inconsistency in handling of managed/unmanaged transactions

Reported by: mwrobel Owned by: aaugustin
Component: Database layer (models, ORM) Version: 1.2
Severity: Normal Keywords:
Cc: 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 that there are some bugs in the way the 'managed' flag is handled in enter_transaction_management() and leave_transaction_management() in django/db/transaction.py.

  1. enter_transaction_management() has a 'managed' parameter. I guess it should set the new transaction in managed mode depending on the value of this parameter, but it takes the mode from the surrounding transaction or from the settings (as stated in a comment). Surprisingly, connection._enter_transaction_management() is called with the value of the 'managed' parameter instead of the mode that is set for the new transaction.
  1. leave_transaction_management() calls connection._leave_transaction_management() passing the mode of the current transaction (the transaction that is being left) as a parameter. From what I can see in django/db/backends/postgresql_psycopg2/base.py, it expects to get the mode of the transaction that will be active after leave_transaction_management() finishes.

Now I will describe the use case in which I encountered the problems: I use the psycopg2 backend and I want to have the connection in database-level autocommit mode by default. Sometimes I want to begin a managed transaction and after some operations commit or rollback it and return to the database-level autocommit mode.

Currently I have to enter the transaction as follows:

enter_transaction_management(managed=True)
managed(True)

and leave:

managed(False)
leave_transaction_management()

I think that the managed() calls should be redundant, but the first/second one is needed to circumvent the first/second problem I mentioned.

My proposal is to:

  1. change enter_transaction_management() to set mode depending on the parameter (and when parameter is not given, take the mode from the surrounding transaction or the settings).
  1. change leave_transaction_management() to pass the mode of the transaction that becomes active to connection._leave_transaction_management() (I don't know what was the intended semantics, but it is what pycopg2 backend seems to expect, and other backends don't override _leave_transaction_management()).

I see that the proposed changes can break backward compatibility, but I think that they don't change any documented behavior.

I can create patches that fix the problem, but I would like to hear from Django developers if the fix is acceptable before I write the patches.

Change History (6)

comment:1 Changed 5 years ago by russellm

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Accepted

Accepting, because what you describe makes sense.

However, backwards compatibility can't be ignored here. Transactions are one of those areas where very subtle changes in behavior have the potential to wreak havoc in live systems. I haven't sat down to fully work out the potential impact of what you're proposing, but before anything is committed, I'll need to see a very detailed analysis of any potential impact. The interaction with Postgres is particularly important, because the _enter_transaction_managmeent call on the backend exists for the benefit of Postgres.

comment:2 Changed 4 years ago by anonymous

  • Severity set to Normal
  • Type set to New feature

marking as new feature due to backward-compatibility concerns.

comment:3 Changed 4 years ago by aaugustin

  • UI/UX unset

Change UI/UX from NULL to False.

comment:4 Changed 4 years ago by aaugustin

  • Easy pickings unset

Change Easy pickings from NULL to False.

comment:5 Changed 3 years ago by aaugustin

  • Owner changed from nobody to aaugustin
  • Status changed from new to assigned

comment:6 Changed 2 years ago by Aymeric Augustin <aymeric.augustin@…>

  • Resolution set to fixed
  • Status changed from assigned to closed

In 14cddf51c5f001bb426ce7f7a83fdc52c8d8aee9:

Merged branch 'database-level-autocommit'.

Fixed #2227: atomic supports nesting.
Fixed #6623: commit_manually is deprecated and atomic doesn't suffer from this defect.
Fixed #8320: the problem wasn't identified, but the legacy transaction management is deprecated.
Fixed #10744: the problem wasn't identified, but the legacy transaction management is deprecated.
Fixed #10813: since autocommit is enabled, it isn't necessary to rollback after errors any more.
Fixed #13742: savepoints are now implemented for SQLite.
Fixed #13870: transaction management in long running processes isn't a problem any more, and it's documented.
Fixed #14970: while it digresses on transaction management, this ticket essentially asks for autocommit on PostgreSQL.
Fixed #15694: atomic supports nesting.
Fixed #17887: autocommit makes it impossible for a connection to stay "idle of transaction".

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