Re: Rollback on close [Was: Fwd: [DB-SIG] conn.close() idempotence]

On Wed, Oct 19, 2011 at 12:46 PM, Daniele Varrazzo
<daniele(dot)varrazzo(at)gmail(dot)com> wrote:
> This excerpt is from the discussion going on about conn.close()
> idempotence on the dbsig mailing list. However this is related to
> psycopg implementation:
>
>> Me wrote:
>>On Wed, Oct 19, 2011 at 8:59 AM, M.-A. Lemburg <mal(at)egenix(dot)com> wrote:
>>
>>> [...] .close() issues an implicit
>>> .rollback() and usually also tells the database backend to free up
>>> resources maintained for the connection, so it does require communication
>>> with the backend.
>>
>> If the rollback is implicit in the backend instead of in the driver,
>> the driver can just cut the communication with the server and obtain
>> the dbapi semantics without issuing a new command. While this is just
>> a trick when close() is called explicitly, it is about necessary on
>> del: not only sending a rollback is a complex operation to be executed
>> by __del__ (just because: it can fail and del would swallow the
>> exception, thing we don't like): in complex environments (such as
>> nonblocking aynchronous communication) pushing the rollback to the
>> server and reading its response implies interaction between other
>> python code and the connection being destroyed, whose results are at
>> best undetermined.
>
> Now, what happened exactly in psycopg has been:
>
> - before 2.2 we used to call ROLLBACK on close() and del, in a common code path
> - in 2.2 this was dropped as an error of mine during some code refactoring
> - talking about reintroducing them, it's been deemed a bad idea, for
> the problems highlighted at del time (as async communication had been
> added)
> - we decided to keep the bug promoting it as feature.
>
> Actually though, while we are fine from the PoV of the data integrity,
> it is true that cutting the communication without a rollback causes
> some resource issue, specifically we know pgpool is not happy and
> discards the connection. We suggest to call rollback explicitly to
> make middleware happy, but this violates the dbapi requirement of the
> "implicit rollback": resources should be well handled on close.
>
> The problem only stems from close() and del sharing almost all the
> code. I think we could split the two implementations instead and call
> an explicit ROLLBACK when close() is called (and we are in
> transaction), and not call it instead when the destructor is invoked,
> but just close the communication and let the backend roll back for us.
> The rollback wouldn't be called in async mode, which is always
> autocommit, and would cause no problem in green mode, as the greenlet
> would be blocked on the close() until communication has finished, so
> the object is guaranteed to be alive.

First, "in transaction" is not enough, it must check if connections
is "idle in transaction" and no query has been sent.

Secondly, I think there are two types of code to consider:

- Sloppy code - eg read-only web page that does

db = connect()
curs.execute('select ...')
curs.execute('select ...')
db.close()

- Robust code, where in-transaction-close means
problem, and it wants to get rid of connection
without touching network.

Although I understand the urge to optimize for first case,
you take away the possibility of robust code to behave well.

So if you really want to restore the rollback-on-close
behaviour, at least make it configurable.

OTOH, as the lightweight .close() is only problematic
with middleware, it seems to hint that this idle-in-tx
check should be moved into middleware, and psycopg
should not need to worry about it..

--
marko