Thread: autocommit and Django
Hi, the connection.autocommit feature has created the problem shown here: https://code.djangoproject.com/ticket/16250 I've taken a look and they have a set_autocommit method <https://code.djangoproject.com/browser/django/trunk/django/db/backends/creation.py#L347> implemented in a painful way: instead of being driver-specific they just invoke some random method on the connection, and the thing is compounds with the fact they *don't even know* there is a transaction somehow already open. As per discussion <http://archives.postgresql.org/psycopg/2011-05/msg00033.php>, psycopg's set_session gives an error if invoked in a transaction. Now, I would love to argue that Django's set_autocommit is written with the wrong anatomical part, the bug is theirs and it's all their problem. However, knowing the painful process they use to fix a bug I wouldn't be surprised it would take months (see how they handled the idle in transaction mess) and this would only be a problem for django, postgres and psycopg users, as 2.4.2 is the version installed by default by easy_install and friends. So I'm positive to change the semantics of set_session/autocommit and issue an implicit rollback if in transaction instead of raising an exception, as set_isolation_level does. Thoughts? -- Daniele
On Tue, Jun 14, 2011 at 10:42:15AM +0100, Daniele Varrazzo wrote: > the connection.autocommit feature has created the problem shown here: > > https://code.djangoproject.com/ticket/16250 > > I've taken a look and they have a set_autocommit method > <https://code.djangoproject.com/browser/django/trunk/django/db/backends/creation.py#L347> > implemented in a painful way: instead of being driver-specific they > just invoke some random method on the connection, and the thing is > compounds with the fact they *don't even know* there is a transaction > somehow already open. As per discussion > <http://archives.postgresql.org/psycopg/2011-05/msg00033.php>, > psycopg's set_session gives an error if invoked in a transaction. > > Now, I would love to argue that Django's set_autocommit is written > with the wrong anatomical part, the bug is theirs and it's all their > problem. However, knowing the painful process they use to fix a bug I > wouldn't be surprised it would take months (see how they handled the > idle in transaction mess) and this would only be a problem for django, > postgres and psycopg users, as 2.4.2 is the version installed by > default by easy_install and friends. So I'm positive to change the > semantics of set_session/autocommit and issue an implicit rollback if > in transaction instead of raising an exception, as set_isolation_level > does. IIRC it was argued by a PG developer that .autocommit = True should *not* issue an implicit rollback. If that's so: Please don't make psycopg2 do the "wrong" thing so others can "fix" their problems more easily ("fix": because they wouldn't really fix it if psycopg2 allowed the "wrong" thing to happen). Instead, add a state variable somewhere: .changing_autocommit_causes_transaction_rollback defaulting to False (the current, "correct" behaviour) which Django can set to True if the insist on keeping to do the "wrong" thing. I am aware that this introduces some penalty even for other, well-behaved code because setting autocommit will now require checking the state of said variable. Or else have another argument to .set_transaction(): def set_transaction(autocommit=True/False/None, autocommit_implicit_rollback=False/True): where autocommit_implicit_rollback defaults to False. Karsten -- GPG key ID E4071346 @ gpg-keyserver.de E167 67FD A291 2BEA 73BD 4537 78B9 A9F9 E407 1346
On 14/06/11 11:42, Daniele Varrazzo wrote: > the connection.autocommit feature has created the problem shown here: > > https://code.djangoproject.com/ticket/16250 > > I've taken a look and they have a set_autocommit method > <https://code.djangoproject.com/browser/django/trunk/django/db/backends/creation.py#L347> > implemented in a painful way: instead of being driver-specific they > just invoke some random method on the connection, and the thing is > compounds with the fact they *don't even know* there is a transaction > somehow already open. As per discussion > <http://archives.postgresql.org/psycopg/2011-05/msg00033.php>, > psycopg's set_session gives an error if invoked in a transaction. > > Now, I would love to argue that Django's set_autocommit is written > with the wrong anatomical part, the bug is theirs and it's all their > problem. However, knowing the painful process they use to fix a bug I > wouldn't be surprised it would take months (see how they handled the > idle in transaction mess) and this would only be a problem for django, > postgres and psycopg users, as 2.4.2 is the version installed by > default by easy_install and friends. So I'm positive to change the > semantics of set_session/autocommit and issue an implicit rollback if > in transaction instead of raising an exception, as set_isolation_level > does. > > Thoughts? Yes, it is wrong. set_isolation_level() was wrong from the start but we had to keep it that way to avoid breaking compatibility. The discussion you linked still holds so I won't go back to sending a magic ROLLBACK. If we don't want to wait for the #@§%£$! person that wrote the Django code to fix it we can just make .autocommit a real attribute and set the session autocommit mode when it is assigned True/False. -- Federico Di Gregorio federico.digregorio@dndg.it Studio Associato Di Nunzio e Di Gregorio http://dndg.it Ma nostro di chi? Cosa abbiamo in comune io e te? -- Md
On 14/06/11 12:02, Federico Di Gregorio wrote: >> Thoughts? > Yes, it is wrong. set_isolation_level() was wrong from the start but we > had to keep it that way to avoid breaking compatibility. The discussion > you linked still holds so I won't go back to sending a magic ROLLBACK. > > If we don't want to wait for the #@§%£$! person that wrote the Django > code to fix it we can just make .autocommit a real attribute and set the > session autocommit mode when it is assigned True/False. Ok, I am completely #@§%£$! like a Django developer today (in fact I am working on a project using it...). Rewind. Yes, it is wrong. [Rest of paragraph above...] I think there is no way to fix this on our side. Even if we just ad an attribute, like Karsten suggested Django code should still check it but if they change the code they can just fix the problem without the need to add an attribute. federico -- Federico Di Gregorio federico.digregorio@dndg.it Studio Associato Di Nunzio e Di Gregorio http://dndg.it The devil speaks truth much oftener than he's deemed. He has an ignorant audience. -- Byron (suggested by Alice Fontana)
On Tue, Jun 14, 2011 at 12:09:57PM +0200, Federico Di Gregorio wrote: > I think there is no way to fix this on our side. Even if we just add an > attribute, like Karsten suggested Django code should still check it but > if they change the code they can just fix the problem without the need > to add an attribute. Aaah, the sweet smell of sanity, reason, and logic :-) Karsten -- GPG key ID E4071346 @ gpg-keyserver.de E167 67FD A291 2BEA 73BD 4537 78B9 A9F9 E407 1346
On Tue, Jun 14, 2011 at 11:02 AM, Federico Di Gregorio <federico.digregorio@dndg.it> wrote: > If we don't want to wait for the #@§%£$! person that wrote the Django > code to fix it we can just make .autocommit a real attribute and set the > session autocommit mode when it is assigned True/False. This wouldn't work as if the exception is raised it means that they have already started a transaction, probably by setting time_zone to chicago or something like that. The "create database" they mean to run outside the transaction is then doomed to fail. -- Daniele
On Tue, 14 Jun 2011 12:02:26 +0200, Federico Di Gregorio wrote: > If we don't want to wait for the #@§%£$! person that wrote the Django > code to fix it… Hi - that's me. Could you do me a favor and in the future just drop me a line instead of insulting the entire Django community? Anyway, though: how would you suggest we fix this? Does the patch (https://code.djangoproject.com/attachment/ticket/16250/16250-2.diff) look reasonable? Jacob
On 22/06/11 22:00, Jacob Kaplan-Moss wrote: > On Tue, 14 Jun 2011 12:02:26 +0200, Federico Di Gregorio wrote: >> If we don't want to wait for the #@§%£$! person that wrote the Django >> code to fix it… > > Hi - that's me. Could you do me a favor and in the future just drop me > a line instead of insulting the entire Django community? Well, sorry. One should not suppose somebody else having read Asterix comics and be able to smile while looking at "#@§%£$!". Anyway, nice to meet you! > Anyway, though: how would you suggest we fix this? Does the patch > (https://code.djangoproject.com/attachment/ticket/16250/16250-2.diff) > look reasonable? It is just half of the fix. You can't assume that switching to autocommit will do an implicit ROLLBACK for you. This worked until now by chance (and because some drivers share the same logic and even some code) but will break with new drivers or when old drivers decide to "fix" the switch as we did for psycopg. So, IMHO, the best thing is to always call .rollback() before switching transaction level. federico -- Federico Di Gregorio federico.digregorio@dndg.it Studio Associato Di Nunzio e Di Gregorio http://dndg.it And anyone who yells "fork" deserves to get one stuck in them. -- Dan Winship
On Wed, Jun 22, 2011 at 4:47 PM, Federico Di Gregorio <federico.digregorio@dndg.it> wrote: > It is just half of the fix. You can't assume that switching to > autocommit will do an implicit ROLLBACK for you. This worked until now > by chance (and because some drivers share the same logic and even some > code) but will break with new drivers or when old drivers decide to > "fix" the switch as we did for psycopg. So, IMHO, the best thing is to > always call .rollback() before switching transaction level. OK, thanks. Couple more questions if you don't mind: Are there any backwards-compatability implications here? It seems not: if prior versions of psycopg2 implicitly issued a ROLLBACK upon switching to autocommit then we're just making that explicit. Nobody using an older version of psycopg2 will get burned by a "new" ROLLBACK, right? Second, is `conn.set_isolation_level(0)` the right invocation even in the light of the new `set_session()` API? I'd prefer to use `set_isolation_level` because it's backwards compatible, but if it's going away in some future version of psycopg2 I don't want to have a similar problem crop up later. Is `set_isolation_level` considered a stable method? Thanks again! Jacob
On 22/06/11 23:59, Jacob Kaplan-Moss wrote: > On Wed, Jun 22, 2011 at 4:47 PM, Federico Di Gregorio > <federico.digregorio@dndg.it> wrote: >> It is just half of the fix. You can't assume that switching to >> autocommit will do an implicit ROLLBACK for you. This worked until now >> by chance (and because some drivers share the same logic and even some >> code) but will break with new drivers or when old drivers decide to >> "fix" the switch as we did for psycopg. So, IMHO, the best thing is to >> always call .rollback() before switching transaction level. > > OK, thanks. > > Couple more questions if you don't mind: > > Are there any backwards-compatability implications here? It seems not: > if prior versions of psycopg2 implicitly issued a ROLLBACK upon > switching to autocommit then we're just making that explicit. Nobody > using an older version of psycopg2 will get burned by a "new" > ROLLBACK, right? That's correct. > Second, is `conn.set_isolation_level(0)` the right invocation even in > the light of the new `set_session()` API? I'd prefer to use > `set_isolation_level` because it's backwards compatible, but if it's > going away in some future version of psycopg2 I don't want to have a > similar problem crop up later. Is `set_isolation_level` considered a > stable method? New code should use set_session() because of its cleeaner API and exact control over session parameters. But we're _very_ careful about future compatibility: there are just too may psycopg applications to simply remove obsolete methods from future versions. I can say that `set_isolation_level` will stay almost forever. > Thanks again! You're welcome. federico -- Federico Di Gregorio federico.digregorio@dndg.it Studio Associato Di Nunzio e Di Gregorio http://dndg.it Gli esseri umani, a volte, sono destinati, per il solo fatto di esistere, a fare del male a qualcuno. -- Haruki Murakami
On Thu, Jun 23, 2011 at 9:08 AM, Federico Di Gregorio <federico.digregorio@dndg.it> wrote:
> Thanks again!You're welcome.
federico
I cannot resist such an opportunity to say thanks with one email to Django and Psycopg team!
Both of you (and I add all Django community and Daniele "piro" Varrazzo) made my (job) life better :-)
Ciao.
Marco.