Hi Russell, Christophe, Thomas and list, Thanks for your kind words and comments. I'm very encouraged by them.
On Thursday 25 November 2010, Russell Keith-Magee wrote: > > 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)? > As far as I can see, while the connection wrappers all derive from BaseDatabaseWrapper, the cursor wrappers do not have a non-trivial common ancestor: Sqlite's derives from the dbapi Cursor object, MySql's derives from object. The DebugCursorWrapper, as its name implies, is only used in debug. So unless I am missing something, a wrapper really is necessary. The modifiable "on_use" can probably be removed in favor of embedding the code in the wrapper (the patch started out a little more complicated than it is now, and that's one simplification I missed). I'll fix that together with other issues which might come up. > 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. > This is true. I think the problem is mitigated by the fact that code that needs to be modified announces it with an exception, but of course that only goes so far. > 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 > [deprecation warnings]. > I suppose we could do this by duplicating the dirty flag -- essentially, have "new dirty" (whenever the cursor is touched) and "old dirty" (current dirty logic); commit_on_success would close the transaction if any of them are set; if we leave a managed block with old-dirty set it's an error; and if we leave with only new-dirty set it's a warning. I don't like this solution, because I think it complicates things more than necessary, but if that's what it will take, it can be done. But first, let me try to push in the other direction: > b) We need to declare that the current behavior to be a bug. [...] If you > (or anyone else) has any opinions on this, I'd be interested in hearing > them. > Yes, I think current behavior is a bug. It leaves transactions dangling when the user explicitly asked for them to be closed. It can have manifestations other than #9964. One is #6669: A save operation fails under commit_on_success, and the exception is caught outside of the decorator, but because nothing has been saved yet, the transaction is not rolled back, and at least on Postgres, operations (incl. select) in the following code fail. Granted, this could be fixed by wrapping every piece of code which calls commit_unless_managed with a try: ... except: rollback_unless_managed() raise but as long as we all agree that our ultimate goal looks like the patch, that is yet another uncleanly complication which can be avoided. Another case -- theoretical as far as I know -- is opened up by the introduction of multiple databases. Of course, the presentation here is completely contrived. Assume alias1 and alias2 refer to the same database; the transaction isolation level is Serializable; and MyModel has a name field. Now, @commit_on_success(alias1) def f(name): MyModel.create(name=name, using=alias1) @commit_on_success(alias2) def g(name): m = MyModel.get(name=name, using=alias2) def h(names): for name in names: f(name) g(name) With current behavior, If h() is given more than one name, the second invocation of g() should raise a DoesNotExist exception. Since, as I said, current behavior does not always do what the user asked, I'm guessing more such cases, perhaps less far-fetched, can be given. > 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 > Just to clarify further, this includes the case where there has been no commit or rollback at all (but there has been a select or manual activity within the transaction management block). > 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. > On Thursday 25 November 2010, Christophe Pettus wrote: > > In the case (not the least bit unusual) of Django applications connecting > to PostgreSQL through a connection pooler (usually pg_bouncer), it's > pretty common to see Idle in Transaction connections start piling up > because of this problem. Actually, I'd argue that this is a pg_bouncer bug. In normal use, Django closes the connection at the end of every request; if a connection pooler puts a connection back in its pool with a transaction pending, Django is not to blame. Thanks, Shai. -- 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.