RFC #9964 - fix "missing" db commits by forcing managed transactions to close
Shai Berger
#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.
Your comments are welcome,
Shai.
Thomas Guettler
I tested your patch with my applications. All my unittests pass.
Shai Berger 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.
> ...
--
Thomas Guettler, http://www.thomas-guettler.de/
E-Mail: guettli (*) thomas-guettler + de
Russell Keith-Magee
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 %-)
Christophe Pettus
On Nov 25, 2010, at 7:46 AM, Russell Keith-Magee wrote:
> 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.
I'd definitely argue that the current behavior is a bug. 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. Thus, real-world consequences arise from this, but I'd also argue it is a bug even just considering the behavior within Django.
More below...
> 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.
That's correct, with one caveat below.
My argument for this being a bug is that the behavior is indeterminate, and the most likely behavior is (in my view) surprising.
Right now, the transaction will stay open until the connection closes; this will probably cause a rollback at the database, but it's not a promise, simply the way the database happens to work. This is both relying on a pretty gritty level of implementation, and doing a rollback rather than a commit is, I'd argue, surprising for the most typical cases.
The caveat is that there is also a behavior change to code which uses @commit_on_success (or creates similar behavior), does not do anything to cause is_dirty to be set, modifies the database in some other way, and then *relies* on the typical rollback behavior. In the proposed fix, such transactions will commit instead. This is, however, pretty much the same as relying on uninitialized values in memory to be consistent, and I don't see any reason not to declare such code buggy.
I've gotten around this by having my own version of commit_on_success that always commits rather than checking the is_dirty flag, but it would be great to have this in the core.
--
-- Christophe Pettus
x...@thebuild.com
Shai Berger
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.
Shai Berger
This is just to let you know that, assuming interested parties have been
notified, I'm taking the discussion back to the bug where it belongs (and
patches can be added).
Thanks,
Shai.
Jacob Kaplan-Moss
<rus...@keith-magee.com> wrote:
> However, that doesn't mean we're completely prevented from making
> change. We could make this change under two circumstances. Either:
> 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.
Unless there are objections, I'm going to accept this approach and
check in a change based on Shai's latest -bugfix patch.
More info:
I spent some time with both of Shai's latest patches and all the
Django code I have access to [1] and I can't find a single case where
the "treat this as a bugfix" approach causes breakage in practice. In
a few places it *does* break work-arounds -- thinks like custom
@commit_on_* decorators -- but in all those cases those are people
working around the previous behavior.
I've also run Shai's -bugfix patch (modulo a few changes) in
production, and found it to reduce the load on the (PostgreSQL)
database without any ill side-effects.
So even though this is, technically, a backwards-incompatibility it's
(a) very minor and (b) carries a performance benefit especially on
heavily-loaded databases. This is enough for me to be willing to incur
the possibility of breakage.
Jacob
[1] All in all about 30 standalone apps and a 15 full-fledged Django sites.
Christophe Pettus
On Dec 21, 2010, at 11:39 AM, Jacob Kaplan-Moss wrote:
> Unless there are objections, I'm going to accept this approach and
> check in a change based on Shai's latest -bugfix patch.
FWIW, +1.
Russell Keith-Magee
+1 from me. As I said in a different thread, I gave Shai's patch a
quick review at the Sydney sprint, and I'm pretty much on board with
(b) being the right approach; I just wanted to give the patch a bit
more of a workout. It sounds like you've done that workout, so I'm
happy to see this committed.
Yours,
Russ Magee %-)