Limit and filter the number of backfill points to get from the database

synapse/handlers/federation.py

@@ -221,8 +221,8 @@ async def _maybe_backfill_inner(
             room_id: The room to backfill in.
             current_depth: The depth to check at for any upcoming backfill points.
             limit: The max number of events to request from the remote federated server.
-            processing_start_time: The time when `maybe_backfill` started
-                processing. Only used for timing.
+            processing_start_time: The time when `maybe_backfill` started processing.
+                Only used for timing. If `None`, no timing observation will be made.
         """
         backwards_extremities = [
             _BackfillPoint(event_id, depth, _BackfillPointType.BACKWARDS_EXTREMITY)
@@ -284,22 +284,23 @@ async def _maybe_backfill_inner(
         # backfilling anyway just in case we do get relevant events.
         if not sorted_backfill_points and current_depth != MAX_DEPTH:
             logger.debug(
-                "_maybe_backfill_inner: all backfill points are *after* current depth. Backfilling anyway."
+                "_maybe_backfill_inner: all backfill points are *after* current depth. Trying again with later backfill points."
             )
             return await self._maybe_backfill_inner(
                 room_id=room_id,
                 # We use `MAX_DEPTH` so that we find all backfill points next
                 # time (all events are below the `MAX_DEPTH`)
                 current_depth=MAX_DEPTH,
                 limit=limit,
-                # We don't want to another timing observation from this nested
-                # recursive call. The top-most call can record the time overall
-                # otherwise the smaller one will throw off the results.
+                # We don't want to start another timing observation from this
+                # nested recursive call. The top-most call can record the time
+                # overall otherwise the smaller one will throw off the results.
                 processing_start_time=None,
             )
-        elif not sorted_backfill_points and current_depth == MAX_DEPTH:
-            # Even after trying again with `MAX_DEPTH`, we didn't find any
-            # backward extremities to backfill from.
+
+        # Even after recursing with `MAX_DEPTH`, we didn't find any
+        # backward extremities to backfill from.
+        if not sorted_backfill_points:
             logger.debug(
                 "_maybe_backfill_inner: Not backfilling as no backward extremeties found."
             )
@@ -314,10 +315,6 @@ async def _maybe_backfill_inner(
         # chose more than one times the limit in case of failure, but choosing a
         # much larger factor will result in triggering a backfill request much
         # earlier than necessary.
-        #
-        # XXX: shouldn't we do this *after* the filter by depth below? Again, we don't
-        # care about events that have happened after our current position.
-        #
         max_depth_of_backfill_points = sorted_backfill_points[0].depth
         if current_depth - 2 * limit > max_depth_of_backfill_points:
             logger.debug(

synapse/storage/databases/main/event_federation.py

@@ -730,17 +730,13 @@ async def get_backfill_points_in_room(
         limit: int,
     ) -> List[Tuple[str, int]]:
         """
-        Gets the oldest events(backwards extremities) in the room along with the
-        approximate depth. Sorted by depth, highest to lowest (descending) so the closest
-        events to the `current_depth` are first in the list.
+        Get the backward extremities to backfill from in the room along with the
+        approximate depth. Only returns events that are at a depth lower than or
+        equal to the `current_depth`. Sorted by depth, highest to lowest (descending)
+        so the closest events to the `current_depth` are first in the list.
 
-        We use this function so that we can compare and see if a client's
-        `current_depth` at their current scrollback is within pagination range
-        of the event extremities. If the `current_depth` is close to the depth
-        of given oldest event, we can trigger a backfill.
-
-        We ignore extremities that have a greater depth than our `current_depth`
-        as:
+        We ignore extremities that are newer than the user's current scroll position
+        (ie, those with depth greater than `current_depth`) as:
             1. we don't really care about getting events that have happened
                after our current position; and
             2. by the nature of paginating and scrolling back, we have likely
@@ -750,9 +746,7 @@ async def get_backfill_points_in_room(
 
         Args:
             room_id: Room where we want to find the oldest events
-            current_depth: The depth at the users current scrollback position
-                because we only care about finding events older than the given
-                `current_depth` when scrolling and paginating backwards.
+            current_depth: The depth at the user's current scrollback position
             limit: The max number of backfill points to return
 
         Returns:
@@ -812,9 +806,10 @@ def get_backfill_points_in_room_txn(
                      * means less than the given depth) because we're looking backwards
                      * from the `current_depth` when backfilling.
                      *
-                     *                           current_depth (ignore events that come after this, ignore 2-4)
-                     *                           |
-                     * <oldest-in-time> [0]<--[1]▼<--[2]<--[3]<--[4] <newest-in-time>
+                     *                         current_depth (ignore events that come after this, ignore 2-4)
+                     *                         |
+                     *                         ▼
+                     * <oldest-in-time> [0]<--[1]<--[2]<--[3]<--[4] <newest-in-time>
                      */
                     AND event.depth <= ? /* current_depth */
                     /**
@@ -877,17 +872,13 @@ async def get_insertion_event_backward_extremities_in_room(
     ) -> List[Tuple[str, int]]:
         """
         Get the insertion events we know about that we haven't backfilled yet
-        along with the approximate depth. Sorted by depth, highest to lowest
-        (descending) so the closest events to the `current_depth` are first
-        in the list.
-
-        We use this function so that we can compare and see if someones
-        `current_depth` at their current scrollback is within pagination range
-        of the insertion event. If the `current_depth` is close to the depth
-        of the given insertion event, we can trigger a backfill.
+        along with the approximate depth. Only returns insertion events that are
+        at a depth lower than or equal to the `current_depth`. Sorted by depth,
+        highest to lowest (descending) so the closest events to the
+        `current_depth` are first in the list.
 
-        We ignore insertion events that have a greater depth than our `current_depth`
-        as:
+        We ignore insertion events that are newer than the user's current scroll
+        position (ie, those with depth greater than `current_depth`) as:
             1. we don't really care about getting events that have happened
                after our current position; and
             2. by the nature of paginating and scrolling back, we have likely
@@ -897,9 +888,7 @@ async def get_insertion_event_backward_extremities_in_room(
 
         Args:
             room_id: Room where we want to find the oldest events
-            current_depth: The depth at the users current scrollback position because
-                we only care about finding events older than the given
-                `current_depth` when scrolling and paginating backwards.
+            current_depth: The depth at the user's current scrollback position
             limit: The max number of insertion event extremities to return
 
         Returns:
@@ -934,9 +923,10 @@ def get_insertion_event_backward_extremities_in_room_txn(
                      * means less than the given depth) because we're looking backwards
                      * from the `current_depth` when backfilling.
                      *
-                     *                           current_depth (ignore events that come after this, ignore 2-4)
-                     *                           |
-                     * <oldest-in-time> [0]<--[1]▼<--[2]<--[3]<--[4] <newest-in-time>
+                     *                         current_depth (ignore events that come after this, ignore 2-4)
+                     *                         |
+                     *                         ▼
+                     * <oldest-in-time> [0]<--[1]<--[2]<--[3]<--[4] <newest-in-time>
                      */
                     AND event.depth <= ? /* current_depth */
                     /**