Cache room active_users

jagerman · jagerman · commit c47cfb790602 · 2022-03-25T17:12:29.000-03:00
This replaces active counts of room_users to get the active user count,
instead caching the current value in a new `rooms.active_users` column,
updating it every 10s during the cleanup task.

The count ended up being moderately expensive, (the most expensive
regular query we have right now since the earlier change on this
branch), and gets worse both as the count gets larger and as the table
gets increasingly fragmented.  It isn't a noticeable problem *yet*
because legacy Session clients only query room info once in a while
(seemingly randomly) but new Session endpoints will hit this much more
often and so this preemptively addresses the issue.
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/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/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(
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
@@ -24,7 +24,7 @@ def get_room_info(room):
         'info_updates': room.info_updates,
         'message_sequence': room.message_sequence,
         'created': room.created,
-        'active_users': room.active_users(),
+        'active_users': room.active_users,
         'active_users_cutoff': int(config.ROOM_DEFAULT_ACTIVE_THRESHOLD),
         'moderators': mods,
         'admins': admins,
@@ -722,7 +722,7 @@ def poll_room_info(room, info_updated):
 
     result = {
         'token': room.token,
-        'active_users': room.active_users(),
+        'active_users': room.active_users,
         'read': room.check_read(g.user),
         'write': room.check_write(g.user),
         'upload': room.check_upload(g.user),
diff --git a/sogs/schema.pgsql b/sogs/schema.pgsql
@@ -12,6 +12,7 @@ CREATE TABLE rooms (
     created FLOAT NOT NULL DEFAULT (extract(epoch from now())),
     message_sequence BIGINT NOT NULL DEFAULT 0, /* monotonic current top message.seqno value: +1 for each new message, edit or deletion */
     info_updates BIGINT NOT NULL DEFAULT 0, /* +1 for any room metadata update (name/desc/image/pinned/mods) */
+    active_users BIGINT NOT NULL DEFAULT 0,
     read BOOLEAN NOT NULL DEFAULT TRUE, /* Whether users can read by default */
     accessible BOOLEAN NOT NULL DEFAULT TRUE, /* Whether room metadata is accessible when `read` is false */
     write BOOLEAN NOT NULL DEFAULT TRUE, /* Whether users can post by default */
@@ -445,8 +446,8 @@ CREATE INDEX user_permissions_future_room_user ON user_permission_futures(room,
 -- their user_permissions.banned to TRUE then add a row here with banned = FALSE to schedule the
 -- unban.  (You can also schedule a future *ban* here, but the utility of that is less clear).
 CREATE TABLE user_ban_futures (
-    room INTEGER REFERENCES rooms ON DELETE CASCADE,
-    "user" INTEGER NOT NULL REFERENCES users ON DELETE CASCADE,
+    room BIGINT REFERENCES rooms ON DELETE CASCADE,
+    "user" BIGINT NOT NULL REFERENCES users ON DELETE CASCADE,
     at FLOAT NOT NULL, /* when the change should take effect (unix epoch) */
     banned BOOLEAN NOT NULL /* if true then ban at `at`, if false then unban */
 );
diff --git a/sogs/schema.sqlite b/sogs/schema.sqlite
@@ -12,6 +12,7 @@ CREATE TABLE rooms (
     created FLOAT NOT NULL DEFAULT ((julianday('now') - 2440587.5)*86400.0), /* unix epoch */
     message_sequence INTEGER NOT NULL DEFAULT 0, /* monotonic current top message.seqno value: +1 for each new message, edit or deletion */
     info_updates INTEGER NOT NULL DEFAULT 0, /* +1 for any room metadata update (name/desc/image/pinned/mods) */
+    active_users INTEGER NOT NULL DEFAULT 0,
     read BOOLEAN NOT NULL DEFAULT TRUE, /* Whether users can read by default */
     accessible BOOLEAN NOT NULL DEFAULT TRUE, /* Whether room metadata is accessible when `read` is false */
     write BOOLEAN NOT NULL DEFAULT TRUE, /* Whether users can post by default */
diff --git a/tests/test_room_routes.py b/tests/test_room_routes.py
@@ -374,15 +374,15 @@ def test_polling(client, room, user, user2, mod, admin, global_mod, global_admin
     info_up = r.json['info_updates']
     assert info_up == 2
 
-    basic = {'token': 'test-room', 'active_users': 1, 'read': True, 'write': True, 'upload': True}
+    basic = {'token': 'test-room', 'active_users': 0, 'read': True, 'write': True, 'upload': True}
     details = {
         "token": "test-room",
         "name": "Test room",
         "description": "Test suite testing room",
         "info_updates": 2,
         "message_sequence": 0,
         "created": room.created,
-        "active_users": 1,
+        "active_users": 0,
         "active_users_cutoff": int(sogs.config.ROOM_DEFAULT_ACTIVE_THRESHOLD),
         "moderators": [mod.session_id],
         "admins": [admin.session_id],
@@ -396,6 +396,17 @@ def test_polling(client, room, user, user2, mod, admin, global_mod, global_admin
     assert r.status_code == 200
     assert r.json == basic
 
+    r = sogs_get(client, f"/room/test-room/pollInfo/{info_up}", user)
+    assert r.status_code == 200
+    assert r.json == basic
+
+    # We need to call the cleanup handler to update rooms.active_users
+    from sogs.cleanup import cleanup
+
+    cleanup()
+    basic['active_users'] += 1
+    details['active_users'] += 1
+
     # Make various changes that should each update the room info updates:
 
     # Changing name
@@ -407,17 +418,22 @@ def test_polling(client, room, user, user2, mod, admin, global_mod, global_admin
     assert r.json == {**basic, 'details': details}
 
     info_up += 1
+    cleanup()
     r = sogs_get(client, f"/room/test-room/pollInfo/{info_up}", mod)
-    basic['active_users'] += 1
-    details['active_users'] += 1
     assert r.status_code == 200
     assert r.json == {**basic, 'moderator': True, **defs}
 
+    cleanup()
+    basic['active_users'] += 1
+    details['active_users'] += 1
+
     r = sogs_get(client, f"/room/test-room/pollInfo/{info_up}", admin)
     assert r.status_code == 200
+    assert r.json == {**basic, 'moderator': True, 'admin': True, **defs}
+
+    cleanup()
     basic['active_users'] += 1
     details['active_users'] += 1
-    assert r.json == {**basic, 'moderator': True, 'admin': True, **defs}
 
     # Changing description
     room.description = 'Test suite testing room new desc'
diff --git a/tests/test_rooms.py b/tests/test_rooms.py
@@ -602,3 +602,28 @@ def test_pinning(room, room2, user, mod, admin, global_admin, no_rate_limit):
         room2.pin(7, global_admin)
 
     assert not room2.pinned_messages
+
+
+def test_active_users(room, user, user2):
+    assert room.active_users == 0
+    user.update_room_activity(room)
+    room._refresh()
+    assert room.active_users_last(1) == 1
+    assert room.active_users == 0  # Doesn't update until the cleanup cycle
+
+    from sogs.cleanup import cleanup
+
+    cleanup()
+    room._refresh()
+
+    assert room.active_users == 1
+    user2.update_room_activity(room)
+    room._refresh()
+    assert room.active_users == 1
+    assert room.active_users_last(1) == 2
+
+    cleanup()
+    room._refresh()
+
+    assert room.active_users == 2
+    assert room.active_users_last(1) == 2

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`