Fix UPSERTs on SQLite 3.24+ (#4477)

8 files changed, 30 insertions, 18 deletions

diff --git a/changelog.d/4306.misc b/changelog.d/4306.misc
index 7f48b02fbf..58130b6190 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 7f48b02fbf..994801fd1e 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 0000000000..58130b6190
--- /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 f62f70b9f1..5109bc3e2e 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 31b8449ca1..059ab81055 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 d6fc8edd4c..9e7e09b8c1 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 452d76ddd5..f18db8c384 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 9605301b59..d6569a82bb 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()