Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Remove whole table locks on push rule add/delete #16051

Merged
merged 11 commits into from
Nov 13, 2023
Prev Previous commit
Next Next commit
Fix FOR SHARE handling of push rule transactions
Fizzadar committed Aug 11, 2023

Verified

This commit was signed with the committer’s verified signature. The key has expired.
Fizzadar Nick Mills-Barrett
commit d90cef157062d1ca4baf6d8225c8f3362e765eb6
32 changes: 23 additions & 9 deletions synapse/storage/databases/main/push_rule.py
Original file line number Diff line number Diff line change
@@ -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
"""
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be worth doing a txn.execute for this SQL? 😆

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But also, you probably want to do this as part of the COUNT(*) query below?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep good spot - 2ec17da

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still missing a tx.execute here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gah, missed after rewriting it again in 2ec17da; fix: 376313e

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"