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

Stop populating unused table local_invites. #7793

Merged
merged 1 commit into from
Jul 7, 2020
Merged

Stop populating unused table local_invites. #7793

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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/7793.misc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Stop populating unused table `local_invites`.
98 changes: 23 additions & 75 deletions synapse/storage/data_stores/main/events.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,12 +27,7 @@
from twisted.internet import defer

import synapse.metrics
from synapse.api.constants import (
EventContentFields,
EventTypes,
Membership,
RelationTypes,
)
from synapse.api.constants import EventContentFields, EventTypes, RelationTypes
from synapse.api.room_versions import RoomVersions
from synapse.crypto.event_signing import compute_event_reference_hash
from synapse.events import EventBase # noqa: F401
Expand Down Expand Up @@ -819,7 +814,6 @@ def _delete_existing_rows_txn(cls, txn, events_and_contexts):
"event_reference_hashes",
"event_search",
"event_to_state_groups",
"local_invites",
"state_events",
"rejections",
"redactions",
Expand Down Expand Up @@ -1196,65 +1190,27 @@ def _store_room_members_txn(self, txn, events, backfilled):
(event.state_key,),
)

# We update the local_invites table only if the event is "current",
# i.e., its something that has just happened. If the event is an
# outlier it is only current if its an "out of band membership",
# like a remote invite or a rejection of a remote invite.
is_new_state = not backfilled and (
not event.internal_metadata.is_outlier()
or event.internal_metadata.is_out_of_band_membership()
)
is_mine = self.is_mine_id(event.state_key)
if is_new_state and is_mine:
if event.membership == Membership.INVITE:
self.db.simple_insert_txn(
txn,
table="local_invites",
values={
"event_id": event.event_id,
"invitee": event.state_key,
"inviter": event.sender,
"room_id": event.room_id,
"stream_id": event.internal_metadata.stream_ordering,
},
)
else:
sql = (
"UPDATE local_invites SET stream_id = ?, replaced_by = ? WHERE"
" room_id = ? AND invitee = ? AND locally_rejected is NULL"
" AND replaced_by is NULL"
)

txn.execute(
sql,
(
event.internal_metadata.stream_ordering,
event.event_id,
event.room_id,
event.state_key,
),
)

# We also update the `local_current_membership` table with
# latest invite info. This will usually get updated by the
# `current_state_events` handling, unless its an outlier.
if event.internal_metadata.is_outlier():
# This should only happen for out of band memberships, so
# we add a paranoia check.
assert event.internal_metadata.is_out_of_band_membership()

self.db.simple_upsert_txn(
txn,
table="local_current_membership",
keyvalues={
"room_id": event.room_id,
"user_id": event.state_key,
},
values={
"event_id": event.event_id,
"membership": event.membership,
},
)
# We update the local_current_membership table only if the event is
# "current", i.e., its something that has just happened.
#
# This will usually get updated by the `current_state_events` handling,
# unless its an outlier, and an outlier is only "current" if it's an "out of
# band membership", like a remote invite or a rejection of a remote invite.
if (
self.is_mine_id(event.state_key)
and not backfilled
and event.internal_metadata.is_outlier()
and event.internal_metadata.is_out_of_band_membership()
Comment on lines +1201 to +1203
Copy link
Member

Choose a reason for hiding this comment

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

This logic looks slightly different not sure if that was done on purpose as part of this change or is introducing a bug.

not backfilled and (not outlier or OOB membership) becomes not backfilled and outlier and OOB membership.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, I see it is a combination of this clause + the is outlier clause below. 👍

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah, I hope it is the same thing, but a second pair of eyes to confirm that is much appreciated.

Copy link
Member

Choose a reason for hiding this comment

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

I went through it again and I believe it is correct!

):
self.db.simple_upsert_txn(
txn,
table="local_current_membership",
keyvalues={"room_id": event.room_id, "user_id": event.state_key},
values={
"event_id": event.event_id,
"membership": event.membership,
},
)

def _handle_event_relations(self, txn, event):
"""Handles inserting relation data during peristence of events
Expand Down Expand Up @@ -1591,16 +1547,8 @@ async def locally_reject_invite(self, user_id: str, room_id: str) -> int:
create a leave event for it.
"""

sql = (
"UPDATE local_invites SET stream_id = ?, locally_rejected = ? WHERE"
" room_id = ? AND invitee = ? AND locally_rejected is NULL"
" AND replaced_by is NULL"
)

def f(txn, stream_ordering):
Copy link
Member Author

Choose a reason for hiding this comment

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

(I know that stream_ordering is unused here, but I'm about to hook into it in another PR)

txn.execute(sql, (stream_ordering, True, room_id, user_id))

# We also clear this entry from `local_current_membership`.
# Clear this entry from `local_current_membership`.
# Ideally we'd point to a leave event, but we don't have one, so
# nevermind.
self.db.simple_delete_txn(
Expand Down
5 changes: 1 addition & 4 deletions synapse/storage/data_stores/main/events_worker.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,10 +82,7 @@ def __init__(self, database: Database, db_conn, hs):
# We are the process in charge of generating stream ids for events,
# so instantiate ID generators based on the database
self._stream_id_gen = StreamIdGenerator(
db_conn,
"events",
"stream_ordering",
extra_tables=[("local_invites", "stream_id")],
db_conn, "events", "stream_ordering",
)
self._backfill_id_gen = StreamIdGenerator(
db_conn,
Expand Down
1 change: 0 additions & 1 deletion synapse/storage/data_stores/main/purge_events.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -361,7 +361,6 @@ def _purge_room_txn(self, txn, room_id):
"event_push_summary",
"pusher_throttle",
"group_summary_rooms",
"local_invites",
Copy link
Member

Choose a reason for hiding this comment

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

Would we still want to potentially purge the table until it is fully removed from the database? I doubt it matters much either way, but...

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm minded to say "no". Say we leave this line here for now and drop the table in Synapse 1.17.0: that will mean that anybody trying to roll back from 1.17.0 to 1.16.0 will end up with broken purges.

OTOH leaving it here seems fairly harmless, especially if we actually drop the table soon.

Copy link
Member

Choose a reason for hiding this comment

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

I can see it going either way. I guess my thought was to think about why an admin might be purging a room and if it could leave them liable to have information in the local_invites table still.

I think removing it from the purges is the simplest way for us to handle rollbacks, however. I'm convinced!

"room_account_data",
"room_tags",
"local_current_membership",
Expand Down
1 change: 0 additions & 1 deletion tests/rest/admin/test_room.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -213,7 +213,6 @@ def test_purge_room(self):
"event_push_summary",
"pusher_throttle",
"group_summary_rooms",
"local_invites",
"room_account_data",
"room_tags",
# "state_groups", # Current impl leaves orphaned state groups around.
Expand Down