Opened 9 years ago

Closed 3 years ago

Last modified 3 years ago

#2304 closed Bug (fixed)

[patch] DISABLE_TRANSACTION_MANAGEMENT is not working as described in Doc's

Reported by: scott.benninghoff@… Owned by: tjshewmake
Component: Database layer (models, ORM) Version:
Severity: Normal Keywords: Unit of work, commit, rollback, transactions
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description

DISABLE_TRANSACTION_MANAGEMENT switch from the settings.py file doesn't do anything to the functions in transactions.py. I debugged the code yesterday and no matter what settings I had, .save() method for various models would either commit changes or raise the error about pending commits.

Created a patch against the function set_dirty() that evaluates the settings.DISABLE_TRANSACTION_MANAGEMENT for being True and if so, simply returns out of the function.

Attachments (4)

transaction.py (7.4 KB) - added by scott.benninghoff@… 9 years ago.
[patch] against ticket 2304
2304.diff (850 bytes) - added by thejaswi_puthraya 7 years ago.
added the patch wrt r7277
fixed_transaction_settings_variable_name.diff (2.1 KB) - added by tjshewmake 5 years ago.
Proposed fix of manual transactions variable name.
2304.1.diff (4.9 KB) - added by ramiro 4 years ago.

Download all attachments as: .zip

Change History (24)

Changed 9 years ago by scott.benninghoff@…

[patch] against ticket 2304

comment:1 Changed 9 years ago by anonymous

  • Summary changed from DISABLE_TRANSACTION_MANAGEMENT is not working as described in Doc's to [patch] DISABLE_TRANSACTION_MANAGEMENT is not working as described in Doc's

comment:2 Changed 8 years ago by Gary Wilson <gary.wilson@…>

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

Please include the patch in diff format.

comment:3 Changed 8 years ago by sam@…

Here is a unified diff of the attached transaction.py against django/db/transaction.py from SVN revision 3297 (which was current at 2006-07-07 23:39:43):

--- django/db/transaction.py	2007-07-20 19:25:54.000000000 +0100
+++ /dev/stdin	2007-07-20 19:26:43.430521786 +0100
@@ -82,7 +82,17 @@
     Sets a dirty flag for the current thread and code streak. This can be used
     to decide in a managed block of code to decide whether there are open
     changes waiting for commit.
-    """
+    ...
+    Patch: DISABLE_TRANSACTION_MANAGEMENT puts the developer in control
+    of the Unit of Work.  This will ignore the framework of the changes and allow
+    the developer to make the determination of when to commit and rollback as well
+    as assume the responsibility.  This code was not working previous to this change
+    """
+    if (settings.DISABLE_TRANSACTION_MANAGEMENT):
+        return
+    
+    # End of changes
+    
     thread_ident = thread.get_ident()
     if dirty.has_key(thread_ident):
         dirty[thread_ident] = True

Not much there...

Changed 7 years ago by thejaswi_puthraya

added the patch wrt r7277

comment:4 Changed 7 years ago by thejaswi_puthraya

  • Patch needs improvement unset

comment:5 Changed 6 years ago by ben

  • Keywords transactions added

comment:6 Changed 6 years ago by carljm

This patch has been sitting around for quite a while. Meanwhile, the documentation still claims DISABLE_TRANSACTION_MANAGEMENT works, when in fact it does nothing. If this patch isn't ready to go in, would it be worth getting a patch in temporarily to remove the documentation?

comment:7 Changed 6 years ago by henrybaxter

I agree, it's pretty annoying to go through the process of figuring out the setting does nothing.

comment:8 Changed 6 years ago by kmtracey

  • Patch needs improvement set

The existing patch is not the correct solution.

Per Jacob's response in this thread: http://groups.google.com/group/django-developers/browse_thread/thread/b633d56fdc7d4107/

the problem is not that the function isn't implemented, but rather that that docs and code do not agree on the name for the setting that controls the function. The code uses a setting named TRANSACTIONS_MANAGED (default value False in django/conf/global_settings.py), not a setting named DISABLE_TRANSACTION_MANAGEMENT. The fix, I believe, is a simple global replace in either the code or the doc of one for the other, depending on which one is chosen to keep.

DISABLE_TRANSACTION_MANAGEMENT is the one I would keep. It seems to more clearly convey that a setting of True means Django will not manage transactions. I found the actual default value for TRANSACTIONS_MANAGED to be surprising when I checked on it, because I expected it to reflect whether Django code was responsible for transaction management. In fact its apparently the reverse of what I expected based just on the name (I did not look at the code that uses it, just noted that there is in fact code that references its value, unlike DISABLE_TRANSACTION_MANAGEMENT).

Note neither name is documented in the full list of settings, so it might be good to add doc there as well when this is done.

comment:9 Changed 5 years ago by tjshewmake

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

Changed 5 years ago by tjshewmake

Proposed fix of manual transactions variable name.

comment:10 Changed 4 years ago by lrekucki

  • Type changed from defect to Bug

comment:11 Changed 4 years ago by lrekucki

  • Severity changed from major to Normal

comment:12 Changed 4 years ago by ramiro

  • Easy pickings unset
  • Needs documentation set
  • UI/UX unset

Changed 4 years ago by ramiro

comment:13 Changed 4 years ago by ramiro

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

comment:14 Changed 4 years ago by aaugustin

#16492 was a duplicate.

comment:15 Changed 3 years ago by lrekucki

  • Patch needs improvement set

Patch doesn't apply and it's probably too late to deprecate it in 1.4

comment:16 Changed 3 years ago by jdunck

This flag has never worked the way it's been documented, while TRANSACTIONS_MANAGED does - probably there is deployed code depending upon TRANSACTIONS_MANAGED, though it is undocumented.

I don't see the point of deprecating TRANSACTIONS_MANAGED while keeping the documented but useless DISABLE_TRANSACTION_MANAGEMENT.

I'd prefer to document TRANSACTIONS_MANAGED and remove DISABLE_TRANSACTION_MANAGEMENT from docs; it's not even a backwards-compatibility issue because people can still happily set the useless (but newly undocumented) flag. - Which is to say, I agree w/ kmtracey of 3 years ago. I'll provide an updated docs patch shortly.

Last edited 3 years ago by jdunck (previous) (diff)

comment:17 Changed 3 years ago by kmtracey

Well three years ago I preferred to keep the documented name and change the code to match, but at this point just correcting the doc to match the existing code seems better. There likely are people who have found (and thus use) the undocumented name and we shouldn't break them by fixing this.

comment:18 Changed 3 years ago by aaugustin

Since this setting is only useful in extreme circumstances, it doesn't make much sense to go through a deprecation path just to improve its name.

I agree that we should just document the current name. That's what I suggested in #16492 some time ago.

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

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

In a001f3c31e751e6ab1622ae7f65f7f8aeca2ef9a:

Fixed #2304 -- Documented TRANSACTIONS_MANAGED.

comment:20 Changed 3 years ago by Aymeric Augustin <aymeric.augustin@…>

In 44e56238d79890176a747181d32fbc53be157706:

[1.5.x] Fixed #2304 -- Documented TRANSACTIONS_MANAGED.

Backport of a001f3c.

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