Opened 2 years ago

Closed 2 years ago

#22802 closed Cleanup/optimization (wontfix)

update Atomic class to make use of public transaction module-level API functions

Reported by: gwilson Owned by: gwilson
Component: Database layer (models, ORM) Version: 1.6
Severity: Normal Keywords:
Cc: Triage Stage: Unreviewed
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description (last modified by gwilson)

Currently, the class operates using the connection object directly; however, this makes it more difficult for libraries that need to customize transaction management, e.g. (see also

Ideally, I believe that wrapping the entire transaction API into a class and making it configurable would be even better, as it would allow for transaction management customization without monkey patching. That said, I propose we at least make use of the public API functions for the 1.7 release, as that makes the transactions interface backwards-compatible with versions before the Atomic class landed.

Change History (9)

comment:1 Changed 2 years ago by gwilson

  • Version changed from master to 1.6

comment:2 Changed 2 years ago by gwilson

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

comment:4 Changed 2 years ago by gwilson

  • Description modified (diff)

comment:5 Changed 2 years ago by aaugustin

The main goal of my work on transactions in Django 1.6 was to provide strong guarantees of atomicity.

I'm uncomfortable with monkey-patching that has a non-negligible chance of breaking these guarantees.

comment:6 Changed 2 years ago by aaugustin

That change also makes the implementation of Atomic a bit inconsistent.

It accesses some attributes directly on the connection object but performs an indirection through the module-level method for accessing methods.

comment:7 Changed 2 years ago by gwilson

Agree that the solution here is more pragmatic, rather than ideal. It does, in fact, make the Atomic class inconsistent in that it accesses both the module-level functions and the connection object directly. That said, it also makes the transaction code paths more consistent, while still ultimately making use of the methods on the connection object.

For a longer term solution, we should solidify a transaction API to use and make it possible customize the behavior/implementation without the need for third-party libraries to monkey-patch and/or recreate multiple connection class hierarchies.

Possible solutions may include:

  • Maintain the transactions module, using either the existing functions or a new class. The latter would be desirable as it would be easy to then expose that as a database setting.
  • Deprecate the functions in the transactions module, and instead make use of the DatabaseWrapper methods directly. The heart of the transaction management code lives in the DatabaseWrapper objects anyway; however, database settings currently only allow specifying the engine module, meaning that to swap out a customized DatabaseWrapper subclass would mean creating a custom engine package with a module, etc. importing the original classes for everything but the custom DatabaseWrapper.
  • Factoring out a base transaction management class that gets composed into the DatabaseWrapper instances. This is similar to the first item, except the idea here would be to actually contain the meat of the transaction management code instead of just a light wrapper around the connection object. This class, too, could then be something that could be swapped out with a database setting; however, they may still be complexities with class hierarchy should different database backends require custom transaction management classes.

That said, for a long-term solution, we should open a new ticket.

comment:8 Changed 2 years ago by timo

  • Has patch set
  • Patch needs improvement set

The tests don't pass on Jenkins (see link on pull request) and segfault for me locally.

comment:9 Changed 2 years ago by aaugustin

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

I checked the previous implementation. Indeed the old transaction decorators / context managers relied the module level functions. That was consistent because they were stateless (which was technically a bug).

The new atomic decorator / context manager is stateful. It stores its state on the connection object because, if several atomics are nested, they don't know about one another, they just share a connection. So the new implementation switched to using the connection everywhere for consistency.

I acknowledged that the DatabaseWrapper API is a bit scary, but transactions-related methods are well docstring'd. I don't belive they're much more difficult to patch than django.db.transactions. One just needs to inspect settings.DATABASES['...']['ENGINE'] to determine which classes need patching.

In addition, monkey-patching should really happen at that level. The docs say that connections and cursors do not follow PEP 249 for transaction handling, but connection.commit() or connection.rollback() work as expected and there's certainly some code that relies on this.

Considering how bad corruption at that level could get, I'm -1 on making changes to allow the johnny-cache authors to implement an unsafe solution.

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