Update comments docs to explain the why github/madlittlemods/11300-refactor-backfilled-pt1 madlittlemods/11300-refactor-backfilled-pt1

4 files changed, 126 insertions, 65 deletions

diff --git a/synapse/handlers/federation_event.py b/synapse/handlers/federation_event.py
index 46606775d3..fc1570f5ac 100644
--- a/synapse/handlers/federation_event.py
+++ b/synapse/handlers/federation_event.py
@@ -1814,7 +1814,7 @@ class FederationEventHandler:
                 # We should not send notifications about backfilled events.
                 inhibit_push_notifications=backfilled,
                 # We don't need to calculate the state for backfilled events and
-                # we there is no need to update the forward extrems because we
+                # there is no need to update the forward extrems because we
                 # already know this event happened in the past if it was
                 # backfilled.
                 should_calculate_state_and_forward_extrems=not backfilled,
@@ -1823,7 +1823,7 @@ class FederationEventHandler:
                 use_negative_stream_ordering=backfilled,
                 # Backfilled events do not affect the current local state
                 inhibit_local_membership_updates=backfilled,
-                # Backfilled events have negative stream ordering and happened
+                # Backfilled events have negative stream_ordering and happened
                 # in the past so we know that we don't need to update the
                 # stream_ordering tip for the room.
                 update_room_forward_stream_ordering=not backfilled,
@@ -1854,21 +1854,30 @@ class FederationEventHandler:
                 context that should be persisted. All events must belong to
                 the same room.
             inhibit_push_notifications: Whether to stop the notifiers/pushers
-                from knowing about the event. Usually this is done for any backfilled
-                event.
+                from knowing about the event. This should be set as True
+                for backfilled events because there is no need to send push
+                notifications for events in the past.
             should_calculate_state_and_forward_extrems: Determines whether we
                 need to calculate the state and new forward extremities for the
-                room. This should be set to false for backfilled events.
+                room. This should be set to false for backfilled events because
+                we don't need to calculate the state for backfilled events and
+                there is no need to update the forward extrems because we
+                already know this event happened in the past if it was
+                backfilled.
             use_negative_stream_ordering: Whether to start stream_ordering on
-                the negative side and decrement. Usually this is done for any
-                backfilled event.
+                the negative side and decrement. This should be set as True
+                for backfilled events because backfilled events get a negative
+                stream ordering so they don't come down incremental `/sync`.
             inhibit_local_membership_updates: Stop the local_current_membership
-                from being updated by these events. Usually this is done for
-                backfilled events.
+                from being updated by these events. This should be set to True
+                for backfilled events because backfilled events in the past do
+                not affect the current local state.
             update_room_forward_stream_ordering: Whether to update the
                 stream_ordering position to mark the latest event as the front
-                of the room. This should only be set as false for backfilled
-                events.
+                of the room. This should be set as False for backfilled
+                events because backfilled events have negative stream_ordering
+                and happened in the past so we know that we don't need to
+                update the stream_ordering tip for the room.
 
         Returns:
             The stream ID after which all events have been persisted.
diff --git a/synapse/replication/http/federation.py b/synapse/replication/http/federation.py
index ff5f919161..71101bba6c 100644
--- a/synapse/replication/http/federation.py
+++ b/synapse/replication/http/federation.py
@@ -85,21 +85,30 @@ class ReplicationFederationSendEventsRestServlet(ReplicationEndpoint):
             room_id (str)
             event_and_contexts (list[tuple[FrozenEvent, EventContext]])
             inhibit_push_notifications: Whether to stop the notifiers/pushers
-                from knowing about the event. Usually this is done for any backfilled
-                event.
+                from knowing about the event. This should be set as True
+                for backfilled events because there is no need to send push
+                notifications for events in the past.
             should_calculate_state_and_forward_extrems: Determines whether we
                 need to calculate the state and new forward extremities for the
-                room. This should be set to false for backfilled events.
+                room. This should be set to false for backfilled events because
+                we don't need to calculate the state for backfilled events and
+                there is no need to update the forward extrems because we
+                already know this event happened in the past if it was
+                backfilled.
             use_negative_stream_ordering: Whether to start stream_ordering on
-                the negative side and decrement. Usually this is done for any
-                backfilled event.
+                the negative side and decrement. This should be set as True
+                for backfilled events because backfilled events get a negative
+                stream ordering so they don't come down incremental `/sync`.
             inhibit_local_membership_updates: Stop the local_current_membership
-                from being updated by these events. Usually this is done for
-                backfilled events.
+                from being updated by these events. This should be set to True
+                for backfilled events because backfilled events in the past do
+                not affect the current local state.
             update_room_forward_stream_ordering: Whether to update the
                 stream_ordering position to mark the latest event as the front
-                of the room. This should only be set as false for backfilled
-                events.
+                of the room. This should be set as False for backfilled
+                events because backfilled events have negative stream_ordering
+                and happened in the past so we know that we don't need to
+                update the stream_ordering tip for the room.
         """
         event_payloads = []
         for event, context in event_and_contexts:
diff --git a/synapse/storage/databases/main/events.py b/synapse/storage/databases/main/events.py
index 1e1fd8e425..f4291af608 100644
--- a/synapse/storage/databases/main/events.py
+++ b/synapse/storage/databases/main/events.py
@@ -141,15 +141,19 @@ class PersistEventsStore:
             new_forward_extremities: Map from room_id to list of event IDs
                 that are the new forward extremities of the room.
             use_negative_stream_ordering: Whether to start stream_ordering on
-                the negative side and decrement. Usually this is done for any
-                backfilled event.
+                the negative side and decrement. This should be set as True
+                for backfilled events because backfilled events get a negative
+                stream ordering so they don't come down incremental `/sync`.
             inhibit_local_membership_updates: Stop the local_current_membership
-                from being updated by these events. Usually this is done for
-                backfilled events.
+                from being updated by these events. This should be set to True
+                for backfilled events because backfilled events in the past do
+                not affect the current local state.
             update_room_forward_stream_ordering: Whether to update the
                 stream_ordering position to mark the latest event as the front
-                of the room. This should only be set as false for backfilled
-                events.
+                of the room. This should be set as False for backfilled
+                events because backfilled events have negative stream_ordering
+                and happened in the past so we know that we don't need to
+                update the stream_ordering tip for the room.
 
         Returns:
             Resolves when the events have been persisted
@@ -348,12 +352,15 @@ class PersistEventsStore:
             events_and_contexts: events to persist
             backfilled: True if the events were backfilled
             inhibit_local_membership_updates: Stop the local_current_membership
-                from being updated by these events. Usually this is done for
-                backfilled events.
+                from being updated by these events. This should be set to True
+                for backfilled events because backfilled events in the past do
+                not affect the current local state.
             update_room_forward_stream_ordering: Whether to update the
                 stream_ordering position to mark the latest event as the front
-                of the room. This should only be set as false for backfilled
-                events.
+                of the room. This should be set as False for backfilled
+                events because backfilled events have negative stream_ordering
+                and happened in the past so we know that we don't need to
+                update the stream_ordering tip for the room.
             delete_existing True to purge existing table rows for the events
                 from the database. This is useful when retrying due to
                 IntegrityError.
@@ -1234,10 +1241,12 @@ class PersistEventsStore:
             txn (twisted.enterprise.adbapi.Connection): db connection
             events_and_contexts (list[(EventBase, EventContext)]): events
                 we are persisting
-            update_room_forward_stream_ordering (bool): Whether to update the
+            update_room_forward_stream_ordering: Whether to update the
                 stream_ordering position to mark the latest event as the front
-                of the room. This should only be set as false for backfilled
-                events.
+                of the room. This should be set as False for backfilled
+                events because backfilled events have negative stream_ordering
+                and happened in the past so we know that we don't need to
+                update the stream_ordering tip for the room.
         """
         depth_updates: Dict[str, int] = {}
         for event, context in events_and_contexts:
@@ -1474,8 +1483,9 @@ class PersistEventsStore:
                 we've already persisted, etc, that wouldn't appear in
                 events_and_context.
             inhibit_local_membership_updates: Stop the local_current_membership
-                from being updated by these events. Usually this is done for
-                backfilled events.
+                from being updated by these events. This should be set to True
+                for backfilled events because backfilled events in the past do
+                not affect the current local state.
         """
 
         # Insert all the push actions into the event_push_actions table.
@@ -1684,8 +1694,9 @@ class PersistEventsStore:
             txn: The transaction to use.
             events: List of events to store.
             inhibit_local_membership_updates: Stop the local_current_membership
-                from being updated by these events. Usually this is done for
-                backfilled events.
+                from being updated by these events. This should be set to True
+                for backfilled events because backfilled events in the past do
+                not affect the current local state.
         """
 
         def non_null_str_or_none(val: Any) -> Optional[str]:
diff --git a/synapse/storage/persist_events.py b/synapse/storage/persist_events.py
index 18b991d607..d90c817e7d 100644
--- a/synapse/storage/persist_events.py
+++ b/synapse/storage/persist_events.py
@@ -180,17 +180,25 @@ class _EventPeristenceQueue(Generic[_PersistResult]):
             events_and_contexts (list[(EventBase, EventContext)]):
             should_calculate_state_and_forward_extrems: Determines whether we
                 need to calculate the state and new forward extremities for the
-                room. This should be set to false for backfilled events.
+                room. This should be set to false for backfilled events because
+                we don't need to calculate the state for backfilled events and
+                there is no need to update the forward extrems because we
+                already know this event happened in the past if it was
+                backfilled.
             use_negative_stream_ordering: Whether to start stream_ordering on
-                the negative side and decrement. Usually this is done for any
-                backfilled event.
+                the negative side and decrement. This should be set as True
+                for backfilled events because backfilled events get a negative
+                stream ordering so they don't come down incremental `/sync`.
             inhibit_local_membership_updates: Stop the local_current_membership
-                from being updated by these events. Usually this is done for
-                backfilled events.
+                from being updated by these events. This should be set to True
+                for backfilled events because backfilled events in the past do
+                not affect the current local state.
             update_room_forward_stream_ordering: Whether to update the
                 stream_ordering position to mark the latest event as the front
-                of the room. This should only be set as false for backfilled
-                events.
+                of the room. This should be set as False for backfilled
+                events because backfilled events have negative stream_ordering
+                and happened in the past so we know that we don't need to
+                update the stream_ordering tip for the room.
 
         Returns:
             the result returned by the `_per_item_callback` passed to
@@ -348,17 +356,25 @@ class EventsPersistenceStorage:
             events_and_contexts: list of tuples of (event, context)
             should_calculate_state_and_forward_extrems: Determines whether we
                 need to calculate the state and new forward extremities for the
-                room. This should be set to false for backfilled events.
+                room. This should be set to false for backfilled events because
+                we don't need to calculate the state for backfilled events and
+                there is no need to update the forward extrems because we
+                already know this event happened in the past if it was
+                backfilled.
             use_negative_stream_ordering: Whether to start stream_ordering on
-                the negative side and decrement. Usually this is done for any
-                backfilled event.
+                the negative side and decrement. This should be set as True
+                for backfilled events because backfilled events get a negative
+                stream ordering so they don't come down incremental `/sync`.
             inhibit_local_membership_updates: Stop the local_current_membership
-                from being updated by these events. Usually this is done for
-                backfilled events.
+                from being updated by these events. This should be set to True
+                for backfilled events because backfilled events in the past do
+                not affect the current local state.
             update_room_forward_stream_ordering: Whether to update the
                 stream_ordering position to mark the latest event as the front
-                of the room. This should only be set as false for backfilled
-                events.
+                of the room. This should be set as False for backfilled
+                events because backfilled events have negative stream_ordering
+                and happened in the past so we know that we don't need to
+                update the stream_ordering tip for the room.
 
         Returns:
             List of events persisted, the current position room stream position.
@@ -426,17 +442,25 @@ class EventsPersistenceStorage:
             context:
             should_calculate_state_and_forward_extrems: Determines whether we
                 need to calculate the state and new forward extremities for the
-                room. This should be set to false for backfilled events.
+                room. This should be set to false for backfilled events because
+                we don't need to calculate the state for backfilled events and
+                there is no need to update the forward extrems because we
+                already know this event happened in the past if it was
+                backfilled.
             use_negative_stream_ordering: Whether to start stream_ordering on
-                the negative side and decrement. Usually this is done for any
-                backfilled event.
+                the negative side and decrement. This should be set as True
+                for backfilled events because backfilled events get a negative
+                stream ordering so they don't come down incremental `/sync`.
             inhibit_local_membership_updates: Stop the local_current_membership
-                from being updated by these events. Usually this is done for
-                backfilled events.
+                from being updated by these events. This should be set to True
+                for backfilled events because backfilled events in the past do
+                not affect the current local state.
             update_room_forward_stream_ordering: Whether to update the
                 stream_ordering position to mark the latest event as the front
-                of the room. This should only be set as false for backfilled
-                events.
+                of the room. This should be set as False for backfilled
+                events because backfilled events have negative stream_ordering
+                and happened in the past so we know that we don't need to
+                update the stream_ordering tip for the room.
 
         Returns:
             The event, stream ordering of `event`, and the stream ordering of the
@@ -484,17 +508,25 @@ class EventsPersistenceStorage:
             events_and_contexts:
             should_calculate_state_and_forward_extrems: Determines whether we
                 need to calculate the state and new forward extremities for the
-                room. This should be set to false for backfilled events.
+                room. This should be set to false for backfilled events because
+                we don't need to calculate the state for backfilled events and
+                there is no need to update the forward extrems because we
+                already know this event happened in the past if it was
+                backfilled.
             use_negative_stream_ordering: Whether to start stream_ordering on
-                the negative side and decrement. Usually this is done for any
-                backfilled event.
+                the negative side and decrement. This should be set as True
+                for backfilled events because backfilled events get a negative
+                stream ordering so they don't come down incremental `/sync`.
             inhibit_local_membership_updates: Stop the local_current_membership
-                from being updated by these events. Usually this is done for
-                backfilled events.
+                from being updated by these events. This should be set to True
+                for backfilled events because backfilled events in the past do
+                not affect the current local state.
             update_room_forward_stream_ordering: Whether to update the
                 stream_ordering position to mark the latest event as the front
-                of the room. This should only be set as false for backfilled
-                events.
+                of the room. This should be set as False for backfilled
+                events because backfilled events have negative stream_ordering
+                and happened in the past so we know that we don't need to
+                update the stream_ordering tip for the room.
 
         Returns:
             A dictionary of event ID to event ID we didn't persist as we already