From 44073cc421dd8287a774571aa0456e7501f9b128 Mon Sep 17 00:00:00 2001 From: Andrew Morgan Date: Tue, 16 Nov 2021 11:18:55 +0000 Subject: [PATCH 1/4] Rename remove_deleted_devices_from_device_inbox to ensure it is always run --- synapse/storage/databases/main/deviceinbox.py | 31 ++++++++++++++++--- ...ove_deleted_devices_from_device_inbox.sql} | 8 ++++- .../databases/main/test_deviceinbox.py | 2 +- 3 files changed, 34 insertions(+), 7 deletions(-) rename synapse/storage/schema/main/delta/65/{05remove_deleted_devices_from_device_inbox.sql => 06remove_deleted_devices_from_device_inbox.sql} (67%) diff --git a/synapse/storage/databases/main/deviceinbox.py b/synapse/storage/databases/main/deviceinbox.py index 264e625bd713..7743b4613e6e 100644 --- a/synapse/storage/databases/main/deviceinbox.py +++ b/synapse/storage/databases/main/deviceinbox.py @@ -560,8 +560,11 @@ def _add_messages_to_local_device_inbox_txn( 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_DELETED_DEVICES = "remove_deleted_devices_from_device_inbox" + # See '05remove_deleted_devices_from_device_inbox.sql' for an explanation on + # why this is needed. + REMOVE_DELETED_DEVICES_V2 = "remove_deleted_devices_from_device_inbox_v2" def __init__(self, database: DatabasePool, db_conn, hs: "HomeServer"): super().__init__(database, db_conn, hs) @@ -582,6 +585,11 @@ def __init__(self, database: DatabasePool, db_conn, hs: "HomeServer"): self._remove_deleted_devices_from_device_inbox, ) + self.db_pool.updates.register_background_update_handler( + self.REMOVE_DELETED_DEVICES_V2, + self._remove_deleted_devices_from_device_inbox_v2, + ) + self.db_pool.updates.register_background_update_handler( self.REMOVE_HIDDEN_DEVICES, self._remove_hidden_devices_from_device_inbox, @@ -599,12 +607,15 @@ def reindex_txn(conn): return 1 - async def _remove_deleted_devices_from_device_inbox( + async def _remove_deleted_devices_from_device_inbox_v2( self, progress: JsonDict, batch_size: int ) -> int: """A background update that deletes all device_inboxes for deleted devices. - This should only need to be run once (when users upgrade to v1.47.0) + This should only need to be run once (when users upgrade to v1.47.0). + + See '05remove_deleted_devices_from_device_inbox.sql' for an explanation on + why this is needed. Args: progress: JsonDict used to store progress of this background update @@ -659,7 +670,7 @@ def _remove_deleted_devices_from_device_inbox_txn( # it may be that the stream_id does not change in several runs self.db_pool.updates._background_update_progress_txn( txn, - self.REMOVE_DELETED_DEVICES, + self.REMOVE_DELETED_DEVICES_V2, { "device_id": rows[-1][0], "user_id": rows[-1][1], @@ -677,11 +688,21 @@ def _remove_deleted_devices_from_device_inbox_txn( # The task is finished when no more lines are deleted. if not number_deleted: await self.db_pool.updates._end_background_update( - self.REMOVE_DELETED_DEVICES + self.REMOVE_DELETED_DEVICES_V2 ) return number_deleted + async def _remove_deleted_devices_from_device_inbox( + self, progress: JsonDict, batch_size: int + ) -> int: + """ + This background update has been converted to a no-op. See + _remove_deleted_devices_from_device_inbox_v2 instead. + """ + await self.db_pool.updates._end_background_update(self.REMOVE_DELETED_DEVICES) + return 0 + async def _remove_hidden_devices_from_device_inbox( self, progress: JsonDict, batch_size: int ) -> int: diff --git a/synapse/storage/schema/main/delta/65/05remove_deleted_devices_from_device_inbox.sql b/synapse/storage/schema/main/delta/65/06remove_deleted_devices_from_device_inbox.sql similarity index 67% rename from synapse/storage/schema/main/delta/65/05remove_deleted_devices_from_device_inbox.sql rename to synapse/storage/schema/main/delta/65/06remove_deleted_devices_from_device_inbox.sql index 076179123dbd..6e72e793cd39 100644 --- a/synapse/storage/schema/main/delta/65/05remove_deleted_devices_from_device_inbox.sql +++ b/synapse/storage/schema/main/delta/65/06remove_deleted_devices_from_device_inbox.sql @@ -18,5 +18,11 @@ -- when a device was deleted using Synapse earlier than 1.47.0. -- This runs as background task, but may take a bit to finish. +-- The name of this update ending is '_v2' is due to it accidentally +-- being included in schema version 64 during v1.47.0rc1,rc2. If a +-- homeserver had updated from Synapse <=v1.45.0 (schema version <=64), +-- then they would have run the original version of this background update +-- already. So we rename it here, to ensure it is run regardless of upgrade path. + INSERT INTO background_updates (ordering, update_name, progress_json) VALUES - (6505, 'remove_deleted_devices_from_device_inbox', '{}'); + (6506, 'remove_deleted_devices_from_device_inbox_v2', '{}'); diff --git a/tests/storage/databases/main/test_deviceinbox.py b/tests/storage/databases/main/test_deviceinbox.py index 4b67bd15b75b..23cc5c57198c 100644 --- a/tests/storage/databases/main/test_deviceinbox.py +++ b/tests/storage/databases/main/test_deviceinbox.py @@ -66,7 +66,7 @@ def test_background_remove_deleted_devices_from_device_inbox(self): self.store.db_pool.simple_insert( "background_updates", { - "update_name": "remove_deleted_devices_from_device_inbox", + "update_name": "remove_deleted_devices_from_device_inbox_v2", "progress_json": "{}", }, ) From 02bef0c0c4c69f2a065d42f91b21d7cc48f4c4f9 Mon Sep 17 00:00:00 2001 From: Andrew Morgan Date: Tue, 16 Nov 2021 11:32:37 +0000 Subject: [PATCH 2/4] Changelog --- changelog.d/11353.misc | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/11353.misc diff --git a/changelog.d/11353.misc b/changelog.d/11353.misc new file mode 100644 index 000000000000..419b9bdf1f43 --- /dev/null +++ b/changelog.d/11353.misc @@ -0,0 +1 @@ +Fix an issue which prevented the 'remove deleted devices from device_inbox column' background process from running when updating from a recent Synapse version. \ No newline at end of file From 92b06815114f72419a5d3ecc60967beeeecc5688 Mon Sep 17 00:00:00 2001 From: Andrew Morgan Date: Tue, 16 Nov 2021 12:40:56 +0000 Subject: [PATCH 3/4] Switch to upserting bg update name, rather than renaming --- synapse/storage/databases/main/deviceinbox.py | 31 +++---------------- ...move_deleted_devices_from_device_inbox.sql | 14 ++++++--- .../databases/main/test_deviceinbox.py | 2 +- 3 files changed, 16 insertions(+), 31 deletions(-) diff --git a/synapse/storage/databases/main/deviceinbox.py b/synapse/storage/databases/main/deviceinbox.py index 7743b4613e6e..264e625bd713 100644 --- a/synapse/storage/databases/main/deviceinbox.py +++ b/synapse/storage/databases/main/deviceinbox.py @@ -560,11 +560,8 @@ def _add_messages_to_local_device_inbox_txn( class DeviceInboxBackgroundUpdateStore(SQLBaseStore): DEVICE_INBOX_STREAM_ID = "device_inbox_stream_drop" - REMOVE_HIDDEN_DEVICES = "remove_hidden_devices_from_device_inbox" REMOVE_DELETED_DEVICES = "remove_deleted_devices_from_device_inbox" - # See '05remove_deleted_devices_from_device_inbox.sql' for an explanation on - # why this is needed. - REMOVE_DELETED_DEVICES_V2 = "remove_deleted_devices_from_device_inbox_v2" + REMOVE_HIDDEN_DEVICES = "remove_hidden_devices_from_device_inbox" def __init__(self, database: DatabasePool, db_conn, hs: "HomeServer"): super().__init__(database, db_conn, hs) @@ -585,11 +582,6 @@ def __init__(self, database: DatabasePool, db_conn, hs: "HomeServer"): self._remove_deleted_devices_from_device_inbox, ) - self.db_pool.updates.register_background_update_handler( - self.REMOVE_DELETED_DEVICES_V2, - self._remove_deleted_devices_from_device_inbox_v2, - ) - self.db_pool.updates.register_background_update_handler( self.REMOVE_HIDDEN_DEVICES, self._remove_hidden_devices_from_device_inbox, @@ -607,15 +599,12 @@ def reindex_txn(conn): return 1 - async def _remove_deleted_devices_from_device_inbox_v2( + async def _remove_deleted_devices_from_device_inbox( self, progress: JsonDict, batch_size: int ) -> int: """A background update that deletes all device_inboxes for deleted devices. - This should only need to be run once (when users upgrade to v1.47.0). - - See '05remove_deleted_devices_from_device_inbox.sql' for an explanation on - why this is needed. + This should only need to be run once (when users upgrade to v1.47.0) Args: progress: JsonDict used to store progress of this background update @@ -670,7 +659,7 @@ def _remove_deleted_devices_from_device_inbox_txn( # it may be that the stream_id does not change in several runs self.db_pool.updates._background_update_progress_txn( txn, - self.REMOVE_DELETED_DEVICES_V2, + self.REMOVE_DELETED_DEVICES, { "device_id": rows[-1][0], "user_id": rows[-1][1], @@ -688,21 +677,11 @@ def _remove_deleted_devices_from_device_inbox_txn( # The task is finished when no more lines are deleted. if not number_deleted: await self.db_pool.updates._end_background_update( - self.REMOVE_DELETED_DEVICES_V2 + self.REMOVE_DELETED_DEVICES ) return number_deleted - async def _remove_deleted_devices_from_device_inbox( - self, progress: JsonDict, batch_size: int - ) -> int: - """ - This background update has been converted to a no-op. See - _remove_deleted_devices_from_device_inbox_v2 instead. - """ - await self.db_pool.updates._end_background_update(self.REMOVE_DELETED_DEVICES) - return 0 - async def _remove_hidden_devices_from_device_inbox( self, progress: JsonDict, batch_size: int ) -> int: diff --git a/synapse/storage/schema/main/delta/65/06remove_deleted_devices_from_device_inbox.sql b/synapse/storage/schema/main/delta/65/06remove_deleted_devices_from_device_inbox.sql index 6e72e793cd39..82f6408b3634 100644 --- a/synapse/storage/schema/main/delta/65/06remove_deleted_devices_from_device_inbox.sql +++ b/synapse/storage/schema/main/delta/65/06remove_deleted_devices_from_device_inbox.sql @@ -18,11 +18,17 @@ -- when a device was deleted using Synapse earlier than 1.47.0. -- This runs as background task, but may take a bit to finish. --- The name of this update ending is '_v2' is due to it accidentally +-- Remove any existing instances of this job running. It's OK to stop and restart this job, +-- as it's just deleting entries from a table - no progress will be lost. +-- +-- This is necessary due a similar migration running the job accidentally -- being included in schema version 64 during v1.47.0rc1,rc2. If a -- homeserver had updated from Synapse <=v1.45.0 (schema version <=64), --- then they would have run the original version of this background update --- already. So we rename it here, to ensure it is run regardless of upgrade path. +-- then they would have started running this background update already. +-- If that update was still running, then simply inserting it again would +-- cause an SQL failure. So we effectively do an "upsert" here instead. + +DELETE FROM background_updates WHERE update_name = 'remove_deleted_devices_from_device_inbox'; INSERT INTO background_updates (ordering, update_name, progress_json) VALUES - (6506, 'remove_deleted_devices_from_device_inbox_v2', '{}'); + (6506, 'remove_deleted_devices_from_device_inbox', '{}'); diff --git a/tests/storage/databases/main/test_deviceinbox.py b/tests/storage/databases/main/test_deviceinbox.py index 23cc5c57198c..4b67bd15b75b 100644 --- a/tests/storage/databases/main/test_deviceinbox.py +++ b/tests/storage/databases/main/test_deviceinbox.py @@ -66,7 +66,7 @@ def test_background_remove_deleted_devices_from_device_inbox(self): self.store.db_pool.simple_insert( "background_updates", { - "update_name": "remove_deleted_devices_from_device_inbox_v2", + "update_name": "remove_deleted_devices_from_device_inbox", "progress_json": "{}", }, ) From 030a50a8e38f86b6d61d1c55226220e609a17b64 Mon Sep 17 00:00:00 2001 From: Andrew Morgan <1342360+anoadragon453@users.noreply.github.com> Date: Tue, 16 Nov 2021 12:48:15 +0000 Subject: [PATCH 4/4] Update changelog.d/11353.misc Co-authored-by: reivilibre --- changelog.d/11353.misc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/changelog.d/11353.misc b/changelog.d/11353.misc index 419b9bdf1f43..fa96dae9194e 100644 --- a/changelog.d/11353.misc +++ b/changelog.d/11353.misc @@ -1 +1 @@ -Fix an issue which prevented the 'remove deleted devices from device_inbox column' background process from running when updating from a recent Synapse version. \ No newline at end of file +Fix an issue which prevented the 'remove deleted devices from `device_inbox` column' background process from running when updating from a recent Synapse version. \ No newline at end of file