Merge lp:~openerp-dev/openobject-server/7.0-rstcnx-chs into lp:openobject-server/7.0

Proposed by Olivier Dony (Odoo)
Status: Merged
Approved by: Christophe Simonis (OpenERP)
Approved revision: 4842
Merge reported by: Christophe Simonis (OpenERP)
Merged at revision: not available
Proposed branch: lp:~openerp-dev/openobject-server/7.0-rstcnx-chs
Merge into: lp:openobject-server/7.0
Diff against target: 32 lines (+8/-8)
1 file modified
openerp/sql_db.py (+8/-8)
To merge this branch: bzr merge lp:~openerp-dev/openobject-server/7.0-rstcnx-chs
Reviewer Review Type Date Requested Status
OpenERP Core Team Pending
Review via email: mp+148848@code.launchpad.net

This proposal supersedes a proposal from 2013-02-14.

Description of the change

Resubmitting the branch after updating the code for better performance: we don't want to force a connection reset on all connections that are present in the pool every time a new cursor is being opened. We only want to do that for the actual unused connection we are planning to pick from the pool - as a last minute validation check. If the check fails we can immediately remove the connection from the pool and try the next one.
BTW doing the reset() call looks like a good thing when preparing a connection for a new cursor: it will cleanup any possible leftover changes to the session done by previous cursors. But it must not be done on connections that are in use, of course.

To post a comment you must log in.
Revision history for this message
Olivier Dony (Odoo) (odo-openerp) wrote : Posted in a previous version of this proposal

LGTM, but after testing with older psycopg2 versions I had to add a check before calling cnx.close(), as close() used to raise an Exception if the connection was already marked as closed (that is, before psycopg2 2.4.5)

Note that reset() was introduced in psycopg2 2.0.12, so we might want to make the requirement explicit in setup.py. For 7.0 we actually require the `current version from debian stable as of the release date`, and this has been 2.2.1 since 2010.

See also the rationale for this patch in the comments on https://code.launchpad.net/~florent.x/openobject-server/trunk-bug-905257-fix-reconnect/+merge/132149

4842. By Olivier Dony (Odoo)

[FIX] sql_db: immediately remove the connections from the pool when detected to be dead

Revision history for this message
Florent (florent.x) wrote :

Proposed fix (for the enumerate glitch):

- for i, (cnx, used) in enumerate(self._connections):
+ for i, (cnx, used) in enumerate(self._connections[:]):

Explanation of the mis-behaviour was posted here:
https://code.launchpad.net/~florent.x/openobject-server/trunk-bug-905257-fix-reconnect/+merge/132149/comments/323526

Revision history for this message
Florent (florent.x) wrote :

... and the moment I press the "Save Comment" button I realize the fix is wrong.

Revision history for this message
Florent (florent.x) wrote :

The simplest fix is to revert http://bazaar.launchpad.net/~openerp/openobject-server/7.0/revision/4837.1.5

No impact on performance.

Revision history for this message
Christophe Simonis (OpenERP) (kangol) wrote :

No. The solution is to use `tools.reverse_enumerate`, not to duplicate the list

Revision history for this message
Olivier Dony (Odoo) (odo-openerp) wrote :

Florent, thanks for spotting the undesired side-effect! I prefer your solution of removing that commit rather than switching to reverse_enumerate(), which could have other side-effects (e.g. like keeping dead connections in the pool for a longer time). That part was reverted at revision 4849 rev-id: <email address hidden>.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'openerp/sql_db.py'
2--- openerp/sql_db.py 2013-02-15 15:12:37 +0000
3+++ openerp/sql_db.py 2013-02-16 01:21:20 +0000
4@@ -400,14 +400,6 @@
5
6 # free dead and leaked connections
7 for i, (cnx, _) in tools.reverse_enumerate(self._connections):
8- try:
9- cnx.reset()
10- except psycopg2.OperationalError:
11- self._debug('Cannot reset connection at index %d: %r', i, cnx.dsn)
12- # psycopg2 2.4.4 and earlier do not allow closing a closed connection
13- if not cnx.closed:
14- cnx.close()
15-
16 if cnx.closed:
17 self._connections.pop(i)
18 self._debug('Removing closed connection at index %d: %r', i, cnx.dsn)
19@@ -421,6 +413,14 @@
20 for i, (cnx, used) in enumerate(self._connections):
21 if not used and dsn_are_equals(cnx.dsn, dsn):
22 self._connections.pop(i)
23+ try:
24+ cnx.reset()
25+ except psycopg2.OperationalError:
26+ self._debug('Cannot reset connection at index %d: %r, removing it', i, cnx.dsn)
27+ # psycopg2 2.4.4 and earlier do not allow closing a closed connection
28+ if not cnx.closed:
29+ cnx.close()
30+ continue
31 self._connections.append((cnx, True))
32 self._debug('Existing connection found at index %d', i)
33