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

[Olivier Dony (Odoo)]

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.

[Olivier Dony (Odoo) (odo-openerp)]

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

[lp:~openerp-dev/openobject-server/7.0-rstcnx-chs]

4842. By Olivier Dony (Odoo) on 2013-02-16

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

[Florent (florent.x)]

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

[Florent (florent.x)]

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

[Florent (florent.x)]

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

No impact on performance.

[Christophe Simonis (OpenERP) (kangol)]

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

[Olivier Dony (Odoo) (odo-openerp)]

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>.

=== modified file 'openerp/sql_db.py'
--- openerp/sql_db.py	2013-02-15 15:12:37 +0000
+++ openerp/sql_db.py	2013-02-16 01:21:20 +0000
@@ -400,14 +400,6 @@
 
         # free dead and leaked connections
         for i, (cnx, _) in tools.reverse_enumerate(self._connections):
-            try:
-                cnx.reset()
-            except psycopg2.OperationalError:
-                self._debug('Cannot reset connection at index %d: %r', i, cnx.dsn)
-                # psycopg2 2.4.4 and earlier do not allow closing a closed connection
-                if not cnx.closed:
-                    cnx.close()
-
             if cnx.closed:
                 self._connections.pop(i)
                 self._debug('Removing closed connection at index %d: %r', i, cnx.dsn)
@@ -421,6 +413,14 @@
         for i, (cnx, used) in enumerate(self._connections):
             if not used and dsn_are_equals(cnx.dsn, dsn):
                 self._connections.pop(i)
+                try:
+                    cnx.reset()
+                except psycopg2.OperationalError:
+                    self._debug('Cannot reset connection at index %d: %r, removing it', i, cnx.dsn)
+                    # psycopg2 2.4.4 and earlier do not allow closing a closed connection
+                    if not cnx.closed:
+                        cnx.close()
+                    continue
                 self._connections.append((cnx, True))
                 self._debug('Existing connection found at index %d', i)