[FIX] core: recycle more connections in multi-tenant servers

During revision 415525cecc
the code comparing the connections without password has been
dropped, for an unknown reason.

Because of the above, and the fact psycopg2 alter the password
with 'xxx' for security reasons, the connections recycling was
no longer working as expected, and was recycling less connections.

closes odoo/odoo#37005

Signed-off-by: Rémy Voet <ryv@odoo.com>

odoo/addons/base/tests/test_db_cursor.py

@@ -7,9 +7,10 @@ import psycopg2
 from psycopg2.extensions import ISOLATION_LEVEL_REPEATABLE_READ
 
 import odoo
-from odoo.sql_db import TestCursor
+from odoo.sql_db import db_connect, TestCursor
 from odoo.tests import common
 from odoo.tests.common import BaseCase
+from odoo.tools.misc import config
 
 ADMIN_USER_ID = common.ADMIN_USER_ID
 
@@ -151,6 +152,54 @@ class TestTestCursor(common.TransactionCase):
         # as well as ensure the stack matches our expectations
         self.assertEqual(a._cursors_stack.pop(), a)
 
+    def test_borrow_connection(self):
+        """Tests the behavior of the postgresql connection pool recycling/borrowing"""
+        origin_db_port = config['db_port']
+        if not origin_db_port and hasattr(self.env.cr._cnx, 'info'):
+            # Check the edge case of the db port set,
+            # which is set as an integer in our DSN/connection_info
+            # but as string in the DSN of psycopg2
+            # The connections must be recycled/borrowed when the db_port is set
+            # e.g
+            # `connection.dsn`
+            # {'database': '14.0', 'port': 5432, 'sslmode': 'prefer'}
+            # must match
+            # `cr._cnx.dsn`
+            # 'port=5432 sslmode=prefer dbname=14.0'
+            config['db_port'] = self.env.cr._cnx.info.port
+
+        cursors = []
+        try:
+            connection = db_connect(self.cr.dbname)
+
+            # Case #1: 2 cursors, both opened/used, do not recycle/borrow.
+            # The 2nd cursor must not use the connection of the 1st cursor as it's used (not closed).
+            cursors.append(connection.cursor())
+            cursors.append(connection.cursor())
+            # Ensure the port is within psycopg's dsn, as explained in an above comment,
+            # we want to test the behavior of the connections borrowing including the port provided in the dsn.
+            if config['db_port']:
+                self.assertTrue('port=' in cursors[0]._cnx.dsn)
+            # Check the connection of the 1st cursor is different than the connection of the 2nd cursor.
+            self.assertNotEqual(id(cursors[0]._cnx), id(cursors[1]._cnx))
+
+            # Case #2: Close 1st cursor, open 3rd cursor, must recycle/borrow.
+            # The 3rd must recycle/borrow the connection of the 1st one.
+            cursors[0].close()
+            cursors.append(connection.cursor())
+            # Check the connection of this 3rd cursor uses the connection of the 1st cursor that has been closed.
+            self.assertEqual(id(cursors[0]._cnx), id(cursors[2]._cnx))
+
+        finally:
+            # Cleanups:
+            # - Close the cursors which have been left opened
+            # - Reset the config `db_port`
+            for cursor in cursors:
+                if not cursor.closed:
+                    cursor.close()
+            config['db_port'] = origin_db_port
+
+
 class TestCursorHooks(common.TransactionCase):
     def setUp(self):
         super().setUp()

odoo/sql_db.py

@@ -549,6 +549,15 @@ class PsycoConnection(psycopg2.extensions.connection):
     def lobject(*args, **kwargs):
         pass
 
+    if hasattr(psycopg2.extensions, 'ConnectionInfo'):
+        @property
+        def info(self):
+            class PsycoConnectionInfo(psycopg2.extensions.ConnectionInfo):
+                @property
+                def password(self):
+                    pass
+            return PsycoConnectionInfo(self)
+
 
 class ConnectionPool(object):
     """ The pool of connections to database(s)
@@ -597,7 +606,7 @@ class ConnectionPool(object):
                 _logger.info('%r: Free leaked connection to %r', self, cnx.dsn)
 
         for i, (cnx, used, _) in enumerate(self._connections):
-            if not used and cnx._original_dsn == connection_info:
+            if not used and self._dsn_equals(cnx.dsn, connection_info):
                 try:
                     cnx.reset()
                 except psycopg2.OperationalError:
@@ -631,7 +640,6 @@ class ConnectionPool(object):
         except psycopg2.Error:
             _logger.info('Connection to the database failed')
             raise
-        result._original_dsn = connection_info
         self._connections.append([result, True, 0])
         self._debug('Create new connection backend PID %d', result.get_backend_pid())
         return result
@@ -659,25 +667,48 @@ class ConnectionPool(object):
         count = 0
         last = None
         for i, (cnx, _, _) in tools.reverse_enumerate(self._connections):
-            if dsn is None or cnx._original_dsn == dsn:
+            if dsn is None or self._dsn_equals(cnx.dsn, dsn):
                 cnx.close()
                 last = self._connections.pop(i)[0]
                 count += 1
         _logger.info('%r: Closed %d connections %s', self, count,
                     (dsn and last and 'to %r' % last.dsn) or '')
 
+    def _dsn_equals(self, dsn1, dsn2):
+        alias_keys = {'dbname': 'database'}
+        ignore_keys = ['password']
+        dsn1, dsn2 = ({
+            alias_keys.get(key, key): str(value)
+            for key, value in (isinstance(dsn, str) and self._dsn_to_dict(dsn) or dsn).items()
+            if key not in ignore_keys
+        } for dsn in (dsn1, dsn2))
+        return dsn1 == dsn2
+
+    def _dsn_to_dict(self, dsn):
+        return dict(value.split('=', 1) for value in dsn.strip().split())
+
 
 class Connection(object):
     """ A lightweight instance of a connection to postgres
     """
     def __init__(self, pool, dbname, dsn):
-        self.dbname = dbname
-        self.dsn = dsn
+        self.__dbname = dbname
+        self.__dsn = dsn
         self.__pool = pool
 
+    @property
+    def dsn(self):
+        dsn = dict(self.__dsn)
+        dsn.pop('password', None)
+        return dsn
+
+    @property
+    def dbname(self):
+        return self.__dbname
+
     def cursor(self):
         _logger.debug('create cursor to %r', self.dsn)
-        return Cursor(self.__pool, self.dbname, self.dsn)
+        return Cursor(self.__pool, self.__dbname, self.__dsn)
 
     def __bool__(self):
         raise NotImplementedError()