From 53b77b203ac12f20a6534393464588d5d49435f5 Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Mon, 13 Jun 2022 14:06:27 -0400 Subject: [PATCH] Replace noop background updates with DELETE. (#12954) Removes the `register_noop_background_update` and deletes the background updates directly in a delta file. --- changelog.d/12954.misc | 1 + synapse/_scripts/synapse_port_db.py | 2 - synapse/storage/background_updates.py | 19 ------ synapse/storage/databases/main/__init__.py | 2 - synapse/storage/databases/main/deviceinbox.py | 11 ---- synapse/storage/databases/main/devices.py | 9 --- .../databases/main/events_bg_updates.py | 5 -- .../storage/databases/main/group_server.py | 34 ----------- .../databases/main/media_repository.py | 10 --- .../storage/databases/main/registration.py | 11 ---- synapse/storage/databases/main/search.py | 10 --- synapse/storage/databases/main/stats.py | 5 -- .../70/02remove_noop_background_updates.sql | 61 +++++++++++++++++++ tests/handlers/test_stats.py | 28 --------- 14 files changed, 62 insertions(+), 146 deletions(-) create mode 100644 changelog.d/12954.misc delete mode 100644 synapse/storage/databases/main/group_server.py create mode 100644 synapse/storage/schema/main/delta/70/02remove_noop_background_updates.sql diff --git a/changelog.d/12954.misc b/changelog.d/12954.misc new file mode 100644 index 0000000000..20bf136732 --- /dev/null +++ b/changelog.d/12954.misc @@ -0,0 +1 @@ +Replace noop background updates with `DELETE` delta. diff --git a/synapse/_scripts/synapse_port_db.py b/synapse/_scripts/synapse_port_db.py index c753dfa7cb..9586086c03 100755 --- a/synapse/_scripts/synapse_port_db.py +++ b/synapse/_scripts/synapse_port_db.py @@ -61,7 +61,6 @@ from synapse.storage.databases.main.end_to_end_keys import EndToEndKeyBackground from synapse.storage.databases.main.events_bg_updates import ( EventsBackgroundUpdatesStore, ) -from synapse.storage.databases.main.group_server import GroupServerStore from synapse.storage.databases.main.media_repository import ( MediaRepositoryBackgroundUpdateStore, ) @@ -218,7 +217,6 @@ class Store( PushRuleStore, PusherWorkerStore, PresenceBackgroundUpdateStore, - GroupServerStore, ): def execute(self, f: Callable[..., R], *args: Any, **kwargs: Any) -> Awaitable[R]: return self.db_pool.runInteraction(f.__name__, f, *args, **kwargs) diff --git a/synapse/storage/background_updates.py b/synapse/storage/background_updates.py index b1e5208c76..555b4e77d2 100644 --- a/synapse/storage/background_updates.py +++ b/synapse/storage/background_updates.py @@ -507,25 +507,6 @@ class BackgroundUpdater: update_handler ) - def register_noop_background_update(self, update_name: str) -> None: - """Register a noop handler for a background update. - - This is useful when we previously did a background update, but no - longer wish to do the update. In this case the background update should - be removed from the schema delta files, but there may still be some - users who have the background update queued, so this method should - also be called to clear the update. - - Args: - update_name: Name of update - """ - - async def noop_update(progress: JsonDict, batch_size: int) -> int: - await self._end_background_update(update_name) - return 1 - - self.register_background_update_handler(update_name, noop_update) - def register_background_index_update( self, update_name: str, diff --git a/synapse/storage/databases/main/__init__.py b/synapse/storage/databases/main/__init__.py index 11d9d16c19..9121badb3a 100644 --- a/synapse/storage/databases/main/__init__.py +++ b/synapse/storage/databases/main/__init__.py @@ -45,7 +45,6 @@ from .event_push_actions import EventPushActionsStore from .events_bg_updates import EventsBackgroundUpdatesStore from .events_forward_extremities import EventForwardExtremitiesStore from .filtering import FilteringStore -from .group_server import GroupServerStore from .keys import KeyStore from .lock import LockStore from .media_repository import MediaRepositoryStore @@ -117,7 +116,6 @@ class DataStore( DeviceStore, DeviceInboxStore, UserDirectoryStore, - GroupServerStore, UserErasureStore, MonthlyActiveUsersWorkerStore, StatsStore, diff --git a/synapse/storage/databases/main/deviceinbox.py b/synapse/storage/databases/main/deviceinbox.py index 599b418383..422e0e65ca 100644 --- a/synapse/storage/databases/main/deviceinbox.py +++ b/synapse/storage/databases/main/deviceinbox.py @@ -834,8 +834,6 @@ class DeviceInboxWorkerStore(SQLBaseStore): class DeviceInboxBackgroundUpdateStore(SQLBaseStore): DEVICE_INBOX_STREAM_ID = "device_inbox_stream_drop" - REMOVE_DELETED_DEVICES = "remove_deleted_devices_from_device_inbox" - REMOVE_HIDDEN_DEVICES = "remove_hidden_devices_from_device_inbox" REMOVE_DEAD_DEVICES_FROM_INBOX = "remove_dead_devices_from_device_inbox" def __init__( @@ -857,15 +855,6 @@ class DeviceInboxBackgroundUpdateStore(SQLBaseStore): self.DEVICE_INBOX_STREAM_ID, self._background_drop_index_device_inbox ) - # Used to be a background update that deletes all device_inboxes for deleted - # devices. - self.db_pool.updates.register_noop_background_update( - self.REMOVE_DELETED_DEVICES - ) - # Used to be a background update that deletes all device_inboxes for hidden - # devices. - self.db_pool.updates.register_noop_background_update(self.REMOVE_HIDDEN_DEVICES) - self.db_pool.updates.register_background_update_handler( self.REMOVE_DEAD_DEVICES_FROM_INBOX, self._remove_dead_devices_from_device_inbox, diff --git a/synapse/storage/databases/main/devices.py b/synapse/storage/databases/main/devices.py index 71e7863dd8..2414a7dc38 100644 --- a/synapse/storage/databases/main/devices.py +++ b/synapse/storage/databases/main/devices.py @@ -1240,15 +1240,6 @@ class DeviceBackgroundUpdateStore(SQLBaseStore): self._remove_duplicate_outbound_pokes, ) - # a pair of background updates that were added during the 1.14 release cycle, - # but replaced with 58/06dlols_unique_idx.py - self.db_pool.updates.register_noop_background_update( - "device_lists_outbound_last_success_unique_idx", - ) - self.db_pool.updates.register_noop_background_update( - "drop_device_lists_outbound_last_success_non_unique_idx", - ) - async def _drop_device_list_streams_non_unique_indexes( self, progress: JsonDict, batch_size: int ) -> int: diff --git a/synapse/storage/databases/main/events_bg_updates.py b/synapse/storage/databases/main/events_bg_updates.py index d5f0059665..bea34a4c4a 100644 --- a/synapse/storage/databases/main/events_bg_updates.py +++ b/synapse/storage/databases/main/events_bg_updates.py @@ -177,11 +177,6 @@ class EventsBackgroundUpdatesStore(SQLBaseStore): self._purged_chain_cover_index, ) - # The event_thread_relation background update was replaced with the - # event_arbitrary_relations one, which handles any relation to avoid - # needed to potentially crawl the entire events table in the future. - self.db_pool.updates.register_noop_background_update("event_thread_relation") - self.db_pool.updates.register_background_update_handler( "event_arbitrary_relations", self._event_arbitrary_relations, diff --git a/synapse/storage/databases/main/group_server.py b/synapse/storage/databases/main/group_server.py deleted file mode 100644 index c15a7136b6..0000000000 --- a/synapse/storage/databases/main/group_server.py +++ /dev/null @@ -1,34 +0,0 @@ -# Copyright 2017 Vector Creations Ltd -# Copyright 2018 New Vector Ltd -# -# 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 -# -# http://www.apache.org/licenses/LICENSE-2.0 -# -# Unless required by applicable law or agreed to in writing, software -# distributed under the License is distributed on an "AS IS" BASIS, -# 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 typing import TYPE_CHECKING - -from synapse.storage._base import SQLBaseStore -from synapse.storage.database import DatabasePool, LoggingDatabaseConnection - -if TYPE_CHECKING: - from synapse.server import HomeServer - - -class GroupServerStore(SQLBaseStore): - def __init__( - self, - database: DatabasePool, - db_conn: LoggingDatabaseConnection, - hs: "HomeServer", - ): - # Register a legacy groups background update as a no-op. - database.updates.register_noop_background_update("local_group_updates_index") - super().__init__(database, db_conn, hs) diff --git a/synapse/storage/databases/main/media_repository.py b/synapse/storage/databases/main/media_repository.py index d028be16de..9b172a64d8 100644 --- a/synapse/storage/databases/main/media_repository.py +++ b/synapse/storage/databases/main/media_repository.py @@ -37,9 +37,6 @@ from synapse.types import JsonDict, UserID if TYPE_CHECKING: from synapse.server import HomeServer -BG_UPDATE_REMOVE_MEDIA_REPO_INDEX_WITHOUT_METHOD = ( - "media_repository_drop_index_wo_method" -) BG_UPDATE_REMOVE_MEDIA_REPO_INDEX_WITHOUT_METHOD_2 = ( "media_repository_drop_index_wo_method_2" ) @@ -111,13 +108,6 @@ class MediaRepositoryBackgroundUpdateStore(SQLBaseStore): unique=True, ) - # the original impl of _drop_media_index_without_method was broken (see - # https://github.com/matrix-org/synapse/issues/8649), so we replace the original - # impl with a no-op and run the fixed migration as - # media_repository_drop_index_wo_method_2. - self.db_pool.updates.register_noop_background_update( - BG_UPDATE_REMOVE_MEDIA_REPO_INDEX_WITHOUT_METHOD - ) self.db_pool.updates.register_background_update_handler( BG_UPDATE_REMOVE_MEDIA_REPO_INDEX_WITHOUT_METHOD_2, self._drop_media_index_without_method, diff --git a/synapse/storage/databases/main/registration.py b/synapse/storage/databases/main/registration.py index 4991360b70..cb63cd9b7d 100644 --- a/synapse/storage/databases/main/registration.py +++ b/synapse/storage/databases/main/registration.py @@ -1805,21 +1805,10 @@ class RegistrationBackgroundUpdateStore(RegistrationWorkerStore): columns=["creation_ts"], ) - # we no longer use refresh tokens, but it's possible that some people - # might have a background update queued to build this index. Just - # clear the background update. - self.db_pool.updates.register_noop_background_update( - "refresh_tokens_device_index" - ) - self.db_pool.updates.register_background_update_handler( "users_set_deactivated_flag", self._background_update_set_deactivated_flag ) - self.db_pool.updates.register_noop_background_update( - "user_threepids_grandfather" - ) - self.db_pool.updates.register_background_index_update( "user_external_ids_user_id_idx", index_name="user_external_ids_user_id_idx", diff --git a/synapse/storage/databases/main/search.py b/synapse/storage/databases/main/search.py index 78e0773b2a..f6e24b68d2 100644 --- a/synapse/storage/databases/main/search.py +++ b/synapse/storage/databases/main/search.py @@ -113,7 +113,6 @@ class SearchBackgroundUpdateStore(SearchWorkerStore): EVENT_SEARCH_UPDATE_NAME = "event_search" EVENT_SEARCH_ORDER_UPDATE_NAME = "event_search_order" - EVENT_SEARCH_USE_GIST_POSTGRES_NAME = "event_search_postgres_gist" EVENT_SEARCH_USE_GIN_POSTGRES_NAME = "event_search_postgres_gin" EVENT_SEARCH_DELETE_NON_STRINGS = "event_search_sqlite_delete_non_strings" @@ -132,15 +131,6 @@ class SearchBackgroundUpdateStore(SearchWorkerStore): self.EVENT_SEARCH_ORDER_UPDATE_NAME, self._background_reindex_search_order ) - # we used to have a background update to turn the GIN index into a - # GIST one; we no longer do that (obviously) because we actually want - # a GIN index. However, it's possible that some people might still have - # the background update queued, so we register a handler to clear the - # background update. - self.db_pool.updates.register_noop_background_update( - self.EVENT_SEARCH_USE_GIST_POSTGRES_NAME - ) - self.db_pool.updates.register_background_update_handler( self.EVENT_SEARCH_USE_GIN_POSTGRES_NAME, self._background_reindex_gin_search ) diff --git a/synapse/storage/databases/main/stats.py b/synapse/storage/databases/main/stats.py index b95dbef678..538451b05f 100644 --- a/synapse/storage/databases/main/stats.py +++ b/synapse/storage/databases/main/stats.py @@ -120,11 +120,6 @@ class StatsStore(StateDeltasStore): self.db_pool.updates.register_background_update_handler( "populate_stats_process_users", self._populate_stats_process_users ) - # we no longer need to perform clean-up, but we will give ourselves - # the potential to reintroduce it in the future – so documentation - # will still encourage the use of this no-op handler. - self.db_pool.updates.register_noop_background_update("populate_stats_cleanup") - self.db_pool.updates.register_noop_background_update("populate_stats_prepare") async def _populate_stats_process_users( self, progress: JsonDict, batch_size: int diff --git a/synapse/storage/schema/main/delta/70/02remove_noop_background_updates.sql b/synapse/storage/schema/main/delta/70/02remove_noop_background_updates.sql new file mode 100644 index 0000000000..fa96ac50c2 --- /dev/null +++ b/synapse/storage/schema/main/delta/70/02remove_noop_background_updates.sql @@ -0,0 +1,61 @@ +/* Copyright 2022 The Matrix.org Foundation C.I.C + * + * 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 + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * 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. + */ + +-- Clean-up background updates which should no longer be run. Previously these +-- used the (now removed) register_noop_background_update method. + +-- Used to be a background update that deletes all device_inboxes for deleted +-- devices. +DELETE FROM background_updates WHERE update_name = 'remove_deleted_devices_from_device_inbox'; +-- Used to be a background update that deletes all device_inboxes for hidden +-- devices. +DELETE FROM background_updates WHERE update_name = 'remove_hidden_devices_from_device_inbox'; + +-- A pair of background updates that were added during the 1.14 release cycle, +-- but replaced with 58/06dlols_unique_idx.py +DELETE FROM background_updates WHERE update_name = 'device_lists_outbound_last_success_unique_idx'; +DELETE FROM background_updates WHERE update_name = 'drop_device_lists_outbound_last_success_non_unique_idx'; + +-- The event_thread_relation background update was replaced with the +-- event_arbitrary_relations one, which handles any relation to avoid +-- needed to potentially crawl the entire events table in the future. +DELETE FROM background_updates WHERE update_name = 'event_thread_relation'; + +-- A legacy groups background update. +DELETE FROM background_updates WHERE update_name = 'local_group_updates_index'; + +-- The original impl of _drop_media_index_without_method was broken (see +-- https://github.com/matrix-org/synapse/issues/8649), so we replace the original +-- impl with a no-op and run the fixed migration as +-- media_repository_drop_index_wo_method_2. +DELETE FROM background_updates WHERE update_name = 'media_repository_drop_index_wo_method'; + +-- We no longer use refresh tokens, but it's possible that some people +-- might have a background update queued to build this index. Just +-- clear the background update. +DELETE FROM background_updates WHERE update_name = 'refresh_tokens_device_index'; + +DELETE FROM background_updates WHERE update_name = 'user_threepids_grandfather'; + +-- We used to have a background update to turn the GIN index into a +-- GIST one; we no longer do that (obviously) because we actually want +-- a GIN index. However, it's possible that some people might still have +-- the background update queued, so we register a handler to clear the +-- background update. +DELETE FROM background_updates WHERE update_name = 'event_search_postgres_gist'; + +-- We no longer need to perform clean-up. +DELETE FROM background_updates WHERE update_name = 'populate_stats_cleanup'; +DELETE FROM background_updates WHERE update_name = 'populate_stats_prepare'; diff --git a/tests/handlers/test_stats.py b/tests/handlers/test_stats.py index ecd78fa369..05f9ec3c51 100644 --- a/tests/handlers/test_stats.py +++ b/tests/handlers/test_stats.py @@ -43,19 +43,12 @@ class StatsRoomTests(unittest.HomeserverTestCase): # Ugh, have to reset this flag self.store.db_pool.updates._all_done = False - self.get_success( - self.store.db_pool.simple_insert( - "background_updates", - {"update_name": "populate_stats_prepare", "progress_json": "{}"}, - ) - ) self.get_success( self.store.db_pool.simple_insert( "background_updates", { "update_name": "populate_stats_process_rooms", "progress_json": "{}", - "depends_on": "populate_stats_prepare", }, ) ) @@ -69,16 +62,6 @@ class StatsRoomTests(unittest.HomeserverTestCase): }, ) ) - self.get_success( - self.store.db_pool.simple_insert( - "background_updates", - { - "update_name": "populate_stats_cleanup", - "progress_json": "{}", - "depends_on": "populate_stats_process_users", - }, - ) - ) async def get_all_room_state(self): return await self.store.db_pool.simple_select_list( @@ -533,7 +516,6 @@ class StatsRoomTests(unittest.HomeserverTestCase): { "update_name": "populate_stats_process_rooms", "progress_json": "{}", - "depends_on": "populate_stats_prepare", }, ) ) @@ -547,16 +529,6 @@ class StatsRoomTests(unittest.HomeserverTestCase): }, ) ) - self.get_success( - self.store.db_pool.simple_insert( - "background_updates", - { - "update_name": "populate_stats_cleanup", - "progress_json": "{}", - "depends_on": "populate_stats_process_users", - }, - ) - ) self.wait_for_background_updates()