Address review comments

matrix-org · Aug 31, 2020 · f8870f3 · f8870f3
1 parent 5a682ba
commit f8870f3
Show file tree

Hide file tree

Showing 3 changed files with 34 additions and 42 deletions.
diff --git a/synapse/storage/data_stores/main/media_repository.py b/synapse/storage/data_stores/main/media_repository.py
@@ -12,11 +12,13 @@
 # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
 # See the License for the specific language governing permissions and
 # limitations under the License.
-from twisted.internet import defer
-
 from synapse.storage._base import SQLBaseStore
 from synapse.storage.database import Database
 
+BG_UPDATE_REMOVE_MEDIA_REPO_INDEX_WITHOUT_METHOD = (
+    "media_repository_drop_index_wo_method"
+)
+
 
 class MediaRepositoryBackgroundUpdateStore(SQLBaseStore):
     def __init__(self, database: Database, db_conn, hs):
@@ -32,6 +34,10 @@ def __init__(self, database: Database, db_conn, hs):
             where_clause="url_cache IS NOT NULL",
         )
 
+        # The following the updates add the method to the unique constraint of
+        # the thumbnail databases. That fixes an issue, where thumbnails of the
+        # same resolution, but different methods could overwrite one another.
+        # This can happen with custom thumbnail configs or with dynamic thumbnailing.
         self.db.updates.register_background_index_update(
             update_name="local_media_repository_thumbnails_method_idx",
             index_name="local_media_repository_thumbn_media_id_width_height_method_key",
@@ -62,42 +68,22 @@ def __init__(self, database: Database, db_conn, hs):
         )
 
         self.db.updates.register_background_update_handler(
-            "local_media_repository_drop_index_wo_method",
-            self._drop_local_media_index_without_method,
-        )
-
-        self.db.updates.register_background_update_handler(
-            "remote_media_repository_drop_index_wo_method",
-            self._drop_remote_media_index_without_method,
+            BG_UPDATE_REMOVE_MEDIA_REPO_INDEX_WITHOUT_METHOD,
+            self._drop_media_index_without_method,
         )
 
-    @defer.inlineCallbacks
-    def _drop_local_media_index_without_method(self, progress, batch_size):
-        def f(conn):
-            txn = conn.cursor()
+    async def _drop_media_index_without_method(self, progress, batch_size):
+        def f(txn):
             txn.execute(
                 "ALTER TABLE local_media_repository_thumbnails DROP CONSTRAINT IF EXISTS local_media_repository_thumbn_media_id_thumbnail_width_thum_key"
             )
-            txn.close()
-
-        yield self.db.runWithConnection(f)
-        yield self.db.updates._end_background_update(
-            "local_media_repository_drop_index_wo_method"
-        )
-        return 1
-
-    @defer.inlineCallbacks
-    def _drop_remote_media_index_without_method(self, progress, batch_size):
-        def f(conn):
-            txn = conn.cursor()
             txn.execute(
                 "ALTER TABLE remote_media_cache_thumbnails DROP CONSTRAINT IF EXISTS remote_media_repository_thumbn_media_id_thumbnail_width_thum_key"
             )
-            txn.close()
 
-        yield self.db.runWithConnection(f)
-        yield self.db.updates._end_background_update(
-            "remote_media_repository_drop_index_wo_method"
+        await self.db.runInteraction(f)
+        await self.db.updates._end_background_update(
+            BG_UPDATE_REMOVE_MEDIA_REPO_INDEX_WITHOUT_METHOD
         )
         return 1
 

diff --git a/...thod_to_thumbnail_constraint.sql.postgres → ...thod_to_thumbnail_constraint.sql.postgres b/...thod_to_thumbnail_constraint.sql.postgres → ...thod_to_thumbnail_constraint.sql.postgres
@@ -1,6 +1,4 @@
-/* Copyright 2020 The Matrix.org Foundation C.I.C.
- *
- * Licensed under the Apache License, Version 2.0 (the "License");
+/* Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
  * You may obtain a copy of the License at
  *
@@ -13,19 +11,23 @@
  * limitations under the License.
  */
 
+/*
+ * This adds the method to the unique key constraint of the thumbnail databases.
+ * Otherwise you can't have a scaled and a cropped thumbnail with the same
+ * resolution, which happens quite often with dynamic thumbnailing.
+ * This is the postgres specific migration modifying the table with a background
+ * migration.
+ */
+
 -- add new index that includes method to local media
 INSERT INTO background_updates (update_name, progress_json) VALUES
   ('local_media_repository_thumbnails_method_idx', '{}');
 
--- drop old index
-INSERT INTO background_updates (update_name, progress_json, depends_on) VALUES
-  ('local_media_repository_drop_index_wo_method', '{}', 'local_media_repository_thumbnails_method_idx');
-
 -- add new index that includes method to remote media
 INSERT INTO background_updates (update_name, progress_json, depends_on) VALUES
-  ('remote_media_repository_thumbnails_method_idx', '{}', 'local_media_repository_drop_index_wo_method');
+  ('remote_media_repository_thumbnails_method_idx', '{}', 'local_media_repository_thumbnails_method_idx');
 
 -- drop old index
 INSERT INTO background_updates (update_name, progress_json, depends_on) VALUES
-  ('remote_media_repository_drop_index_wo_method', '{}', 'remote_media_repository_thumbnails_method_idx');
+  ('media_repository_drop_index_wo_method', '{}', 'remote_media_repository_thumbnails_method_idx');
 
diff --git a/...method_to_thumbnail_constraint.sql.sqlite → ...method_to_thumbnail_constraint.sql.sqlite b/...method_to_thumbnail_constraint.sql.sqlite → ...method_to_thumbnail_constraint.sql.sqlite
@@ -1,6 +1,4 @@
-/* Copyright 2020 The Matrix.org Foundation C.I.C.
- *
- * Licensed under the Apache License, Version 2.0 (the "License");
+/* Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
  * You may obtain a copy of the License at
  *
@@ -13,7 +11,13 @@
  * limitations under the License.
  */
 
-DROP INDEX IF EXISTS local_media_repository_thumbnails_media_id;
+/*
+ * This adds the method to the unique key constraint of the thumbnail databases.
+ * Otherwise you can't have a scaled and a cropped thumbnail with the same
+ * resolution, which happens quite often with dynamic thumbnailing.
+ * This is a sqlite specific migration, since sqlite can't modify the unique
+ * constraint of a table without recreating it.
+ */
 
 CREATE TABLE local_media_repository_thumbnails_new ( media_id TEXT, thumbnail_width INTEGER, thumbnail_height INTEGER, thumbnail_type TEXT, thumbnail_method TEXT, thumbnail_length INTEGER, UNIQUE ( media_id, thumbnail_width, thumbnail_height, thumbnail_type, thumbnail_method ) );