Opened 18 years ago

Closed 11 years ago

Last modified 11 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@… 18 years ago.
[patch] against ticket 2304
2304.diff (850 bytes ) - added by Thejaswi Puthraya 16 years ago.
added the patch wrt r7277
fixed_transaction_settings_variable_name.diff (2.1 KB ) - added by tjshewmake 14 years ago.
Proposed fix of manual transactions variable name.
2304.1.diff (4.9 KB ) - added by Ramiro Morales 13 years ago.

Download all attachments as: .zip

Change History (24)

by scott.benninghoff@…, 18 years ago

Attachment: transaction.py added

[patch] against ticket 2304

comment:1 by anonymous, 18 years ago

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

comment:2 by Gary Wilson <gary.wilson@…>, 17 years ago

Needs tests: set
Patch needs improvement: set
Triage Stage: UnreviewedAccepted

Please include the patch in diff format.

comment:3 by sam@…, 17 years ago

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...

by Thejaswi Puthraya, 16 years ago

Attachment: 2304.diff added

added the patch wrt r7277

comment:4 by Thejaswi Puthraya, 16 years ago

Patch needs improvement: unset

comment:5 by ben, 14 years ago

Keywords: transactions added

comment:6 by Carl Meyer, 14 years ago

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 by henrybaxter, 14 years ago

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

comment:8 by Karen Tracey, 14 years ago

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 by tjshewmake, 14 years ago

Owner: changed from nobody to tjshewmake
Status: newassigned

by tjshewmake, 14 years ago

Proposed fix of manual transactions variable name.

comment:10 by Łukasz Rekucki, 13 years ago

Type: defectBug

comment:11 by Łukasz Rekucki, 13 years ago

Severity: majorNormal

comment:12 by Ramiro Morales, 13 years ago

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

by Ramiro Morales, 13 years ago

Attachment: 2304.1.diff added

comment:13 by Ramiro Morales, 13 years ago

Needs documentation: unset
Needs tests: unset
Patch needs improvement: unset

comment:14 by Aymeric Augustin, 13 years ago

#16492 was a duplicate.

comment:15 by Łukasz Rekucki, 12 years ago

Patch needs improvement: set

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

comment:16 by Jeremy Dunck, 12 years ago

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 12 years ago by Jeremy Dunck (previous) (diff)

comment:17 by Karen Tracey, 12 years ago

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 by Aymeric Augustin, 11 years ago

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 by Aymeric Augustin <aymeric.augustin@…>, 11 years ago

Resolution: fixed
Status: assignedclosed

In a001f3c31e751e6ab1622ae7f65f7f8aeca2ef9a:

Fixed #2304 -- Documented TRANSACTIONS_MANAGED.

comment:20 by Aymeric Augustin <aymeric.augustin@…>, 11 years ago

In 44e56238d79890176a747181d32fbc53be157706:

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

Backport of a001f3c.

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