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

Add new columns tracking when we partial-joined #13892

Merged
merged 8 commits into from
Sep 27, 2022
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
Prev Previous commit
Next Next commit
Add back the foreign key constraint
  • Loading branch information
David Robertson committed Sep 26, 2022
commit 94ee1e25b2424de4155e5655c628d223f8a99868
15 changes: 11 additions & 4 deletions synapse/handlers/federation.py
Original file line number Diff line number Diff line change
Expand Up @@ -584,10 +584,9 @@ async def do_invite_join(
# The background process is responsible for unmarking this flag,
# even if the join fails.
await self.store.store_partial_state_room(
room_id,
ret.servers_in_room,
ret.event.event_id,
self.store.get_device_stream_token(),
room_id=room_id,
servers=ret.servers_in_room,
device_lists_stream_id=self.store.get_device_stream_token(),
)

try:
Expand All @@ -613,6 +612,14 @@ async def do_invite_join(
room_id,
)
raise LimitExceededError(msg=e.msg, errcode=e.errcode, retry_after_ms=0)
else:
# Record the join event id for future use (when we finish the full
# join). We have to do this after persisting the event to keep foreign
# key constraints intact.
if ret.partial_state:
await self.store.write_partial_state_rooms_join_event_id(
room_id, event.event_id
)
Copy link
Member

Choose a reason for hiding this comment

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

Why do we do this in an else block rather than in the try or afterwards?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My thinking was that

  • If process_remote_join fails then the event may not be persisted
  • keeping the try block small helps clarify what the PartialStateConflictError is catching

Happy to just bung it at the bottom of the try though if you prefer.

finally:
# Always kick off the background process that asynchronously fetches
# state for the room.
Expand Down
41 changes: 35 additions & 6 deletions synapse/storage/databases/main/room.py
Original file line number Diff line number Diff line change
Expand Up @@ -1777,7 +1777,6 @@ async def store_partial_state_room(
self,
room_id: str,
servers: Collection[str],
join_event_id: str,
device_lists_stream_id: int,
) -> None:
"""Mark the given room as containing events with partial state.
Expand All @@ -1786,11 +1785,12 @@ async def store_partial_state_room(
room, which helps us to keep other homeservers in sync when we finally fully
join this room.

We do not include a `join_event_id` here---we need to wait for the join event
to be persisted first.

Args:
room_id: the ID of the room
servers: other servers known to be in the room
join_event_id: the event ID of the join membership event returned in the
(partial) /send_join response.
device_lists_stream_id: the device_lists stream ID at the time when we first
joined the room.
"""
Expand All @@ -1799,7 +1799,6 @@ async def store_partial_state_room(
self._store_partial_state_room_txn,
room_id,
servers,
join_event_id,
device_lists_stream_id,
)

Expand All @@ -1808,7 +1807,6 @@ def _store_partial_state_room_txn(
txn: LoggingTransaction,
room_id: str,
servers: Collection[str],
join_event_id: str,
device_lists_stream_id: int,
) -> None:
DatabasePool.simple_insert_txn(
Expand All @@ -1817,7 +1815,8 @@ def _store_partial_state_room_txn(
values={
"room_id": room_id,
"device_lists_stream_id": device_lists_stream_id,
"join_event_id": join_event_id,
# To be updated later once the join event is persisted.
"join_event_id": None,
},
)
DatabasePool.simple_insert_many_txn(
Expand All @@ -1828,6 +1827,36 @@ def _store_partial_state_room_txn(
)
self._invalidate_cache_and_stream(txn, self.is_partial_state_room, (room_id,))

async def write_partial_state_rooms_join_event_id(
self,
room_id: str,
join_event_id: str,
) -> None:
"""Record the join event which resulted from a partial join.

We do this separately to `store_partial_state_room` because we need to wait for
the join event to be persisted. Otherwise we violate a foreign key constraint.
"""
await self.db_pool.runInteraction(
"write_partial_state_rooms_join_event_id",
self._store_partial_state_room_txn,
Copy link
Member

Choose a reason for hiding this comment

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

Also wrong function

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh bugger. Why didn't mypy spot that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because we don't use ParamSpec in synapse.storage.database.py. Will refrain for now but I've found my next brain cleaner.

room_id,
join_event_id,
)

async def _write_partial_state_rooms_join_event_id(
Copy link
Member

Choose a reason for hiding this comment

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

This shouldn't be an async def

self,
txn: LoggingTransaction,
room_id: str,
join_event_id: str,
) -> None:
DatabasePool.simple_update_txn(
txn,
table="partial_state_rooms",
keyvalues={"room_id": room_id},
updatevalues={"join_event_id": join_event_id},
)

async def maybe_store_room_on_outlier_membership(
self, room_id: str, room_version: RoomVersion
) -> None:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,4 +20,4 @@
--
-- Both columns are backwards compatible.
ALTER TABLE partial_state_rooms ADD COLUMN device_lists_stream_id BIGINT NOT NULL DEFAULT 0;
ALTER TABLE partial_state_rooms ADD COLUMN join_event_id TEXT;
ALTER TABLE partial_state_rooms ADD COLUMN join_event_id TEXT REFERENCES events(event_id);