From 708f5997c588132fb4324aeaf7fe64bcbab8f8a6 Mon Sep 17 00:00:00 2001 From: bve-odoo Date: Thu, 16 Feb 2023 18:28:31 +0000 Subject: [PATCH] [FIX] core: recycle more connections in multi-tenant servers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit During revision 415525cecceb9af054b57fd85e3791cc779f3d6c 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 --- odoo/addons/base/tests/test_db_cursor.py | 51 +++++++++++++++++++++++- odoo/sql_db.py | 43 +++++++++++++++++--- 2 files changed, 87 insertions(+), 7 deletions(-) diff --git a/odoo/addons/base/tests/test_db_cursor.py b/odoo/addons/base/tests/test_db_cursor.py index 8492026a7dd..985ff228783 100644 --- a/odoo/addons/base/tests/test_db_cursor.py +++ b/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() diff --git a/odoo/sql_db.py b/odoo/sql_db.py index 31bf7ba2ec8..ea2feafb6b2 100644 --- a/odoo/sql_db.py +++ b/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()