From 7dbcc96b8eb7248acdffbddc6540a2f81f4a3d87 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?C=C3=A9dric=20Krier?= Date: Mon, 8 Jul 2019 21:24:33 +0200 Subject: [PATCH] Use separate connection to query cache table The table is a hot spot for contention so we must minimize the duration lock are kept on it. As at the start of a transaction, the cache may be sync and so it locks all the row in the table, the transaction used for this query should not be the main one that lasts a long time. Idem for the commit. issue8447 review283591002 --- trytond/cache.py | 85 ++++++++++++++++++++++--------------- trytond/modules/__init__.py | 3 ++ trytond/tests/test_cache.py | 13 ++++++ 3 files changed, 66 insertions(+), 35 deletions(-) diff --git a/trytond/cache.py b/trytond/cache.py index df428cb991..208c27e1d9 100644 --- a/trytond/cache.py +++ b/trytond/cache.py @@ -165,8 +165,9 @@ class MemoryCache(BaseCache): @classmethod def sync(cls, transaction): - dbname = transaction.database.name - if not _clear_timeout and transaction.database.has_channel(): + database = transaction.database + dbname = database.name + if not _clear_timeout and database.has_channel(): with cls._listener_lock: if dbname not in cls._listener: cls._listener[dbname] = listener = threading.Thread( @@ -175,12 +176,17 @@ class MemoryCache(BaseCache): return if (datetime.now() - cls._clean_last).total_seconds() < _clear_timeout: return - with transaction.connection.cursor() as cursor: - table = Table(cls._table) - cursor.execute(*table.select(_cast(table.timestamp), table.name)) - timestamps = {} - for timestamp, name in cursor.fetchall(): - timestamps[name] = timestamp + connection = database.get_connection(readonly=True, autocommit=True) + try: + with connection.cursor() as cursor: + table = Table(cls._table) + cursor.execute(*table.select( + _cast(table.timestamp), table.name)) + timestamps = {} + for timestamp, name in cursor.fetchall(): + timestamps[name] = timestamp + finally: + database.put_connection(connection) for name, timestamp in timestamps.items(): try: inst = cls._instances[name] @@ -197,39 +203,48 @@ class MemoryCache(BaseCache): reset = cls._reset.setdefault(transaction, set()) if not reset: return - dbname = transaction.database.name - with transaction.connection.cursor() as cursor: - if not _clear_timeout and transaction.database.has_channel(): + database = transaction.database + dbname = database.name + if not _clear_timeout and transaction.database.has_channel(): + with transaction.connection.cursor() as cursor: cursor.execute( 'NOTIFY "%s", %%s' % cls._channel, (json.dumps(list(reset), separators=(',', ':')),)) - else: - for name in reset: - cursor.execute(*table.select(table.name, - where=table.name == name, - limit=1)) - if cursor.fetchone(): - # It would be better to insert only - cursor.execute(*table.update([table.timestamp], - [CurrentTimestamp()], + else: + connection = database.get_connection( + readonly=False, autocommit=True) + try: + with connection.cursor() as cursor: + for name in reset: + cursor.execute(*table.select(table.name, table.id, + table.timestamp, + where=table.name == name, + limit=1)) + if cursor.fetchone(): + # It would be better to insert only + cursor.execute(*table.update([table.timestamp], + [CurrentTimestamp()], + where=table.name == name)) + else: + cursor.execute(*table.insert( + [table.timestamp, table.name], + [[CurrentTimestamp(), name]])) + + cursor.execute(*table.select( + Max(table.timestamp), where=table.name == name)) - else: - cursor.execute(*table.insert( - [table.timestamp, table.name], - [[CurrentTimestamp(), name]])) + timestamp, = cursor.fetchone() - cursor.execute(*table.select( - Max(table.timestamp), - where=table.name == name)) - timestamp, = cursor.fetchone() + cursor.execute(*table.select( + _cast(Max(table.timestamp)), + where=table.name == name)) + timestamp, = cursor.fetchone() - cursor.execute(*table.select( - _cast(Max(table.timestamp)), - where=table.name == name)) - timestamp, = cursor.fetchone() - - inst = cls._instances[name] - inst._clear(dbname, timestamp) + inst = cls._instances[name] + inst._clear(dbname, timestamp) + connection.commit() + finally: + database.put_connection(connection) reset.clear() @classmethod diff --git a/trytond/modules/__init__.py b/trytond/modules/__init__.py index e39a0eedaf..11185137d6 100644 --- a/trytond/modules/__init__.py +++ b/trytond/modules/__init__.py @@ -14,6 +14,7 @@ from sql import Table from sql.functions import CurrentTimestamp import trytond.tools as tools +from trytond.cache import Cache from trytond.config import config from trytond.exceptions import MissingDependenciesException from trytond.transaction import Transaction @@ -266,6 +267,8 @@ def load_module_graph(graph, pool, update=None, lang=None): ])) module2state[module] = 'activated' + # Avoid clearing cache to prevent dead lock on ir.cache table + Cache.rollback(transaction) transaction.commit() if not update: diff --git a/trytond/tests/test_cache.py b/trytond/tests/test_cache.py index de7cb532e4..0ec712a116 100644 --- a/trytond/tests/test_cache.py +++ b/trytond/tests/test_cache.py @@ -104,6 +104,19 @@ class MemoryCacheTestCase(unittest.TestCase): self.wait_cache_sync() self.assertEqual(cache.get('foo'), None) + def test_memory_cache_nested_transactions(self): + "Test MemoryCache with nested transactions" + # Create entry in the cache table to trigger 2 updates + with Transaction().start(DB_NAME, USER): + cache.clear() + # Ensure sync is performed on start + time.sleep(cache_mod._clear_timeout) + + with Transaction().start(DB_NAME, USER) as transaction1: + cache.clear() + with transaction1.new_transaction(): + cache.clear() + def test_memory_cache_sync(self): "Test MemoryCache synchronisation" with Transaction().start(DB_NAME, USER):