Thread: [snafu] isolation-level change in 2.4.2
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. -- marko
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). federico
On Wed, Dec 14, 2011 at 2:03 PM, Federico Di Gregorio <fog@dndg.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
On 14/12/11 17:15, Daniele Varrazzo wrote: > 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. I'll do that later today. Thanks for the note. federico
On Wed, Dec 14, 2011 at 6:15 PM, Daniele Varrazzo <daniele.varrazzo@gmail.com> wrote: > 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. Please remove any kind of usage of plain ints from psycopg test and example code, if you really want discourage such use. db.set_isolation_level(0) quite strongly hints that other numeric usage is valid too. Esp. note the usage in lib/psycopg1.py that was not updated with your previous changes. -- marko
On Wed, Dec 14, 2011 at 7:28 PM, Marko Kreen <markokr@gmail.com> wrote: Marko, I've noted just now the "snafu" in the title: do you have anything to complain about? Please make an explicit list of your points because - but it may be just me not understanding the subtleties of the English language - I feel your tone in this thread a little bit on the unpleasant side. > On Wed, Dec 14, 2011 at 6:15 PM, Daniele Varrazzo > <daniele.varrazzo@gmail.com> wrote: >> 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. > > Please remove any kind of usage of plain ints from psycopg > test and example code, if you really want discourage such use. > > db.set_isolation_level(0) quite strongly hints that other > numeric usage is valid too. You can choose whether to write your code by induction over examples or by reading the documentation. The constants have been there for more than 6 years and are the only usage explicitly documented. > Esp. note the usage in lib/psycopg1.py that was not updated > with your previous changes. Right: psycopg1.py is not maintained, not documented and hasn't got a single test. Fog: shall we fix it or drop it? -- Daniele,
On Wednesday, December 14, 2011 2:00:50 pm Daniele Varrazzo wrote: > On Wed, Dec 14, 2011 at 7:28 PM, Marko Kreen <markokr@gmail.com> wrote: > > Marko, I've noted just now the "snafu" in the title: do you have > anything to complain about? Please make an explicit list of your > points because - but it may be just me not understanding the > subtleties of the English language - I feel your tone in this thread a > little bit on the unpleasant side. > From what I read Marko was pointing out that the symbolic codes to numeric codes relationship represents a mapping and that mapping works both ways. To help with backwards compatibility he was looking to retain the original meaning of the numeric codes as much as possible. It is a legitimate concern. > > -- Daniele, -- Adrian Klaver adrian.klaver@gmail.com
On Thu, Dec 15, 2011 at 12:00 AM, Daniele Varrazzo <daniele.varrazzo@gmail.com> wrote: > Marko, I've noted just now the "snafu" in the title: do you have > anything to complain about? Please make an explicit list of your > points because - but it may be just me not understanding the > subtleties of the English language - I feel your tone in this thread a > little bit on the unpleasant side. Well, I'm bit frustrated but I can't complain much - thats the reason for "snafu" - two relatively innocent problems on their own (SN) created mess when put together (AFU). If you really want me to complain then I would complain about non-bugfix changes in pscyopg micro releases. Looking at the changes, the 2.4.[23] should have bumped the minor version at least. Basically, no psycopg micro version can be considered as "safe upgrade". OTOH, if the isolation-level shuffle would have been in 2.5, instead 2.4.2, it would not helped much, because that part of code did not have dedicated tests, and was considered stable for years. So the mess would have happened anyway. So I cannot complain too much. -- marko
Marko, Daniele, can you both pull from my devel and report any regressions/errors? Marko, please check this changeset fixes the problems you reported. Thanks, federico
On Thu, Dec 15, 2011 at 11:28 AM, Federico Di Gregorio <fog@dndg.it> wrote:
> Marko, Daniele,
>
> can you both pull from my devel and report any regressions/errors? Marko,
> please check this changeset fixes the problems you reported.
First note: even if currently postgres behaviour is the same at levels
read committed and read uncommitted, they are still two distinct
levels:
test=> set default_transaction_isolation = 'read uncommitted';
SET
test=> SHOW default_transaction_isolation;
default_transaction_isolation
-------------------------------
read uncommitted
(1 row)
I think psycopg shouldn't assume the two being equivalent, otherwise
in the event they will become different in future postgres versions we
would have older psycopg version not supporting them. Doing it now
doesn't cost anything.
I've also just noted that the check for pg version not supporting
levels UNC/REPREAD is in two different points (unfortunately
set_session and set_isolation_level have no common code path).
I've made a patch against these points, but I'll be able to run the
complete test grid only this evening.
-- Daniele
On 15/12/11 13:55, Daniele Varrazzo wrote: > First note: even if currently postgres behaviour is the same at levels > read committed and read uncommitted, they are still two distinct > levels: > > test=> set default_transaction_isolation = 'read uncommitted'; > SET > test=> SHOW default_transaction_isolation; > default_transaction_isolation > ------------------------------- > read uncommitted > (1 row) > > I think psycopg shouldn't assume the two being equivalent, otherwise > in the event they will become different in future postgres versions we > would have older psycopg version not supporting them. Doing it now > doesn't cost anything. > > I've also just noted that the check for pg version not supporting > levels UNC/REPREAD is in two different points (unfortunately > set_session and set_isolation_level have no common code path). > > I've made a patch against these points, but I'll be able to run the > complete test grid only this evening. Merged. federico
On Thu, Dec 15, 2011 at 1:01 PM, Federico Di Gregorio <fog@dndg.it> wrote: > On 15/12/11 13:55, Daniele Varrazzo wrote: >> I've made a patch against these points, but I'll be able to run the >> complete test grid only this evening. > > Merged. All the tests pass. All green for me. Before releasing, didn't you want to include the zope patch provided in ticket #73? <http://psycopg.lighthouseapp.com/projects/62710/tickets/73-create-false>. Cheers, -- Daniele
On Thu, Dec 15, 2011 at 1:28 PM, Federico Di Gregorio <fog@dndg.it> wrote: > can you both pull from my devel and report any regressions/errors? Marko, > please check this changeset fixes the problems you reported. Looks good. Thanks. -- marko
On 15/12/11 22:45, Daniele Varrazzo wrote: > On Thu, Dec 15, 2011 at 1:01 PM, Federico Di Gregorio<fog@dndg.it> wrote: >> > On 15/12/11 13:55, Daniele Varrazzo wrote: >>> >> I've made a patch against these points, but I'll be able to run the >>> >> complete test grid only this evening. >> > >> > Merged. > All the tests pass. All green for me. > > Before releasing, didn't you want to include the zope patch provided > in ticket #73?<http://psycopg.lighthouseapp.com/projects/62710/tickets/73-create-false>. Sure. We should setup some kind of notification for the tickets. I completely missed that one. federico
On 15/12/11 22:45, Daniele Varrazzo wrote: > Before releasing, didn't you want to include the zope patch provided > in ticket #73?<http://psycopg.lighthouseapp.com/projects/62710/tickets/73-create-false>. Uh! But _that_ ticket? The patch is already included. Just need to close the bug report. federico
On 16/12/11 10:20, Federico Di Gregorio wrote: > On 15/12/11 22:45, Daniele Varrazzo wrote: >> Before releasing, didn't you want to include the zope patch provided >> in ticket >> #73?<http://psycopg.lighthouseapp.com/projects/62710/tickets/73-create-false>. >> > > Uh! But _that_ ticket? The patch is already included. Just need to close > the bug report. Mm.. no, sorry. It was only included on my private branch. Will be in 2.4.4. federico