Merge pull request #95 from jagerman/query-optimizations

jagerman · web-flow · commit a1e73cf92e2a · 2022-03-25T18:43:49.000-03:00
Query optimizations
diff --git a/contrib/upgrade-tests/common.sh b/contrib/upgrade-tests/common.sh
@@ -101,4 +101,7 @@ do_upgrades() {
     # This should exit cleanly to indicate no needed migrations (if it doesn't, i.e. we still
     # require migrations after doing a migration then Something Getting Wrong in migrations).
     python3 -msogs --check-upgrades
+
+    # Run the cleanup job to make sure we have the proper rooms.active_users values
+    python3 -c 'from sogs.cleanup import cleanup; cleanup()'
 }
diff --git a/contrib/upgrade-tests/v0.1.10-expected.txt b/contrib/upgrade-tests/v0.1.10-expected.txt
@@ -1,10 +1,10 @@
 rooms:
-+----+------------+-------------------+-------------+-------+--------------+------------------+------+------------+-------+--------+
-| id | token      | name              | description | image | info_updates | message_sequence | read | accessible | write | upload |
-+----+------------+-------------------+-------------+-------+--------------+------------------+------+------------+-------+--------+
-|  1 | round-room | A spherical room! | NULL        |  NULL |            1 |                0 |    1 |          1 |     1 |      1 |
-|  2 | sudoku     | Sudoku lovers     | NULL        |     3 |            2 |                1 |    1 |          1 |     1 |      1 |
-+----+------------+-------------------+-------------+-------+--------------+------------------+------+------------+-------+--------+
++----+------------+-------------------+-------------+-------+--------------+--------------+------------------+------+------------+-------+--------+
+| id | token      | name              | description | image | active_users | info_updates | message_sequence | read | accessible | write | upload |
++----+------------+-------------------+-------------+-------+--------------+--------------+------------------+------+------------+-------+--------+
+|  1 | round-room | A spherical room! | NULL        |  NULL |            0 |            1 |                0 |    1 |          1 |     1 |      1 |
+|  2 | sudoku     | Sudoku lovers     | NULL        |     3 |            0 |            2 |                1 |    1 |          1 |     1 |      1 |
++----+------------+-------------------+-------------+-------+--------------+--------------+------------------+------+------------+-------+--------+
 
 room_import_hacks:
 +------+-------------------+--------------------+
diff --git a/contrib/upgrade-tests/v0.2.0-expected.txt b/contrib/upgrade-tests/v0.2.0-expected.txt
@@ -1,12 +1,12 @@
 rooms:
-+----+------------+--------------+----------------------------------------------------------------+-------+--------------+------------------+------+------------+-------+--------+
-| id | token      | name         | description                                                    | image | info_updates | message_sequence | read | accessible | write | upload |
-+----+------------+--------------+----------------------------------------------------------------+-------+--------------+------------------+------+------------+-------+--------+
-|  1 | gibberish  | Gibberish 🥄 | Delivering your daily dose of gibberish.                       |     3 |            3 |                7 |    1 |          1 |     1 |      1 |
-|  2 | sudoku     | Sudoku       | Delivering your daily dose of gib... sudoku.                   |     1 |            3 |                5 |    1 |          1 |     1 |      1 |
-|  3 | cool-room  | Private room | If you're reading these posts then you're too cool for school. |  NULL |            1 |                4 |    0 |          1 |     0 |      0 |
-|  4 | empty-room | Empty        | We don't go there anymore.                                     |  NULL |            1 |                0 |    1 |          1 |     1 |      1 |
-+----+------------+--------------+----------------------------------------------------------------+-------+--------------+------------------+------+------------+-------+--------+
++----+------------+--------------+----------------------------------------------------------------+-------+--------------+--------------+------------------+------+------------+-------+--------+
+| id | token      | name         | description                                                    | image | active_users | info_updates | message_sequence | read | accessible | write | upload |
++----+------------+--------------+----------------------------------------------------------------+-------+--------------+--------------+------------------+------+------------+-------+--------+
+|  1 | gibberish  | Gibberish 🥄 | Delivering your daily dose of gibberish.                       |     3 |            0 |            3 |                7 |    1 |          1 |     1 |      1 |
+|  2 | sudoku     | Sudoku       | Delivering your daily dose of gib... sudoku.                   |     1 |            0 |            3 |                5 |    1 |          1 |     1 |      1 |
+|  3 | cool-room  | Private room | If you're reading these posts then you're too cool for school. |  NULL |            0 |            1 |                4 |    0 |          1 |     0 |      0 |
+|  4 | empty-room | Empty        | We don't go there anymore.                                     |  NULL |            0 |            1 |                0 |    1 |          1 |     1 |      1 |
++----+------------+--------------+----------------------------------------------------------------+-------+--------------+--------------+------------------+------+------------+-------+--------+
 
 room_import_hacks:
 message_metadata:
diff --git a/contrib/upgrade-tests/v0.3.0-pg-expected.txt b/contrib/upgrade-tests/v0.3.0-pg-expected.txt
@@ -1,12 +1,12 @@
 rooms:
-+----+------------+--------------+----------------------------------------------------------------+-------+--------------+------------------+------+------------+-------+--------+
-| id | token      | name         | description                                                    | image | info_updates | message_sequence | read | accessible | write | upload |
-+----+------------+--------------+----------------------------------------------------------------+-------+--------------+------------------+------+------------+-------+--------+
-|  1 | gibberish  | Gibberish 🥄 | Delivering your daily dose of gibberish.                       |     2 |            3 |               10 |    1 |          1 |     1 |      1 |
-|  2 | sudoku     | Sudoku       | Delivering your daily dose of gib... sudoku.                   |     1 |            2 |                4 |    1 |          1 |     1 |      1 |
-|  3 | cool-room  | Private room | If you're reading these posts then you're too cool for school. |  NULL |            1 |                4 |    1 |          1 |     1 |      1 |
-|  4 | empty-room | Empty        | We don't go there anymore.                                     |  NULL |            1 |                0 |    1 |          1 |     1 |      1 |
-+----+------------+--------------+----------------------------------------------------------------+-------+--------------+------------------+------+------------+-------+--------+
++----+------------+--------------+----------------------------------------------------------------+-------+--------------+--------------+------------------+------+------------+-------+--------+
+| id | token      | name         | description                                                    | image | active_users | info_updates | message_sequence | read | accessible | write | upload |
++----+------------+--------------+----------------------------------------------------------------+-------+--------------+--------------+------------------+------+------------+-------+--------+
+|  1 | gibberish  | Gibberish 🥄 | Delivering your daily dose of gibberish.                       |     2 |            0 |            3 |               10 |    1 |          1 |     1 |      1 |
+|  2 | sudoku     | Sudoku       | Delivering your daily dose of gib... sudoku.                   |     1 |            0 |            2 |                4 |    1 |          1 |     1 |      1 |
+|  3 | cool-room  | Private room | If you're reading these posts then you're too cool for school. |  NULL |            0 |            1 |                4 |    1 |          1 |     1 |      1 |
+|  4 | empty-room | Empty        | We don't go there anymore.                                     |  NULL |            0 |            1 |                0 |    1 |          1 |     1 |      1 |
++----+------------+--------------+----------------------------------------------------------------+-------+--------------+--------------+------------------+------+------------+-------+--------+
 
 room_import_hacks:
 message_metadata:
diff --git a/sogs/__main__.py b/sogs/__main__.py
@@ -169,7 +169,7 @@ def print_room(room: Room):
     msgs_size /= 1_000_000
     files_size /= 1_000_000
 
-    active = [room.active_users(x * 86400) for x in (7, 14, 30)]
+    active = [room.active_users_last(x * 86400) for x in (7, 14, 30)]
     m, a, hm, ha = room.get_all_moderators()
     admins = len(a) + len(ha)
     mods = len(m) + len(hm)
diff --git a/sogs/cleanup.py b/sogs/cleanup.py
@@ -84,6 +84,15 @@ def prune_room_activity():
 
     if count > 0:
         app.logger.info("Prune {} old room activity records".format(count))
+
+    query(
+        """
+        UPDATE rooms SET active_users = (
+            SELECT COUNT(*) FROM room_users WHERE room = rooms.id AND last_active >= :since)
+        """,
+        since=time.time() - config.ROOM_DEFAULT_ACTIVE_THRESHOLD,
+    )
+
     return count
 
 
diff --git a/sogs/migrations/__init__.py b/sogs/migrations/__init__.py
@@ -11,6 +11,7 @@
     new_columns,
     new_tables,
     room_accessible,
+    room_moderators,
     seqno_etc,
     user_permissions,
     user_perm_futures,
@@ -42,6 +43,7 @@ def migrate(conn, *, check_only=False):
         message_views,
         user_perm_futures,
         room_accessible,
+        room_moderators,
         user_permissions,
         file_message,
         import_hacks,
diff --git a/sogs/migrations/new_columns.py b/sogs/migrations/new_columns.py
@@ -15,7 +15,8 @@ def migrate(conn, *, check_only):
             'whisper': 'INTEGER REFERENCES users(id)',
             'whisper_mods': 'BOOLEAN NOT NULL DEFAULT FALSE',
             'filtered': 'BOOLEAN NOT NULL DEFAULT FALSE',
-        }
+        },
+        'rooms': {'active_users': 'BIGINT NOT NULL DEFAULT 0'},
     }
 
     added = False
diff --git a/sogs/migrations/room_moderators.py b/sogs/migrations/room_moderators.py
@@ -0,0 +1,102 @@
+import logging
+from .exc import DatabaseUpgradeRequired
+
+
+def migrate(conn, *, check_only):
+    """
+    Adds the room_moderators view, along with a couple other optimizations that came at the same
+    time:
+    - we drop the user_permissions view (to be recreated in the user_permissions migration code)
+    - we drop the user_permission_overrides_public_mods index and recreate a tighter index
+    """
+
+    from .. import db
+
+    if 'room_moderators' in db.metadata.tables:
+        return False
+
+    logging.warning("DB migration: create room_moderators view")
+    if check_only:
+        raise DatabaseUpgradeRequired("Create room_moderators view")
+
+    if db.engine.name == "sqlite":
+        conn.execute(
+            """
+CREATE VIEW room_moderators AS
+SELECT session_id, mods.* FROM (
+    SELECT
+        room,
+        "user",
+        MAX(visible_mod) & 1 AS visible_mod,
+        MAX(admin) AS admin,
+        MAX(room_moderator) AS room_moderator,
+        MAX(global_moderator) AS global_moderator
+    FROM (
+        SELECT
+            room,
+            "user",
+            CASE WHEN visible_mod THEN 3 ELSE 2 END AS visible_mod,
+            admin,
+            TRUE AS room_moderator,
+            FALSE AS global_moderator
+        FROM user_permission_overrides WHERE moderator
+
+        UNION ALL
+
+        SELECT
+            rooms.id AS room,
+            users.id as "user",
+            CASE WHEN visible_mod THEN 1 ELSE 0 END AS visible_mod,
+            admin,
+            FALSE as room_moderator,
+            TRUE as global_moderator
+        FROM users CROSS JOIN rooms WHERE moderator
+    ) m GROUP BY "user", room
+) mods JOIN users on "user" = users.id
+"""
+        )
+    else:  # postgres
+        conn.execute(
+            """
+CREATE VIEW room_moderators AS
+SELECT session_id, mods.* FROM (
+    SELECT
+        room,
+        "user",
+        CAST(MAX(visible_mod) & 1 AS BOOLEAN) AS visible_mod,
+        bool_or(admin) AS admin,
+        bool_or(room_moderator) AS room_moderator,
+        bool_or(global_moderator) AS global_moderator
+    FROM (
+        SELECT
+            room,
+            "user",
+            CASE WHEN visible_mod THEN 3 ELSE 2 END AS visible_mod,
+            admin,
+            TRUE AS room_moderator,
+            FALSE AS global_moderator
+        FROM user_permission_overrides WHERE moderator
+
+        UNION ALL
+
+        SELECT
+            rooms.id AS room,
+            users.id as "user",
+            CASE WHEN visible_mod THEN 1 ELSE 0 END AS visible_mod,
+            admin,
+            FALSE as room_moderator,
+            TRUE as global_moderator
+        FROM users CROSS JOIN rooms WHERE moderator
+    ) m GROUP BY "user", room
+) mods JOIN users on "user" = users.id
+"""
+        )
+
+    conn.execute("DROP VIEW IF EXISTS user_permissions")
+    conn.execute("DROP INDEX IF EXISTS user_permission_overrides_public_mods")
+    conn.execute(
+        "CREATE INDEX IF NOT EXISTS user_permission_overrides_mods "
+        "ON user_permission_overrides(room) WHERE moderator"
+    )
+
+    return True
diff --git a/sogs/migrations/user_permissions.py b/sogs/migrations/user_permissions.py
@@ -17,13 +17,12 @@ def migrate(conn, *, check_only):
     if check_only:
         raise DatabaseUpgradeRequired("Recreate user_permissions view")
 
-    sqlite = db.engine.name == "sqlite"
     conn.execute(
-        f"""
+        """
 CREATE VIEW user_permissions AS
 SELECT
     rooms.id AS room,
-    users.id AS {'user' if sqlite else '"user"'},
+    users.id AS "user",
     users.session_id,
     CASE WHEN users.banned THEN TRUE ELSE COALESCE(user_permission_overrides.banned, FALSE) END AS banned,
     CASE WHEN users.moderator THEN TRUE ELSE COALESCE(user_permission_overrides.read, rooms.read) END AS read,
@@ -33,19 +32,19 @@ def migrate(conn, *, check_only):
     CASE WHEN users.moderator THEN TRUE ELSE COALESCE(user_permission_overrides.moderator, FALSE) END AS moderator,
     CASE WHEN users.admin THEN TRUE ELSE COALESCE(user_permission_overrides.admin, FALSE) END AS admin,
     -- room_moderator will be TRUE if the user is specifically listed as a moderator of the room
-    COALESCE(user_permission_overrides.moderator OR user_permission_overrides.admin, FALSE) AS room_moderator,
+    COALESCE(user_permission_overrides.moderator, FALSE) AS room_moderator,
     -- global_moderator will be TRUE if the user is a global moderator/admin (note that this is
     -- *not* exclusive of room_moderator: a moderator/admin could be listed in both).
-    COALESCE(users.moderator OR users.admin, FALSE) as global_moderator,
+    users.moderator as global_moderator,
     -- visible_mod will be TRUE if this mod is a publicly viewable moderator of the room
     CASE
-        WHEN user_permission_overrides.moderator OR user_permission_overrides.admin THEN user_permission_overrides.visible_mod
-        WHEN users.moderator OR users.admin THEN users.visible_mod
+        WHEN user_permission_overrides.moderator THEN user_permission_overrides.visible_mod
+        WHEN users.moderator THEN users.visible_mod
         ELSE FALSE
     END AS visible_mod
 FROM
-    users {'JOIN' if sqlite else 'CROSS JOIN'} rooms LEFT OUTER JOIN user_permission_overrides ON
-        (users.id = user_permission_overrides.{'user' if sqlite else '"user"'} AND rooms.id = user_permission_overrides.room)
+    users CROSS JOIN rooms LEFT OUTER JOIN user_permission_overrides ON
+        (users.id = user_permission_overrides."user" AND rooms.id = user_permission_overrides.room)
 """  # noqa E501
     )
 
diff --git a/sogs/model/room.py b/sogs/model/room.py
@@ -45,6 +45,8 @@ class Room:
             new/edited/deleted messages.
         info_updates - counter on room metadata that is automatically incremented whenever room
             metadata (name, description, image, etc.) changes for the room.
+        active_users - count of the number of active users in the past
+            config.ROOM_DEFAULT_ACTIVE_THRESHOLD seconds.
         default_read - True if default user permissions includes read permission
         default_accessible - True if default user permissions include accessible permission
         default_write - True if default user permissions includes write permission
@@ -92,6 +94,7 @@ def _refresh(self, *, id=None, token=None, row=None, perms=False):
             self.created,
             self.message_sequence,
             self.info_updates,
+            self.active_users,
         ) = (
             row[c]
             for c in (
@@ -103,6 +106,7 @@ def _refresh(self, *, id=None, token=None, row=None, perms=False):
                 'created',
                 'message_sequence',
                 'info_updates',
+                'active_users',
             )
         )
         self._default_read, self._default_accessible, self._default_write, self._default_upload = (
@@ -374,11 +378,15 @@ def default_upload(self, upload: bool):
                 query("UPDATE rooms SET upload = :upload WHERE id = :r", r=self.id, upload=upload)
                 self._refresh(perms=True)
 
-    def active_users(self, cutoff=config.ROOM_DEFAULT_ACTIVE_THRESHOLD):
+    def active_users_last(self, cutoff: float):
         """
-        Queries the number of active users in the past `cutoff` seconds.  Defaults to
-        config.ROOM_DEFAULT_ACTIVE_THRESHOLD.  Note that room activity records are periodically
-        removed, so going beyond config.ROOM_ACTIVE_PRUNE_THRESHOLD days is useless.
+        Queries the number of active users in the past `cutoff` seconds.  This is like the
+        `active_users` property except that it always queries for the instantaneous value
+        (`active_users` is only updated every few seconds in sogs.cleanup), and supports values
+        other than the default activity threshold.
+
+        Note that room activity records are periodically removed, so specifying a cutoff above
+        config.ROOM_ACTIVE_PRUNE_THRESHOLD days is useless.
         """
 
         return query(
@@ -981,11 +989,12 @@ def get_mods(self, user=None):
         ([public_mods], [public_admins], [hidden_mods], [hidden_admins])
         """
 
+        visible_clause = "" if self.check_moderator(user) else "AND visible_mod"
         m, hm, a, ha = [], [], [], []
         for session_id, visible, admin in query(
-            """
-            SELECT session_id, visible_mod, admin FROM user_permissions
-            WHERE room = :r AND moderator
+            f"""
+            SELECT session_id, visible_mod, admin FROM room_moderators
+            WHERE room = :r {visible_clause}
             ORDER BY session_id
             """,
             r=self.id,
@@ -996,9 +1005,6 @@ def get_mods(self, user=None):
 
             ((a if admin else m) if visible else (ha if admin else hm)).append(session_id)
 
-        if user is None or not any(user.session_id in modlist for modlist in (m, hm, a, ha)):
-            hm, ha = [], []
-
         return m, a, hm, ha
 
     def get_all_moderators(self):
@@ -1205,14 +1211,20 @@ def unban_user(self, to_unban: User, *, mod: User):
 
     def get_bans(self):
         """
-        Retrieves all the session IDs banned from this room.  This does not check permissions: i.e.
-        it should only be accessed by moderators/admins.
+        Retrieves all the session IDs specifically banned from this room (not including global
+        bans).  This does not check permissions: i.e.  it should only be accessed by
+        moderators/admins.
         """
 
         return sorted(
             r[0]
             for r in query(
-                "SELECT session_id FROM user_permissions WHERE room = :r AND banned", r=self.id
+                """
+                SELECT session_id
+                FROM user_permission_overrides upo JOIN users ON upo."user" = users.id
+                WHERE room = :r AND upo.banned
+                """,
+                r=self.id,
             )
         )
 
diff --git a/sogs/routes/legacy.py b/sogs/routes/legacy.py
@@ -137,7 +137,7 @@ def legacy_serve_room_image(room):
 def legacy_member_count():
     user, room = legacy_check_user_room(accessible=True)
 
-    return jsonify({"status_code": http.OK, "member_count": room.active_users()})
+    return jsonify({"status_code": http.OK, "member_count": room.active_users})
 
 
 @legacy.post("/claim_auth_token")
diff --git a/sogs/routes/rooms.py b/sogs/routes/rooms.py
diff --git a/sogs/schema.pgsql b/sogs/schema.pgsql
diff --git a/sogs/schema.sqlite b/sogs/schema.sqlite
diff --git a/tests/test_blinding.py b/tests/test_blinding.py
diff --git a/tests/test_room_routes.py b/tests/test_room_routes.py
diff --git a/tests/test_rooms.py b/tests/test_rooms.py

Original file line number	Diff line number	Diff line change
`@@ -101,4 +101,7 @@ do_upgrades() {`
`101`	`101`	`# This should exit cleanly to indicate no needed migrations (if it doesn't, i.e. we still`
`102`	`102`	`# require migrations after doing a migration then Something Getting Wrong in migrations).`
`103`	`103`	`python3 -msogs --check-upgrades`
	`104`	`+`
	`105`	`+ # Run the cleanup job to make sure we have the proper rooms.active_users values`
	`106`	`+ python3 -c 'from sogs.cleanup import cleanup; cleanup()'`
`104`	`107`	`}`
Original file line number	Diff line number	Diff line change
`@@ -15,7 +15,8 @@ def migrate(conn, *, check_only):`
`15`	`15`	`'whisper': 'INTEGER REFERENCES users(id)',`
`16`	`16`	`'whisper_mods': 'BOOLEAN NOT NULL DEFAULT FALSE',`
`17`	`17`	`'filtered': 'BOOLEAN NOT NULL DEFAULT FALSE',`
`18`		`- }`
	`18`	`+ },`
	`19`	`+ 'rooms': {'active_users': 'BIGINT NOT NULL DEFAULT 0'},`
`19`	`20`	`}`
`20`	`21`
`21`	`22`	`added = False`