Store state groups separately from events #2784

erikjohnston · 2018-01-15T16:14:46Z

We want to be able to calculate and use state groups on workers other than the master process. To make this easier we store new state groups immediately from the state handler, rather than later when the event is stored.

This means that the state group IDs have to be generated in the DB rather than in process, since multiple workers will potentially be generating them.

This allows other workers to use it, and so resolve state.

erikjohnston · 2018-02-05T15:23:41Z

Oh, I suppose we should rename StateReadStore, as it now also writes rather than just reads?

richvdh · 2018-02-05T17:03:41Z

Oh, I suppose we should rename StateReadStore, as it now also writes rather than just reads?

yes please

richvdh

I haven't looked at the tests at all; generally much of this is mysterious to me :/

richvdh · 2018-02-05T17:24:24Z

synapse/handlers/federation.py

@@ -1923,11 +1923,14 @@ def do_auth(self, origin, event, context, auth_events):
            logger.warn("Failed auth resolution for %r because %s", event, e)
            raise e

-    def _update_context_for_auth_events(self, context, auth_events,
+    @defer.inlineCallbacks
+    def _update_context_for_auth_events(self, event, context, auth_events,
                                        event_key):
        """Update the state_ids in an event context after auth event resolution


could you mention that this will also store the state group?

richvdh · 2018-02-05T17:27:33Z

synapse/storage/schema/delta/47/state_group_seq.py

+
+def run_create(cur, database_engine, *args, **kwargs):
+    if isinstance(database_engine, PostgresEngine):
+        cur.execute("CREATE SEQUENCE state_group_id_seq")


do you not need to initialise this? (see ab8567a which I did when playing with this)

Ooh, yes. I've cherry-picked that.

richvdh · 2018-02-05T17:29:35Z

synapse/storage/state.py

+            where_clause="type='m.room.member'",
+        )
+
+    def _have_persisted_state_group_txn(self, txn, state_group):


this looks to be redundant now?

richvdh · 2018-02-05T17:35:21Z

synapse/storage/events.py

@@ -744,7 +744,7 @@ def _persist_events_txn(self, txn, events_and_contexts, backfilled,

        # Insert into the state_groups, state_groups_state, and


this comment is misleading now

richvdh · 2018-02-05T17:36:01Z

synapse/storage/events.py

@@ -982,7 +982,7 @@ def _update_outliers_txn(self, txn, events_and_contexts):
                # insert into the state_group, state_groups_state and


richvdh · 2018-02-05T17:41:26Z

synapse/state.py

+            context.state_group = yield self.store.store_state_group(
+                event.event_id,
+                event.room_id,
+                prev_group=entry.prev_group,


why is this entry.prev_group rather than context.prev_group as calculated above? (any idea when entry.state_group is falsy so we do the prev_group thing?)

generally the logic in compute_event_context is mysterious :/

why is this entry.prev_group rather than context.prev_group as calculated above?

That was a typo tbh.

(any idea when entry.state_group is falsy so we do the prev_group thing?)

entry.state_group is falsey if the resolved state doesn't already have a state group. This would typically happen when the prev events for the event point to two different state groups.

generally the logic in compute_event_context is mysterious :/

I've tried to add some comments

richvdh · 2018-02-05T17:42:43Z

synapse/state.py

@@ -229,6 +236,14 @@ def compute_event_context(self, event, old_state=None):
            else:
                context.current_state_ids = context.prev_state_ids

+            context.state_group = yield self.store.store_state_group(


do we need to create a new state_group if it's not a state event?

Yup. Basically, we've been explicitly told the state at this point (old_state) and we need to persist that. This e.g. happens when we're backfilling and we're told the state at this oldest event. We could try and be clever and check if we've already persisted a state group with identical state, but we're not that clever atm.

... otherwise we try to recreate old state groups

richvdh · 2018-02-06T11:37:29Z

synapse/storage/events.py

@@ -979,8 +978,7 @@ def _update_outliers_txn(self, txn, events_and_contexts):
                # an outlier in the database. We now have some state at that
                # so we need to update the state_groups table with that state.

-                # insert into the state_group, state_groups_state and
-                # event_to_state_groups tables.
+                # insert into event_to_state_groups tables.


richvdh · 2018-02-06T11:45:05Z

synapse/state.py

            defer.returnValue(context)

        if old_state:
+            # We already have the state, so we don't to calculate it.


richvdh · 2018-02-06T11:45:44Z

synapse/state.py

            defer.returnValue(context)

        if old_state:
+            # We already have the state, so we don't to calculate it.
+            # Lets just correctly fill out the context and create a


richvdh · 2018-02-06T11:46:25Z

synapse/state.py

@@ -208,15 +215,26 @@ def compute_event_context(self, event, old_state=None):
                context.current_state_ids = {}
                context.prev_state_ids = {}
            context.prev_state_events = []
-            context.state_group = self.store.get_next_state_group()
+
+            context.state_group = yield self.store.store_state_group(


as per #synapse-dev: we don't currently store a state group here, and it's not obvious to me that the state group we will store will be meaningful.

richvdh · 2018-02-06T11:59:15Z

synapse/state.py

@@ -229,6 +236,14 @@ def compute_event_context(self, event, old_state=None):
            else:
                context.current_state_ids = context.prev_state_ids

+            context.state_group = yield self.store.store_state_group(


richvdh · 2018-02-06T12:05:08Z

synapse/storage/events.py

-        # Insert into the state_groups, state_groups_state, and
-        # event_to_state_groups tables.
-        self._store_mult_state_groups_txn(txn, events_and_contexts)
+        # Insert into event_to_state_groups tables.


s/tables/table/. Or just s/tables//

richvdh · 2018-02-06T14:20:18Z

synapse/state.py

-            )
+            # We don't store state for outliers, so we don't generate a state
+            # froup for it.
+            context.state_group = None


ok. the state_group field on EventContext is documented as 'state_group (int): state group id', so if it can now be None as well as an int that needs updating, please.

richvdh · 2018-02-06T14:21:47Z

synapse/storage/state.py

+        Args:
+            event_id (str): The event ID for which the state was calculated
+            room_id (str)
+            prev_group (str|None): A previous state group for the room, optional.


aren't state groups ints?

richvdh

I haven't really reviewed the tests so I'll trust you not to be a crank ;)

richvdh · 2018-02-06T14:26:41Z

oh btw please squash this, we don't need 15 "fix the comments" commits to pick through and as already established the commits don't stand on their own merits

erikjohnston · 2018-02-06T14:26:44Z

I haven't really reviewed the tests so I'll trust you not to be a crank ;)

Just be grateful I didn't rage delete them :p

erikjohnston force-pushed the erikj/store_state_seperately branch from 4941653 to 6c22639 Compare January 19, 2018 13:49

erikjohnston force-pushed the erikj/store_state_seperately branch from e156876 to 1323f9c Compare February 5, 2018 12:48

erikjohnston added 5 commits February 5, 2018 15:16

Split state group persist into seperate storage func

cb222e2

Add per database engine code for state group id gen

0f18d58

Move store_state_group to StateReadStore

c8c8f53

This allows other workers to use it, and so resolve state.

Hook up store_state_group

1b2af72

Fix tests

2fdf245

erikjohnston force-pushed the erikj/store_state_seperately branch from a9acc01 to 2fdf245 Compare February 5, 2018 15:19

erikjohnston assigned richvdh Feb 5, 2018

erikjohnston added 2 commits February 5, 2018 17:25

Rename _store_mult_state_groups_txn

e66f2c3

Rename StateGroupReadStore

693ea27

richvdh reviewed Feb 5, 2018

View reviewed changes

erikjohnston and others added 4 commits February 6, 2018 09:58

Remove redundant _have_persisted_state_group_txn

a742398

Update comments

90cd2e6

Comment compute_event_context

c695725

Set start val for state_group_id_seq

22be520

... otherwise we try to recreate old state groups

richvdh reviewed Feb 6, 2018

View reviewed changes

richvdh assigned erikjohnston and unassigned richvdh Feb 6, 2018

erikjohnston added 2 commits February 6, 2018 12:53

Update comments

2694524

Don't store state for outliers

7018b68

erikjohnston assigned richvdh and unassigned erikjohnston Feb 6, 2018

richvdh reviewed Feb 6, 2018

View reviewed changes

erikjohnston added 2 commits February 6, 2018 14:21

Update comment

a6b415c

Update docstring as state groups are ints

4a72132

richvdh approved these changes Feb 6, 2018

View reviewed changes

richvdh removed their assignment Feb 6, 2018

richvdh assigned erikjohnston Feb 6, 2018

erikjohnston merged commit 3d33eef into develop Feb 6, 2018

erikjohnston deleted the erikj/store_state_seperately branch March 5, 2018 15:56

richvdh mentioned this pull request Apr 5, 2018

postgres DB migrated from sqlite fails with lots of duplicate state_groups_pkey violations #3050

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Store state groups separately from events #2784

Store state groups separately from events #2784

erikjohnston commented Jan 15, 2018

erikjohnston commented Feb 5, 2018

richvdh commented Feb 5, 2018 •

edited

richvdh left a comment

richvdh Feb 5, 2018

richvdh Feb 5, 2018

erikjohnston Feb 6, 2018

richvdh Feb 5, 2018

richvdh Feb 5, 2018

richvdh Feb 5, 2018

richvdh Feb 5, 2018

richvdh Feb 5, 2018

erikjohnston Feb 6, 2018

richvdh Feb 5, 2018

erikjohnston Feb 6, 2018

richvdh Feb 6, 2018

richvdh Feb 6, 2018

richvdh Feb 6, 2018

richvdh Feb 6, 2018

richvdh Feb 6, 2018

richvdh Feb 6, 2018

richvdh Feb 6, 2018

richvdh Feb 6, 2018

richvdh Feb 6, 2018

richvdh left a comment

richvdh commented Feb 6, 2018

erikjohnston commented Feb 6, 2018

		@@ -744,7 +744,7 @@ def _persist_events_txn(self, txn, events_and_contexts, backfilled,

		# Insert into the state_groups, state_groups_state, and

		@@ -982,7 +982,7 @@ def _update_outliers_txn(self, txn, events_and_contexts):
		# insert into the state_group, state_groups_state and

Store state groups separately from events #2784

Store state groups separately from events #2784

Conversation

erikjohnston commented Jan 15, 2018

erikjohnston commented Feb 5, 2018

richvdh commented Feb 5, 2018 • edited

richvdh left a 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

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

richvdh left a comment

Choose a reason for hiding this comment

richvdh commented Feb 6, 2018

erikjohnston commented Feb 6, 2018

richvdh commented Feb 5, 2018 •

edited