From 32e0bd3d634caeb356ab92b8109c66dc65b605ee Mon Sep 17 00:00:00 2001 From: Nick Mills-Barrett Date: Wed, 2 Aug 2023 14:02:32 +0100 Subject: [PATCH 1/9] Remove whole table locks on push rule add/delete The statements are already executed within a transaction thus a table level lock is unnecessary. --- synapse/storage/databases/main/push_rule.py | 8 -------- 1 file changed, 8 deletions(-) diff --git a/synapse/storage/databases/main/push_rule.py b/synapse/storage/databases/main/push_rule.py index c13c0bc7d725..23a9f7f53488 100644 --- a/synapse/storage/databases/main/push_rule.py +++ b/synapse/storage/databases/main/push_rule.py @@ -396,10 +396,6 @@ def _add_push_rule_relative_txn( before: str, after: str, ) -> None: - # Lock the table since otherwise we'll have annoying races between the - # SELECT here and the UPSERT below. - self.database_engine.lock_table(txn, "push_rules") - relative_to_rule = before or after res = self.db_pool.simple_select_one_txn( @@ -464,10 +460,6 @@ def _add_push_rule_highest_priority_txn( conditions_json: str, actions_json: str, ) -> None: - # Lock the table since otherwise we'll have annoying races between the - # SELECT here and the UPSERT below. - self.database_engine.lock_table(txn, "push_rules") - # find the highest priority rule in that class sql = ( "SELECT COUNT(*), MAX(priority) FROM push_rules" From 0938542a88671824be87b57f1926aa9825a11ee3 Mon Sep 17 00:00:00 2001 From: Nick Mills-Barrett Date: Wed, 2 Aug 2023 14:07:26 +0100 Subject: [PATCH 2/9] Add changelog --- changelog.d/16051.misc | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/16051.misc diff --git a/changelog.d/16051.misc b/changelog.d/16051.misc new file mode 100644 index 000000000000..1420d2eb3f36 --- /dev/null +++ b/changelog.d/16051.misc @@ -0,0 +1 @@ +Remove whole table locks on push rule modifications. Contributed by Nick @ Beeper (@fizzadar). From ab88d3fdf4b8f6be612a863c15798b91752105aa Mon Sep 17 00:00:00 2001 From: Nick Mills-Barrett Date: Fri, 11 Aug 2023 14:16:01 +0100 Subject: [PATCH 3/9] Use `FOR SHARE` to lock selected push rule rows on relative update --- synapse/storage/databases/main/push_rule.py | 21 ++++++++++++--------- 1 file changed, 12 insertions(+), 9 deletions(-) diff --git a/synapse/storage/databases/main/push_rule.py b/synapse/storage/databases/main/push_rule.py index 23a9f7f53488..89ecabce9603 100644 --- a/synapse/storage/databases/main/push_rule.py +++ b/synapse/storage/databases/main/push_rule.py @@ -398,21 +398,24 @@ def _add_push_rule_relative_txn( ) -> None: relative_to_rule = before or after - res = self.db_pool.simple_select_one_txn( - txn, - table="push_rules", - keyvalues={"user_name": user_id, "rule_id": relative_to_rule}, - retcols=["priority_class", "priority"], - allow_none=True, + txn.execute( + """ + SELECT priority, priority_class FROM push_rules FOR SHARE + WHERE user_name = ? AND rule_id = ? + """, + ( + user_id, + relative_to_rule, + ), ) + row = txn.fetchone() - if not res: + if row is None: raise RuleNotFoundException( "before/after rule not found: %s" % (relative_to_rule,) ) - base_priority_class = res["priority_class"] - base_rule_priority = res["priority"] + base_rule_priority, base_priority_class = row if base_priority_class != priority_class: raise InconsistentRuleException( From d90cef157062d1ca4baf6d8225c8f3362e765eb6 Mon Sep 17 00:00:00 2001 From: Nick Mills-Barrett Date: Fri, 11 Aug 2023 17:46:45 +0100 Subject: [PATCH 4/9] Fix `FOR SHARE` handling of push rule transactions --- synapse/storage/databases/main/push_rule.py | 32 +++++++++++++++------ 1 file changed, 23 insertions(+), 9 deletions(-) diff --git a/synapse/storage/databases/main/push_rule.py b/synapse/storage/databases/main/push_rule.py index 89ecabce9603..66a9d572f28d 100644 --- a/synapse/storage/databases/main/push_rule.py +++ b/synapse/storage/databases/main/push_rule.py @@ -398,16 +398,18 @@ def _add_push_rule_relative_txn( ) -> None: relative_to_rule = before or after - txn.execute( - """ - SELECT priority, priority_class FROM push_rules FOR SHARE + sql = """ + SELECT priority, priority_class FROM push_rules WHERE user_name = ? AND rule_id = ? - """, - ( - user_id, - relative_to_rule, - ), - ) + """ + + if isinstance(self.database_engine, PostgresEngine): + sql += " FOR SHARE" + else: + # Annoyingly SQLite doesn't support row level locking, so lock the whole table + self.database_engine.lock_table(txn, "push_rules") + + txn.execute(sql, (user_id, relative_to_rule)) row = txn.fetchone() if row is None: @@ -463,6 +465,18 @@ def _add_push_rule_highest_priority_txn( conditions_json: str, actions_json: str, ) -> None: + if isinstance(self.database_engine, PostgresEngine): + # Postgres doesn't do FOR SHARE on aggregate functions, so select the rows first + # then re-select the count/max below. + sql = """ + SELECT * FROM push_rules + WHERE user_name = ? and priority_class = ? + FOR SHARE + """ + else: + # Annoyingly SQLite doesn't support row level locking, so lock the whole table + self.database_engine.lock_table(txn, "push_rules") + # find the highest priority rule in that class sql = ( "SELECT COUNT(*), MAX(priority) FROM push_rules" From 4dbee684885ffe3784b6548ea824755725b392d8 Mon Sep 17 00:00:00 2001 From: Nick Mills-Barrett Date: Sun, 5 Nov 2023 11:11:26 +0000 Subject: [PATCH 5/9] Use `FOR UPDATE` to ensure selects conflict --- synapse/storage/databases/main/push_rule.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/synapse/storage/databases/main/push_rule.py b/synapse/storage/databases/main/push_rule.py index 66a9d572f28d..17ea6907f1cd 100644 --- a/synapse/storage/databases/main/push_rule.py +++ b/synapse/storage/databases/main/push_rule.py @@ -404,7 +404,7 @@ def _add_push_rule_relative_txn( """ if isinstance(self.database_engine, PostgresEngine): - sql += " FOR SHARE" + sql += " FOR UPDATE" else: # Annoyingly SQLite doesn't support row level locking, so lock the whole table self.database_engine.lock_table(txn, "push_rules") From 2ec17da8985afb78ae619c72d7eaebccc38afbef Mon Sep 17 00:00:00 2001 From: Nick Mills-Barrett Date: Sun, 5 Nov 2023 11:11:56 +0000 Subject: [PATCH 6/9] Rewrite highest priority rule txn to use FOR UPDATE in one query --- synapse/storage/databases/main/push_rule.py | 17 ++++++----------- 1 file changed, 6 insertions(+), 11 deletions(-) diff --git a/synapse/storage/databases/main/push_rule.py b/synapse/storage/databases/main/push_rule.py index 17ea6907f1cd..0aa229a44be8 100644 --- a/synapse/storage/databases/main/push_rule.py +++ b/synapse/storage/databases/main/push_rule.py @@ -465,23 +465,18 @@ def _add_push_rule_highest_priority_txn( conditions_json: str, actions_json: str, ) -> None: + sql = """ + SELECT COUNT(*), MAX(priority) FROM push_rules" + WHERE user_name = ? and priority_class = ? + """ + if isinstance(self.database_engine, PostgresEngine): - # Postgres doesn't do FOR SHARE on aggregate functions, so select the rows first - # then re-select the count/max below. - sql = """ - SELECT * FROM push_rules - WHERE user_name = ? and priority_class = ? - FOR SHARE - """ + sql += " FOR UPDATE" else: # Annoyingly SQLite doesn't support row level locking, so lock the whole table self.database_engine.lock_table(txn, "push_rules") # find the highest priority rule in that class - sql = ( - "SELECT COUNT(*), MAX(priority) FROM push_rules" - " WHERE user_name = ? and priority_class = ?" - ) txn.execute(sql, (user_id, priority_class)) res = txn.fetchall() (how_many, highest_prio) = res[0] From 7d10544ebcb7b5ffee83f9f8f0d20c19b19e2075 Mon Sep 17 00:00:00 2001 From: Nick Mills-Barrett Date: Sun, 5 Nov 2023 11:25:54 +0000 Subject: [PATCH 7/9] Remove leftover quote --- synapse/storage/databases/main/push_rule.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/synapse/storage/databases/main/push_rule.py b/synapse/storage/databases/main/push_rule.py index 90fb106824a0..bb11a3da5f2d 100644 --- a/synapse/storage/databases/main/push_rule.py +++ b/synapse/storage/databases/main/push_rule.py @@ -507,7 +507,7 @@ def _add_push_rule_highest_priority_txn( actions_json: str, ) -> None: sql = """ - SELECT COUNT(*), MAX(priority) FROM push_rules" + SELECT COUNT(*), MAX(priority) FROM push_rules WHERE user_name = ? and priority_class = ? """ From 576605e0d0d78f4d70cbd853d286492fee6b5d57 Mon Sep 17 00:00:00 2001 From: Nick Mills-Barrett Date: Sun, 5 Nov 2023 11:48:31 +0000 Subject: [PATCH 8/9] Revert "Rewrite highest priority rule txn to use FOR UPDATE in one query" This reverts commit 2ec17da8985afb78ae619c72d7eaebccc38afbef. # Conflicts: # synapse/storage/databases/main/push_rule.py --- synapse/storage/databases/main/push_rule.py | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-) diff --git a/synapse/storage/databases/main/push_rule.py b/synapse/storage/databases/main/push_rule.py index bb11a3da5f2d..a9ce6cf220a4 100644 --- a/synapse/storage/databases/main/push_rule.py +++ b/synapse/storage/databases/main/push_rule.py @@ -506,18 +506,23 @@ def _add_push_rule_highest_priority_txn( conditions_json: str, actions_json: str, ) -> None: - sql = """ - SELECT COUNT(*), MAX(priority) FROM push_rules - WHERE user_name = ? and priority_class = ? - """ - if isinstance(self.database_engine, PostgresEngine): - sql += " FOR UPDATE" + # Postgres doesn't do FOR UPDATE on aggregate functions, so select the rows first + # then re-select the count/max below. + sql = """ + SELECT * FROM push_rules + WHERE user_name = ? and priority_class = ? + FOR UPDATE + """ else: # Annoyingly SQLite doesn't support row level locking, so lock the whole table self.database_engine.lock_table(txn, "push_rules") # find the highest priority rule in that class + sql = ( + "SELECT COUNT(*), MAX(priority) FROM push_rules" + " WHERE user_name = ? and priority_class = ?" + ) txn.execute(sql, (user_id, priority_class)) res = txn.fetchall() (how_many, highest_prio) = res[0] From 376313e40bf44350360af04b82b8e72d95b18c4a Mon Sep 17 00:00:00 2001 From: Nick Mills-Barrett Date: Wed, 8 Nov 2023 16:19:05 +0000 Subject: [PATCH 9/9] Add missing `txn.execute` on `FOR UPDATE` statement --- synapse/storage/databases/main/push_rule.py | 1 + 1 file changed, 1 insertion(+) diff --git a/synapse/storage/databases/main/push_rule.py b/synapse/storage/databases/main/push_rule.py index a9ce6cf220a4..543890319a4e 100644 --- a/synapse/storage/databases/main/push_rule.py +++ b/synapse/storage/databases/main/push_rule.py @@ -514,6 +514,7 @@ def _add_push_rule_highest_priority_txn( WHERE user_name = ? and priority_class = ? FOR UPDATE """ + txn.execute(sql, (user_id, priority_class)) else: # Annoyingly SQLite doesn't support row level locking, so lock the whole table self.database_engine.lock_table(txn, "push_rules")