-
-
Notifications
You must be signed in to change notification settings - Fork 502
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Pool broken for zope #125
Comments
Attached proposed patch to fix the issue |
Zope has its own transaction management: like any sensible web framework the transaction is rolled back on errors, and commited just before sending the HTTP response. You don't want to ROLLBACK on a Zope connection. Peraphs we can have a specialized pool that doesn't ROLLBACK while keeping the fix for #62 in? |
An optional solution would be to replace entirely PersistentConnectionPool.putconn and not invoke _putconn, but it leads to some code duplication. For me, any solution is fine, but because I just don't know how to test it, I'm happy to get the report "patch X is working" (for any X, this or any other one) and do with it. |
Originally submitted by: Wolfgang Eibner Hm, after some quick investigation I think the problem is located at another place. Your patched block gets never called cause get_transaction_status() returns 0 on every one of the Zope connections at putting them back into the pool. I get the following statement logs: 2012-09-18 18:21:48 CEST 5619 1121366 LOG: Anweisung: SHOW default_transaction_isolation 2012-09-18 18:21:56 CEST 5619 1121369 LOG: Anweisung: SHOW default_transaction_isolation 2012-09-18 18:22:00 CEST 5619 1121371 LOG: Anweisung: SHOW default_transaction_isolation BTW: Why is the default transaction isolation level repeatable read although serializable is checked in the ZPsycopgDA? After implementing some debug prints in the python code I found out, that the three lines As far as I can see (without further debugging) the rollback happens in connection_int.c at line 1155 (after /* abort the current transaction, to set the encoding ouside of transactions */). So, while Zope retrieves the connection (which still has a transaction open) from the pool or before the next statement is executed (?), the encoding is set by psycopg. But this should be done outside transactions and so a rollback is issued. I simply tried a return 0; at the beginning of conn_set_client_encoding and with that modification everything looks fine: 2012-09-18 19:00:11 CEST 5888 1121832 LOG: Anweisung: SHOW default_transaction_isolation Of course this is no patch but maybe it helps you to fix the problem ;-) |
Thank you for the analysis, Wolfgang. As you describe it, seems that most of the mess happens in ZPsycopgDA/db.py
Looks like there is code dealing with not trying to re-init an already init'ed connection, maybe somehow it's not doing what it should. rolling back the connection before setting the client encoding is something has always happened. Can you take a look there? Thank you very much. |
Originally submitted by: Wolfgang Eibner Regarding the isolation level: In ZPsycopgDA/dtml/edit.dtml serializable is referenced as value 2 (but it has 4): selected="YES"> SerializableLike mentioned here (of course autocommit is no isolation level): BUT: for my case I need serializable and so I've to change the value to 4. But I don't know which would be the right value for read committed (1 or 2?) cause I don't know the postgres api. Regarding the client_encoding: I'll debug that later. |
Originally submitted by: Wolfgang Eibner Sorry for the bad formatting and there was also an error: At my postgresql database (version 8.2) the mapping is as following:
for my case I need serializable and so I've to change the value to 3. But I don't know which would be the right value for read committed (1 or 2?) cause I don't know the postgres api. |
Oh, I see. In PG the serializable and repeatable read used to be the same, since 9.0 iirc serializable became stronger and the previous semantics is available as repeatable read. So in PG serializable now requires value 3. Actually the story is slightly different resulting in some rants, a few buggy versions and probably some grief at Skype... let's say that now it's this way :) So, these options in add.dtml are out of date respect to the ISOLATION_LEVEL_* constants available in extensions.py. Psycopg is also changed in another way since ZPsycopgDA was implemented: it used to query the server for the the default level and re-issue it at every begin, resulting in a query mostly useless at connection time. The best thing would be to add another option to that menu: "default", which is whatever the server defines, and which results in no query performed neither to know the default nor to set the isolevel. This is usually "read committed" but can be set in the server config file. The article you have linked is right. Too bad the author is a total moron has never contacted us to report the bug. What a shit attitude! Also, comments on his site are broken and there's no email link nor even a name to contact him... Who the hell is he? To that list I'd add a "default" value, for instance an empty string or -1, whatever, meaning "just don't set the isolation level and let the server deal with it". Also, db.py should be implemented using set_session instead of set_isolation_level, so that no "SHOW default_transaction_isolation" is performed. So, the bugs here are:
In the resulting communication sequence, there should be no "SHOW default_transaction_isolation", only one "SET default_transaction_isolation to LEVEL" (unless the level chosen is DEFAULT), only one SET client_encoding TO 'UTF8', no spurious rollback. Can you help fixing this mess? Thank you. |
I've fixed the menu for add/edit pages showing all the available isolation level. Note that the default is "repeatable read" because in postgres 9.1 serializable has changed becoming something stricter than before. In previous pg version "repeatable read" and "serializable" are aliases. I've also fixed db.py so that "SHOW default_transaction_isolation" is not queried anymore; anyway that's not what is causing your problem. These corrections are available in my "zope-pool" branch https://github.com/dvarrazzo/psycopg/tree/zope-pool As I see it, just looking at the code statically, the problem is caused by DB.getcursor() being called twice, once before your "insert" the other before your "select". This in turn causes getconn with init=True twice, which calls the set_client_encoding() which issues a rollback. It should be easy to verify it with a couple of prints, but I don't know how to setup such a zope page. |
Originally submitted by: Wolfgang Eibner Hi, it will take me some time to test/fix this because I'll be not at the office till Thursday and I'm very busy at the moment. Best regards, |
I've fixed the pool by adding a zope-specific implementation of the base pool copied from Psycopg 2.4. the reset upon set_client_encoding is still there, but it has always been. If it is an issue you will have to provide a solution yourself, as I'm not able to set up a test page to reproduce it. |
Zope regularly puts connections in transaction into the pool and doesn't expect them to be rolled back. This patch assumes Zope only creates PersistentConnectionPool subclasses. Submitted to ticket #125 and to the ML for testing.
set_session can work without querying show default_transaction_isolation, see example of the useless queries in ticket #125
Apparently the connection pool rolling back connections in transaction (ticket #62) has broken the zope adapter (see ticket #123 and ML message http://archives.postgresql.org/psycopg/2012-08/msg00000.php)
If this is the case, releases from 2.4.3 to 2.4.5 present the bug. It will be fixed in release 2.4.6.
The text was updated successfully, but these errors were encountered: