On Mon, Nov 22, 2010 at 6:03 AM, Shai Berger <s...@platonix.com> wrote: > Hi list, > > #9964 is about managed transactions not being committed under transaction > middleware (or transaction.commit_on_success decorator) after the database was > modified via raw SQL. The root cause of that is that, today, managed > transactions only become "dirty" when there is clear evidence of database > change -- that is, when a known-to-change-the-database operation (such as > saving a model) is performed. If this isn't the case (as in the case of raw > SQL), the user is required to call transaction.set_dirty() to get correct > behavior. > > More generally, transactions which stay "clean" are not really closed by the > transaction-management mechanism. If the transaction is part of a request, it > will be implicitly rolled back at the end of the request when the connection > is closed. If, under any circumstances, the connection stays open, so does the > transaction; this is rare, but wrong, and a user will have no warning except > for bugs creeping up. > > Yesterday I submitted a patch for this bug, which corrects this behavior by > stipulating that every transaction where an operation was performed in managed > mode should be considered dirty. In other words, there are no more "read-only > transactions". Every (managed) transaction that is opened must be closed. > > This is a backward-incompatible change: most users, who either use automatic > mode or some form of commit_on_success, should see no difference, but users > working with commit_manually will be forced to close read-only transactions > explicitly -- which they could get away with not doing before. > > I believe this leads to more correct code even in these cases, and all in all, > it trades a silent gotcha in one situation for a clear exception in another: > Under the current scheme, raw SQL execution can get rolled back silently if > set_dirty() isn't called; while this is documented, it is a gotcha. Under my > proposed fix, unclosed read-only transactions raise a > TransactionManagementError. I think this is an overall improvement. > > But I also think more people would have an opinion about it, than those who > follow http://code.djangoproject.com/ticket/9964.
Hi Shai, First off - thanks for the effort you've put into this issue, and the gentle persistence with which you've pursued it. The proposal you've got on the table is a lot closer to being trunk-ready than it has been historically. It's a lot simpler, and makes a lot more sense. However, I have two comments on the patch you've proposed. 1) Why introduce a full class wrapper with a customizable on_use() handler when there is only one meaningful course of action -- if managed, set dirty. Why not just embed that logic direct into Django's cursor (which is itself a wrapper anyway)? 2) The introduction of a backwards incompatibility doesn't fill me with joy. Code that works today must work tomorrow, however inconvenient it is to accommodate. The fact that Django's own test suite needs modifications in order to pass sends up a warning flag for me -- if our test suite needs modifications in non-obvious places (i.e., places that aren't directly related to testing transactions), then so will end-user code. However, that doesn't mean we're completely prevented from making change. We could make this change under two circumstances. Either: a) We need to find a way to gracefully introduce this change, raising a PendingDeprecationWarning in 1.3, a DeprecationWarning in 1.4, and finally making the change in 1.5; but if the user opts-in, the warning will be silenced. A setting isn't the right way to handle this, because settings apply across an entire project, but different reusable apps may be in different states of compliance. b) We need to declare that the current behavior to be a bug. We can break backwards compatibility to correct behavior that is clearly wrong. I haven't fully thought through the consequences here, but I think the combination of the footprint of affected cases, combined with the side effects of having dangling connections and transactions might almost be enough to convince me that can invoke the bug clause to break backwards compatibility. If you (or anyone else) has any opinions on this, I'd be interested in hearing them. To be clear -- as I understand it, we're talking about any code that: * is in a transaction managed block (i.e., between manually invoked enter/leave_transaction_management() calls, or within the scope of a commit_manually decorator/context manager), and * has a select or other manual cursor activity *after* the last commit/rollback, but *before* the end of the transaction management block, when there hasn't been a model save or other 'dirtying' behavior invoked after the last commit/rollback At present, such code is allowed to pass, and the transaction dangles. The proposed change would declare this situation a bug, requiring a manual commit/rollback at the end of any database activity. Yours, Russ Magee %-) -- You received this message because you are subscribed to the Google Groups "Django developers" group. To post to this group, send email to django-develop...@googlegroups.com. To unsubscribe from this group, send email to django-developers+unsubscr...@googlegroups.com. For more options, visit this group at http://groups.google.com/group/django-developers?hl=en.
