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

Make the get_global_account_data_by_type_for_user cache be a tree-cache whose key is prefixed with the user ID #11788

Merged
merged 2 commits into from
Jan 21, 2022
Merged

Make the get_global_account_data_by_type_for_user cache be a tree-cache whose key is prefixed with the user ID #11788

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1c13c2a
Make `get_global_account_data_by_type_for_user` tree-cached with a pr…
reivilibre Jan 20, 2022
3ac7933
Newsfile
reivilibre Jan 20, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions changelog.d/11788.feature
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Remove account data (including client config, push rules and ignored users) upon user deactivation.
2 changes: 1 addition & 1 deletion synapse/handlers/sync.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1619,7 +1619,7 @@ async def _get_ignored_users(self, user_id: str) -> FrozenSet[str]:
# TODO: Can we `SELECT ignored_user_id FROM ignored_users WHERE ignorer_user_id=?;` instead?
ignored_account_data = (
await self.store.get_global_account_data_by_type_for_user(
AccountDataTypes.IGNORED_USER_LIST, user_id=user_id
user_id=user_id, data_type=AccountDataTypes.IGNORED_USER_LIST
Copy link
Contributor Author

@reivilibre reivilibre Jan 20, 2022

Choose a reason for hiding this comment

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

How does this interact with the caching? It uses kwargs so I don't know how our caching handles that

Copy link
Member

@clokep clokep Jan 20, 2022

Choose a reason for hiding this comment

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

I think the cached functions just base it off name, so this should work OK. I would be fine with you making them into normal args if you'd prefer though.

Copy link
Contributor Author

@reivilibre reivilibre Jan 21, 2022

Choose a reason for hiding this comment

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

I dug in to the (slightly confusing) code a bit and it looks like you're right :). Good to know.

)
)

Expand Down
2 changes: 1 addition & 1 deletion synapse/rest/client/account_data.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ async def on_GET(
raise AuthError(403, "Cannot get account data for other users.")

event = await self.store.get_global_account_data_by_type_for_user(
account_data_type, user_id
user_id, account_data_type
)

if event is None:
Expand Down
8 changes: 4 additions & 4 deletions synapse/storage/databases/main/account_data.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -158,9 +158,9 @@ def get_account_data_for_user_txn(
"get_account_data_for_user", get_account_data_for_user_txn
)

@cached(num_args=2, max_entries=5000)
@cached(num_args=2, max_entries=5000, tree=True)
async def get_global_account_data_by_type_for_user(
self, data_type: str, user_id: str
self, user_id: str, data_type: str
) -> Optional[JsonDict]:
"""
Returns:
Expand Down Expand Up @@ -392,7 +392,7 @@ def process_replication_rows(
for row in rows:
if not row.room_id:
self.get_global_account_data_by_type_for_user.invalidate(
(row.data_type, row.user_id)
(row.user_id, row.data_type)
)
self.get_account_data_for_user.invalidate((row.user_id,))
self.get_account_data_for_room.invalidate((row.user_id, row.room_id))
Expand Down Expand Up @@ -476,7 +476,7 @@ async def add_account_data_for_user(
self._account_data_stream_cache.entity_has_changed(user_id, next_id)
self.get_account_data_for_user.invalidate((user_id,))
self.get_global_account_data_by_type_for_user.invalidate(
(account_data_type, user_id)
(user_id, account_data_type)
)
Comment on lines 478 to 480
Copy link
Member

@clokep clokep Jan 20, 2022

Choose a reason for hiding this comment

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

The room case of this seems to prefill instead of invalidate, weirdly. I think this change is fine though, just noting a discrepancy:

synapse/synapse/storage/databases/main/account_data.py

Lines 444 to 446 in 3e0536c

self.get_account_data_for_room_and_type.prefill(
(user_id, room_id, account_data_type), content
)

Copy link
Contributor Author

@reivilibre reivilibre Jan 21, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

It is a little bit odd, but it does seem correct either way — maybe it was an intentional optimisation, maybe it's just inconsistent.

Copy link
Member

@clokep clokep Jan 21, 2022

Choose a reason for hiding this comment

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

I think prefilling would be a bit of an optimization, but probably not worth it. 🤷


return self._account_data_id_gen.get_current_token()
Expand Down
2 changes: 1 addition & 1 deletion synapse/visibility.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,7 @@ async def filter_events_for_client(
)

ignore_dict_content = await storage.main.get_global_account_data_by_type_for_user(
AccountDataTypes.IGNORED_USER_LIST, user_id
user_id, AccountDataTypes.IGNORED_USER_LIST
)

ignore_list: FrozenSet[str] = frozenset()
Expand Down
4 changes: 2 additions & 2 deletions tests/replication/slave/storage/test_account_data.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,13 +30,13 @@ def test_user_account_data(self):
)
self.replicate()
self.check(
"get_global_account_data_by_type_for_user", [TYPE, USER_ID], {"a": 1}
"get_global_account_data_by_type_for_user", [USER_ID, TYPE], {"a": 1}
)

self.get_success(
self.master_store.add_account_data_for_user(USER_ID, TYPE, {"a": 2})
)
self.replicate()
self.check(
"get_global_account_data_by_type_for_user", [TYPE, USER_ID], {"a": 2}
"get_global_account_data_by_type_for_user", [USER_ID, TYPE], {"a": 2}
)