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.
