Re: [snafu] isolation-level change in 2.4.2

On Wed, Dec 14, 2011 at 2:03 PM, Federico Di Gregorio <fog(at)dndg(dot)it> wrote:
> On 14/12/11 14:56, Marko Kreen wrote:
>>
>> In 2.4.2 you shuffled numeric codes for isolation levels freely,
>> because "everybody should be using symbolic codes".
>>
>> Generally that would be true, except this case: psycopg 1.x
>> did not have symbolic codes, so numeric codes were
>> part of public API.  So old code that were written
>> for psycopg1 will not work anymore with psycopg2.
>>
>> What makes is especially bad is that such code will not fail
>> clearly, but instead will result in silent data corruption.
>> We experienced that with Skytools.
>>
>> Also note that even in psycopg2, the .set_isolation_level()
>> is part of core API, but the constants are under psycopg2.extensions.
>> So even pure-2.x code may be using numerical constants.
>>
>>
>> I suggest 2 ways to fix it properly:
>>
>> 1) Use numeric codes out of range compared to 1.x:
>>
>> ISOLATION_LEVEL_AUTOCOMMIT = 10
>> ISOLATION_LEVEL_READ_UNCOMMITTED = 11
>> ISOLATION_LEVEL_READ_COMMITTED = 12
>> ISOLATION_LEVEL_REPEATABLE_READ = 13
>> ISOLATION_LEVEL_SERIALIZABLE = 14
>>
>> and give errors to old codes - that would give clear detection
>> that somebody is using old codes.
>>
>> 2) Use codes that are compatible with 1.x:
>>
>> ISOLATION_LEVEL_AUTOCOMMIT = 0
>> ISOLATION_LEVEL_READ_UNCOMMITTED = 1
>> ISOLATION_LEVEL_READ_COMMITTED = 1
>> ISOLATION_LEVEL_REPEATABLE_READ = 2
>> ISOLATION_LEVEL_SERIALIZABLE = 3
>>
>> the REP_READ=2, SER=3 change is deliberate, because before 9.1
>> they were equal, and in 9.1+ the REP_READ corresponds
>> to old SER.  The SER level in 9.1 is new, and unlikely
>> to be expected by old code.
>>
>> I would suggest even releasing 2.4.4 quite soon with
>> this fixed, to avoid anybody else tripping on this bug.
>
>
> You're right. I rather like (2) and I'll do the fix unless Daniele has a
> very good reason to go with (1).

Honestly this looks much more a bug in skytools than in psycopg for
me. The use of symbolic constants is there exactly to allow these
values to change. However, my fault: I've never maintained psycopg 1,
so I wasn't aware the numbers had ever been part of the api.

The only widespread use of a numeric value I've seen (including many
official psycopg2 examples) is for autocommit=0, so when I've changed
the values I've cared to keep that value stable only. For this reason,
my preference between the solutions goes to 2 too (in the unlikely
case READ UNC started meaning anything for Postgres we can
differentiate it from 1).

Are you making the patch or shall I do it myself? If you change the
values, take care of this check too:
<https://github.com/dvarrazzo/psycopg/blob/devel/psycopg/connection_int.c#L1042>.
In particular, before releasing, let me run the test against all the
supported PG versions: these branch are exhaustively checked.

-- Daniele