Thread: Transaction control overhauling

Hello,

I think psycopg needs some cleanup in the area of the transactions control.

The main points of the current implementation are:

1. at connection time, psycopg queries default_isolation_level from the server;

2. when starting a transaction, it executes "begin; set isolation
level LEVEL" after what received in 1.

3. the isolation level can be read from conn.isolation_level and
changed using using set_isolation_level(). Supported values are 1
(read committed), 2 (serializable).

4. the set_isolation_level() is also used to put the connection in
"autocommit" mode, passing the value 0.


Shortcomings:

a. In PG 9.1 level "repeatable read" is no more an alias for
serializable, so all the 4 SQL levels should be supported (note that
this wasn't happening before because this area was designed in pre-8.0
era, when levels read uncommitted/repeatable read were not accepted by
SET TRANSACTION)

b. autocommit is not alternative to the isolation level: it is
orthogonal. A connection may be autocommit + serializable. The current
behaviour is implicitly autocommit + GUC default.

c. there is no support to switch a connection read only - feature
sometimes requested. This, again, is orthogonal to the isolation
level, so an implementation such as the one proposed in the ticket #12
(http://psycopg.lighthouseapp.com/projects/62710/tickets/12) doesn't
convince me.

d. PG 9.1 also introduced "set transaction [not] deferrable"
(http://www.postgresql.org/docs/9.1/static/sql-set-transaction.html).

I also feel the combination of points 1 and 2 above somewhat silly: we
query the connection and then explicitly force the level we know to be
default. The only extra thing respect of not doing nothing at all is
that the connection then knows the isolation level.


I think a more sensible behaviour would be:

I. don't issue any query at startup and, by default, don't pass any
isolation level together with BEGIN:

II. add a method conn.set_transaction(isolation_level=None,
read_only=None, deferrable=None) allowing to change one or more of the
transaction settings. Calling the method would terminate the current
transaction and put the new settings in place. Note that there are
several ways for implementing this:

II. 1. store variables in the connection object and pass the relative
SET TRANSACTION at the following BEGIN

II. 2. run a query SET SESSION CHARACTERISTICS AS ... and not store
anything in the connection status

II. 3. run a query to set the GUC instead (SET default_whatever AS
value): very similar to II. 2., but it also allows passing the value
"default", meaning "reset to the configuration value", an option
apparently missing with the SET SESSION CHARACTERISTICS syntax.

III. add an independent method conn.autocommit(value=True). A less
verbose method for a frequently used functionality.

IV. for backwards compatibility, convert isolation_level into a
property, querying the server to know the current status.

V. keep set_isolation_level() for backwards compatibility, converting
it into a wrapper for the other methods.

VI. don't write so many lists in a single email anymore: I'm out of alphabets.


With these changes, I think we should keep complete compatibility with
the past, both as interface and as behaviour, but we end up with less
queries performed and complete support for all the current and
upcoming Postgres features (plus a natural way of extending, adding
new keyword arguments to conn.set_transaction() should the need
arise).

Comments? Shall we go for it?

Cheers,

-- Daniele

I'm not a specialist, but I think that's perfectly valid.

About the II alternatives:

II.1 - Every begin would set transaction again or only the next one? Anyway, either of them don't look nice.

II.2 - That would be my choice but...

II.3 - I didn't know that the 'default' cant be applied to it, so II.3 is the easy winner.

Go for it.

On 12/05/11 00:43, Daniele Varrazzo wrote:
> Hello,
>
> I think psycopg needs some cleanup in the area of the transactions control.
[snip]

Hi Daniele,

I pretty much agree with your analisys, I just have a couple of comments:

> I. don't issue any query at startup and, by default, don't pass any
> isolation level together with BEGIN:
>
> II. add a method conn.set_transaction(isolation_level=None,
> read_only=None, deferrable=None) allowing to change one or more of the
> transaction settings. Calling the method would terminate the current
> transaction and put the new settings in place. Note that there are
> several ways for implementing this:

I'd make this a keyword argument function with the following signature:

conn.set_transaction(
    isolation_level=None,
    autocommit=None,
    deferrable=None,
    readonly=None)

to keep everything related to transaction management in a single
function. I don't like the proposed autocommit() method because it is
yet another DBAPI extension and must keep that to a minimum. Also, I
sorted the parameters in set_transaction() from the probably most used
to least.

> II. 1. store variables in the connection object and pass the relative
> SET TRANSACTION at the following BEGIN
>
> II. 2. run a query SET SESSION CHARACTERISTICS AS ... and not store
> anything in the connection status
>
> II. 3. run a query to set the GUC instead (SET default_whatever AS
> value): very similar to II. 2., but it also allows passing the value
> "default", meaning "reset to the configuration value", an option
> apparently missing with the SET SESSION CHARACTERISTICS syntax.

Do we need the ability to pass "default"? When the user call
set_transaction() with missing or None parameters do we want to send
"default" or stay with the current value? I favor the latter, e.g.,

conn.set_transaction(isolation_level=XXX, deferrable=YYY)

should not reset the autocommit or readonly values to default.

> III. add an independent method conn.autocommit(value=True). A less
> verbose method for a frequently used functionality.

Nope, see above.

> IV. for backwards compatibility, convert isolation_level into a
> property, querying the server to know the current status.

Yesss.

>
> V. keep set_isolation_level() for backwards compatibility, converting
> it into a wrapper for the other methods.

Yesss.

federico

--
Federico Di Gregorio                         federico.digregorio@dndg.it
Studio Associato Di Nunzio e Di Gregorio                  http://dndg.it
 In some countries (e.g., Germany) even many brands of toilet paper have
  format A6.             [http://www.cl.cam.ac.uk/~mgk25/iso-paper.html]

On Thu, May 12, 2011 at 9:01 AM, Federico Di Gregorio
<federico.digregorio@dndg.it> wrote:

>> II. add a method conn.set_transaction(isolation_level=None,
>> read_only=None, deferrable=None) allowing to change one or more of the
>> transaction settings. Calling the method would terminate the current
>> transaction and put the new settings in place. Note that there are
>> several ways for implementing this:
>
> I'd make this a keyword argument function with the following signature:
>
> conn.set_transaction(
>        isolation_level=None,
>        autocommit=None,
>        deferrable=None,
>        readonly=None)
>
> to keep everything related to transaction management in a single
> function. I don't like the proposed autocommit() method because it is
> yet another DBAPI extension and must keep that to a minimum.

The idea though was to have set_transaction mapping closely PG's SET
TRANSACTION statement, and autocommit is a different beast. I also
thought autocommit was a pretty standard extension. But actually,
making a survey of other drivers:

- MySQLdb: conn.autocommit()
- cx_Oracle: conn.autocommit (attribute, not function)
- pyodbc: conn.autocommit (attribute)
- sqlite3: conn.isolation_level = None (shared dna with psycopg, eh?
:) however it's an attribute)
- KInterbaseDB: not supported

So, total anarchy here :\. The autocommit attribute would have been my
favourite, but psycopg uses more often methods than read/write
attributes (probably there is none of them) so the autocommit() method
would blend better. But now, thinking about that, there would be no
natural way to read back the value, for which there is no PG parameter
to SHOW... so the attribute solution seems really the best option
(unless making a pair set_autocommit/autocommit... ugh).

To summarize: an autocommit parameter to set_transaction would be ok
enough as it's independent from the other ones. But it has the
shortcoming of giving no way to read the value back. We would have

    conn.set_transaction(autocommit=True)

which is not bad. but

    conn.autocommit = True

feels better and allows to read the value back. And it's used quite a
lot, more than going serialized I'd say.


> Also, I
> sorted the parameters in set_transaction() from the probably most used
> to least.

I would think that read_only would more used than deferrable, which
looks a pretty specialized level. No problem anyway as I expect all
the parameters after the first to be called as keyword, e.g. people
may want to use:

    conn.set_transaction(READ_COMMITTED)
    conn.set_transaction(read_only=True)
    conn.set_transaction(SERIALIZED, read_only=True, deferrable=False)

and not

    conn.set_transaction(SERIALIZED, None, False, True)


>> II. 3. run a query to set the GUC instead (SET default_whatever AS
>> value): very similar to II. 2., but it also allows passing the value
>> "default", meaning "reset to the configuration value", an option
>> apparently missing with the SET SESSION CHARACTERISTICS syntax.
>
> Do we need the ability to pass "default"? When the user call
> set_transaction() with missing or None parameters do we want to send
> "default" or stay with the current value? I favor the latter, e.g.,

Yes, me too: None would mean don't change, not reset to default, and
setting the default would require a different symbol, such as
set_transaction(read_only=DEFAULT). We don't strictly need it of
course: people can still query the connection and later reset to the
original value. I agree it's not the most likely use case though, we
may also avoid this feature.


I'll leave you with the thorny autocommit question... :)

Bye!


-- Daniele

On 12/05/11 11:33, Daniele Varrazzo wrote:
> On Thu, May 12, 2011 at 9:01 AM, Federico Di Gregorio
[snip]
> So, total anarchy here :\. The autocommit attribute would have been my
> favourite, but psycopg uses more often methods than read/write
> attributes (probably there is none of them) so the autocommit() method

Yep, probably because they didn't existed when psycopg2 was started.

> would blend better. But now, thinking about that, there would be no
> natural way to read back the value, for which there is no PG parameter
> to SHOW... so the attribute solution seems really the best option
> (unless making a pair set_autocommit/autocommit... ugh).

There is no PG parameter because autocommit doesn't really exists. It is
a side effect of not issuing a BEGIN before other commands: every
command gets its own (implicit) transaction.

> To summarize: an autocommit parameter to set_transaction would be ok
> enough as it's independent from the other ones. But it has the
> shortcoming of giving no way to read the value back. We would have
>
>     conn.set_transaction(autocommit=True)
>
> which is not bad. but
>
>     conn.autocommit = True
>
> feels better and allows to read the value back. And it's used quite a
> lot, more than going serialized I'd say.

Agreed. I'd rather have both to have a single point to set transaction
parameters AND autocommit and the attribute to recover the value. Or,
maybe even better, we can have 4 attributes and the set_transaction() as
a shortcut:

    conn.autocommit
    conn.transaction_isolation_level
    conn.transaction_readonly
    conn.transaction_deferrable
    conn.set_transaction(isolation_level, autocommit, readonly, ...)

> I would think that read_only would more used than deferrable, which
> looks a pretty specialized level. No problem anyway as I expect all
> the parameters after the first to be called as keyword, e.g. people
> may want to use:
>
>     conn.set_transaction(READ_COMMITTED)
>     conn.set_transaction(read_only=True)
>     conn.set_transaction(SERIALIZED, read_only=True, deferrable=False)
>
> and not
>
>     conn.set_transaction(SERIALIZED, None, False, True)

Agreed. Note that you write read_only while I write readonly ... the
DBAPI has a long story of not using "_" for short, two word flags.

>> Do we need the ability to pass "default"? When the user call
>> set_transaction() with missing or None parameters do we want to send
>> "default" or stay with the current value? I favor the latter, e.g.,
>
> Yes, me too: None would mean don't change, not reset to default, and
> setting the default would require a different symbol, such as
> set_transaction(read_only=DEFAULT). We don't strictly need it of
> course: people can still query the connection and later reset to the
> original value. I agree it's not the most likely use case though, we
> may also avoid this feature.

DEFAULT is fine, but I don't think it will ever be used. :D

federico

--
Federico Di Gregorio                         federico.digregorio@dndg.it
Studio Associato Di Nunzio e Di Gregorio                  http://dndg.it
 To prepare such test data, I get into the meaniest, nastiest frame of
  mind that I can manage, and I write the cruelest code I can think of;
  then I turn around and I embed that in even nastier constructions
  that are almost obscene.                                  -- D.E.Knuth

On Thu, May 12, 2011 at 10:57 AM, Federico Di Gregorio
<federico.digregorio@dndg.it> wrote:

>> To summarize: an autocommit parameter to set_transaction would be ok
>> enough as it's independent from the other ones. But it has the
>> shortcoming of giving no way to read the value back. We would have
>>
>>     conn.set_transaction(autocommit=True)
>>
>> which is not bad. but
>>
>>     conn.autocommit = True
>>
>> feels better and allows to read the value back. And it's used quite a
>> lot, more than going serialized I'd say.
>
> Agreed. I'd rather have both to have a single point to set transaction
> parameters AND autocommit and the attribute to recover the value.

Works for me.

> Or,
> maybe even better, we can have 4 attributes and the set_transaction() as
> a shortcut:
>
>        conn.autocommit
>        conn.transaction_isolation_level
>        conn.transaction_readonly
>        conn.transaction_deferrable
>        conn.set_transaction(isolation_level, autocommit, readonly, ...)

Wow, I thought you wanted to limit the dbapi extensions :)


> Note that you write read_only while I write readonly ... the

Funny, I wanted to write you "note that you use readonly but isolation_level" :D

> DBAPI has a long story of not using "_" for short, two word flags.

I see... no problem with it.


> DEFAULT is fine, but I don't think it will ever be used. :D

This is likely. However the important is to agree to set the
connection parameter by sending a command to the backend as opposite
of storing it in our state and repeating at every begin.


Looks like we are starting having a plan. I can start working on these
ideas in the next days, meanwhile if anybody else has comment, they
are welcome.


-- Daniele

On Thu, May 12, 2011 at 00:43, Daniele Varrazzo
<daniele.varrazzo@gmail.com> wrote:
> Hello,

A couple of quick notes (don't know enough to answer the implementation details)


> 2. when starting a transaction, it executes "begin; set isolation
> level LEVEL" after what received in 1.

You are aware this can be done in one query, right? just "BEGIN
ISOLATION LEVEL SERIALIZABLE".. That should be marginally more
efficient.


> I. don't issue any query at startup and, by default, don't pass any
> isolation level together with BEGIN:

That seems very reasonable, particularly the first one :-)


> II. add a method conn.set_transaction(isolation_level=None,
> read_only=None, deferrable=None) allowing to change one or more of the
> transaction settings. Calling the method would terminate the current
> transaction and put the new settings in place. Note that there are
> several ways for implementing this:

Ugh. Big -1 on the "terminate current transaction". First of all, I
*assume* you mean ROLLBACK and not COMMIT. But even so, I think it's a
much better idea to raise a local exception when trying to change it
while inside a transaction.

> VI. don't write so many lists in a single email anymore: I'm out of alphabets.

Nah, just switch alphabet!


--
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/

On 12/05/11 13:57, Magnus Hagander wrote:
>> II. add a method conn.set_transaction(isolation_level=None,
>> > read_only=None, deferrable=None) allowing to change one or more of the
>> > transaction settings. Calling the method would terminate the current
>> > transaction and put the new settings in place. Note that there are
>> > several ways for implementing this:
> Ugh. Big -1 on the "terminate current transaction". First of all, I
> *assume* you mean ROLLBACK and not COMMIT. But even so, I think it's a
> much better idea to raise a local exception when trying to change it
> while inside a transaction.
>

That makes sense. Currently psycopg keeps track of the current
transaction level to be able to decide if to terminare the transaction
(with a ROLLBACK) or not. But that's less than optimal because the
result of set_transaction_isolation() depends both on internal state and
issued queries. Much much better to just raise an exception if inside a
transaction.

federico

--
Federico Di Gregorio                         federico.digregorio@dndg.it
Studio Associato Di Nunzio e Di Gregorio                  http://dndg.it
 The only thing I see is if you are pumping so much data into the
  database all the time when do you expect to look at it?
                                                        -- Charlie Clark

On Thu, May 12, 2011 at 12:57 PM, Magnus Hagander <magnus@hagander.net> wrote:

>> II. add a method conn.set_transaction(isolation_level=None,
>> read_only=None, deferrable=None) allowing to change one or more of the
>> transaction settings. Calling the method would terminate the current
>> transaction and put the new settings in place. Note that there are
>> several ways for implementing this:
>
> Ugh. Big -1 on the "terminate current transaction". First of all, I
> *assume* you mean ROLLBACK and not COMMIT. But even so, I think it's a
> much better idea to raise a local exception when trying to change it
> while inside a transaction.

Yes, the idea was to issue a rollback. This is the current behaviour
of set_isolation_level(). No problem in having an exception raised
instead: it seems actually a better defined behaviour.

-- Daniele

On 12/05/11 12:36, Daniele Varrazzo wrote:
[snip]
>> Or,
>> maybe even better, we can have 4 attributes and the set_transaction() as
>> a shortcut:
>>
>>        conn.autocommit
>>        conn.transaction_isolation_level
>>        conn.transaction_readonly
>>        conn.transaction_deferrable
>>        conn.set_transaction(isolation_level, autocommit, readonly, ...)
>
> Wow, I thought you wanted to limit the dbapi extensions :)

Well, I know. But, in fact, if we introduce the possibility to set such
parameters we also _need_ a way to retrieve them and attributes are the
only sensible way to retrive them (a function returning a tuple is
another way but stinks.)

federico

--
Federico Di Gregorio                         federico.digregorio@dndg.it
Studio Associato Di Nunzio e Di Gregorio                  http://dndg.it
 [parlando di un amica] ora mi tampina perche' vuole i miei cacciaviti.
  ma ti rendi conto?                                           -- <dani>

On Thu, May 12, 2011 at 3:07 PM, Federico Di Gregorio
<federico.digregorio@dndg.it> wrote:
> On 12/05/11 12:36, Daniele Varrazzo wrote:
> [snip]
>>> Or,
>>> maybe even better, we can have 4 attributes and the set_transaction() as
>>> a shortcut:
>>>
>>>        conn.autocommit
>>>        conn.transaction_isolation_level
>>>        conn.transaction_readonly
>>>        conn.transaction_deferrable
>>>        conn.set_transaction(isolation_level, autocommit, readonly, ...)
>>
>> Wow, I thought you wanted to limit the dbapi extensions :)
>
> Well, I know. But, in fact, if we introduce the possibility to set such
> parameters we also _need_ a way to retrieve them and attributes are the
> only sensible way to retrive them (a function returning a tuple is
> another way but stinks.)

I was thinking about documenting that you can issue "SHOW
default_whatever" and get the result from there. It isn't the handiest
of the procedures, OTOH it's also not a very common use case not
knowing what the level is and wanting to know it - the biggest use
case is just to set the level desired and use the connection. This
possibility is not available with "autocommit" instead, as it's
something all internal to the connection object and not related to the
PG session.

-- Daniele

El 12/05/2011 16:07, Federico Di Gregorio escribió:
> On 12/05/11 12:36, Daniele Varrazzo wrote:
> [snip]
>>> Or,
>>> maybe even better, we can have 4 attributes and the set_transaction() as
>>> a shortcut:
>>>
>>>         conn.autocommit
>>>         conn.transaction_isolation_level
>>>         conn.transaction_readonly
>>>         conn.transaction_deferrable
>>>         conn.set_transaction(isolation_level, autocommit, readonly, ...)
>>
>> Wow, I thought you wanted to limit the dbapi extensions :)
>
> Well, I know. But, in fact, if we introduce the possibility to set such
> parameters we also _need_ a way to retrieve them and attributes are the
> only sensible way to retrive them (a function returning a tuple is
> another way but stinks.)
>

Another way is setting transacion mode with predefined constants and
combining it with '|':

previous_mode = conn.set_transaction(psycppg2.TM_SERIALIZABLE |
psycppg2.TM_READONLY)



--
Oswaldo Hernández

On 12/05/11 17:15, Oswaldo wrote:
[snip]
> Another way is setting transacion mode with predefined constants and
> combining it with '|':
>
> previous_mode = conn.set_transaction(psycppg2.TM_SERIALIZABLE |
> psycppg2.TM_READONLY)

Please don't do that C-ish thing.. Every time you do that a kitten
somewhere dies.

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

El 12/05/2011 17:47, Federico Di Gregorio escribió:
> On 12/05/11 17:15, Oswaldo wrote:
> [snip]
>> Another way is setting transacion mode with predefined constants and
>> combining it with '|':
>>
>> previous_mode = conn.set_transaction(psycppg2.TM_SERIALIZABLE |
>> psycppg2.TM_READONLY)
>
> Please don't do that C-ish thing.. Every time you do that a kitten
> somewhere dies.
>

Yes, not too pythonic but often practical to set object styles.

Sorry for the kittens ;)


--
Oswaldo Hernández

On Thu, May 12, 2011 at 1:13 PM, Federico Di Gregorio
<federico.digregorio@dndg.it> wrote:
> On 12/05/11 13:57, Magnus Hagander wrote:
>>> II. add a method conn.set_transaction(isolation_level=None,
>>> > read_only=None, deferrable=None) allowing to change one or more of the
>>> > transaction settings. Calling the method would terminate the current
>>> > transaction and put the new settings in place. Note that there are
>>> > several ways for implementing this:
>> Ugh. Big -1 on the "terminate current transaction". First of all, I
>> *assume* you mean ROLLBACK and not COMMIT. But even so, I think it's a
>> much better idea to raise a local exception when trying to change it
>> while inside a transaction.
>>
>
> That makes sense. Currently psycopg keeps track of the current
> transaction level to be able to decide if to terminare the transaction
> (with a ROLLBACK) or not. But that's less than optimal because the
> result of set_transaction_isolation() depends both on internal state and
> issued queries. Much much better to just raise an exception if inside a
> transaction.

I've implemented what discussed in this thread. Documentation
explaining the new features is available in the docs online:

    http://initd.org/psycopg/docs/connection.html#connection.set_transaction

The code is merged into my devel branch.

Shall we resume the old tradition of adding features in "3rd digit
releases" and have the library released as 2.4.2? Backward
compatibility should be entirely preserved.

Before releasing though I want to try to fix two issues: the
multithread problem reported a few days ago (ticket #55) and possibly
try to make life easier into virtualenv: even if the setup.py has been
patched to avoid building mx support when the library can't be
imported (but the includes are there), there is still some struggling
about the matter [1] [2]. I think mx support should be optional even
if built: at import time, if mx.DateTime is not found, psycopg should
simply avoid registering mx adapters/typecasters and keep going on
with stdlib's datetime objects.

-- Daniele

[1] https://github.com/dvarrazzo/psycopg/commit/af424821b70299704449337904d3ab7fb1b40751
[2] https://twitter.com/#!/mike360/statuses/75295024346177537

On 03/06/11 12:35, Daniele Varrazzo wrote:
[snip]
> Shall we resume the old tradition of adding features in "3rd digit
> releases" and have the library released as 2.4.2? Backward
> compatibility should be entirely preserved.

Yes, I'd do that.
>
> Before releasing though I want to try to fix two issues: the
> multithread problem reported a few days ago (ticket #55) and possibly
> try to make life easier into virtualenv: even if the setup.py has been
> patched to avoid building mx support when the library can't be
> imported (but the includes are there), there is still some struggling
> about the matter [1] [2]. I think mx support should be optional even
> if built: at import time, if mx.DateTime is not found, psycopg should
> simply avoid registering mx adapters/typecasters and keep going on
> with stdlib's datetime objects.

Agreed again.

federico

--
Federico Di Gregorio                         federico.digregorio@dndg.it
Studio Associato Di Nunzio e Di Gregorio                  http://dndg.it
 heisenbug /hi:'zen-buhg/ /n./ [from Heisenberg's Uncertainty Principle
  in quantum physics] A bug that disappears or alters its behavior when
  one attempts to probe or isolate it.

On 03/06/11 12:35, Daniele Varrazzo wrote:
> The code is merged into my devel branch.

I just reviewed and merged all code.

federico

--
Federico Di Gregorio                         federico.digregorio@dndg.it
Studio Associato Di Nunzio e Di Gregorio                  http://dndg.it
 - Ma cos'ha il tuo pesce rosso, l'orchite?
 - Si, ha un occhio solo, la voce roca e mangia gli altri pesci.