From 1b5a33e546893e728f224e32a0bba7a003470ef7 Mon Sep 17 00:00:00 2001 From: "Olivier Wilkinson (reivilibre)" Date: Fri, 17 Sep 2021 12:28:59 +0100 Subject: [PATCH 1/6] Fix the dishonest type annotations that allowed the bug We iterate twice over the same Iterable, expecting it to be the same size each time. This is not valid; instead we should demand a Collection. --- synapse/storage/database.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/synapse/storage/database.py b/synapse/storage/database.py index 0084d9f96ccc..5a2b33ccd180 100644 --- a/synapse/storage/database.py +++ b/synapse/storage/database.py @@ -1891,7 +1891,7 @@ def simple_delete_many_txn( txn: LoggingTransaction, table: str, column: str, - iterable: Iterable[Any], + iterable: Collection[Any], keyvalues: Dict[str, Any], ) -> int: """Executes a DELETE query on the named table. @@ -2098,7 +2098,7 @@ def simple_search_list_txn( def make_in_list_sql_clause( - database_engine: BaseDatabaseEngine, column: str, iterable: Iterable + database_engine: BaseDatabaseEngine, column: str, iterable: Collection[Any] ) -> Tuple[str, list]: """Returns an SQL clause that checks the given column is in the iterable. From 683bb365347831bae42e3c5eb515bd30c721b532 Mon Sep 17 00:00:00 2001 From: "Olivier Wilkinson (reivilibre)" Date: Fri, 17 Sep 2021 12:36:10 +0100 Subject: [PATCH 2/6] Rename `iterable` to `values` and clarify docstring --- synapse/storage/database.py | 17 ++++++++++------- synapse/storage/databases/main/account_data.py | 2 +- synapse/storage/databases/main/events.py | 2 +- .../storage/databases/main/events_bg_updates.py | 4 ++-- synapse/storage/databases/main/pusher.py | 4 ++-- synapse/storage/databases/main/state.py | 4 ++-- synapse/storage/databases/main/ui_auth.py | 6 +++--- synapse/storage/databases/state/store.py | 6 +++--- 8 files changed, 24 insertions(+), 21 deletions(-) diff --git a/synapse/storage/database.py b/synapse/storage/database.py index 5a2b33ccd180..224244d95181 100644 --- a/synapse/storage/database.py +++ b/synapse/storage/database.py @@ -1891,29 +1891,32 @@ def simple_delete_many_txn( txn: LoggingTransaction, table: str, column: str, - iterable: Collection[Any], + values: Collection[Any], keyvalues: Dict[str, Any], ) -> int: """Executes a DELETE query on the named table. - Filters rows by if value of `column` is in `iterable`. + Deletes the rows: + - whose value of `column` is in `values`; AND + - that match extra column-value pairs specified in `keyvalues`. Args: txn: Transaction object table: string giving the table name - column: column name to test for inclusion against `iterable` - iterable: list - keyvalues: dict of column names and values to select the rows with + column: column name to test for inclusion against `values` + values: values of `column` which choose rows to delete + keyvalues: dict of extra column names and values to select the rows + with. They will be ANDed together with the main predicate. Returns: Number rows deleted """ - if not iterable: + if not values: return 0 sql = "DELETE FROM %s" % table - clause, values = make_in_list_sql_clause(txn.database_engine, column, iterable) + clause, values = make_in_list_sql_clause(txn.database_engine, column, values) clauses = [clause] for key, value in keyvalues.items(): diff --git a/synapse/storage/databases/main/account_data.py b/synapse/storage/databases/main/account_data.py index 1d02795f4346..d0cf3460da53 100644 --- a/synapse/storage/databases/main/account_data.py +++ b/synapse/storage/databases/main/account_data.py @@ -494,7 +494,7 @@ def _add_account_data_for_user( txn, table="ignored_users", column="ignored_user_id", - iterable=previously_ignored_users - currently_ignored_users, + values=previously_ignored_users - currently_ignored_users, keyvalues={"ignorer_user_id": user_id}, ) diff --git a/synapse/storage/databases/main/events.py b/synapse/storage/databases/main/events.py index 8e691678e543..dec7e8594e9a 100644 --- a/synapse/storage/databases/main/events.py +++ b/synapse/storage/databases/main/events.py @@ -667,7 +667,7 @@ def _add_chain_cover_index( table="event_auth_chain_to_calculate", keyvalues={}, column="event_id", - iterable=new_chain_tuples, + values=new_chain_tuples, ) # Now we need to calculate any new links between chains caused by diff --git a/synapse/storage/databases/main/events_bg_updates.py b/synapse/storage/databases/main/events_bg_updates.py index 6fcb2b8353aa..1afc59fafbf2 100644 --- a/synapse/storage/databases/main/events_bg_updates.py +++ b/synapse/storage/databases/main/events_bg_updates.py @@ -490,7 +490,7 @@ def _cleanup_extremities_bg_update_txn(txn): txn=txn, table="event_forward_extremities", column="event_id", - iterable=to_delete, + values=to_delete, keyvalues={}, ) @@ -520,7 +520,7 @@ def _cleanup_extremities_bg_update_txn(txn): txn=txn, table="_extremities_to_check", column="event_id", - iterable=original_set, + values=original_set, keyvalues={}, ) diff --git a/synapse/storage/databases/main/pusher.py b/synapse/storage/databases/main/pusher.py index 63ac09c61dad..0fd53d5e4afe 100644 --- a/synapse/storage/databases/main/pusher.py +++ b/synapse/storage/databases/main/pusher.py @@ -324,7 +324,7 @@ def _delete_pushers(txn) -> int: txn, table="pushers", column="user_name", - iterable=users, + values=users, keyvalues={}, ) @@ -373,7 +373,7 @@ def _delete_pushers(txn) -> int: txn, table="pushers", column="id", - iterable=(pusher_id for pusher_id, token in pushers if token is None), + values=(pusher_id for pusher_id, token in pushers if token is None), keyvalues={}, ) diff --git a/synapse/storage/databases/main/state.py b/synapse/storage/databases/main/state.py index 8e22da99ae60..a8e8dd4577c4 100644 --- a/synapse/storage/databases/main/state.py +++ b/synapse/storage/databases/main/state.py @@ -473,7 +473,7 @@ def _background_remove_left_rooms_txn(txn): txn, table="current_state_events", column="room_id", - iterable=to_delete, + values=to_delete, keyvalues={}, ) @@ -481,7 +481,7 @@ def _background_remove_left_rooms_txn(txn): txn, table="event_forward_extremities", column="room_id", - iterable=to_delete, + values=to_delete, keyvalues={}, ) diff --git a/synapse/storage/databases/main/ui_auth.py b/synapse/storage/databases/main/ui_auth.py index 4d6bbc94c774..340ca9e47d47 100644 --- a/synapse/storage/databases/main/ui_auth.py +++ b/synapse/storage/databases/main/ui_auth.py @@ -326,7 +326,7 @@ def _delete_old_ui_auth_sessions_txn( txn, table="ui_auth_sessions_ips", column="session_id", - iterable=session_ids, + values=session_ids, keyvalues={}, ) @@ -377,7 +377,7 @@ def _delete_old_ui_auth_sessions_txn( txn, table="ui_auth_sessions_credentials", column="session_id", - iterable=session_ids, + values=session_ids, keyvalues={}, ) @@ -386,7 +386,7 @@ def _delete_old_ui_auth_sessions_txn( txn, table="ui_auth_sessions", column="session_id", - iterable=session_ids, + values=session_ids, keyvalues={}, ) diff --git a/synapse/storage/databases/state/store.py b/synapse/storage/databases/state/store.py index f1e3a27e637e..c4c8c0021bca 100644 --- a/synapse/storage/databases/state/store.py +++ b/synapse/storage/databases/state/store.py @@ -664,7 +664,7 @@ def _purge_room_state_txn( txn, table="state_groups_state", column="state_group", - iterable=state_groups_to_delete, + values=state_groups_to_delete, keyvalues={}, ) @@ -675,7 +675,7 @@ def _purge_room_state_txn( txn, table="state_group_edges", column="state_group", - iterable=state_groups_to_delete, + values=state_groups_to_delete, keyvalues={}, ) @@ -686,6 +686,6 @@ def _purge_room_state_txn( txn, table="state_groups", column="id", - iterable=state_groups_to_delete, + values=state_groups_to_delete, keyvalues={}, ) From 456e5e2ef377e527a9001ab5a10dd2905e3264ee Mon Sep 17 00:00:00 2001 From: "Olivier Wilkinson (reivilibre)" Date: Fri, 17 Sep 2021 12:38:05 +0100 Subject: [PATCH 3/6] Fix additional type signatures --- synapse/storage/database.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/synapse/storage/database.py b/synapse/storage/database.py index 224244d95181..f5a8f90a0f98 100644 --- a/synapse/storage/database.py +++ b/synapse/storage/database.py @@ -1632,7 +1632,7 @@ def simple_select_many_txn( txn: LoggingTransaction, table: str, column: str, - iterable: Iterable[Any], + iterable: Collection[Any], keyvalues: Dict[str, Any], retcols: Iterable[str], ) -> List[Dict[str, Any]]: From 1c4cf56eb8a8b921d2efb826cb38d6a094ef46f5 Mon Sep 17 00:00:00 2001 From: "Olivier Wilkinson (reivilibre)" Date: Fri, 17 Sep 2021 12:38:53 +0100 Subject: [PATCH 4/6] Fix the bug itself --- synapse/storage/databases/main/pusher.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/synapse/storage/databases/main/pusher.py b/synapse/storage/databases/main/pusher.py index 0fd53d5e4afe..a93caae8d02c 100644 --- a/synapse/storage/databases/main/pusher.py +++ b/synapse/storage/databases/main/pusher.py @@ -373,7 +373,7 @@ def _delete_pushers(txn) -> int: txn, table="pushers", column="id", - values=(pusher_id for pusher_id, token in pushers if token is None), + values=[pusher_id for pusher_id, token in pushers if token is None], keyvalues={}, ) From 60b189a6224b7645656fc5ef6a3dc06298d2790e Mon Sep 17 00:00:00 2001 From: "Olivier Wilkinson (reivilibre)" Date: Fri, 17 Sep 2021 12:40:52 +0100 Subject: [PATCH 5/6] Newsfile Signed-off-by: Olivier Wilkinson (reivilibre) --- changelog.d/10843.bugfix | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/10843.bugfix diff --git a/changelog.d/10843.bugfix b/changelog.d/10843.bugfix new file mode 100644 index 000000000000..afa98bdd5da5 --- /dev/null +++ b/changelog.d/10843.bugfix @@ -0,0 +1 @@ +Fix a bug causing the `remove_stale_pushers` background job to repeatedly fail and log errors, when SQLite is in use and Synapse is upgraded from version 1.28 or older. From 3a3565ddc90d3cbc41dd41096a62903af1eabec3 Mon Sep 17 00:00:00 2001 From: "Olivier Wilkinson (reivilibre)" Date: Fri, 17 Sep 2021 14:50:04 +0100 Subject: [PATCH 6/6] Update Newsfile Signed-off-by: Olivier Wilkinson (reivilibre) --- changelog.d/10843.bugfix | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/changelog.d/10843.bugfix b/changelog.d/10843.bugfix index afa98bdd5da5..5027a1dbefa8 100644 --- a/changelog.d/10843.bugfix +++ b/changelog.d/10843.bugfix @@ -1 +1 @@ -Fix a bug causing the `remove_stale_pushers` background job to repeatedly fail and log errors, when SQLite is in use and Synapse is upgraded from version 1.28 or older. +Fix a bug causing the `remove_stale_pushers` background job to repeatedly fail and log errors. This bug affected Synapse servers that had been upgraded from version 1.28 or older and are using SQLite.