From 7072fe30846c47849c3cb142acef03ef3825a892 Mon Sep 17 00:00:00 2001 From: Amber Brown Date: Mon, 28 Jan 2019 15:43:32 +0000 Subject: [PATCH] Fix UPSERTs on SQLite 3.24+ (#4477) --- changelog.d/4306.misc | 2 +- changelog.d/4471.misc | 2 +- changelog.d/4477.misc | 1 + synapse/storage/_base.py | 10 ++++++++-- synapse/storage/engines/sqlite.py | 10 +++------- synapse/storage/monthly_active_users.py | 12 +++++++++--- tests/storage/test_base.py | 7 +++++-- tests/storage/test_monthly_active_users.py | 4 ++-- 8 files changed, 30 insertions(+), 18 deletions(-) create mode 100644 changelog.d/4477.misc diff --git a/changelog.d/4306.misc b/changelog.d/4306.misc index 7f48b02fb..58130b619 100644 --- a/changelog.d/4306.misc +++ b/changelog.d/4306.misc @@ -1 +1 @@ -Synapse will now take advantage of native UPSERT functionality in PostgreSQL 9.5+. +Synapse will now take advantage of native UPSERT functionality in PostgreSQL 9.5+ and SQLite 3.24+. diff --git a/changelog.d/4471.misc b/changelog.d/4471.misc index 7f48b02fb..994801fd1 100644 --- a/changelog.d/4471.misc +++ b/changelog.d/4471.misc @@ -1 +1 @@ -Synapse will now take advantage of native UPSERT functionality in PostgreSQL 9.5+. + Synapse will now take advantage of native UPSERT functionality in PostgreSQL 9.5+ and SQLite 3.24+. diff --git a/changelog.d/4477.misc b/changelog.d/4477.misc new file mode 100644 index 000000000..58130b619 --- /dev/null +++ b/changelog.d/4477.misc @@ -0,0 +1 @@ +Synapse will now take advantage of native UPSERT functionality in PostgreSQL 9.5+ and SQLite 3.24+. diff --git a/synapse/storage/_base.py b/synapse/storage/_base.py index f62f70b9f..5109bc3e2 100644 --- a/synapse/storage/_base.py +++ b/synapse/storage/_base.py @@ -27,7 +27,7 @@ from twisted.internet import defer from synapse.api.errors import StoreError from synapse.metrics.background_process_metrics import run_as_background_process -from synapse.storage.engines import PostgresEngine +from synapse.storage.engines import PostgresEngine, Sqlite3Engine from synapse.util.caches.descriptors import Cache from synapse.util.logcontext import LoggingContext, PreserveLoggingContext from synapse.util.stringutils import exception_to_unicode @@ -196,6 +196,12 @@ class SQLBaseStore(object): # A set of tables that are not safe to use native upserts in. self._unsafe_to_upsert_tables = {"user_ips"} + # We add the user_directory_search table to the blacklist on SQLite + # because the existing search table does not have an index, making it + # unsafe to use native upserts. + if isinstance(self.database_engine, Sqlite3Engine): + self._unsafe_to_upsert_tables.add("user_directory_search") + if self.database_engine.can_native_upsert: # Check ASAP (and then later, every 1s) to see if we have finished # background updates of tables that aren't safe to update. @@ -230,7 +236,7 @@ class SQLBaseStore(object): self._unsafe_to_upsert_tables.discard("user_ips") # If there's any tables left to check, reschedule to run. - if self._unsafe_to_upsert_tables: + if self.updates: self._clock.call_later( 15.0, run_as_background_process, diff --git a/synapse/storage/engines/sqlite.py b/synapse/storage/engines/sqlite.py index 31b8449ca..059ab8105 100644 --- a/synapse/storage/engines/sqlite.py +++ b/synapse/storage/engines/sqlite.py @@ -33,14 +33,10 @@ class Sqlite3Engine(object): @property def can_native_upsert(self): """ - Do we support native UPSERTs? + Do we support native UPSERTs? This requires SQLite3 3.24+, plus some + more work we haven't done yet to tell what was inserted vs updated. """ - # SQLite3 3.24+ supports them, but empirically the unit tests don't work - # when its enabled. - # FIXME: Figure out what is wrong so we can re-enable native upserts - - # return self.module.sqlite_version_info >= (3, 24, 0) - return False + return self.module.sqlite_version_info >= (3, 24, 0) def check_database(self, txn): pass diff --git a/synapse/storage/monthly_active_users.py b/synapse/storage/monthly_active_users.py index d6fc8edd4..9e7e09b8c 100644 --- a/synapse/storage/monthly_active_users.py +++ b/synapse/storage/monthly_active_users.py @@ -197,15 +197,21 @@ class MonthlyActiveUsersStore(SQLBaseStore): if is_support: return - is_insert = yield self.runInteraction( + yield self.runInteraction( "upsert_monthly_active_user", self.upsert_monthly_active_user_txn, user_id ) - if is_insert: - self.user_last_seen_monthly_active.invalidate((user_id,)) + user_in_mau = self.user_last_seen_monthly_active.cache.get( + (user_id,), + None, + update_metrics=False + ) + if user_in_mau is None: self.get_monthly_active_count.invalidate(()) + self.user_last_seen_monthly_active.invalidate((user_id,)) + def upsert_monthly_active_user_txn(self, txn, user_id): """Updates or inserts monthly active user member diff --git a/tests/storage/test_base.py b/tests/storage/test_base.py index 452d76ddd..f18db8c38 100644 --- a/tests/storage/test_base.py +++ b/tests/storage/test_base.py @@ -49,14 +49,17 @@ class SQLBaseStoreTestCase(unittest.TestCase): self.db_pool.runWithConnection = runWithConnection config = Mock() - config._enable_native_upserts = False + config._disable_native_upserts = True config.event_cache_size = 1 config.database_config = {"name": "sqlite3"} + engine = create_engine(config.database_config) + fake_engine = Mock(wraps=engine) + fake_engine.can_native_upsert = False hs = TestHomeServer( "test", db_pool=self.db_pool, config=config, - database_engine=create_engine(config.database_config), + database_engine=fake_engine, ) self.datastore = SQLBaseStore(None, hs) diff --git a/tests/storage/test_monthly_active_users.py b/tests/storage/test_monthly_active_users.py index 9605301b5..d6569a82b 100644 --- a/tests/storage/test_monthly_active_users.py +++ b/tests/storage/test_monthly_active_users.py @@ -18,12 +18,12 @@ from twisted.internet import defer from synapse.api.constants import UserTypes -from tests.unittest import HomeserverTestCase +from tests import unittest FORTY_DAYS = 40 * 24 * 60 * 60 -class MonthlyActiveUsersTestCase(HomeserverTestCase): +class MonthlyActiveUsersTestCase(unittest.HomeserverTestCase): def make_homeserver(self, reactor, clock): hs = self.setup_test_homeserver()