Add `rooms.room_version` column #6729

erikjohnston · 2020-01-17T15:10:40Z

This is so that we don't have to rely on pulling it out from current_state_events table.

This includes some clean ups along the way, so reading through the commits might make sense.

richvdh · 2020-01-20T16:42:25Z

synapse/federation/federation_client.py

-            room_version = ret.get("room_version", RoomVersions.V1.identifier)
-            event_format = room_version_to_event_format(room_version)
+            room_version_id = ret.get("room_version", RoomVersions.V1.identifier)
+            room_version = KNOWN_ROOM_VERSIONS.get(room_version_id)


this could conceivably return None? in which case that should be documented in the docstring?

Or it should be changed to KNOWN_ROOM_VERSIONS[room_version_id]

It now raises an UnsupportedRoomVersionError

could probably do with putting that in the docstring too.

synapse/handlers/federation.py

richvdh · 2020-01-20T16:47:09Z

synapse/handlers/federation.py

            "room_version", RoomVersions.V1.identifier
        )

+        if room_version.identifier != room_version_id:
+            raise SynapseError(400, "Room version mismatch")


I'm not sure that 400 is the right response code when the remote server lies to us, but 🤷‍♂️

It's the closest error code to "please bugger off and never sully our doors with that request again" that I know of :)

synapse/storage/data_stores/main/state.py

richvdh · 2020-01-20T16:52:48Z

synapse/storage/data_stores/main/state.py

+            keyvalues={"room_id": room_id},
+            retcol="room_version",
+            desc="get_room_version",
+            allow_none=True,


this shouldn't be needed? the row should be there whether or not the version is updated?

I'm not 10000000% comfortable asserting that we definitely do have a row in rooms for every single room right now

richvdh · 2020-01-20T16:53:37Z

synapse/storage/data_stores/main/state.py

@@ -60,24 +60,31 @@ class StateGroupWorkerStore(EventsWorkerStore, SQLBaseStore):
    def __init__(self, database: Database, db_conn, hs):
        super(StateGroupWorkerStore, self).__init__(database, db_conn, hs)

-    @defer.inlineCallbacks
-    def get_room_version(self, room_id):
+    @cached(max_entries=10000)


ohhh did I miss the memo where async functions can be cached?

Oh, yes, turns out that somewhere in the depths the cache descriptor calls run_in_background on the cahced function

richvdh · 2020-01-20T16:55:39Z

synapse/storage/data_stores/main/room.py

+
+        def _background_add_rooms_room_version_column_txn(txn: LoggingTransaction):
+            sql = """
+                SELECT room_id, json FROM current_state_events


is this going to table-scan CSE? shouldn't we base it on rooms instead?

There's a unique index on (room_id, type, state_key) so it just walks the index.

right, but it's going to need to walk the entire index? Or am I misunderstanding how btree indexes work?

Well, it'll iterate over each room and then look up the create event for each within that, AIUI.

The query plan for it on jki.re seems to show it doing that:

Limit (cost=1.11..12920.09 rows=1 width=1090) -> Nested Loop (cost=1.11..12920.09 rows=1 width=1090) -> Index Scan using current_state_events_room_id_type_state_key_key on current_state_events (cost=0.55..12846.65 rows=16 width=64) Index Cond: ((room_id > ''::text) AND (type = 'm.room.create'::text) AND (state_key = ''::text)) -> Index Scan using event_json_event_id_key on event_json (cost=0.56..4.58 rows=1 width=1125) Index Cond: (event_id = current_state_events.event_id) Filter: (current_state_events.room_id = room_id)

right, my concern is the index scan over that entire index, which is presumably huge on big servers, and it seems like a bunch of unnecessary I/O when we don't care about all the other events in that index.

this isn't a blocking comment; if you think it's going to be ok then that's fine. it was just a thought.

I'm reasonably sure it'll be fine. It will efficiently jump straight to the correct room ID, and jumping to the next room ID is fast (it won't have to iterate over all the state in the room, as it uses the tree structure to do it), so it should just be looking up the next room ID in the btree index, using the index to pull out the create event ID, repeating.

richvdh · 2020-01-20T16:58:12Z

synapse/storage/data_stores/main/room.py

+
+            new_last_room_id = ""
+            for room_id, creator, room_version_id in updates:
+                self.db.simple_upsert_txn(


... we're expecting to find rooms which aren't currently in rooms here?

Nooooooooooooooooo not really. But equally I don't fancy betting on that fact, given we don't insert it in the same places we insert current_state_events. If the rooms column doesn't exist when there is an entry in current state events then that could end up leading to badness when handling events related to it so I'm inclined to be a bit paranoid.

I could add a comment

stale

richvdh · 2020-01-27T13:07:33Z

synapse/federation/federation_client.py

-            room_version = ret.get("room_version", RoomVersions.V1.identifier)
-            event_format = room_version_to_event_format(room_version)
+            room_version_id = ret.get("room_version", RoomVersions.V1.identifier)
+            room_version = KNOWN_ROOM_VERSIONS.get(room_version_id)


could probably do with putting that in the docstring too.

…om_version_column

Fixes a confusion about whether room_version is a string or an object. This was introduced in #6729 and half-fixed in #6788.

Synapse 1.10.0 (2020-02-12) =========================== **WARNING to client developers**: As of this release Synapse validates `client_secret` parameters in the Client-Server API as per the spec. See [\#6766](#6766) for details. Updates to the Docker image --------------------------- - Update the docker images to Alpine Linux 3.11. ([\#6897](#6897)) Synapse 1.10.0rc5 (2020-02-11) ============================== Bugfixes -------- - Fix the filtering introduced in 1.10.0rc3 to also apply to the state blocks returned by `/sync`. ([\#6884](#6884)) Synapse 1.10.0rc4 (2020-02-11) ============================== This release candidate was built incorrectly and is superceded by 1.10.0rc5. Synapse 1.10.0rc3 (2020-02-10) ============================== Features -------- - Filter out `m.room.aliases` from the CS API to mitigate abuse while a better solution is specced. ([\#6878](#6878)) Internal Changes ---------------- - Fix continuous integration failures with old versions of `pip`, which were introduced by a release of the `zipp` library. ([\#6880](#6880)) Synapse 1.10.0rc2 (2020-02-06) ============================== Bugfixes -------- - Fix an issue with cross-signing where device signatures were not sent to remote servers. ([\#6844](#6844)) - Fix to the unknown remote device detection which was introduced in 1.10.rc1. ([\#6848](#6848)) Internal Changes ---------------- - Detect unexpected sender keys on remote encrypted events and resync device lists. ([\#6850](#6850)) Synapse 1.10.0rc1 (2020-01-31) ============================== Features -------- - Add experimental support for updated authorization rules for aliases events, from [MSC2260](matrix-org/matrix-spec-proposals#2260). ([\#6787](#6787), [\#6790](#6790), [\#6794](#6794)) Bugfixes -------- - Warn if postgres database has a non-C locale, as that can cause issues when upgrading locales (e.g. due to upgrading OS). ([\#6734](#6734)) - Minor fixes to `PUT /_synapse/admin/v2/users` admin api. ([\#6761](#6761)) - Validate `client_secret` parameter using the regex provided by the Client-Server API, temporarily allowing `:` characters for older clients. The `:` character will be removed in a future release. ([\#6767](#6767)) - Fix persisting redaction events that have been redacted (or otherwise don't have a redacts key). ([\#6771](#6771)) - Fix outbound federation request metrics. ([\#6795](#6795)) - Fix bug where querying a remote user's device keys that weren't cached resulted in only returning a single device. ([\#6796](#6796)) - Fix race in federation sender worker that delayed sending of device updates. ([\#6799](#6799), [\#6800](#6800)) - Fix bug where Synapse didn't invalidate cache of remote users' devices when Synapse left a room. ([\#6801](#6801)) - Fix waking up other workers when remote server is detected to have come back online. ([\#6811](#6811)) Improved Documentation ---------------------- - Clarify documentation related to `user_dir` and `federation_reader` workers. ([\#6775](#6775)) Internal Changes ---------------- - Record room versions in the `rooms` table. ([\#6729](#6729), [\#6788](#6788), [\#6810](#6810)) - Propagate cache invalidates from workers to other workers. ([\#6748](#6748)) - Remove some unnecessary admin handler abstraction methods. ([\#6751](#6751)) - Add some debugging for media storage providers. ([\#6757](#6757)) - Detect unknown remote devices and mark cache as stale. ([\#6776](#6776), [\#6819](#6819)) - Attempt to resync remote users' devices when detected as stale. ([\#6786](#6786)) - Delete current state from the database when server leaves a room. ([\#6792](#6792)) - When a client asks for a remote user's device keys check if the local cache for that user has been marked as potentially stale. ([\#6797](#6797)) - Add background update to clean out left rooms from current state. ([\#6802](#6802), [\#6816](#6816)) - Refactoring work in preparation for changing the event redaction algorithm. ([\#6803](#6803), [\#6805](#6805), [\#6806](#6806), [\#6807](#6807), [\#6820](#6820))

* commit '8df862e45': Add `rooms.room_version` column (#6729)

erikjohnston added 7 commits January 17, 2020 15:03

Have make_membership_event return room version

7b6be4b

Check room_version from make_join matches returned state.

f01d0d5

Use RoomVersion consistently throughout room upgrade

a42b675

Add rooms.room_version column and populate it in store_room

963237f

Read room version from rooms table.

e7618af

Newsfile

0bb025a

Add background update

892bab9

erikjohnston requested a review from a team January 17, 2020 16:15

richvdh previously requested changes Jan 20, 2020

View reviewed changes

erikjohnston added 4 commits January 21, 2020 11:17

Correctly raise exception if unknown room version

70be703

Add comment

ced73b3

Use explicit is not None

53a75fa

Add comments about paranoia

e825fd0

erikjohnston requested a review from richvdh January 21, 2020 11:48

richvdh approved these changes Jan 27, 2020

View reviewed changes

erikjohnston added 2 commits January 27, 2020 14:03

Correctly handle and document UnsupportedRoomVersionError

1474a49

Merge branch 'develop' of github.com:matrix-org/synapse into erikj/ro…

126599e

…om_version_column

erikjohnston merged commit 8df862e into develop Jan 27, 2020

erikjohnston mentioned this pull request Jan 30, 2020

Fix bug with getting missing auth event during join 500'ed #6810

Merged

richvdh added a commit that referenced this pull request Jan 31, 2020

Fix room_version in _persist_auth_tree

54437f6

Fixes a confusion about whether room_version is a string or an object. This was introduced in #6729 and half-fixed in #6788.

richvdh mentioned this pull request Jan 31, 2020

Fix room_version in _persist_auth_tree #6818

Closed

erikjohnston deleted the erikj/room_version_column branch February 5, 2020 17:35

richvdh mentioned this pull request Mar 11, 2020

Initial /sync is returning 404 due to "unknown room" when processing an event. #7065

Closed

richvdh mentioned this pull request Mar 26, 2020

57/rooms_version_column_3.sql taking ages on upgrade #7144

Closed

babolivier pushed a commit that referenced this pull request Sep 1, 2021

Add rooms.room_version column (#6729)

92ec7ba

* commit '8df862e45': Add `rooms.room_version` column (#6729)

clokep mentioned this pull request Jun 6, 2022

Fix a stale comment in get_room_version_id_txn. #12969

Merged

clokep mentioned this pull request Jan 9, 2023

populate_stats_process_rooms failing with an unknown room #14800

Closed

MadLittleMods mentioned this pull request Jul 31, 2024

Sliding Sync: Pre-populate room data for quick filtering/sorting element-hq/synapse#17512

Merged

17 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `rooms.room_version` column #6729

Add `rooms.room_version` column #6729

erikjohnston commented Jan 17, 2020

richvdh Jan 20, 2020

erikjohnston Jan 21, 2020

richvdh Jan 27, 2020

richvdh Jan 20, 2020

erikjohnston Jan 21, 2020

richvdh Jan 20, 2020

erikjohnston Jan 21, 2020

richvdh Jan 20, 2020

erikjohnston Jan 21, 2020

richvdh Jan 20, 2020

erikjohnston Jan 21, 2020

richvdh Jan 22, 2020

erikjohnston Jan 22, 2020

richvdh Jan 22, 2020

erikjohnston Jan 22, 2020

richvdh Jan 20, 2020

erikjohnston Jan 21, 2020

richvdh Jan 27, 2020

Add rooms.room_version column #6729

Add rooms.room_version column #6729

Conversation

erikjohnston commented Jan 17, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Add `rooms.room_version` column #6729

Add `rooms.room_version` column #6729