diff --git a/changelog.d/18024.bugfix b/changelog.d/18024.bugfix new file mode 100644 index 00000000000..956f43f0362 --- /dev/null +++ b/changelog.d/18024.bugfix @@ -0,0 +1 @@ +Properly purge state groups tables when purging a room with the admin API. diff --git a/synapse/storage/controllers/purge_events.py b/synapse/storage/controllers/purge_events.py index e794b370c25..b65be2257ce 100644 --- a/synapse/storage/controllers/purge_events.py +++ b/synapse/storage/controllers/purge_events.py @@ -42,8 +42,7 @@ async def purge_room(self, room_id: str) -> None: """Deletes all record of a room""" with nested_logging_context(room_id): - state_groups_to_delete = await self.stores.main.purge_room(room_id) - await self.stores.state.purge_room_state(room_id, state_groups_to_delete) + await self.stores.main.purge_room(room_id) async def purge_history( self, room_id: str, token: str, delete_local_events: bool diff --git a/synapse/storage/databases/main/purge_events.py b/synapse/storage/databases/main/purge_events.py index 08244153a39..c0b2f513ece 100644 --- a/synapse/storage/databases/main/purge_events.py +++ b/synapse/storage/databases/main/purge_events.py @@ -20,7 +20,7 @@ # import logging -from typing import Any, List, Set, Tuple, cast +from typing import Any, Set, Tuple, cast from synapse.api.errors import SynapseError from synapse.storage.database import LoggingTransaction @@ -332,7 +332,7 @@ def _purge_history_txn( return referenced_state_groups - async def purge_room(self, room_id: str) -> List[int]: + async def purge_room(self, room_id: str) -> None: """Deletes all record of a room Args: @@ -348,7 +348,7 @@ async def purge_room(self, room_id: str) -> List[int]: # purge any of those rows which were added during the first. logger.info("[purge] Starting initial main purge of [1/2]") - state_groups_to_delete = await self.db_pool.runInteraction( + await self.db_pool.runInteraction( "purge_room", self._purge_room_txn, room_id=room_id, @@ -356,18 +356,15 @@ async def purge_room(self, room_id: str) -> List[int]: ) logger.info("[purge] Starting secondary main purge of [2/2]") - state_groups_to_delete.extend( - await self.db_pool.runInteraction( - "purge_room", - self._purge_room_txn, - room_id=room_id, - ), + await self.db_pool.runInteraction( + "purge_room", + self._purge_room_txn, + room_id=room_id, ) - logger.info("[purge] Done with main purge") - return state_groups_to_delete + logger.info("[purge] Done with main purge") - def _purge_room_txn(self, txn: LoggingTransaction, room_id: str) -> List[int]: + def _purge_room_txn(self, txn: LoggingTransaction, room_id: str) -> None: # This collides with event persistence so we cannot write new events and metadata into # a room while deleting it or this transaction will fail. if isinstance(self.database_engine, PostgresEngine): @@ -376,19 +373,6 @@ def _purge_room_txn(self, txn: LoggingTransaction, room_id: str) -> List[int]: (room_id,), ) - # First, fetch all the state groups that should be deleted, before - # we delete that information. - txn.execute( - """ - SELECT DISTINCT state_group FROM events - INNER JOIN event_to_state_groups USING(event_id) - WHERE events.room_id = ? - """, - (room_id,), - ) - - state_groups = [row[0] for row in txn] - # Get all the auth chains that are referenced by events that are to be # deleted. txn.execute( @@ -484,6 +468,8 @@ def _purge_room_txn(self, txn: LoggingTransaction, room_id: str) -> List[int]: logger.info("[purge] removing from %s", table) txn.execute("DELETE FROM %s WHERE room_id=?" % (table,), (room_id,)) + self._purge_room_state_txn(txn, room_id) + # Other tables we do NOT need to clear out: # # - blocked_rooms @@ -509,4 +495,37 @@ def _purge_room_txn(self, txn: LoggingTransaction, room_id: str) -> List[int]: self._invalidate_caches_for_room_and_stream(txn, room_id) - return state_groups + def _purge_room_state_txn( + self, + txn: LoggingTransaction, + room_id: str, + ) -> None: + # Delete all edges that reference a state group linked to room_id + logger.info("[purge] removing %s from state_group_edges", room_id) + txn.execute( + """ + DELETE FROM state_group_edges AS sge WHERE sge.state_group IN ( + SELECT id FROM state_groups AS sg WHERE sg.room_id = ? + )""", + (room_id,), + ) + + # state_groups_state table has a room_id column but no index on it, unlike state_groups, + # so we delete them by matching the room_id through the state_groups table. + logger.info("[purge] removing %s from state_groups_state", room_id) + txn.execute( + """ + DELETE FROM state_groups_state AS sgs WHERE sgs.state_group IN ( + SELECT id FROM state_groups AS sg WHERE sg.room_id = ? + )""", + (room_id,), + ) + + logger.info("[purge] removing %s from state_groups", room_id) + self.db_pool.simple_delete_many_txn( + txn, + table="state_groups", + column="room_id", + values=[room_id], + keyvalues={}, + ) diff --git a/synapse/storage/databases/state/store.py b/synapse/storage/databases/state/store.py index f7a59c8992d..871253e2831 100644 --- a/synapse/storage/databases/state/store.py +++ b/synapse/storage/databases/state/store.py @@ -839,61 +839,3 @@ async def get_previous_state_groups( ) return dict(rows) - - async def purge_room_state( - self, room_id: str, state_groups_to_delete: Collection[int] - ) -> None: - """Deletes all record of a room from state tables - - Args: - room_id: - state_groups_to_delete: State groups to delete - """ - - logger.info("[purge] Starting state purge") - await self.db_pool.runInteraction( - "purge_room_state", - self._purge_room_state_txn, - room_id, - state_groups_to_delete, - ) - logger.info("[purge] Done with state purge") - - def _purge_room_state_txn( - self, - txn: LoggingTransaction, - room_id: str, - state_groups_to_delete: Collection[int], - ) -> None: - # first we have to delete the state groups states - logger.info("[purge] removing %s from state_groups_state", room_id) - - self.db_pool.simple_delete_many_txn( - txn, - table="state_groups_state", - column="state_group", - values=state_groups_to_delete, - keyvalues={}, - ) - - # ... and the state group edges - logger.info("[purge] removing %s from state_group_edges", room_id) - - self.db_pool.simple_delete_many_txn( - txn, - table="state_group_edges", - column="state_group", - values=state_groups_to_delete, - keyvalues={}, - ) - - # ... and the state groups - logger.info("[purge] removing %s from state_groups", room_id) - - self.db_pool.simple_delete_many_txn( - txn, - table="state_groups", - column="id", - values=state_groups_to_delete, - keyvalues={}, - ) diff --git a/tests/rest/admin/test_room.py b/tests/rest/admin/test_room.py index 95ed7364510..99df5915290 100644 --- a/tests/rest/admin/test_room.py +++ b/tests/rest/admin/test_room.py @@ -3050,7 +3050,7 @@ def _block_room(self, room_id: str) -> None: "pusher_throttle", "room_account_data", "room_tags", - # "state_groups", # Current impl leaves orphaned state groups around. + "state_groups", "state_groups_state", "federation_inbound_events_staging", ]