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 #(at)§%£$! 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(dot)digregorio(at)dndg(dot)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 #(at)§%£$! 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 #(at)§%£$! 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(dot)digregorio(at)dndg(dot)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(dot)digregorio(at)dndg(dot)it> wrote:

> If we don't want to wait for the #(at)§%£$! 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